diff --git a/system/HTTP/SiteURIFactory.php b/system/HTTP/SiteURIFactory.php index b1def3293a17..3533b8383c6a 100644 --- a/system/HTTP/SiteURIFactory.php +++ b/system/HTTP/SiteURIFactory.php @@ -20,7 +20,7 @@ /** * Creates SiteURI using superglobals. * - * This class also updates superglobal $_SERVER and $_GET. + * This class also updates superglobal $_REQUEST, $_SERVER, and $_GET. */ final readonly class SiteURIFactory { @@ -31,7 +31,7 @@ public function __construct(private App $appConfig, private Superglobals $superg /** * Create the current URI object from superglobals. * - * This method updates superglobal $_SERVER and $_GET. + * This method updates superglobals $_SERVER, $_GET, and $_REQUEST. */ public function createFromGlobals(): SiteURI { @@ -103,7 +103,7 @@ public function detectRoutePath(string $protocol = ''): string * Will parse the REQUEST_URI and automatically detect the URI from it, * fixing the query string if necessary. * - * This method updates superglobal $_SERVER and $_GET. + * This method updates superglobals $_SERVER, $_GET, and $_REQUEST. * * @return string The route path (before normalization). */ @@ -167,9 +167,11 @@ private function parseRequestURI(): string $this->superglobals->setServer('QUERY_STRING', $query); } - // Update our global GET for values likely to have been changed + // Refresh only the request values derived from superglobals after GET changes. + $previousRequestData = $this->superglobals->getRequestData(); parse_str($this->superglobals->server('QUERY_STRING'), $get); $this->superglobals->setGetArray($get); + $this->refreshRequestAfterGetChange($previousRequestData); return URI::removeDotSegments($path); } @@ -177,7 +179,7 @@ private function parseRequestURI(): string /** * Will parse QUERY_STRING and automatically detect the URI from it. * - * This method updates superglobal $_SERVER and $_GET. + * This method updates superglobals $_SERVER, $_GET, and $_REQUEST. * * @return string The route path (before normalization). */ @@ -199,9 +201,11 @@ private function parseQueryString(): string $path = $query; } - // Update our global GET for values likely to have been changed + // Refresh only the request values derived from superglobals after GET changes. + $previousRequestData = $this->superglobals->getRequestData(); parse_str($this->superglobals->server('QUERY_STRING'), $get); $this->superglobals->setGetArray($get); + $this->refreshRequestAfterGetChange($previousRequestData); return URI::removeDotSegments($path); } @@ -220,6 +224,38 @@ private function createURIFromRoutePath(string $routePath): SiteURI return new SiteURI($this->appConfig, $relativePath, $this->getHost()); } + /** + * Update REQUEST once during URI parsing, leaving application-defined values alone. + * + * @param array $previousRequestData + */ + private function refreshRequestAfterGetChange(array $previousRequestData): void + { + $request = $this->superglobals->getRequestArray(); + $currentData = $this->superglobals->getRequestData(); + + foreach ($previousRequestData as $key => $value) { + // A different value was supplied directly in REQUEST; preserve it. + if (! array_key_exists($key, $request) || $request[$key] !== $value) { + continue; + } + + if (array_key_exists($key, $currentData)) { + $request[$key] = $currentData[$key]; + } else { + unset($request[$key]); + } + } + + foreach ($currentData as $key => $value) { + if (! array_key_exists($key, $previousRequestData) && ! array_key_exists($key, $request)) { + $request[$key] = $value; + } + } + + $this->superglobals->setRequestArray($request); + } + /** * @return string|null The current hostname. Returns null if no valid host. */ diff --git a/system/Superglobals.php b/system/Superglobals.php index 461a41199104..7049d035785f 100644 --- a/system/Superglobals.php +++ b/system/Superglobals.php @@ -386,6 +386,51 @@ public function setRequestArray(array $array): self return $this; } + /** + * Returns the merged $_GET, $_POST, and $_COOKIE data according to the + * `request_order` (or `variables_order`) ini setting, without mutating + * $_REQUEST. + * + * PHP populates $_REQUEST only once at the start of the request. When + * $_GET is modified later (e.g. by SiteURIFactory), $_REQUEST becomes + * stale. This method returns the current merged values so callers can + * read up-to-date request data without relying on the stale $_REQUEST. + * + * @param string|null $requestOrder Overrides the `request_order` ini + * setting. Useful for testing, since the + * ini setting cannot be changed at runtime. + * + * @return array + */ + public function getRequestData(?string $requestOrder = null): array + { + $requestOrder ??= (string) ini_get('request_order'); + + if ($requestOrder === '') { + $requestOrder = (string) ini_get('variables_order'); + } + + if ($requestOrder === '') { + $requestOrder = 'GP'; + } + + $request = []; + + foreach (array_unique(str_split(strtoupper($requestOrder))) as $type) { + match ($type) { + // array_replace_recursive() matches PHP's own $_REQUEST merge + // (php_autoglobal_merge): numeric keys are preserved and + // array values are merged recursively. + 'G' => $request = array_replace_recursive($request, $this->get), + 'P' => $request = array_replace_recursive($request, $this->post), + 'C' => $request = array_replace_recursive($request, $this->cookie), + default => null, + }; + } + + return $request; + } + /** * Get all $_FILES values. * diff --git a/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php b/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php index 4bc29fbecc4d..a61f19a2fefe 100644 --- a/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php +++ b/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php @@ -258,6 +258,99 @@ public function testQueryStringWithQueryString(): void $this->assertSame(['code' => 'good'], $_GET); } + public function testQueryStringRefreshesRequestWithoutOverwritingCustomValues(): void + { + if (! $this->effectiveRequestOrderIncludesGet()) { + $this->markTestSkipped('The effective request order does not include GET.'); + } + + $superglobals = new Superglobals( + [ + 'REQUEST_URI' => '/index.php?/ci/woot?code=good', + 'QUERY_STRING' => '/ci/woot?code=good', + 'SCRIPT_NAME' => '/index.php', + ], + ['/ci/woot?code' => 'good', 'override' => 'from get'], + ['posted' => 'post'], + [], + [], + [ + '/ci/woot?code' => 'good', + 'override' => 'custom', + 'posted' => 'post', + 'unrelated' => 'keep', + ], + ); + Services::injectMock('superglobals', $superglobals); + + $factory = new SiteURIFactory(new App(), $superglobals); + + $this->assertSame('ci/woot', $factory->detectRoutePath('QUERY_STRING')); + $this->assertSame([ + 'override' => 'custom', + 'posted' => 'post', + 'unrelated' => 'keep', + 'code' => 'good', + ], $superglobals->getRequestArray()); + $this->assertSame($superglobals->getRequestArray(), $_REQUEST); + + $superglobals->setGet('code', 'later'); + + $this->assertSame('good', $superglobals->request('code')); + } + + public function testRequestURIRefreshesRequestForValidation(): void + { + if (! $this->effectiveRequestOrderIncludesGet()) { + $this->markTestSkipped('The effective request order does not include GET.'); + } + + $superglobals = new Superglobals( + [ + 'REQUEST_URI' => '/index.php/woot?code=good', + 'SCRIPT_NAME' => '/index.php', + ], + ['code' => 'stale'], + [], + [], + [], + ['code' => 'stale', 'unrelated' => 'keep'], + ); + Services::injectMock('superglobals', $superglobals); + + $config = new App(); + $factory = new SiteURIFactory($config, $superglobals); + + $this->assertSame('woot', $factory->detectRoutePath('REQUEST_URI')); + $this->assertSame(['code' => 'good', 'unrelated' => 'keep'], $superglobals->getRequestArray()); + + $request = new IncomingRequest($config, new SiteURI($config), null, new UserAgent()); + + $this->assertSame('good', $request->getVar('code')); + $this->assertTrue(service('validation')->withRequest($request)->setRules(['code' => 'in_list[good]'])->run()); + } + + /** + * Whether GET is part of the effective request order on this host. + * + * Mirrors Superglobals::getRequestData(): uses `request_order`, falls + * back to `variables_order`, then to PHP's default GP. + */ + private function effectiveRequestOrderIncludesGet(): bool + { + $order = (string) ini_get('request_order'); + + if ($order === '') { + $order = (string) ini_get('variables_order'); + } + + if ($order === '') { + $order = 'GP'; + } + + return str_contains(strtoupper($order), 'G'); + } + public function testQueryStringEmpty(): void { // /index.php? diff --git a/tests/system/SuperglobalsTest.php b/tests/system/SuperglobalsTest.php index e4b8b23b2b28..2aa24f5b64c5 100644 --- a/tests/system/SuperglobalsTest.php +++ b/tests/system/SuperglobalsTest.php @@ -302,6 +302,106 @@ public function testRequestSetArray(): void $this->assertSame($data, $_REQUEST); } + public function testGetRequestDataMergesGetAndPost(): void + { + $this->superglobals->setGetArray(['get_key' => 'get_value']); + $this->superglobals->setPostArray(['post_key' => 'post_value']); + + $data = $this->superglobals->getRequestData('GP'); + + $this->assertSame('get_value', $data['get_key']); + $this->assertSame('post_value', $data['post_key']); + } + + public function testGetRequestDataReflectsGetChanges(): void + { + $this->superglobals->setGetArray(['key' => 'old']); + + $this->assertSame('old', $this->superglobals->getRequestData('G')['key']); + + // Simulate SiteURIFactory updating $_GET after the request started. + $this->superglobals->setGetArray(['key' => 'new']); + + $this->assertSame('new', $this->superglobals->getRequestData('G')['key']); + } + + public function testGetRequestDataMergesCookie(): void + { + $this->superglobals->setGetArray(['get_key' => 'get_value']); + $this->superglobals->setPostArray(['post_key' => 'post_value']); + $this->superglobals->setCookieArray(['cookie_key' => 'cookie_value']); + + $data = $this->superglobals->getRequestData('GPC'); + + $this->assertSame('get_value', $data['get_key']); + $this->assertSame('post_value', $data['post_key']); + $this->assertSame('cookie_value', $data['cookie_key']); + } + + public function testGetRequestDataRespectsOrder(): void + { + $this->superglobals->setGetArray(['shared' => 'get']); + $this->superglobals->setPostArray(['shared' => 'post']); + $this->superglobals->setCookieArray(['shared' => 'cookie']); + + // Later sources overwrite earlier ones, matching PHP's request_order. + $this->assertSame('post', $this->superglobals->getRequestData('GP')['shared']); + $this->assertSame('cookie', $this->superglobals->getRequestData('GPC')['shared']); + $this->assertSame('get', $this->superglobals->getRequestData('PG')['shared']); + } + + public function testGetRequestDataEmptyOrderFallsBackToVariablesOrder(): void + { + $this->superglobals->setGetArray(['get' => 'value']); + $this->superglobals->setPostArray(['post' => 'value']); + $this->superglobals->setCookieArray(['cookie' => 'value']); + + $this->assertSame( + $this->superglobals->getRequestData((string) ini_get('variables_order')), + $this->superglobals->getRequestData(''), + ); + } + + public function testGetRequestDataNormalizesOrderAndIgnoresDuplicates(): void + { + $this->superglobals->setGetArray(['shared' => 'get']); + $this->superglobals->setPostArray(['shared' => 'post']); + $this->superglobals->setCookieArray(['shared' => 'cookie']); + + $this->assertSame('post', $this->superglobals->getRequestData('gp')['shared']); + $this->assertSame('post', $this->superglobals->getRequestData('GPG')['shared']); + $this->assertSame('cookie', $this->superglobals->getRequestData('gPcGpC')['shared']); + } + + public function testGetRequestDataIgnoresUnknownOrderTypes(): void + { + $this->superglobals->setGetArray(['get_key' => 'get_value']); + + $data = $this->superglobals->getRequestData('GX'); + + $this->assertSame(['get_key' => 'get_value'], $data); + } + + public function testGetRequestDataPreservesNumericKeys(): void + { + parse_str('100=foo', $get); + $this->superglobals->setGetArray($get); + + $data = $this->superglobals->getRequestData('G'); + + $this->assertSame([100 => 'foo'], $data); + } + + public function testGetRequestDataMergesRecursively(): void + { + $this->superglobals->setGetArray(['a' => ['x' => 'get']]); + $this->superglobals->setPostArray(['a' => ['y' => 'post']]); + + $data = $this->superglobals->getRequestData('GP'); + + $this->assertSame(['a' => ['x' => 'get', 'y' => 'post']], $data); + } + // $_FILES tests public function testFilesGetArray(): void { diff --git a/user_guide_src/source/changelogs/v4.7.6.rst b/user_guide_src/source/changelogs/v4.7.6.rst index 8739e9546d4c..dc63b43dc354 100644 --- a/user_guide_src/source/changelogs/v4.7.6.rst +++ b/user_guide_src/source/changelogs/v4.7.6.rst @@ -38,13 +38,14 @@ Bugs Fixed is used instead. Values that cannot be converted to a string, such as objects not implementing ``Stringable``, now throw a ``FormatException`` instead of a fatal ``Error``. -- **Security:** Fixed a bug where a valid JSON request body that is not an object - (for example a JSON array) was corrupted after CSRF verification when the - token was sent via the ``X-CSRF-TOKEN`` header. +- **HTTP:** Fixed stale ``$_REQUEST`` values after ``SiteURIFactory`` normalizes GET parameters, so ``Validation::withRequest()`` reads the normalized values while unrelated REQUEST entries are preserved. - **HTTP:** Fixed a bug where ``IncomingRequest::isSecure()`` did not recognize ``https`` in the ``X-Forwarded-Proto`` header from a trusted proxy when the value contained uppercase letters or surrounding whitespace, or was represented as a single-element array. +- **Security:** Fixed a bug where a valid JSON request body that is not an object + (for example a JSON array) was corrupted after CSRF verification when the + token was sent via the ``X-CSRF-TOKEN`` header. See the repo's `CHANGELOG.md `_