Protect pager against [] for the page

@BROkEN: Thank you for this bug report, fix and the added tests.

1. +++ b/core/lib/Drupal/Core/Pager/PagerParameters.php
   @@ -63,8 +63,8 @@ public function getPagerQuery() {
   +    if ($request && $page = $request->query->get('page')) {
   

   I do not think that we should put the part $page = $request->query->get('page') in the if-statement. Put it on its own line just before the return-statement.

2. +++ b/core/modules/system/tests/src/Functional/Pager/PagerTest.php
   @@ -52,6 +52,20 @@ protected function setUp(): void {
   +    $this->drupalGet('admin/reports/dblog', ['query' => ['page[offset]' => [1]]]);
   

   Can we change the line to: $this->drupalGet('admin/reports/dblog', ['query' => ['page' => ['offset' => 1]]]);

@BROkEN i will be very happy if i can do something in this issue so i assign this to my self.
Thanks
Pavnish

@daffie I have changed the code as suggest by you in #5.
It's my honour if you can review this patch.
Thanks
Pavnish

Uploading patch from comment #8 with only the added test. This patch should fail.

@daffie
Upload all the patch #8 with fix and test only patch

The patch contains a bugfix and testing for the bugfix.
All code changes look good to me.
For me it is RTBC.

@BROkEN Likes to see another bugfix for this patch:

--- a/core/lib/Drupal/Core/Pager/PagerParameters.php
+++ b/core/lib/Drupal/Core/Pager/PagerParameters.php
@@ -63,8 +63,8 @@ public function getPagerQuery() {
    */
   public function getPagerParameter() {
     $request = $this->requestStack->getCurrentRequest();
-    if ($request) {
-      return $request->query->get('page', '');
+    if ($request && $page = $request->query->get('page')) {
+      return is_string($page) ? $page : '';
     }
     return '';
   }

I leave it to the committer to choose which bugfix to commit.

Hi @daffie,

made changes please review.

Hi @BR0kEN,

Please assign the issue to yourself or add a comment that you are working on this and please try to provide a solution within 1 or 2 days. If it will take more time to complete this please add a comment or draft patch otherwise community members will pick this which is very normal. Please check the guidelines

+ return is_string($page) ? $page : '';

The $request->query object offers a few helper methods to deal with this. Like getDigits() and getInt(). We could use them.

1. Re #23, $request->query->getDigits(), $request->query->getInt() and other variations are inapplicable here, as page is a comma-delimited string of pager element values. For example: ?page=5,2, ?page=2.1,6.999,,,5.

2. +++ b/core/modules/system/tests/src/Functional/Pager/PagerTest.php
   @@ -52,6 +52,26 @@ protected function setUp(): void {
   +   * Test that overlapping `page` GET query does not produce errors.
   

   Fixing one nitpick: Test that -> Tests that

3. See #10 for the TEST-ONLY patch
4. Setting to RTBC

Re-queued

@jungle good point.
For the record, retrieving non-string values with $request->query->get() method was deprecated in Symfony 5 and will throw an exception in Symfony 6.
https://github.com/symfony/symfony/pull/34363

Yeah, please don't assign core issues. Those guidelines specifically says to avoid it and for something like this it generally just adds noise to the discussion. If you're going to propose a different approach just provide a patch without 3 issue updates.

That Symfony changes is pretty frustrating. I guess there's not a great solution to the ambiguity and dealing with "user" input but throwing an exception... seems like the method is becoming unusable. Something to consider for 10,

I was going to +1 the RTBC because in a narrow scope this obviously addresses the issue. But reading the patch one last time one questions lead to another, then a full on review, then digging in more I ended up rewriting the patch trying to sort out the answers. I don't think this fully addresses the problems with these methods.

Here's the review:
1) Why is this a functional test. It should just be a unit test, The extra complexity, boilerplating and slowness provides no benefit.
2) Why the link to the issue in the test comment? Isn't the git log good enough?
3) Why are we only testing one value? This doesn't fully flesh out the possible values affected by this change.
4) Why do we care if its a string? Could it be numeric? Parameter bags definitely allow that. We only care that its not an array right so is_scalar and a string cast seems more resilient.
5) Scope creapish, why is getPagerQuery using empty? The current patch documents how this will fail but only fixes it in getPagerParameter. findPage fixes handles the edge case and since its main entry point in core so functional tests aren't catching this method being broken.
6) More scope creapish, explode() returns an array of _strings_ but the interface requires returning int[]. Fixing it to match the interface _could_ break contrib or sites hacking strings or floats through a pager value. However those cases would be broken in findPage which does the cast already so it was probably already effectively broken. They can still access the request so I think we should fix it. Also not fixing it makes testing inconsistent.

So here's a patch
1) Converted to unit test.
2) Removed in functional test removal.
3) Added edge case assertions around NULL, 0, '0' and []. Tested these for getQuery, getQueryParameter, and findPage to assert they work consistently.
4) fixed
5) fixed
6) fixed

1) Converted to unit test.
2) Removed in functional test removal.

@neclimdul Neither patch #24 nor #28 adds any tests. Have I missed something?

Insomnia review/patches... am i right? I diffed off the wrong branch, sorry.

The failure looks to have been random. Here's the patch I _meant_ to upload in 28.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Attached patch fixes issue raised by the bot. It also cleans up the comment a tad.

* The both complained about the isset() which was introduced in #28, I've replace it with a simple condition because IMO neither isset() nor !empty is necessary here.

Its been 3 years so its hard to remember but pretty sure the isset was the to preserve the behavior of the empty check as demonstrated by the failing tests. What was the problem with it?

Attached patch fixes issues

Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.

raised by the test bot

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Attached a patch without the use of ->all()['page'] for avoid warning like

Undefined array key "page"

Issue summary should be updated to the standard issue template, even if sections don't apply leaving them with NA is easier for reviews/committers.

Updated.

Turns out its been long enough a few things have changed. Specifically, current versions of PHP throw an exception instead of a warning which makes it a bit harder to see. Specifically, real exception gets gobbled up by a more generic exception and Drupal's exception logging in watchdog doesn't show previous exceptions.

Since the actual exception is hidden and its hard to validate the root cause without hacking the exception handler or using a debugger, I've left the original trace in place. The fix is the same and the trace captures the essence of the problem.

Ran patch #47 and it has a test failure that seems legit.

Recommended to convert to MRs

"pretty sure the isset was the to preserve the behavior of the empty check as demonstrated by the failing tests." 🤷

Added it back because it was there to defensively handle a null value from the getPagerParameter method. Since isset is a language level feature there's no real cost to being defensive like this.

I'm already tired of the 500 remotes and all the commit juggling. I'm going to really miss the simple patch workflow. But ok, its in a MR now.

Turns around it was a little more complicated. I went ahead and took the opportunity to update the tests and make things a little more clear.

Thanks looks good!

I'm triaging RTBC issues. I read the IS, the comments and the MR.

Thanks for updating the Issue Summary, that is always helpful! The review in #28 by @neclimdul was also useful as it explained what they were thinking and checking. That provided me with insight into understanding the issue and when else I may need to check.

The changes to getPagerQuery and getPagerParameter are changing the behavior, making it conform to the interface. So, it there any ramification for contrib/custom?

I left suggestions in the MR for coding standards and one asking for documentation.

I hide all the patches.

This issue got reported as a security issue for better_exposed_filter, but was later made public. #3466314: Inclusion of parameter exhausts site memory with exposed filter block

Addressed the feedback

Thanks for working on this.

Regarding @quietone's outstanding question in #56:

The changes to getPagerQuery and getPagerParameter are changing the behavior, making it conform to the interface. So, it there any ramification for contrib/custom?

I think this question has already been answered by @neclimdul in #28 6):

Fixing it to match the interface _could_ break contrib or sites hacking strings or floats through a pager value. However those cases would be broken in findPage which does the cast already so it was probably already effectively broken.

The code changes look good. I added one suggestion and a few comments inline, mostly concerning comments in the test.

The MR introduces new test coverage for the pager parameter. The pipeline is green. The tests fail locally as expected without the fix. I'll run the test only job just before I'm otherwise ready to RTBC, so not quite yet. With the MR applied, I can't reproduce the original issue. I also did some manual testing both with the original and modified (invalid) pager parameters: The administrative content view (using AJAX), a regular view (without AJAX), multiple views with different pager ids on the same page, the administrative URL Alias overview page (no view, probably entity query pager?). I noticed an issue with 3 views on the same page, but re-testing this without the MR applied, the issue was already present before, so no regression.

Tagging security improvement (for contrib) based on #57.

So overall this looks good and almost ready. Ready to RTBC once my very minor MR comments have been addressed.

Did not mean to take so long but gave it a shot. May need to expand on the docs some though.

Thanks Stephen, the changes look good to me.

Looks like the CI runners are a little busy right now, the repeated failures due to GitHub timeouts and no test nodes available gave me sufficient time to also retest this locally. Almost like Drupal CI, oh the good old times, where I could work on another issue while I waited for the tests to complete... Let's call it the DrupalCon review bonus :)

Test only job fails as expected:

Configuration: /builds/issue/drupal-3143617/core/phpunit.xml.dist
..EE.......EE.FF....FEE..F...                                     29 / 29 (100%)
Time: 00:00.034, Memory: 8.00 MB
There were 6 errors:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
2) Drupal\Tests\Core\Pager\PagerParametersTest::testFindPage with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:49
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:47
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:57
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
5) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid empty page array" ([], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
6) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "invalid populated array" ([1, 2, 3], '', [])
Symfony\Component\HttpFoundation\Exception\BadRequestException: Input value "page" contains a non-scalar value.
/builds/issue/drupal-3143617/vendor/symfony/http-foundation/InputBag.php:38
/builds/issue/drupal-3143617/core/lib/Drupal/Core/Pager/PagerParameters.php:67
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
--
There were 4 failures:
1) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a string" ('0', '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 0
 )
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
2) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerQuery with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 0
 )
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:60
3) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "defensive null page value" (null, '', [])
Failed asserting that null is identical to ''.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
4) Drupal\Tests\Core\Pager\PagerParametersTest::testGetPagerParameter with data set "page 0 as a integer" (0, '0', [0])
Failed asserting that 0 is identical to '0'.
/builds/issue/drupal-3143617/core/tests/Drupal/Tests/Core/Pager/PagerParametersTest.php:83
ERRORS!
Tests: 29, Assertions: 33, Errors: 6, Failures: 4.
Exiting with EXIT_CODE=2

The documentation for the data provider return value could always be a bit more verbose of course, but I think documenting that the keys returned by the data provider correspond to the variable names that are also used in the test methods should be enough for developers looking at this test code in the future to get the idea behind this setup. It should also address @quietone's concern:

For whatever reason, I started reading this from the provider and kept wondering why there were three return values when testGetPagerParameter() is only using one.

So let's give the RTBC queue triage team another shot at this... I think this issue is RTBC 🎉!

So let's give the RTBC queue triage team

Who are members of this team? :-)

Thanks for updating the MR and making the improvements to the test documentation. Hopefully, it will help the next person to work on this.
There are no unanswered questions. Leaving at RTBC.

Updating issue credits

Left some comments on the MR. I think we should be using the new Symfony API, not inventing our own protection

trying to update the code as suggested in the MR

Rebased and applied suggestions.

Resolved the threads and believe all feedback has been addressed.

Looks like there is still an unresolved thread on the MR.

Looks like there is still an unresolved thread on the MR.

Thanks @catch for looking at this! I think the open thread you're referring to is from an earlier review I did? This thread had been addressed by the time I wrote #60, also confirmed by another review by @quietone in #61. I would have resolved it at the time, but even though I have push access and I am the original author of the thread, I didn't have permission to resolve the thread and I still don't have permission to do so, unfortunately. I know this is kind of confusing, but imo the thread can be considered resolved.

Setting back to RTBC per #66.

Committed a simplification, but added some comments about additional test cases.

NW for the open threads.

The fix from this MR is already in Drupal 11 core. It was probably added during the Symfony 7 upgrade.

In the current D11 core, PagerParameters.php already has both changes:

getPagerQuery() now uses array_map('intval', ...) to safely handle the page values.
getPagerParameter() now catches BadRequestException when someone sends page[]=1 instead of page=1.
The only small difference is that the MR uses get('page', '') while core uses get('page') ?? '', but the result is the same.

This MR is 1357 commits behind main and the pipeline is failing. Since the problem is already fixed in D11, I think we can close this MR. If a backport to 10.x is still needed, it would require a new MR against the 10.x branch.