diff --git a/system/HTTP/SiteURIFactory.php b/system/HTTP/SiteURIFactory.php index 11dccce6c540..6348b8464611 100644 --- a/system/HTTP/SiteURIFactory.php +++ b/system/HTTP/SiteURIFactory.php @@ -169,9 +169,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); } @@ -201,9 +203,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); } @@ -222,6 +226,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 ac0ea289bd77..21909e432b0e 100644 --- a/system/Superglobals.php +++ b/system/Superglobals.php @@ -387,6 +387,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..07509eeb2a73 100644 --- a/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php +++ b/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php @@ -258,6 +258,70 @@ public function testQueryStringWithQueryString(): void $this->assertSame(['code' => 'good'], $_GET); } + public function testQueryStringRefreshesRequestWithoutOverwritingCustomValues(): void + { + $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 + { + $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()); + } + public function testQueryStringEmpty(): void { // /index.php? diff --git a/tests/system/SuperglobalsTest.php b/tests/system/SuperglobalsTest.php index e4b8b23b2b28..2f4d0cab1ea0 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(); + + $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()['key']); + + // Simulate SiteURIFactory updating $_GET after the request started. + $this->superglobals->setGetArray(['key' => 'new']); + + $this->assertSame('new', $this->superglobals->getRequestData()['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.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index 0897a791ba7c..0bf28cccc23e 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -63,6 +63,7 @@ Bugs Fixed - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. - **Helpers:** Fixed a bug where ``word_wrap()`` entered an infinite loop when the character limit was below 2. Over-length words are now kept whole in that case. - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden). +- **HTTP:** Fixed stale ``$_REQUEST`` values after ``SiteURIFactory`` normalizes GET parameters, so ``Validation::withRequest()`` reads the normalized values while unrelated REQUEST entries are preserved. - **I18n:** Fixed a bug where ``Time::today()``, ``Time::yesterday()``, and ``Time::tomorrow()`` ignored the specified ``$timezone`` and ``setTestNow()`` when calculating the day. - **Logger:** Fixed a bug where interpolating a log message with array or non-stringable context values could raise PHP warnings or errors. - **Validation:** Fixed a bug where ``valid_cc_number`` accepted non-digit characters (e.g., a decimal point) in the card number. Such values could pass the Luhn check and triggered an ``Undefined array key`` warning inside it; the number is now checked with ``ctype_digit()``.