From 8886ab53b888e312b1709f5763f47450a1029d4a Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 22:26:50 +0600 Subject: [PATCH 1/3] feat: pluggable rate limit storage for Web API middleware Extract file-based counters into RateLimitStoreInterface with optional Redis/Memcached backends via MODX settings or env, keeping file as default. Closes #349 --- _build/elements/settings.php | 40 ++++ core/components/minishop3/composer.json | 4 + .../minishop3/config/routes/web.php | 4 +- .../minishop3/lexicon/en/setting.inc.php | 16 ++ .../minishop3/lexicon/ru/setting.inc.php | 16 ++ .../src/Middleware/RateLimitMiddleware.php | 124 ++---------- .../Services/RateLimit/FileRateLimitStore.php | 90 +++++++++ .../RateLimit/MemcachedRateLimitStore.php | 71 +++++++ .../RateLimit/RateLimitStoreFactory.php | 189 ++++++++++++++++++ .../RateLimit/RateLimitStoreInterface.php | 28 +++ .../RateLimit/RedisRateLimitStore.php | 67 +++++++ .../Middleware/RateLimitMiddlewareTest.php | 57 ++++++ .../RateLimit/FileRateLimitStoreTest.php | 66 ++++++ 13 files changed, 664 insertions(+), 108 deletions(-) create mode 100644 core/components/minishop3/src/Services/RateLimit/FileRateLimitStore.php create mode 100644 core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php create mode 100644 core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php create mode 100644 core/components/minishop3/src/Services/RateLimit/RateLimitStoreInterface.php create mode 100644 core/components/minishop3/src/Services/RateLimit/RedisRateLimitStore.php create mode 100644 core/components/minishop3/tests/Unit/Middleware/RateLimitMiddlewareTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/RateLimit/FileRateLimitStoreTest.php diff --git a/_build/elements/settings.php b/_build/elements/settings.php index ab4375ac..e74edc21 100644 --- a/_build/elements/settings.php +++ b/_build/elements/settings.php @@ -517,6 +517,46 @@ 'xtype' => 'numberfield', 'area' => 'ms3_api', ], + 'ms3_rate_limit_store' => [ + 'value' => 'file', + 'xtype' => 'textfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_storage_path' => [ + 'value' => '', + 'xtype' => 'textfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_redis_dsn' => [ + 'value' => '', + 'xtype' => 'textfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_redis_host' => [ + 'value' => '127.0.0.1', + 'xtype' => 'textfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_redis_port' => [ + 'value' => 6379, + 'xtype' => 'numberfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_redis_password' => [ + 'value' => '', + 'xtype' => 'text-password', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_redis_database' => [ + 'value' => 0, + 'xtype' => 'numberfield', + 'area' => 'ms3_api', + ], + 'ms3_rate_limit_memcached_servers' => [ + 'value' => '127.0.0.1:11211', + 'xtype' => 'textfield', + 'area' => 'ms3_api', + ], // Notifications 'ms3_telegram_bot_token' => [ diff --git a/core/components/minishop3/composer.json b/core/components/minishop3/composer.json index c490c96a..e6372827 100644 --- a/core/components/minishop3/composer.json +++ b/core/components/minishop3/composer.json @@ -37,6 +37,10 @@ }, "minimum-stability": "stable", "prefer-stable": true, + "suggest": { + "ext-redis": "Shared rate-limit counters for multi-node Web API (ms3_rate_limit_store=redis)", + "ext-memcached": "Alternative shared rate-limit storage (ms3_rate_limit_store=memcached)" + }, "require-dev": { "phpstan/phpstan": "^2.1", "phpunit/phpunit": "^11.5" diff --git a/core/components/minishop3/config/routes/web.php b/core/components/minishop3/config/routes/web.php index cf2584a1..fe87cca6 100644 --- a/core/components/minishop3/config/routes/web.php +++ b/core/components/minishop3/config/routes/web.php @@ -33,6 +33,7 @@ use MiniShop3\Middleware\CorsMiddleware; use MiniShop3\Middleware\RateLimitMiddleware; use MiniShop3\Middleware\ServiceCheckMiddleware; +use MiniShop3\Services\RateLimit\RateLimitStoreFactory; $tokenMiddleware = new TokenMiddleware($modx); $corsMiddleware = new CorsMiddleware([ @@ -44,7 +45,8 @@ ]); $rateLimitMiddleware = new RateLimitMiddleware( $modx->getOption('ms3_rate_limit_max_attempts', null, 60), - $modx->getOption('ms3_rate_limit_decay_seconds', null, 60) + $modx->getOption('ms3_rate_limit_decay_seconds', null, 60), + RateLimitStoreFactory::fromModx($modx) ); $serviceCheckMiddleware = new ServiceCheckMiddleware($modx); $router->group('/api/v1', function($router) use ($modx, $tokenMiddleware) { diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index effb99af..bd429fb8 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -258,6 +258,22 @@ $_lang['setting_ms3_rate_limit_max_attempts_desc'] = 'Maximum number of API requests per time period. Default is 60.'; $_lang['setting_ms3_rate_limit_decay_seconds'] = 'Rate limit time window (seconds)'; $_lang['setting_ms3_rate_limit_decay_seconds_desc'] = 'Time window in seconds for counting rate limit. Default is 60 seconds.'; +$_lang['setting_ms3_rate_limit_store'] = 'Rate limit storage driver'; +$_lang['setting_ms3_rate_limit_store_desc'] = 'Counter backend: file (default, single PHP node), redis or memcached for multi-node. MS3_RATE_LIMIT_STORE env overrides this setting.'; +$_lang['setting_ms3_rate_limit_storage_path'] = 'Rate limit file storage path'; +$_lang['setting_ms3_rate_limit_storage_path_desc'] = 'Directory for the file driver. Empty uses sys_get_temp_dir().'; +$_lang['setting_ms3_rate_limit_redis_dsn'] = 'Rate limit Redis DSN'; +$_lang['setting_ms3_rate_limit_redis_dsn_desc'] = 'Full DSN such as redis://:password@127.0.0.1:6379/0. When set, host/port/password/database below are ignored. Env: MS3_RATE_LIMIT_REDIS_DSN. Requires ext-redis.'; +$_lang['setting_ms3_rate_limit_redis_host'] = 'Rate limit Redis host'; +$_lang['setting_ms3_rate_limit_redis_host_desc'] = 'Redis host when DSN is not set.'; +$_lang['setting_ms3_rate_limit_redis_port'] = 'Rate limit Redis port'; +$_lang['setting_ms3_rate_limit_redis_port_desc'] = 'Redis port when DSN is not set.'; +$_lang['setting_ms3_rate_limit_redis_password'] = 'Rate limit Redis password'; +$_lang['setting_ms3_rate_limit_redis_password_desc'] = 'Redis password when DSN is not set.'; +$_lang['setting_ms3_rate_limit_redis_database'] = 'Rate limit Redis database'; +$_lang['setting_ms3_rate_limit_redis_database_desc'] = 'Redis database index (default 0) when DSN is not set.'; +$_lang['setting_ms3_rate_limit_memcached_servers'] = 'Rate limit Memcached servers'; +$_lang['setting_ms3_rate_limit_memcached_servers_desc'] = 'Comma-separated host:port list for the memcached driver. Env: MS3_RATE_LIMIT_MEMCACHED_SERVERS. Requires ext-memcached.'; // Notifications $_lang['setting_ms3_telegram_bot_token'] = 'Telegram bot token'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index 58afc901..58df0da7 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -258,6 +258,22 @@ $_lang['setting_ms3_rate_limit_max_attempts_desc'] = 'Максимальное количество API запросов за период. По умолчанию 60.'; $_lang['setting_ms3_rate_limit_decay_seconds'] = 'Период лимита запросов (сек)'; $_lang['setting_ms3_rate_limit_decay_seconds_desc'] = 'Временное окно в секундах для подсчёта лимита запросов. По умолчанию 60 секунд.'; +$_lang['setting_ms3_rate_limit_store'] = 'Хранилище rate limit'; +$_lang['setting_ms3_rate_limit_store_desc'] = 'Драйвер счётчиков: file (по умолчанию, один PHP-узел), redis или memcached для multi-node. Переменная окружения MS3_RATE_LIMIT_STORE имеет приоритет.'; +$_lang['setting_ms3_rate_limit_storage_path'] = 'Каталог file-хранилища rate limit'; +$_lang['setting_ms3_rate_limit_storage_path_desc'] = 'Путь для file-драйвера. Пусто — sys_get_temp_dir().'; +$_lang['setting_ms3_rate_limit_redis_dsn'] = 'Redis DSN для rate limit'; +$_lang['setting_ms3_rate_limit_redis_dsn_desc'] = 'Полный DSN вида redis://:password@127.0.0.1:6379/0. Если задан, host/port/password/database ниже не используются. Env: MS3_RATE_LIMIT_REDIS_DSN. Требует ext-redis.'; +$_lang['setting_ms3_rate_limit_redis_host'] = 'Redis host (rate limit)'; +$_lang['setting_ms3_rate_limit_redis_host_desc'] = 'Хост Redis, если DSN не задан.'; +$_lang['setting_ms3_rate_limit_redis_port'] = 'Redis port (rate limit)'; +$_lang['setting_ms3_rate_limit_redis_port_desc'] = 'Порт Redis, если DSN не задан.'; +$_lang['setting_ms3_rate_limit_redis_password'] = 'Redis password (rate limit)'; +$_lang['setting_ms3_rate_limit_redis_password_desc'] = 'Пароль Redis, если DSN не задан.'; +$_lang['setting_ms3_rate_limit_redis_database'] = 'Redis database (rate limit)'; +$_lang['setting_ms3_rate_limit_redis_database_desc'] = 'Номер БД Redis (0 по умолчанию), если DSN не задан.'; +$_lang['setting_ms3_rate_limit_memcached_servers'] = 'Memcached servers (rate limit)'; +$_lang['setting_ms3_rate_limit_memcached_servers_desc'] = 'Список серверов host:port через запятую для memcached-драйвера. Env: MS3_RATE_LIMIT_MEMCACHED_SERVERS. Требует ext-memcached.'; // Notifications $_lang['setting_ms3_telegram_bot_token'] = 'Токен Telegram бота'; diff --git a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php index b603dfd3..75cb566a 100644 --- a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php +++ b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php @@ -5,14 +5,14 @@ use MiniShop3\Router\Middleware\MiddlewareInterface; use MiniShop3\Router\HttpStatus; use MiniShop3\Router\Response; +use MiniShop3\Services\RateLimit\FileRateLimitStore; +use MiniShop3\Services\RateLimit\RateLimitStoreInterface; /** * Middleware for request rate limiting * * Protection against DDoS attacks and API abuse. - * Uses simple file-based cache mechanism. - * - * TODO: In production it's recommended to use Redis/Memcached for rate limiting + * Storage backend is pluggable (file by default, optional Redis/Memcached). */ class RateLimitMiddleware implements MiddlewareInterface { @@ -22,22 +22,21 @@ class RateLimitMiddleware implements MiddlewareInterface /** @var int Time period in seconds */ private int $decaySeconds; - /** @var string Path to directory for storing rate limit data */ - private string $storagePath; + private RateLimitStoreInterface $store; /** * @param int $maxAttempts Maximum number of requests (default 60) * @param int $decaySeconds Time period in seconds (default 60 - 1 minute) - * @param string $storagePath Path to storage directory (default sys_get_temp_dir()) + * @param RateLimitStoreInterface|null $store Storage backend (default: file in sys_get_temp_dir()) */ public function __construct( int $maxAttempts = 60, int $decaySeconds = 60, - string $storagePath = '' + ?RateLimitStoreInterface $store = null, ) { $this->maxAttempts = $maxAttempts; $this->decaySeconds = $decaySeconds; - $this->storagePath = !empty($storagePath) ? $storagePath : sys_get_temp_dir(); + $this->store = $store ?? new FileRateLimitStore(sys_get_temp_dir(), $decaySeconds); } /** @@ -50,131 +49,42 @@ public function handle(array $params) { $key = $this->resolveRequestKey(); - $attempts = $this->getAttempts($key); - $resetTime = $this->getResetTime($key); + $state = $this->store->read($key); + $attempts = $state['attempts']; + $resetTime = $state['reset_at']; - // If time expired, reset counter if (time() >= $resetTime) { - $this->resetAttempts($key); + $this->store->reset($key); $attempts = 0; + $resetTime = time() + $this->decaySeconds; } - // Check limit if ($attempts >= $this->maxAttempts) { - $retryAfter = $resetTime - time(); + $retryAfter = max(0, $resetTime - time()); header("Retry-After: $retryAfter"); + return Response::error('ms3_err_rate_limit', HttpStatus::TOO_MANY_REQUESTS); } - // Increment counter - $this->incrementAttempts($key); + $state = $this->store->increment($key, $this->decaySeconds); + $this->setRateLimitHeaders($state['attempts'], $state['reset_at']); - // Set rate limit headers - $this->setRateLimitHeaders($attempts + 1, $resetTime); - - return null; // Continue execution + return null; } /** * Get key for client identification - * - * @return string */ private function resolveRequestKey(): string { - // Use combination of IP and token (if available) $ip = $_SERVER['REMOTE_ADDR'] ?? 'unknown'; $token = $_SERVER['HTTP_MS3TOKEN'] ?? ''; return 'rate_limit:' . md5($ip . ':' . $token); } - /** - * Get number of attempts - * - * @param string $key Key - * @return int - */ - private function getAttempts(string $key): int - { - $file = $this->getFilePath($key, 'attempts'); - - if (!file_exists($file)) { - return 0; - } - - $content = file_get_contents($file); - return (int)$content; - } - - /** - * Get counter reset time - * - * @param string $key Key - * @return int Unix timestamp - */ - private function getResetTime(string $key): int - { - $file = $this->getFilePath($key, 'reset'); - - if (!file_exists($file)) { - return time() + $this->decaySeconds; - } - - $content = file_get_contents($file); - return (int)$content; - } - - /** - * Increment attempts counter - * - * @param string $key Key - * @return void - */ - private function incrementAttempts(string $key): void - { - $attempts = $this->getAttempts($key) + 1; - $resetTime = $this->getResetTime($key); - - // If this is first attempt in period, set reset time - if ($attempts === 1) { - $resetTime = time() + $this->decaySeconds; - } - - file_put_contents($this->getFilePath($key, 'attempts'), $attempts); - file_put_contents($this->getFilePath($key, 'reset'), $resetTime); - } - - /** - * Reset attempts counter - * - * @param string $key Key - * @return void - */ - private function resetAttempts(string $key): void - { - @unlink($this->getFilePath($key, 'attempts')); - @unlink($this->getFilePath($key, 'reset')); - } - - /** - * Get file path for data storage - * - * @param string $key Key - * @param string $type Data type (attempts or reset) - * @return string - */ - private function getFilePath(string $key, string $type): string - { - return $this->storagePath . '/' . $key . '_' . $type . '.tmp'; - } - /** * Set rate limit headers - * - * @param int $attempts Current number of attempts - * @param int $resetTime Counter reset time - * @return void */ private function setRateLimitHeaders(int $attempts, int $resetTime): void { diff --git a/core/components/minishop3/src/Services/RateLimit/FileRateLimitStore.php b/core/components/minishop3/src/Services/RateLimit/FileRateLimitStore.php new file mode 100644 index 00000000..f8fb2cbe --- /dev/null +++ b/core/components/minishop3/src/Services/RateLimit/FileRateLimitStore.php @@ -0,0 +1,90 @@ +storagePath === '') { + $this->storagePath = sys_get_temp_dir(); + } + } + + public function read(string $key): array + { + return [ + 'attempts' => $this->readAttempts($key), + 'reset_at' => $this->readResetTime($key), + ]; + } + + public function increment(string $key, int $windowSeconds): array + { + $attempts = $this->readAttempts($key) + 1; + $resetAt = $this->readResetTime($key); + + if ($attempts === 1) { + $resetAt = time() + $windowSeconds; + } + + $this->writeAttempts($key, $attempts); + $this->writeResetTime($key, $resetAt); + + return [ + 'attempts' => $attempts, + 'reset_at' => $resetAt, + ]; + } + + public function reset(string $key): void + { + @unlink($this->filePath($key, 'attempts')); + @unlink($this->filePath($key, 'reset')); + } + + private function readAttempts(string $key): int + { + $file = $this->filePath($key, 'attempts'); + if (!is_file($file)) { + return 0; + } + + return (int) file_get_contents($file); + } + + private function readResetTime(string $key): int + { + $file = $this->filePath($key, 'reset'); + if (!is_file($file)) { + return time() + $this->defaultWindowSeconds; + } + + return (int) file_get_contents($file); + } + + private function writeAttempts(string $key, int $attempts): void + { + file_put_contents($this->filePath($key, 'attempts'), (string) $attempts); + } + + private function writeResetTime(string $key, int $resetAt): void + { + file_put_contents($this->filePath($key, 'reset'), (string) $resetAt); + } + + private function filePath(string $key, string $type): string + { + return rtrim($this->storagePath, DIRECTORY_SEPARATOR) + . DIRECTORY_SEPARATOR + . $key + . '_' + . $type + . '.tmp'; + } +} diff --git a/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php b/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php new file mode 100644 index 00000000..fad5fef0 --- /dev/null +++ b/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php @@ -0,0 +1,71 @@ +memcached->get($this->attemptsKey($key)); + $resetAt = (int) $this->memcached->get($this->resetKey($key)); + + if ($resetAt <= 0) { + $resetAt = time() + $this->defaultWindowSeconds; + } + + return [ + 'attempts' => $attempts, + 'reset_at' => $resetAt, + ]; + } + + public function increment(string $key, int $windowSeconds): array + { + $attemptsKey = $this->attemptsKey($key); + $resetKey = $this->resetKey($key); + + $attempts = $this->memcached->increment($attemptsKey, 1); + if ($attempts === false) { + $this->memcached->set($attemptsKey, 1, $windowSeconds); + $attempts = 1; + } + + $resetAt = (int) $this->memcached->get($resetKey); + if ($attempts === 1 || $resetAt <= 0) { + $resetAt = time() + $windowSeconds; + $this->memcached->set($resetKey, $resetAt, $windowSeconds); + } + + return [ + 'attempts' => $attempts, + 'reset_at' => $resetAt, + ]; + } + + public function reset(string $key): void + { + $this->memcached->delete($this->attemptsKey($key)); + $this->memcached->delete($this->resetKey($key)); + } + + private function attemptsKey(string $key): string + { + return self::KEY_PREFIX . hash('sha256', $key) . '_attempts'; + } + + private function resetKey(string $key): string + { + return self::KEY_PREFIX . hash('sha256', $key) . '_reset'; + } +} diff --git a/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php new file mode 100644 index 00000000..851e322a --- /dev/null +++ b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php @@ -0,0 +1,189 @@ +getOption('ms3_rate_limit_decay_seconds', null, 60); + $driver = self::resolveDriver($modx); + + return match ($driver) { + 'redis' => self::createRedis($modx, $windowSeconds), + 'memcached' => self::createMemcached($modx, $windowSeconds), + default => self::createFile($modx, $windowSeconds), + }; + } + + private static function resolveDriver(modX $modx): string + { + $env = getenv('MS3_RATE_LIMIT_STORE'); + $driver = is_string($env) && $env !== '' + ? $env + : (string) $modx->getOption('ms3_rate_limit_store', null, 'file'); + + return strtolower(trim($driver)); + } + + private static function createFile(modX $modx, int $windowSeconds): FileRateLimitStore + { + $path = (string) $modx->getOption('ms3_rate_limit_storage_path', null, ''); + if ($path === '') { + $path = sys_get_temp_dir(); + } + + return new FileRateLimitStore($path, $windowSeconds); + } + + private static function createRedis(modX $modx, int $windowSeconds): RateLimitStoreInterface + { + if (!class_exists(\Redis::class)) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] ext-redis is not available; falling back to file storage.' + ); + + return self::createFile($modx, $windowSeconds); + } + + $dsn = self::resolveRedisDsn($modx); + $redis = new \Redis(); + + if (!self::connectRedis($redis, $dsn, $modx)) { + return self::createFile($modx, $windowSeconds); + } + + return new RedisRateLimitStore($redis, $windowSeconds); + } + + private static function createMemcached(modX $modx, int $windowSeconds): RateLimitStoreInterface + { + if (!class_exists(\Memcached::class)) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] ext-memcached is not available; falling back to file storage.' + ); + + return self::createFile($modx, $windowSeconds); + } + + $memcached = new \Memcached(); + $servers = self::resolveMemcachedServers($modx); + if ($servers === []) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] ms3_rate_limit_memcached_servers is empty; falling back to file storage.' + ); + + return self::createFile($modx, $windowSeconds); + } + + if ($memcached->addServers($servers) === false) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] Memcached addServers failed; falling back to file storage.' + ); + + return self::createFile($modx, $windowSeconds); + } + + return new MemcachedRateLimitStore($memcached, $windowSeconds); + } + + /** + * @return array + */ + private static function resolveMemcachedServers(modX $modx): array + { + $env = getenv('MS3_RATE_LIMIT_MEMCACHED_SERVERS'); + $raw = is_string($env) && $env !== '' + ? $env + : (string) $modx->getOption('ms3_rate_limit_memcached_servers', null, '127.0.0.1:11211'); + + $servers = []; + foreach (array_filter(array_map('trim', explode(',', $raw))) as $chunk) { + [$host, $port] = array_pad(explode(':', $chunk, 2), 2, '11211'); + if ($host === '') { + continue; + } + $servers[] = [ + 'host' => $host, + 'port' => (int) $port, + 'weight' => 0, + ]; + } + + return $servers; + } + + private static function resolveRedisDsn(modX $modx): string + { + $env = getenv('MS3_RATE_LIMIT_REDIS_DSN'); + if (is_string($env) && $env !== '') { + return $env; + } + + $dsn = trim((string) $modx->getOption('ms3_rate_limit_redis_dsn', null, '')); + if ($dsn !== '') { + return $dsn; + } + + $host = (string) $modx->getOption('ms3_rate_limit_redis_host', null, '127.0.0.1'); + $port = (int) $modx->getOption('ms3_rate_limit_redis_port', null, 6379); + $password = (string) $modx->getOption('ms3_rate_limit_redis_password', null, ''); + $database = (int) $modx->getOption('ms3_rate_limit_redis_database', null, 0); + + if ($password !== '') { + return sprintf('redis://:%s@%s:%d/%d', rawurlencode($password), $host, $port, $database); + } + + return sprintf('redis://%s:%d/%d', $host, $port, $database); + } + + private static function connectRedis(\Redis $redis, string $dsn, modX $modx): bool + { + $parts = parse_url($dsn); + if ($parts === false || !isset($parts['host'])) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] Invalid Redis DSN; falling back to file storage.' + ); + + return false; + } + + $host = $parts['host']; + $port = isset($parts['port']) ? (int) $parts['port'] : 6379; + $timeout = 1.0; + + try { + if (!$redis->connect($host, $port, $timeout)) { + throw new \RuntimeException('connect failed'); + } + + if (isset($parts['pass']) && $parts['pass'] !== '') { + $redis->auth(rawurldecode((string) $parts['pass'])); + } + + if (isset($parts['path']) && $parts['path'] !== '' && $parts['path'] !== '/') { + $db = (int) ltrim($parts['path'], '/'); + $redis->select($db); + } + } catch (\Throwable $e) { + $modx->log( + modX::LOG_LEVEL_WARN, + '[RateLimitStoreFactory] Redis connection failed: ' . $e->getMessage() + ); + + return false; + } + + return true; + } +} diff --git a/core/components/minishop3/src/Services/RateLimit/RateLimitStoreInterface.php b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreInterface.php new file mode 100644 index 00000000..e57d1680 --- /dev/null +++ b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreInterface.php @@ -0,0 +1,28 @@ +redis->get($this->attemptsKey($key)); + $resetAt = (int) $this->redis->get($this->resetKey($key)); + + if ($resetAt <= 0) { + $resetAt = time() + $this->defaultWindowSeconds; + } + + return [ + 'attempts' => $attempts, + 'reset_at' => $resetAt, + ]; + } + + public function increment(string $key, int $windowSeconds): array + { + $attemptsKey = $this->attemptsKey($key); + $resetKey = $this->resetKey($key); + + $attempts = (int) $this->redis->incr($attemptsKey); + $resetAt = (int) $this->redis->get($resetKey); + + if ($attempts === 1 || $resetAt <= 0) { + $resetAt = time() + $windowSeconds; + $this->redis->setex($resetKey, $windowSeconds, (string) $resetAt); + $this->redis->expire($attemptsKey, $windowSeconds); + } + + return [ + 'attempts' => $attempts, + 'reset_at' => $resetAt, + ]; + } + + public function reset(string $key): void + { + $this->redis->del($this->attemptsKey($key), $this->resetKey($key)); + } + + private function attemptsKey(string $key): string + { + return self::KEY_PREFIX . hash('sha256', $key) . ':attempts'; + } + + private function resetKey(string $key): string + { + return self::KEY_PREFIX . hash('sha256', $key) . ':reset'; + } +} diff --git a/core/components/minishop3/tests/Unit/Middleware/RateLimitMiddlewareTest.php b/core/components/minishop3/tests/Unit/Middleware/RateLimitMiddlewareTest.php new file mode 100644 index 00000000..ca41f147 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Middleware/RateLimitMiddlewareTest.php @@ -0,0 +1,57 @@ +storagePath = sys_get_temp_dir() . '/ms3_rl_mw_' . bin2hex(random_bytes(8)); + mkdir($this->storagePath, 0777, true); + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; + unset($_SERVER['HTTP_MS3TOKEN']); + } + + protected function tearDown(): void + { + foreach (glob($this->storagePath . '/*') ?: [] as $file) { + if (is_file($file)) { + unlink($file); + } + } + if (is_dir($this->storagePath)) { + rmdir($this->storagePath); + } + } + + public function testAllowsRequestsUnderLimit(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + $middleware = new RateLimitMiddleware(2, 60, $store); + + self::assertNull($middleware->handle([])); + self::assertSame(1, $store->read('rate_limit:' . md5('127.0.0.1:'))['attempts']); + } + + public function testReturns429WhenLimitExceeded(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + $middleware = new RateLimitMiddleware(1, 60, $store); + + self::assertNull($middleware->handle([])); + + $response = $middleware->handle([]); + self::assertInstanceOf(Response::class, $response); + self::assertSame(HttpStatus::TOO_MANY_REQUESTS, $response->getStatusCode()); + } +} diff --git a/core/components/minishop3/tests/Unit/Services/RateLimit/FileRateLimitStoreTest.php b/core/components/minishop3/tests/Unit/Services/RateLimit/FileRateLimitStoreTest.php new file mode 100644 index 00000000..d4c4b8f5 --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/RateLimit/FileRateLimitStoreTest.php @@ -0,0 +1,66 @@ +storagePath = sys_get_temp_dir() . '/ms3_rl_test_' . bin2hex(random_bytes(8)); + mkdir($this->storagePath, 0777, true); + } + + protected function tearDown(): void + { + foreach (glob($this->storagePath . '/*') ?: [] as $file) { + if (is_file($file)) { + unlink($file); + } + } + if (is_dir($this->storagePath)) { + rmdir($this->storagePath); + } + } + + public function testReadReturnsZeroAttemptsForUnknownKey(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + $state = $store->read('client-a'); + + self::assertSame(0, $state['attempts']); + self::assertGreaterThan(time(), $state['reset_at']); + } + + public function testIncrementPersistsAttemptsAndResetTime(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + + $first = $store->increment('client-a', 60); + $second = $store->increment('client-a', 60); + + self::assertSame(1, $first['attempts']); + self::assertSame(2, $second['attempts']); + self::assertSame($first['reset_at'], $second['reset_at']); + + $read = $store->read('client-a'); + self::assertSame(2, $read['attempts']); + self::assertSame($first['reset_at'], $read['reset_at']); + } + + public function testResetClearsCounter(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + $store->increment('client-a', 60); + $store->reset('client-a'); + + $state = $store->read('client-a'); + self::assertSame(0, $state['attempts']); + } +} From 54d0267a2e98c237f1b99ffbeb8883f2b810dead Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 22:53:01 +0600 Subject: [PATCH 2/3] fix(rate-limit): increment before limit check for shared stores Count the current request atomically before comparing to maxAttempts, and initialize Memcached counters with add() to avoid init races. --- .../src/Middleware/RateLimitMiddleware.php | 13 +++++-------- .../Services/RateLimit/MemcachedRateLimitStore.php | 10 ++++++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php index 75cb566a..b3e7ef45 100644 --- a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php +++ b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php @@ -50,23 +50,20 @@ public function handle(array $params) $key = $this->resolveRequestKey(); $state = $this->store->read($key); - $attempts = $state['attempts']; - $resetTime = $state['reset_at']; - if (time() >= $resetTime) { + if (time() >= $state['reset_at']) { $this->store->reset($key); - $attempts = 0; - $resetTime = time() + $this->decaySeconds; } - if ($attempts >= $this->maxAttempts) { - $retryAfter = max(0, $resetTime - time()); + $state = $this->store->increment($key, $this->decaySeconds); + + if ($state['attempts'] > $this->maxAttempts) { + $retryAfter = max(0, $state['reset_at'] - time()); header("Retry-After: $retryAfter"); return Response::error('ms3_err_rate_limit', HttpStatus::TOO_MANY_REQUESTS); } - $state = $this->store->increment($key, $this->decaySeconds); $this->setRateLimitHeaders($state['attempts'], $state['reset_at']); return null; diff --git a/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php b/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php index fad5fef0..ecac6677 100644 --- a/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php +++ b/core/components/minishop3/src/Services/RateLimit/MemcachedRateLimitStore.php @@ -37,8 +37,14 @@ public function increment(string $key, int $windowSeconds): array $attempts = $this->memcached->increment($attemptsKey, 1); if ($attempts === false) { - $this->memcached->set($attemptsKey, 1, $windowSeconds); - $attempts = 1; + if ($this->memcached->add($attemptsKey, 1, $windowSeconds)) { + $attempts = 1; + } else { + $attempts = $this->memcached->increment($attemptsKey, 1); + if ($attempts === false) { + $attempts = 1; + } + } } $resetAt = (int) $this->memcached->get($resetKey); From e1d7a9386e67e3625a6eb03b02872b6c2d0f14c5 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 23:00:32 +0600 Subject: [PATCH 3/3] fix(rate-limit): atomic Redis increment via Lua + DRY fallback RedisRateLimitStore now uses a single bucket key with a TTL window and an atomic Lua script (INCR + EXPIRE in one round-trip), removing the TOCTOU between the counter and a separate reset key. RateLimitStoreFactory routes shared-store fallbacks through a single fallbackToFile() helper. --- .../RateLimit/RateLimitStoreFactory.php | 19 +++-- .../RateLimit/RedisRateLimitStore.php | 79 +++++++++++++------ 2 files changed, 71 insertions(+), 27 deletions(-) diff --git a/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php index 851e322a..b45cf1f3 100644 --- a/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php +++ b/core/components/minishop3/src/Services/RateLimit/RateLimitStoreFactory.php @@ -49,14 +49,14 @@ private static function createRedis(modX $modx, int $windowSeconds): RateLimitSt '[RateLimitStoreFactory] ext-redis is not available; falling back to file storage.' ); - return self::createFile($modx, $windowSeconds); + return self::fallbackToFile($modx, $windowSeconds); } $dsn = self::resolveRedisDsn($modx); $redis = new \Redis(); if (!self::connectRedis($redis, $dsn, $modx)) { - return self::createFile($modx, $windowSeconds); + return self::fallbackToFile($modx, $windowSeconds); } return new RedisRateLimitStore($redis, $windowSeconds); @@ -70,7 +70,7 @@ private static function createMemcached(modX $modx, int $windowSeconds): RateLim '[RateLimitStoreFactory] ext-memcached is not available; falling back to file storage.' ); - return self::createFile($modx, $windowSeconds); + return self::fallbackToFile($modx, $windowSeconds); } $memcached = new \Memcached(); @@ -81,7 +81,7 @@ private static function createMemcached(modX $modx, int $windowSeconds): RateLim '[RateLimitStoreFactory] ms3_rate_limit_memcached_servers is empty; falling back to file storage.' ); - return self::createFile($modx, $windowSeconds); + return self::fallbackToFile($modx, $windowSeconds); } if ($memcached->addServers($servers) === false) { @@ -90,12 +90,21 @@ private static function createMemcached(modX $modx, int $windowSeconds): RateLim '[RateLimitStoreFactory] Memcached addServers failed; falling back to file storage.' ); - return self::createFile($modx, $windowSeconds); + return self::fallbackToFile($modx, $windowSeconds); } return new MemcachedRateLimitStore($memcached, $windowSeconds); } + /** + * Build the file-backed store, used as the default and as the fallback when + * a shared-store driver is unavailable or misconfigured. + */ + private static function fallbackToFile(modX $modx, int $windowSeconds): FileRateLimitStore + { + return self::createFile($modx, $windowSeconds); + } + /** * @return array */ diff --git a/core/components/minishop3/src/Services/RateLimit/RedisRateLimitStore.php b/core/components/minishop3/src/Services/RateLimit/RedisRateLimitStore.php index fddebcdf..62558725 100644 --- a/core/components/minishop3/src/Services/RateLimit/RedisRateLimitStore.php +++ b/core/components/minishop3/src/Services/RateLimit/RedisRateLimitStore.php @@ -4,11 +4,34 @@ /** * Redis-backed rate limit store for multi-node deployments (requires ext-redis). + * + * Uses a single key per rate-limit bucket with a TTL window. Increment and TTL + * (re)arm happen atomically via a Lua script, so there is no TOCTOU window + * between INCR and EXPIRE and no separate "reset" key that can drift out of + * sync with the counter. */ class RedisRateLimitStore implements RateLimitStoreInterface { private const KEY_PREFIX = 'ms3:rate_limit:'; + /** + * Atomically INCR the bucket key and arm its TTL on the first hit. + * + * Returns `{attempts, ttl}` where ttl is seconds remaining until expiry. + */ + private const INCREMENT_SCRIPT = <<<'LUA' +local attempts = redis.call('INCR', KEYS[1]) +if attempts == 1 then + redis.call('EXPIRE', KEYS[1], ARGV[1]) +end +local ttl = redis.call('TTL', KEYS[1]) +if ttl < 0 then + ttl = tonumber(ARGV[1]) + redis.call('EXPIRE', KEYS[1], ARGV[1]) +end +return {attempts, ttl} +LUA; + public function __construct( private \Redis $redis, private int $defaultWindowSeconds = 60, @@ -17,51 +40,63 @@ public function __construct( public function read(string $key): array { - $attempts = (int) $this->redis->get($this->attemptsKey($key)); - $resetAt = (int) $this->redis->get($this->resetKey($key)); + $bucketKey = $this->bucketKey($key); + $attempts = (int) $this->redis->get($bucketKey); + $ttl = (int) $this->redis->ttl($bucketKey); + + if ($attempts <= 0) { + return [ + 'attempts' => 0, + 'reset_at' => time() + $this->defaultWindowSeconds, + ]; + } - if ($resetAt <= 0) { - $resetAt = time() + $this->defaultWindowSeconds; + if ($ttl < 0) { + $ttl = $this->defaultWindowSeconds; } return [ 'attempts' => $attempts, - 'reset_at' => $resetAt, + 'reset_at' => time() + $ttl, ]; } public function increment(string $key, int $windowSeconds): array { - $attemptsKey = $this->attemptsKey($key); - $resetKey = $this->resetKey($key); + $result = $this->redis->rawCommand( + 'EVAL', + self::INCREMENT_SCRIPT, + 1, + $this->bucketKey($key), + $windowSeconds + ); - $attempts = (int) $this->redis->incr($attemptsKey); - $resetAt = (int) $this->redis->get($resetKey); + if (!is_array($result)) { + return [ + 'attempts' => 1, + 'reset_at' => time() + $windowSeconds, + ]; + } - if ($attempts === 1 || $resetAt <= 0) { - $resetAt = time() + $windowSeconds; - $this->redis->setex($resetKey, $windowSeconds, (string) $resetAt); - $this->redis->expire($attemptsKey, $windowSeconds); + $attempts = (int) $result[0]; + $ttl = (int) $result[1]; + if ($ttl < 0) { + $ttl = $windowSeconds; } return [ 'attempts' => $attempts, - 'reset_at' => $resetAt, + 'reset_at' => time() + $ttl, ]; } public function reset(string $key): void { - $this->redis->del($this->attemptsKey($key), $this->resetKey($key)); - } - - private function attemptsKey(string $key): string - { - return self::KEY_PREFIX . hash('sha256', $key) . ':attempts'; + $this->redis->del($this->bucketKey($key)); } - private function resetKey(string $key): string + private function bucketKey(string $key): string { - return self::KEY_PREFIX . hash('sha256', $key) . ':reset'; + return self::KEY_PREFIX . hash('sha256', $key); } }