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

Open
rahul05ranjan wants to merge 3 commits into
base: develop
Choose a base branch
from
+283 −5
Open

fix: refresh REQUEST after URI GET normalization #10606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ff3dff
fix: refresh REQUEST after URI GET normalization
rahul05ranjan Oct 9, 2026
5d28859
Mark Superglobals::getRequestData() as internal
rahul05ranjan Oct 14, 2026
c247f6e
test: make request-order fixtures deterministic and document REQUEST …
rahul05ranjan Oct 17, 2026
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
47 changes: 42 additions & 5 deletions system/HTTP/SiteURIFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
/**
* Creates SiteURI using superglobals.
*
* This class also updates superglobal $_REQUEST.
* This class also updates superglobal $_SERVER and $_GET.
*
* @see \CodeIgniter\HTTP\SiteURIFactoryTest
Expand All @@ -33,7 +34,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 @@ -105,7 +106,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 @@ -169,17 +170,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 @@ -201,9 +204,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 @@ -222,6 +227,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
47 changes: 47 additions & 0 deletions system/Superglobals.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,8 @@ public function unsetRequest(string $key): self
* Get all $_REQUEST values.
*
* @return array<string, request_items>
*
* @internal
Comment on lines +371 to +372

@paulbalandan paulbalandan Oct 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be on setRequestData, not here.

but Superglobals is already @internal so I think no need to add, @michalsn ?

@michalsn michalsn Oct 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's true - my mistake. I keep forgetting that this class is already internal.

*/
public function getRequestArray(): array
{
Expand All @@ -387,6 +389,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
1 change: 1 addition & 0 deletions user_guide_src/source/changelogs/v4.7.5.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@paulbalandan paulbalandan Oct 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v4.7.5 was already released. This needs to be rebased and moved to v4.7.6

- **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()``.
Expand Down
Loading