Skip to content

Commit bcbf67e

Browse files
rahul05ranjanrahul05ranjan
committed
fix: refresh REQUEST after URI GET normalization
Fixes #9872. SiteURIFactory can normalize $_GET after PHP has populated $_REQUEST, leaving stale query values for getVar() and Validation::withRequest(). Refresh only source-derived REQUEST entries once during URI parsing while preserving unrelated or custom REQUEST values.
1 parent b74fff9 commit bcbf67e

5 files changed

Lines changed: 284 additions & 8 deletions

File tree

‎system/HTTP/SiteURIFactory.php‎

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
/**
2121
* Creates SiteURI using superglobals.
2222
*
23+
* This class also updates superglobal $_REQUEST.
2324
* This class also updates superglobal $_SERVER and $_GET.
2425
*/
2526
final readonly class SiteURIFactory
@@ -31,7 +32,7 @@ public function __construct(private App $appConfig, private Superglobals $superg
3132
/**
3233
* Create the current URI object from superglobals.
3334
*
34-
* This method updates superglobal $_SERVER and $_GET.
35+
* This method updates superglobals $_SERVER, $_GET, and $_REQUEST.
3536
*/
3637
public function createFromGlobals(): SiteURI
3738
{
@@ -103,7 +104,7 @@ public function detectRoutePath(string $protocol = ''): string
103104
* Will parse the REQUEST_URI and automatically detect the URI from it,
104105
* fixing the query string if necessary.
105106
*
106-
* This method updates superglobal $_SERVER and $_GET.
107+
* This method updates superglobals $_SERVER, $_GET, and $_REQUEST.
107108
*
108109
* @return string The route path (before normalization).
109110
*/
@@ -167,17 +168,19 @@ private function parseRequestURI(): string
167168
$this->superglobals->setServer('QUERY_STRING', $query);
168169
}
169170

170-
// Update our global GET for values likely to have been changed
171+
// Refresh only the request values derived from superglobals after GET changes.
172+
$previousRequestData = $this->superglobals->getRequestData();
171173
parse_str($this->superglobals->server('QUERY_STRING'), $get);
172174
$this->superglobals->setGetArray($get);
175+
$this->refreshRequestAfterGetChange($previousRequestData);
173176

174177
return URI::removeDotSegments($path);
175178
}
176179

177180
/**
178181
* Will parse QUERY_STRING and automatically detect the URI from it.
179182
*
180-
* This method updates superglobal $_SERVER and $_GET.
183+
* This method updates superglobals $_SERVER, $_GET, and $_REQUEST.
181184
*
182185
* @return string The route path (before normalization).
183186
*/
@@ -199,9 +202,11 @@ private function parseQueryString(): string
199202
$path = $query;
200203
}
201204

202-
// Update our global GET for values likely to have been changed
205+
// Refresh only the request values derived from superglobals after GET changes.
206+
$previousRequestData = $this->superglobals->getRequestData();
203207
parse_str($this->superglobals->server('QUERY_STRING'), $get);
204208
$this->superglobals->setGetArray($get);
209+
$this->refreshRequestAfterGetChange($previousRequestData);
205210

206211
return URI::removeDotSegments($path);
207212
}
@@ -220,6 +225,38 @@ private function createURIFromRoutePath(string $routePath): SiteURI
220225
return new SiteURI($this->appConfig, $relativePath, $this->getHost());
221226
}
222227

228+
/**
229+
* Update REQUEST once during URI parsing, leaving application-defined values alone.
230+
*
231+
* @param array<array-key, mixed> $previousRequestData
232+
*/
233+
private function refreshRequestAfterGetChange(array $previousRequestData): void
234+
{
235+
$request = $this->superglobals->getRequestArray();
236+
$currentData = $this->superglobals->getRequestData();
237+
238+
foreach ($previousRequestData as $key => $value) {
239+
// A different value was supplied directly in REQUEST; preserve it.
240+
if (! array_key_exists($key, $request) || $request[$key] !== $value) {
241+
continue;
242+
}
243+
244+
if (array_key_exists($key, $currentData)) {
245+
$request[$key] = $currentData[$key];
246+
} else {
247+
unset($request[$key]);
248+
}
249+
}
250+
251+
foreach ($currentData as $key => $value) {
252+
if (! array_key_exists($key, $previousRequestData) && ! array_key_exists($key, $request)) {
253+
$request[$key] = $value;
254+
}
255+
}
256+
257+
$this->superglobals->setRequestArray($request);
258+
}
259+
223260
/**
224261
* @return string|null The current hostname. Returns null if no valid host.
225262
*/

‎system/Superglobals.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,51 @@ public function setRequestArray(array $array): self
386386
return $this;
387387
}
388388

389+
/**
390+
* Returns the merged $_GET, $_POST, and $_COOKIE data according to the
391+
* `request_order` (or `variables_order`) ini setting, without mutating
392+
* $_REQUEST.
393+
*
394+
* PHP populates $_REQUEST only once at the start of the request. When
395+
* $_GET is modified later (e.g. by SiteURIFactory), $_REQUEST becomes
396+
* stale. This method returns the current merged values so callers can
397+
* read up-to-date request data without relying on the stale $_REQUEST.
398+
*
399+
* @param string|null $requestOrder Overrides the `request_order` ini
400+
* setting. Useful for testing, since the
401+
* ini setting cannot be changed at runtime.
402+
*
403+
* @return array<string, request_items>
404+
*/
405+
public function getRequestData(?string $requestOrder = null): array
406+
{
407+
$requestOrder ??= (string) ini_get('request_order');
408+
409+
if ($requestOrder === '') {
410+
$requestOrder = (string) ini_get('variables_order');
411+
}
412+
413+
if ($requestOrder === '') {
414+
$requestOrder = 'GP';
415+
}
416+
417+
$request = [];
418+
419+
foreach (array_unique(str_split(strtoupper($requestOrder))) as $type) {
420+
match ($type) {
421+
// array_replace_recursive() matches PHP's own $_REQUEST merge
422+
// (php_autoglobal_merge): numeric keys are preserved and
423+
// array values are merged recursively.
424+
'G' => $request = array_replace_recursive($request, $this->get),
425+
'P' => $request = array_replace_recursive($request, $this->post),
426+
'C' => $request = array_replace_recursive($request, $this->cookie),
427+
default => null,
428+
};
429+
}
430+
431+
return $request;
432+
}
433+
389434
/**
390435
* Get all $_FILES values.
391436
*

‎tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,99 @@ public function testQueryStringWithQueryString(): void
258258
$this->assertSame(['code' => 'good'], $_GET);
259259
}
260260

261+
public function testQueryStringRefreshesRequestWithoutOverwritingCustomValues(): void
262+
{
263+
if (! $this->effectiveRequestOrderIncludesGet()) {
264+
$this->markTestSkipped('The effective request order does not include GET.');
265+
}
266+
267+
$superglobals = new Superglobals(
268+
[
269+
'REQUEST_URI' => '/index.php?/ci/woot?code=good',
270+
'QUERY_STRING' => '/ci/woot?code=good',
271+
'SCRIPT_NAME' => '/index.php',
272+
],
273+
['/ci/woot?code' => 'good', 'override' => 'from get'],
274+
['posted' => 'post'],
275+
[],
276+
[],
277+
[
278+
'/ci/woot?code' => 'good',
279+
'override' => 'custom',
280+
'posted' => 'post',
281+
'unrelated' => 'keep',
282+
],
283+
);
284+
Services::injectMock('superglobals', $superglobals);
285+
286+
$factory = new SiteURIFactory(new App(), $superglobals);
287+
288+
$this->assertSame('ci/woot', $factory->detectRoutePath('QUERY_STRING'));
289+
$this->assertSame([
290+
'override' => 'custom',
291+
'posted' => 'post',
292+
'unrelated' => 'keep',
293+
'code' => 'good',
294+
], $superglobals->getRequestArray());
295+
$this->assertSame($superglobals->getRequestArray(), $_REQUEST);
296+
297+
$superglobals->setGet('code', 'later');
298+
299+
$this->assertSame('good', $superglobals->request('code'));
300+
}
301+
302+
public function testRequestURIRefreshesRequestForValidation(): void
303+
{
304+
if (! $this->effectiveRequestOrderIncludesGet()) {
305+
$this->markTestSkipped('The effective request order does not include GET.');
306+
}
307+
308+
$superglobals = new Superglobals(
309+
[
310+
'REQUEST_URI' => '/index.php/woot?code=good',
311+
'SCRIPT_NAME' => '/index.php',
312+
],
313+
['code' => 'stale'],
314+
[],
315+
[],
316+
[],
317+
['code' => 'stale', 'unrelated' => 'keep'],
318+
);
319+
Services::injectMock('superglobals', $superglobals);
320+
321+
$config = new App();
322+
$factory = new SiteURIFactory($config, $superglobals);
323+
324+
$this->assertSame('woot', $factory->detectRoutePath('REQUEST_URI'));
325+
$this->assertSame(['code' => 'good', 'unrelated' => 'keep'], $superglobals->getRequestArray());
326+
327+
$request = new IncomingRequest($config, new SiteURI($config), null, new UserAgent());
328+
329+
$this->assertSame('good', $request->getVar('code'));
330+
$this->assertTrue(service('validation')->withRequest($request)->setRules(['code' => 'in_list[good]'])->run());
331+
}
332+
333+
/**
334+
* Whether GET is part of the effective request order on this host.
335+
*
336+
* Mirrors Superglobals::getRequestData(): uses `request_order`, falls
337+
* back to `variables_order`, then to PHP's default GP.
338+
*/
339+
private function effectiveRequestOrderIncludesGet(): bool
340+
{
341+
$order = (string) ini_get('request_order');
342+
343+
if ($order === '') {
344+
$order = (string) ini_get('variables_order');
345+
}
346+
347+
if ($order === '') {
348+
$order = 'GP';
349+
}
350+
351+
return str_contains(strtoupper($order), 'G');
352+
}
353+
261354
public function testQueryStringEmpty(): void
262355
{
263356
// /index.php?

‎tests/system/SuperglobalsTest.php‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -302,6 +302,106 @@ public function testRequestSetArray(): void
302302
$this->assertSame($data, $_REQUEST);
303303
}
304304

305+
public function testGetRequestDataMergesGetAndPost(): void
306+
{
307+
$this->superglobals->setGetArray(['get_key' => 'get_value']);
308+
$this->superglobals->setPostArray(['post_key' => 'post_value']);
309+
310+
$data = $this->superglobals->getRequestData('GP');
311+
312+
$this->assertSame('get_value', $data['get_key']);
313+
$this->assertSame('post_value', $data['post_key']);
314+
}
315+
316+
public function testGetRequestDataReflectsGetChanges(): void
317+
{
318+
$this->superglobals->setGetArray(['key' => 'old']);
319+
320+
$this->assertSame('old', $this->superglobals->getRequestData('G')['key']);
321+
322+
// Simulate SiteURIFactory updating $_GET after the request started.
323+
$this->superglobals->setGetArray(['key' => 'new']);
324+
325+
$this->assertSame('new', $this->superglobals->getRequestData('G')['key']);
326+
}
327+
328+
public function testGetRequestDataMergesCookie(): void
329+
{
330+
$this->superglobals->setGetArray(['get_key' => 'get_value']);
331+
$this->superglobals->setPostArray(['post_key' => 'post_value']);
332+
$this->superglobals->setCookieArray(['cookie_key' => 'cookie_value']);
333+
334+
$data = $this->superglobals->getRequestData('GPC');
335+
336+
$this->assertSame('get_value', $data['get_key']);
337+
$this->assertSame('post_value', $data['post_key']);
338+
$this->assertSame('cookie_value', $data['cookie_key']);
339+
}
340+
341+
public function testGetRequestDataRespectsOrder(): void
342+
{
343+
$this->superglobals->setGetArray(['shared' => 'get']);
344+
$this->superglobals->setPostArray(['shared' => 'post']);
345+
$this->superglobals->setCookieArray(['shared' => 'cookie']);
346+
347+
// Later sources overwrite earlier ones, matching PHP's request_order.
348+
$this->assertSame('post', $this->superglobals->getRequestData('GP')['shared']);
349+
$this->assertSame('cookie', $this->superglobals->getRequestData('GPC')['shared']);
350+
$this->assertSame('get', $this->superglobals->getRequestData('PG')['shared']);
351+
}
352+
353+
public function testGetRequestDataEmptyOrderFallsBackToVariablesOrder(): void
354+
{
355+
$this->superglobals->setGetArray(['get' => 'value']);
356+
$this->superglobals->setPostArray(['post' => 'value']);
357+
$this->superglobals->setCookieArray(['cookie' => 'value']);
358+
359+
$this->assertSame(
360+
$this->superglobals->getRequestData((string) ini_get('variables_order')),
361+
$this->superglobals->getRequestData(''),
362+
);
363+
}
364+
365+
public function testGetRequestDataNormalizesOrderAndIgnoresDuplicates(): void
366+
{
367+
$this->superglobals->setGetArray(['shared' => 'get']);
368+
$this->superglobals->setPostArray(['shared' => 'post']);
369+
$this->superglobals->setCookieArray(['shared' => 'cookie']);
370+
371+
$this->assertSame('post', $this->superglobals->getRequestData('gp')['shared']);
372+
$this->assertSame('post', $this->superglobals->getRequestData('GPG')['shared']);
373+
$this->assertSame('cookie', $this->superglobals->getRequestData('gPcGpC')['shared']);
374+
}
375+
376+
public function testGetRequestDataIgnoresUnknownOrderTypes(): void
377+
{
378+
$this->superglobals->setGetArray(['get_key' => 'get_value']);
379+
380+
$data = $this->superglobals->getRequestData('GX');
381+
382+
$this->assertSame(['get_key' => 'get_value'], $data);
383+
}
384+
385+
public function testGetRequestDataPreservesNumericKeys(): void
386+
{
387+
parse_str('100=foo', $get);
388+
$this->superglobals->setGetArray($get);
389+
390+
$data = $this->superglobals->getRequestData('G');
391+
392+
$this->assertSame([100 => 'foo'], $data);
393+
}
394+
395+
public function testGetRequestDataMergesRecursively(): void
396+
{
397+
$this->superglobals->setGetArray(['a' => ['x' => 'get']]);
398+
$this->superglobals->setPostArray(['a' => ['y' => 'post']]);
399+
400+
$data = $this->superglobals->getRequestData('GP');
401+
402+
$this->assertSame(['a' => ['x' => 'get', 'y' => 'post']], $data);
403+
}
404+
305405
// $_FILES tests
306406
public function testFilesGetArray(): void
307407
{

‎user_guide_src/source/changelogs/v4.7.6.rst‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,14 @@ Bugs Fixed
3838
is used instead. Values that cannot be converted to a string, such as objects
3939
not implementing ``Stringable``, now throw a ``FormatException`` instead of a
4040
fatal ``Error``.
41-
- **Security:** Fixed a bug where a valid JSON request body that is not an object
42-
(for example a JSON array) was corrupted after CSRF verification when the
43-
token was sent via the ``X-CSRF-TOKEN`` header.
41+
- **HTTP:** Fixed stale ``$_REQUEST`` values after ``SiteURIFactory`` normalizes GET parameters, so ``Validation::withRequest()`` reads the normalized values while unrelated REQUEST entries are preserved.
4442
- **HTTP:** Fixed a bug where ``IncomingRequest::isSecure()`` did not
4543
recognize ``https`` in the ``X-Forwarded-Proto`` header from a trusted
4644
proxy when the value contained uppercase letters or surrounding
4745
whitespace, or was represented as a single-element array.
46+
- **Security:** Fixed a bug where a valid JSON request body that is not an object
47+
(for example a JSON array) was corrupted after CSRF verification when the
48+
token was sent via the ``X-CSRF-TOKEN`` header.
4849

4950
See the repo's
5051
`CHANGELOG.md <https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md>`_

0 commit comments

Comments
 (0)