Skip to content

fix: refresh REQUEST after URI GET normalization #10606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
paulbalandan merged 1 commit into from
Oct 23, 2026
+284 −9
Merged

fix: refresh REQUEST after URI GET normalization #10606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 42 additions & 6 deletions system/HTTP/SiteURIFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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
{
Expand Down Expand Up @@ -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).
*/
Expand Down Expand Up @@ -167,17 +167,19 @@ 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.
Comment thread
paulbalandan marked this conversation as resolved.
$previousRequestData = $this->superglobals->getRequestData();
parse_str($this->superglobals->server('QUERY_STRING'), $get);
$this->superglobals->setGetArray($get);
$this->refreshRequestAfterGetChange($previousRequestData);

return URI::removeDotSegments($path);
}

/**
* 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).
*/
Expand All @@ -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);
}
Expand All @@ -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<array-key, mixed> $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.
*/
Expand Down
45 changes: 45 additions & 0 deletions system/Superglobals.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, request_items>
*/
public function getRequestData(?string $requestOrder = null): array
Comment thread
paulbalandan marked this conversation as resolved.
{
$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.
*
Expand Down
93 changes: 93 additions & 0 deletions tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Comment thread
paulbalandan marked this conversation as resolved.
$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?
Expand Down
100 changes: 100 additions & 0 deletions tests/system/SuperglobalsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
7 changes: 4 additions & 3 deletions user_guide_src/source/changelogs/v4.7.6.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_
Expand Down
Loading