Problem

It seems until a route access checker gets called, a new Request gets created several times based on the raw path and because of that, special characters from query parameters get lost. So if you have an access checker that checks access based on a value of a query parameter that may contain special characters (ex.: a hash) then the access checker can return false-positive results. This probably happens because special characters in query params have not been URL encoded.

Callstack:
callstack 1
callstack 2

Proposed solution

Probably everywhere where new Request gets created from a raw path, the path should be exploded to parts and we should warrant that all query parameters get URL decoded before the new Request object is created. Ex.: \Drupal\Core\Url::toUriString()

Remaining tasks

Determine solution

CommentFileSizeAuthor
#16 2902797-16.patch5.2 KBsmustgrave
#16 interdiff-14-16.txt498 bytessmustgrave
#14 2902797-14.patch5.2 KBsmustgrave
#14 interdiff-9-14.txt911 bytessmustgrave
#9 interdiff_test_only_vs_without_special_char.txt1.19 KBmxr576
#9 pathvalidator-query-parameter-malformed-test-only-without-special-char.patch4.99 KBmxr576
#9 pathvalidator-query-parameter-malformed-test-only.patch5 KBmxr576
#4 interdiff_test_only_vs_without_special_char.txt1.19 KBmxr576
#4 screeshot-2020-07-21-16-33-06.png80.38 KBmxr576
#4 screeshot-2020-07-21-16-32-23.png80.77 KBmxr576
#4 pathvalidator-query-parameter-malformed-test-only-without-special-char.patch5.02 KBmxr576
#4 pathvalidator-query-parameter-malformed-test-only.patch5.03 KBmxr576
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joey-santiago created an issue. See original summary.

Version: 8.3.4 » 8.3.x-dev

Core issues are now filed against the dev versions where changes will be made. Document the specific release you are using in your issue comment. More information about choosing a version.

Version: 8.3.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Branches prior to 8.8.x are not supported, and Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Version: 8.9.x-dev » 9.1.x-dev
Category: Support request » Bug report
Priority: Normal » Major
Issue summary: View changes
FileSize
pathvalidator-query-parameter-malformed-test-only.patch5.03 KB
pathvalidator-query-parameter-malformed-test-only-without-special-char.patch5.02 KB
screeshot-2020-07-21-16-32-23.png80.77 KB
screeshot-2020-07-21-16-33-06.png80.38 KB
interdiff_test_only_vs_without_special_char.txt1.19 KB

@joey-santiago, I am updating this issue because I think we have bumped into the same issue. Please let me know if I have misunderstood your problem.

I have created a POC test just to confirm that the issue exists. (Also uploading a seperate test which confirms the POC test is valid :) )

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Title: Using PathValidator to check access to a URL with GET » PathValidator: query parameters gets malformed meanwhile access checking
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
Title: PathValidator: query parameters gets malformed meanwhile access checking » Query parameters get malformed meanwhile access checking
Issue summary: View changes

Probably this is the doing of \Drupal\Core\Routing\AccessAwareRouter::matchRequest() (partially) so I have removed PathValidator from the title.

joey-santiago’s picture

joey-santiago CreditAttribution: joey-santiago commented

wow! at first sight, that looks exactly what i was experiencing, but i haven't looked at the issue in the past 2+ yrs, so i don't recall how i found this out :) I'll try getting a test environment up to test this out

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented

For the record, the test-without-special-char only failed because of some deprecation errors.

  1x: Declaring ::setUp without a void return typehint in Drupal\KernelTests\Core\Path\PathValidatorTest is deprecated in drupal:9.0.0. Typehinting will be required before drupal:10.0.0. See https://www.drupal.org/node/3114724
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 at Pronovix commented
FileSize
pathvalidator-query-parameter-malformed-test-only.patch5 KB
pathvalidator-query-parameter-malformed-test-only-without-special-char.patch4.99 KB
interdiff_test_only_vs_without_special_char.txt1.19 KB

Silenced deprecation warnings.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Active » Needs review
Issue tags: +Bug Smash Initiative
FileSize
interdiff-9-14.txt911 bytes
2902797-14.patch5.2 KB

Rerolled for 9.5.x

Status: Needs review » Needs work

The last submitted patch, 14: 2902797-14.patch, failed testing. View results

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs work » Needs review
FileSize
interdiff-14-16.txt498 bytes
2902797-16.patch5.2 KB

Deprecation fix

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
Status: Needs review » Needs work

Self review

All that was added was the tests. That was my mistake and one of my first issues.

Version: 10.1.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, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.