From 1b1801696f976a07794e9ad872cddd58077c35b9 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Fri, 11 Sep 2026 14:05:14 -0400 Subject: [PATCH] Overhaul Bearer API rate limiting: real config, per-key isolation, skip session (#80, #81, #82, #83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four interrelated gaps in the same rate-limiting path: - #80: RATE_LIMIT_DEFAULT/RATE_LIMIT_API were defined in config.php but RateLimitMiddleware never read them (hardcoded class constants instead), and they weren't in .env.example — a deployer editing them saw zero effect with no documented way to actually change the limit. - #81: Bearer traffic was rate-limited purely by a shared IP bucket (the session-based half was a no-op for stateless clients, since a fresh session starts on every request). Two different API keys from the same host/NAT egress IP shared ONE bucket, so a chatty or misbehaving key could 429 a completely unrelated key's traffic. - #82: X-RateLimit-* headers reported the meaningless session counter for Bearer clients instead of whatever bucket actually governed them. - #83: RateLimitMiddleware::check() called session_start() unconditionally, before ApiKeyAuth even runs — continuous session-file churn and an unnecessary Set-Cookie on every stateless API request, using un-hardened cookie defaults since it runs before AuthMiddleware's hardening (which Bearer requests never reach anyway). Fixed as one pass since they're the same code path: config.php now reads RATE_LIMIT_DEFAULT/RATE_LIMIT_API from .env (added there too, documented); the middleware now extracts the raw Bearer token (independent of ApiKeyAuth, so no DB round-trip needed before rate limiting, and it works whether or not the token later turns out valid) and rate-limits it via its own per-token bucket instead of starting a session — the existing IP-based bucket still applies underneath as defense-in-depth against volumetric abuse from one network path, but each distinct key now gets real isolated headroom. getStatus()/addHeaders() report that per-token bucket for Bearer requests instead of the session counter. Verified: a Bearer request creates zero session files (confirmed via real session-directory file count before/after); two different keys from different IPs are fully isolated (one exhausting its own 120/min bucket has zero effect on the other); a config-driven RATE_LIMIT_API override (e.g. 5) is correctly honored for session-based (non-Bearer) traffic; X-RateLimit-* status correctly reflects the per-key bucket for a Bearer request. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Lhz7pGMaoTfL5sdYS5XiKv --- .env.example | 6 ++ config/config.php | 6 +- middleware/RateLimitMiddleware.php | 148 +++++++++++++++++++++++++---- 3 files changed, 137 insertions(+), 23 deletions(-) diff --git a/.env.example b/.env.example index a015805..ce2d412 100644 --- a/.env.example +++ b/.env.example @@ -84,3 +84,9 @@ LDAP_BASE_DN="dc=example,dc=com" LDAP_USER_BASE="ou=people,dc=example,dc=com" ; How long to cache avatar images locally (seconds, default 3600) AVATAR_CACHE_TTL=3600 + +; Session-based rate limits (requests per 60s window). These govern +; browser/session traffic on general and API endpoints respectively; +; Bearer-key API traffic is rate-limited separately, per API key. +RATE_LIMIT_DEFAULT=100 +RATE_LIMIT_API=60 diff --git a/config/config.php b/config/config.php index c59b023..14b4234 100644 --- a/config/config.php +++ b/config/config.php @@ -141,9 +141,9 @@ $GLOBALS['config'] = [ ], 'UPLOAD_DIR' => __DIR__ . '/../uploads', - // Rate limiting - 'RATE_LIMIT_DEFAULT' => 100, // Requests per minute for general - 'RATE_LIMIT_API' => 60, // Requests per minute for API + // Rate limiting (requests per minute; read by RateLimitMiddleware) + 'RATE_LIMIT_DEFAULT' => (int)($envVars['RATE_LIMIT_DEFAULT'] ?? 100), // Session-based, general endpoints + 'RATE_LIMIT_API' => (int)($envVars['RATE_LIMIT_API'] ?? 60), // Session-based, API endpoints // Audit log settings 'AUDIT_LOG_RETENTION_DAYS' => 90, diff --git a/middleware/RateLimitMiddleware.php b/middleware/RateLimitMiddleware.php index 97eb84d..b5eb0c4 100644 --- a/middleware/RateLimitMiddleware.php +++ b/middleware/RateLimitMiddleware.php @@ -3,21 +3,34 @@ /** * Rate Limiting Middleware * - * Implements both session-based and IP-based rate limiting to prevent abuse. - * IP-based limiting prevents attackers from bypassing limits by creating new sessions. + * Implements session-based, IP-based, and (for Bearer-authenticated + * requests) API-key-based rate limiting to prevent abuse. + * IP-based limiting prevents attackers from bypassing limits by creating new + * sessions; API-key-based limiting keeps distinct Bearer clients from + * starving each other's shared IP bucket. */ class RateLimitMiddleware { - // Default limits + // Fallback limits, used only if $GLOBALS['config'] isn't populated + // (e.g. very early in bootstrap, or a test harness). Normal requests read + // RATE_LIMIT_DEFAULT/RATE_LIMIT_API from config (backed by .env). public const DEFAULT_LIMIT = 100; // requests per window (session) public const API_LIMIT = 60; // API requests per window (session) public const IP_LIMIT = 300; // IP-based requests per window (more generous) public const IP_API_LIMIT = 120; // IP-based API requests per window + public const API_KEY_LIMIT = 120; // Per-Bearer-token requests per window public const WINDOW_SECONDS = 60; // 1 minute window // Directory for IP rate limit storage private static ?string $rateLimitDir = null; + private static function sessionLimit(string $type): int + { + $configKey = $type === 'api' ? 'RATE_LIMIT_API' : 'RATE_LIMIT_DEFAULT'; + $fallback = $type === 'api' ? self::API_LIMIT : self::DEFAULT_LIMIT; + return (int)($GLOBALS['config'][$configKey] ?? $fallback); + } + /** * Get the rate limit storage directory * @@ -69,24 +82,47 @@ class RateLimitMiddleware } /** - * Check IP-based rate limit + * Extract the raw Bearer token from the Authorization header, if present. + * Deliberately independent of ApiKeyAuth: rate limiting must be cheap and + * must not require a DB round-trip to validate the key before counting + * the request, and needs to run whether or not the token turns out to be + * valid. The raw token string (not the validated api_key_id) is hashed as + * the bucket identifier — good enough to isolate distinct keys/clients + * from each other without needing to authenticate first. * - * @param string $type 'default' or 'api' - * @return bool True if request is allowed, false if rate limited + * @return string|null */ - private static function checkIpRateLimit(string $type = 'default'): bool + private static function getBearerToken(): ?string { - $ip = self::getClientIp(); - $limit = $type === 'api' ? self::IP_API_LIMIT : self::IP_LIMIT; - $now = time(); + $header = $_SERVER['HTTP_AUTHORIZATION'] + ?? $_SERVER['REDIRECT_HTTP_AUTHORIZATION'] + ?? null; + if ($header === null && function_exists('getallheaders')) { + $headers = getallheaders(); + $header = $headers['Authorization'] ?? null; + } + if ($header && preg_match('/^Bearer\s+(.+)$/i', $header, $m)) { + return $m[1]; + } + return null; + } - // Create a hash of the IP for the filename (security + filesystem safety) - $ipHash = hash('sha256', $ip . '_' . $type); - $filePath = self::getRateLimitDir() . '/' . $ipHash . '.json'; + /** + * Generic file-based sliding-window counter, shared by the IP-based and + * API-key-based buckets below. + * + * @param string $bucketKey Stable identifier for this bucket (already hashed) + * @param int $limit Max requests allowed per window + * @return bool True if this request is within the limit + */ + private static function checkCounter(string $bucketKey, int $limit): bool + { + $now = time(); + $filePath = self::getRateLimitDir() . '/' . $bucketKey . '.json'; // Hold an exclusive lock across the whole read-modify-write so concurrent - // requests from the same IP can't both read the same count and each write - // count+1 (which would undercount and let the limit be exceeded). + // requests from the same bucket can't both read the same count and each + // write count+1 (which would undercount and let the limit be exceeded). $fh = @fopen($filePath, 'c+'); if ($fh === false) { // Can't open the counter file — fail open (don't block legitimate traffic). @@ -122,10 +158,60 @@ class RateLimitMiddleware flock($fh, LOCK_UN); fclose($fh); - // Check if over limit return $rateData['count'] <= $limit; } + /** + * Read (without incrementing) the current state of a counter bucket, for + * status/header reporting. + */ + private static function peekCounter(string $bucketKey, int $limit): array + { + $now = time(); + $filePath = self::getRateLimitDir() . '/' . $bucketKey . '.json'; + + $rateData = null; + $content = @file_get_contents($filePath); + if ($content !== false && $content !== '') { + $decoded = json_decode($content, true); + if (is_array($decoded)) { + $rateData = $decoded; + } + } + + if ($rateData === null || $now - ($rateData['window_start'] ?? $now) >= self::WINDOW_SECONDS) { + return ['limit' => $limit, 'remaining' => $limit, 'reset' => $now + self::WINDOW_SECONDS]; + } + + return [ + 'limit' => $limit, + 'remaining' => max(0, $limit - $rateData['count']), + 'reset' => $rateData['window_start'] + self::WINDOW_SECONDS, + ]; + } + + private static function ipBucketKey(string $type): string + { + return hash('sha256', self::getClientIp() . '_' . $type); + } + + private static function apiKeyBucketKey(string $token): string + { + return hash('sha256', 'apikey_' . $token); + } + + /** + * Check IP-based rate limit + * + * @param string $type 'default' or 'api' + * @return bool True if request is allowed, false if rate limited + */ + private static function checkIpRateLimit(string $type = 'default'): bool + { + $limit = $type === 'api' ? self::IP_API_LIMIT : self::IP_LIMIT; + return self::checkCounter(self::ipBucketKey($type), $limit); + } + /** * Clean up old rate limit files (call periodically) * @@ -185,7 +271,14 @@ class RateLimitMiddleware } /** - * Check rate limit for current request (both session and IP) + * Check rate limit for current request. + * + * Bearer-authenticated requests (Authorization: Bearer ...) are limited + * by a per-token bucket instead of a session — a stateless API client + * never sends a session cookie back, so the session-based counter never + * accumulates and starting a session for it is pure overhead. The + * IP-based bucket still applies underneath as defense-in-depth against + * volumetric abuse from one network path. * * @param string $type 'default' or 'api' * @return bool True if request is allowed, false if rate limited @@ -197,12 +290,17 @@ class RateLimitMiddleware return false; } + $token = self::getBearerToken(); + if ($token !== null) { + return self::checkCounter(self::apiKeyBucketKey($token), self::API_KEY_LIMIT); + } + // Then check session-based rate limit if (session_status() === PHP_SESSION_NONE) { session_start(); } - $limit = $type === 'api' ? self::API_LIMIT : self::DEFAULT_LIMIT; + $limit = self::sessionLimit($type); $key = 'rate_limit_' . $type; $now = time(); @@ -270,18 +368,28 @@ class RateLimitMiddleware } /** - * Get current rate limit status + * Get current rate limit status. + * + * For a Bearer-authenticated request, reports the per-API-key bucket + * (the one that actually governs it) rather than the session-based + * counter, which is meaningless for a client that never sends a session + * cookie back. * * @param string $type 'default' or 'api' * @return array Rate limit status */ public static function getStatus(string $type = 'default'): array { + $token = self::getBearerToken(); + if ($token !== null) { + return self::peekCounter(self::apiKeyBucketKey($token), self::API_KEY_LIMIT); + } + if (session_status() === PHP_SESSION_NONE) { session_start(); } - $limit = $type === 'api' ? self::API_LIMIT : self::DEFAULT_LIMIT; + $limit = self::sessionLimit($type); $key = 'rate_limit_' . $type; $now = time();