Problem/Motivation

The code assumes that page is a string, passing in an array results in a warning on PHP 7 and a fatal error on PHP 8. We discovered this because a security scanner on one of our websites triggered a lot of those errors.

Steps to reproduce

Visit /node?page[foo]=bla:

The website encountered an unexpected error. Please try again later.
TypeError: explode(): Argument #2 ($string) must be of type string, array given in explode() (line 58 of core/lib/Drupal/Core/Pager/PagerParameters.php).

explode() (Line: 58)
Drupal\Core\Pager\PagerParameters->getPagerQuery() (Line: 49)
Drupal\Core\Pager\PagerParameters->findPage() (Line: 304)
Drupal\views\Plugin\views\pager\SqlBase->setCurrentPage() (Line: 929)
Drupal\views\ViewExecutable->initPager() (Line: 1444)
Drupal\views\Plugin\views\query\Sql->build() (Line: 1321)
Drupal\views\ViewExecutable->build() (Line: 392)

Proposed resolution

Ignore non-string page query parameters.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3314010-6.patch546 bytesnarendra.rajwar27
#4 interdiff-3314010-2_3.txt564 bytesanchal_gupta
#4 3314010-3.patch544 bytesanchal_gupta
#2 3314010-2.patch537 bytesimmaculatexavier

Comments

Berdir created an issue. See original summary.

immaculatexavier’s picture

immaculatexavier commented
Status: Active » Needs review
StatusFileSize
new 3314010-2.patch537 bytes

Attached patch for the stated requirement.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

an isset will not fix that, needs to verify additionaly to the !empty() that it is not an array.

anchal_gupta’s picture

anchal_gupta commented
StatusFileSize
new 3314010-3.patch544 bytes
new interdiff-3314010-2_3.txt564 bytes

I have uploaded the patch and addressed #3. Please review it

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That's also not it. explode() must not be called with a non-string. The problem is not the rsult of it, it's the input. Reproduce manually and you will see that neither change fixes this problem.

narendra.rajwar27’s picture

narendra.rajwar27
he/him
Primary language Hindi
Location India
commented
Status: Needs work » Needs review
StatusFileSize
new 3314010-6.patch546 bytes

Added check not array with the existing one. Tried with multiple arguments(request parameters) seems to be working fine.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Needs tests

Yeah, like that, now we need a test. Maybe we can extend \Drupal\Tests\system\Functional\Pager\PagerTest::testMultiplePagers with another case.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Sending back to NW for the tests requested in #7

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

johnv’s picture

johnv
Primary language Dutch
commented
Status: Needs work » Closed (duplicate)
Related issues: +#3143617: Protect pager against [] for the page

#3143617: Protect pager against [] for the page is older, has more activity .