diff --git a/core/components/minishop3/src/Middleware/CorsMiddleware.php b/core/components/minishop3/src/Middleware/CorsMiddleware.php index 5c1277ff..5df2110a 100644 --- a/core/components/minishop3/src/Middleware/CorsMiddleware.php +++ b/core/components/minishop3/src/Middleware/CorsMiddleware.php @@ -52,8 +52,7 @@ public function handle(array $params) { $origin = $_SERVER['HTTP_ORIGIN'] ?? ''; - // Check if origin is allowed - if ($this->isOriginAllowed($origin)) { + if (CorsConfig::isOriginAllowed($origin, $this->allowedOrigins)) { $this->setCorsHeaders($origin); } @@ -67,45 +66,6 @@ public function handle(array $params) return null; // Continue execution } - /** - * Check if origin is allowed - * - * @param string $origin Origin from header - * @return bool - */ - private function isOriginAllowed(string $origin): bool - { - if (empty($origin)) { - return false; - } - - if ($this->allowedOrigins === []) { - return false; - } - - // If all origins are allowed (credentials must be off — normalized in constructor) - if (CorsConfig::hasWildcardOrigin($this->allowedOrigins)) { - return true; - } - - // Check exact match - if (in_array($origin, $this->allowedOrigins, true)) { - return true; - } - - // Check wildcard patterns (e.g.: *.example.com) - foreach ($this->allowedOrigins as $allowedOrigin) { - if (str_contains($allowedOrigin, '*')) { - $pattern = str_replace('*', '.*', $allowedOrigin); - if (preg_match('#^' . $pattern . '$#', $origin)) { - return true; - } - } - } - - return false; - } - /** * Set CORS headers * diff --git a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php index b3e7ef45..6f99c83c 100644 --- a/core/components/minishop3/src/Middleware/RateLimitMiddleware.php +++ b/core/components/minishop3/src/Middleware/RateLimitMiddleware.php @@ -70,14 +70,14 @@ public function handle(array $params) } /** - * Get key for client identification + * Client bucket key: IP only (#576). + * Do not include client-controlled headers (e.g. MS3TOKEN) — that bypasses the limit. */ private function resolveRequestKey(): string { $ip = $_SERVER['REMOTE_ADDR'] ?? 'unknown'; - $token = $_SERVER['HTTP_MS3TOKEN'] ?? ''; - return 'rate_limit:' . md5($ip . ':' . $token); + return 'rate_limit:' . md5($ip); } /** diff --git a/core/components/minishop3/src/Middleware/TokenMiddleware.php b/core/components/minishop3/src/Middleware/TokenMiddleware.php index 17fb82da..3d7902cd 100644 --- a/core/components/minishop3/src/Middleware/TokenMiddleware.php +++ b/core/components/minishop3/src/Middleware/TokenMiddleware.php @@ -16,8 +16,10 @@ * Token resolution order: * 1. Authorization: Bearer header (mobile apps) * 2. HTTP_MS3TOKEN header (legacy) - * 3. $_REQUEST['ms3_token'] (includes httpOnly cookie via injection) + * 3. httpOnly cookie `ms3_token` (injected into $_REQUEST for controllers) + * 4. Session cache (still DB-validated) * + * Query-string `ms3_token` is stripped and never accepted as API credentials (#576). * Cookie injection at start of handle() copies $_COOKIE['ms3_token'] → $_REQUEST['ms3_token'] * for backward compatibility with controllers reading $_REQUEST. * @@ -49,21 +51,13 @@ public function __construct(modX $modx) } /** - * Handle request - * - * Token resolution order: - * 1. Authorization: Bearer header (for mobile apps) - * 2. HTTP_MS3TOKEN header (legacy) - * 3. $_REQUEST['ms3_token'] (includes httpOnly cookie via injection + legacy URL param) - * - * Cookie injection: copies $_COOKIE['ms3_token'] → $_REQUEST['ms3_token'] - * so all controllers (CartController, OrderController, etc.) work without changes. - * * @param array $params URL parameters from router * @return Response|null Return Response to stop execution, or null to continue */ public function handle(array $params) { + $this->stripQueryStringApiTokens(); + // Cookie injection: make cookie token available via $_REQUEST for backward compat $cookieToken = CookieHelper::getTokenFromCookie(); if (!empty($cookieToken) && empty($_REQUEST['ms3_token'])) { @@ -163,38 +157,46 @@ private function clearClientTokenState(): void } /** - * Resolve token from request sources + * Remove API session token from the query string so controllers cannot pick it up (#576). + * Email verification uses `?token=` on a route without this middleware. + */ + private function stripQueryStringApiTokens(): void + { + if (!array_key_exists('ms3_token', $_GET)) { + return; + } + + unset($_GET['ms3_token'], $_REQUEST['ms3_token']); + } + + /** + * Resolve token from trusted sources only (Bearer, MS3TOKEN header, cookie, session). * * @return string Token or empty string */ private function resolveToken(): string { - // 1. Authorization: Bearer header (for mobile apps) $authHeader = $_SERVER['HTTP_AUTHORIZATION'] ?? ''; if (str_starts_with($authHeader, 'Bearer ')) { $token = substr($authHeader, 7); - if (!empty($token)) { + if ($token !== '') { return $token; } } - // 2. HTTP_MS3TOKEN header (legacy) - $token = $_SERVER['HTTP_MS3TOKEN'] ?? ''; - if (!empty($token)) { - return $token; + $headerToken = $_SERVER['HTTP_MS3TOKEN'] ?? ''; + if ($headerToken !== '') { + return $headerToken; } - // 3. $_REQUEST (includes cookie via injection + legacy URL param) - $token = $_REQUEST['ms3_token'] ?? $_REQUEST['token'] ?? ''; - if (!empty($token)) { - return $token; + $cookieToken = CookieHelper::getTokenFromCookie(); + if ($cookieToken !== '') { + return $cookieToken; } - // 4. Session cache (must still pass DB validation in handle()) - return $_SESSION['ms3']['customer_token'] ?? ''; + return (string) ($_SESSION['ms3']['customer_token'] ?? ''); } - /** * Check if route is public * diff --git a/core/components/minishop3/src/Utils/CorsConfig.php b/core/components/minishop3/src/Utils/CorsConfig.php index e666d676..30fb65de 100644 --- a/core/components/minishop3/src/Utils/CorsConfig.php +++ b/core/components/minishop3/src/Utils/CorsConfig.php @@ -54,6 +54,54 @@ public static function hasWildcardOrigin(array $origins): bool return in_array('*', $origins, true); } + /** + * Whether $origin is allowed by an exact entry or a single-label host wildcard (#576). + * + * Pattern `https://*.example.com` matches `https://shop.example.com` and rejects + * `https://shop.exampleXcom` (dots are literal after preg_quote). + */ + public static function isOriginAllowed(string $origin, array $allowedOrigins): bool + { + if ($origin === '' || $allowedOrigins === []) { + return false; + } + + if (self::hasWildcardOrigin($allowedOrigins)) { + return true; + } + + if (in_array($origin, $allowedOrigins, true)) { + return true; + } + + foreach ($allowedOrigins as $allowedOrigin) { + if ( + is_string($allowedOrigin) + && str_contains($allowedOrigin, '*') + && self::originMatchesWildcardPattern($origin, $allowedOrigin) + ) { + return true; + } + } + + return false; + } + + /** + * Match origin against a pattern where `*` is one DNS label (`[^.]+`), not `.*`. + */ + public static function originMatchesWildcardPattern(string $origin, string $pattern): bool + { + if ($origin === '' || $pattern === '' || !str_contains($pattern, '*')) { + return false; + } + + $quoted = preg_quote($pattern, '#'); + $regex = str_replace('\*', '[^.]+', $quoted); + + return preg_match('#^' . $regex . '$#', $origin) === 1; + } + /** * @return string[] */ diff --git a/core/components/minishop3/tests/CorsConfigNormalizeTest.php b/core/components/minishop3/tests/CorsConfigNormalizeTest.php index e411a42b..a42db30a 100644 --- a/core/components/minishop3/tests/CorsConfigNormalizeTest.php +++ b/core/components/minishop3/tests/CorsConfigNormalizeTest.php @@ -80,5 +80,36 @@ $fail('web.php must not default ms3_cors_allowed_origins to *'); } +// #576: wildcard host patterns must not treat dots as "any char" +$pattern = 'https://*.example.com'; +if (!CorsConfig::originMatchesWildcardPattern('https://shop.example.com', $pattern)) { + $fail('https://*.example.com must allow https://shop.example.com'); +} +if (CorsConfig::originMatchesWildcardPattern('https://shop.exampleXcom', $pattern)) { + $fail('https://*.example.com must reject https://shop.exampleXcom'); +} +if (CorsConfig::originMatchesWildcardPattern('https://evil.example.com.attacker.tld', $pattern)) { + $fail('https://*.example.com must reject nested attacker tld'); +} +if (CorsConfig::originMatchesWildcardPattern('https://foo.bar.example.com', $pattern)) { + $fail('single-label * must reject multi-label subdomain'); +} +if (!CorsConfig::isOriginAllowed('https://shop.example.com', [$pattern])) { + $fail('isOriginAllowed must accept matching wildcard origin'); +} +if (CorsConfig::isOriginAllowed('https://shop.exampleXcom', [$pattern])) { + $fail('isOriginAllowed must reject spoofed wildcard origin'); +} +if (!CorsConfig::isOriginAllowed('https://exact.example.com', ['https://exact.example.com'])) { + $fail('exact origin allowlist must still work'); +} + +if (!str_contains($middleware, 'CorsConfig::isOriginAllowed')) { + $fail('CorsMiddleware must delegate origin match to CorsConfig::isOriginAllowed'); +} +if (str_contains($middleware, "str_replace('*', '.*'")) { + $fail('CorsMiddleware must not use unquoted .* wildcard replacement'); +} + fwrite(STDOUT, "OK CorsConfigNormalizeTest\n"); exit(0); diff --git a/core/components/minishop3/tests/TokenMiddlewareQueryTokenTest.php b/core/components/minishop3/tests/TokenMiddlewareQueryTokenTest.php new file mode 100644 index 00000000..6374f7e0 --- /dev/null +++ b/core/components/minishop3/tests/TokenMiddlewareQueryTokenTest.php @@ -0,0 +1,55 @@ +handle([])); - self::assertSame(1, $store->read('rate_limit:' . md5('127.0.0.1:'))['attempts']); + self::assertSame(1, $store->read('rate_limit:' . md5('127.0.0.1'))['attempts']); } public function testReturns429WhenLimitExceeded(): void @@ -54,4 +54,23 @@ public function testReturns429WhenLimitExceeded(): void self::assertInstanceOf(Response::class, $response); self::assertSame(HttpStatus::TOO_MANY_REQUESTS, $response->getStatusCode()); } + + public function testDifferentMs3TokenHeadersShareIpBucket(): void + { + $store = new FileRateLimitStore($this->storagePath, 60); + $middleware = new RateLimitMiddleware(2, 60, $store); + + $_SERVER['HTTP_MS3TOKEN'] = 'token-aaa'; + self::assertNull($middleware->handle([])); + + $_SERVER['HTTP_MS3TOKEN'] = 'token-bbb'; + self::assertNull($middleware->handle([])); + + $key = 'rate_limit:' . md5('127.0.0.1'); + self::assertSame(2, $store->read($key)['attempts']); + + $response = $middleware->handle([]); + self::assertInstanceOf(Response::class, $response); + self::assertSame(HttpStatus::TOO_MANY_REQUESTS, $response->getStatusCode()); + } }