diff --git a/system/HTTP/IncomingRequest.php b/system/HTTP/IncomingRequest.php index cc66e35f4ddc..908ba02abcb9 100644 --- a/system/HTTP/IncomingRequest.php +++ b/system/HTTP/IncomingRequest.php @@ -49,6 +49,11 @@ */ class IncomingRequest extends Request { + /** + * Distinguishes an explicit request override from fetchGlobal()'s cache. + */ + private bool $requestGlobalWasExplicitlySet = false; + /** * The URI for this request. * @@ -368,9 +373,10 @@ public function getDefaultLocale(): string } /** - * Fetch an item from JSON input stream with fallback to $_REQUEST object. This is the simplest way - * to grab data from the request object and can be used in lieu of the - * other get* methods in most cases. + * Fetch an item from JSON input stream with fallback to the merged + * $_GET, $_POST, and $_COOKIE data. This is the simplest way to grab data + * from the request object and can be used in lieu of the other get* + * methods in most cases. * * @param list|string|null $index * @param int|null $filter Filter constant @@ -387,7 +393,37 @@ public function getVar($index = null, $filter = null, $flags = null) return $this->getJsonVar($index, false, $filter, $flags); } - return $this->fetchGlobal('request', $index, $filter, $flags); + // Preserve request data explicitly supplied through setGlobal(). + if ($this->requestGlobalWasExplicitlySet) { + return $this->fetchGlobal('request', $index, $filter, $flags); + } + + // $_REQUEST can become stale when SiteURIFactory updates $_GET. + // Use the existing filtering path with a fresh merged view, without + // caching it between calls or modifying the superglobals. + $cachedRequest = $this->globals['request'] ?? null; + $this->globals['request'] = service('superglobals')->getRequestData(); + + try { + return $this->fetchGlobal('request', $index, $filter, $flags); + } finally { + if ($cachedRequest === null) { + unset($this->globals['request']); + } else { + $this->globals['request'] = $cachedRequest; + } + } + } + + public function setGlobal(string $name, $value) + { + parent::setGlobal($name, $value); + + if ($name === 'request') { + $this->requestGlobalWasExplicitlySet = true; + } + + return $this; } /** 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/IncomingRequestTest.php b/tests/system/HTTP/IncomingRequestTest.php index 464b46e2ce2c..f88061980123 100644 --- a/tests/system/HTTP/IncomingRequestTest.php +++ b/tests/system/HTTP/IncomingRequestTest.php @@ -29,6 +29,7 @@ use PHPUnit\Framework\Attributes\PreserveGlobalState; use PHPUnit\Framework\Attributes\RunInSeparateProcess; use PHPUnit\Framework\Attributes\WithoutErrorHandler; +use RuntimeException; /** * @internal @@ -70,12 +71,97 @@ private function createRequest(?App $config = null, false|string|null $body = nu public function testCanGrabRequestVars(): void { - service('superglobals')->setRequest('TEST', '5'); + service('superglobals')->setGet('TEST', '5'); $this->assertSame('5', $this->request->getVar('TEST')); $this->assertNull($this->request->getVar('TESTY')); } + public function testGetVarReflectsGetChangesWhenRequestIsStale(): void + { + // Simulate the state after SiteURIFactory updates $_GET: $_REQUEST + // still holds the original value while $_GET has been refreshed. + service('superglobals') + ->setGetArray(['code' => 'good']) + ->setRequestArray(['code' => 'stale']); + + $this->assertSame('good', $this->request->getVar('code')); + } + + public function testGetVarPreservesExplicitRequestOverride(): void + { + service('superglobals')->setGetArray(['foo' => 'get', 'extra' => 'value']); + $this->request->setGlobal('request', ['foo' => 'bar']); + + $this->assertSame('bar', $this->request->getVar('foo')); + $this->assertNull($this->request->getVar('extra')); + $this->assertSame(['foo' => 'bar'], $this->request->getVar()); + + $this->request->setGlobal('request', []); + + $this->assertNull($this->request->getVar('foo')); + } + + public function testGetVarRefreshesMergedDataAfterFirstRead(): void + { + service('superglobals')->setGetArray(['foo' => 'old']); + $this->assertSame('old', $this->request->getVar('foo')); + + service('superglobals')->setGet('foo', 'new'); + + $this->assertSame('new', $this->request->getVar('foo')); + } + + public function testGetVarDoesNotTreatFetchGlobalCacheAsRequestOverride(): void + { + service('superglobals') + ->setGetArray(['foo' => 'fresh']) + ->setRequestArray(['foo' => 'stale']); + + $this->assertSame('stale', $this->request->fetchGlobal('request', 'foo')); + $this->assertSame('fresh', $this->request->getVar('foo')); + $this->assertSame('stale', $this->request->fetchGlobal('request', 'foo')); + + $this->request->setGlobal('request', ['foo' => 'explicit']); + + $this->assertSame('explicit', $this->request->getVar('foo')); + } + + public function testGetVarClearsMergedDataAfterFilterThrows(): void + { + service('superglobals')->setGetArray(['foo' => 'old']); + + try { + $this->request->getVar('foo', FILTER_CALLBACK, [ + 'options' => static function (): never { + throw new RuntimeException('Filter failed'); + }, + ]); + $this->fail('The filter should throw an exception.'); + } catch (RuntimeException $e) { + $this->assertSame('Filter failed', $e->getMessage()); + } + + service('superglobals')->setGet('foo', 'new'); + + $this->assertSame('new', $this->request->getVar('foo')); + } + + public function testGetVarAllowsSubclassWithFetchFromArrayMethod(): void + { + $config = new App(); + $request = new class ($config, new SiteURI($config), null, new UserAgent()) extends IncomingRequest { + protected function fetchFromArray(): string + { + return 'application helper'; + } + }; + + service('superglobals')->setGetArray(['foo' => 'bar']); + + $this->assertSame('bar', $request->getVar('foo')); + } + public function testCanGrabGetVars(): void { service('superglobals')->setGet('TEST', '5'); @@ -525,8 +611,8 @@ public function testGetVarWorksWithJsonAndGetParams(): void $config->baseURL = 'http://example.com/'; // GET method - service('superglobals')->setRequest('foo', 'bar'); - service('superglobals')->setRequest('fizz', 'buzz'); + service('superglobals')->setGet('foo', 'bar'); + service('superglobals')->setGet('fizz', 'buzz'); $request = $this->createRequest($config); $request = $request->withMethod('GET'); diff --git a/tests/system/SuperglobalsTest.php b/tests/system/SuperglobalsTest.php index e4b8b23b2b28..309acbf8eec6 100644 --- a/tests/system/SuperglobalsTest.php +++ b/tests/system/SuperglobalsTest.php @@ -302,6 +302,105 @@ 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 + { + $this->superglobals->setGetArray([100 => 'foo']); // @phpstan-ignore argument.type (numeric keys are valid in superglobals, e.g. ?100=foo) + + $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/tests/system/Validation/ValidationTest.php b/tests/system/Validation/ValidationTest.php index 49baeaba0c79..70adb1a16d18 100644 --- a/tests/system/Validation/ValidationTest.php +++ b/tests/system/Validation/ValidationTest.php @@ -1289,7 +1289,7 @@ public function testRulesForSingleRuleWithAsteriskWillReturnNoError(): void $config = new App(); $config->baseURL = 'http://example.com/'; - service('superglobals')->setRequestArray([ + service('superglobals')->setPostArray([ 'id_user' => [ 1, 3, @@ -1316,7 +1316,7 @@ public function testRulesForSingleRuleWithAsteriskWillReturnError(): void $config = new App(); $config->baseURL = 'http://example.com/'; - service('superglobals')->setRequestArray([ + service('superglobals')->setPostArray([ 'id_user' => [ '1dfd', 3, @@ -1366,7 +1366,7 @@ public function testRulesForSingleRuleWithSingleValue(): void $config = new App(); $config->baseURL = 'http://example.com/'; - service('superglobals')->setRequestArray([ + service('superglobals')->setPostArray([ 'id_user' => 'gh', ]); diff --git a/user_guide_src/source/changelogs/v4.7.5.rst b/user_guide_src/source/changelogs/v4.7.5.rst index deabaf4b7ab4..ffe5f64735af 100644 --- a/user_guide_src/source/changelogs/v4.7.5.rst +++ b/user_guide_src/source/changelogs/v4.7.5.rst @@ -58,6 +58,7 @@ Bugs Fixed - **Files:** Fixed a bug where ``File::move()`` and ``UploadedFile::move()`` set executable and overly permissive file permissions (``0777 & ~umask()`` instead of ``0666 & ~umask()``), and ``UploadedFile::move()`` targeted the parent directory instead of the destination file for ``chmod()``. - **Helpers:** Fixed a bug where ``get_dir_file_info()`` returned incomplete entries for subdirectories and missing files instead of omitting them. - **Honeypot:** Fixed a bug where bot detection returned an HTTP 500 response instead of 403 (Forbidden). +- **HTTP:** Fixed a bug where ``IncomingRequest::getVar()`` returned stale data after ``$_GET`` was updated during URI parsing. It now returns a merged view of ``$_GET``, ``$_POST``, and ``$_COOKIE`` (respecting the ``request_order`` ini setting) instead of reading the stale ``$_REQUEST``. - **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()``. diff --git a/user_guide_src/source/incoming/incomingrequest.rst b/user_guide_src/source/incoming/incomingrequest.rst index 99c5762129d6..27d3a5505af1 100644 --- a/user_guide_src/source/incoming/incomingrequest.rst +++ b/user_guide_src/source/incoming/incomingrequest.rst @@ -162,7 +162,7 @@ getVar() in new projects. Even if you are already using it, we recommend that you use another, more appropriate method. -The ``getVar()`` method will pull from ``$_REQUEST``, so will return any data from ``$_GET``, ``$_POST``, or ``$_COOKIE`` (depending on php.ini `request-order `_). +The ``getVar()`` method returns a merged view of ``$_GET``, ``$_POST``, and ``$_COOKIE`` (depending on php.ini `request-order `_). It does not read or modify ``$_REQUEST``. Data explicitly supplied through ``setGlobal('request', ...)`` takes precedence over the merged view. .. warning:: If you want to validate POST data only, don't use ``getVar()``. Newer values override older values. POST values may be overridden by the @@ -170,7 +170,7 @@ The ``getVar()`` method will pull from ``$_REQUEST``, so will return any data fr `request-order `_. .. note:: If the incoming request has a ``Content-Type`` header set to ``application/json``, - the ``getVar()`` method returns the JSON data instead of ``$_REQUEST`` data. + the ``getVar()`` method returns the JSON data instead of the merged superglobal data. .. _incomingrequest-getting-json-data: @@ -373,14 +373,16 @@ The methods provided by the parent classes that are available are: `Types of filters `__. :param int $flags: Flags to apply. A list of flags can be found in `Filter flags `__. - :returns: ``$_REQUEST`` if no parameters supplied, otherwise the REQUEST value if found, or null if not + :returns: The merged ``$_GET``, ``$_POST``, and ``$_COOKIE`` data if no parameters supplied, otherwise the value if found, or null if not :rtype: array|bool|float|int|object|string|null .. important:: This method exists only for backward compatibility. Do not use it in new projects. Even if you are already using it, we recommend that you use another, more appropriate method. - This method is identical to ``getGet()``, only it fetches REQUEST data. + This method is identical to ``getGet()``, only it fetches the merged + ``$_GET``, ``$_POST``, and ``$_COOKIE`` data (respecting the + ``request_order`` ini setting) instead of ``$_REQUEST``. .. php:method:: getGet([$index = null[, $filter = null[, $flags = null]]])