fix(web-api): hardening rate-limit, CORS wildcards, query tokens

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
      *

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);
     }
 
     /**

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
      *

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[]
      */

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);

core/components/minishop3/tests/TokenMiddlewareQueryTokenTest.php

@@ -0,0 +1,55 @@
+<?php
+
+/**
+ * #576: API session token must not be accepted from the query string.
+ *
+ * Run: php tests/TokenMiddlewareQueryTokenTest.php
+ */
+
+declare(strict_types=1);
+
+$fail = static function (string $message): never {
+    fwrite(STDERR, "FAIL: {$message}\n");
+    exit(1);
+};
+
+$src = file_get_contents(__DIR__ . '/../src/Middleware/TokenMiddleware.php');
+if ($src === false) {
+    $fail('unable to read TokenMiddleware.php');
+}
+
+if (!str_contains($src, 'stripQueryStringApiTokens')) {
+    $fail('TokenMiddleware must strip query-string API tokens');
+}
+
+if (!preg_match('/function resolveToken\(\): string\s*\{([\s\S]*?)\n    \}/', $src, $resolveBody)) {
+    $fail('resolveToken() body not found');
+}
+
+$resolve = $resolveBody[1];
+
+if (!str_contains($resolve, 'HTTP_AUTHORIZATION') || !str_contains($resolve, 'Bearer ')) {
+    $fail('resolveToken must accept Authorization Bearer');
+}
+if (!str_contains($resolve, 'HTTP_MS3TOKEN')) {
+    $fail('resolveToken must accept HTTP_MS3TOKEN');
+}
+if (!str_contains($resolve, 'CookieHelper::getTokenFromCookie()')) {
+    $fail('resolveToken must accept cookie via CookieHelper');
+}
+if (!str_contains($resolve, "\$_SESSION['ms3']['customer_token']")) {
+    $fail('resolveToken must fall back to session cache');
+}
+if (str_contains($resolve, '$_REQUEST') || str_contains($resolve, '$_GET')) {
+    $fail('resolveToken must not read $_REQUEST/$_GET (query is not a credential source)');
+}
+if (str_contains($resolve, "['token']")) {
+    $fail('resolveToken must not read legacy token param');
+}
+
+if (!preg_match('/query[- ]string/i', $src)) {
+    $fail('TokenMiddleware docs must mention query-string rejection');
+}
+
+fwrite(STDOUT, "OK TokenMiddlewareQueryTokenTest\n");
+exit(0);

core/components/minishop3/tests/Unit/Middleware/RateLimitMiddlewareTest.php

@@ -40,7 +40,7 @@ public function testAllowsRequestsUnderLimit(): void
         $middleware = new RateLimitMiddleware(2, 60, $store);
 
         self::assertNull($middleware->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());
+    }
 }