RouteProvider::getRouteCollectionForRequest() can poison query string of next request

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.

Commit was because of this discussion: https://drupal.slack.com/archives/C5A70F7D1/p1694441795848429

With this patch there were issues with symfony 6. Last commit was to fix that.

Great work on this one, it makes sense from a high level. Functionality appears to be covered by tests.

One more thing that the core maintainers might like to see is a tests only run, I was about to do this but have been summoned elsewhere.

There is a test-only MR that is a bit out of date but the previous experience with it at least demonstrates what the issue is. I can put in the work to update it if need be, however this subsystem is very low-change so hopefully it's clear enough from the issue history and prior runs.

@bradjones1 , it looks like @longwave comments have been resolved.

I've rebased the test only issue, cause i kinda want to see a fresh run of that as part of the review. Asked for a retarget in @contribute.

There is still a few open comments on the main MR unfortunately, also awaiting retarget of test branch so we can have a recent test only run. (although that is almost automated on gitlab ci ;))

bradjones1 → changed the visibility of the branch 3085360-exception-poison-test-only to hidden.

bradjones1 → changed the visibility of the branch 3085360-exception-poison-11-x to hidden.

Rebased and updated the cache ID creation logic to ensure it's normalized recursively with any nested arrays.

Raised this PR against Symfony upstream to do so out of the box in the Request object, but I won't hold my breath.

Optimistically marking this NR again.

Al issues are addressed. Code still looks good. IS is still correct. Changerecord is not needed. All seems ready for commit.

Somehow I haven't seen this issue before.

My first thought was 'why do we allow incoming path processors to set query parameters?' and then I re-read the original issue that added the caching, realised it was opened by me, and it all came back.

So the history of this bug is something like this:

1. The router was rewritten on top of Symfony very early in the Drupal 8 cycle, it was railroaded through and almost designed from the start to erode performance, scalability and security (see #1793520-7: Add access control mechanism for new router system → and elsewhere for some background). A lot of things did not work with the initial commit, and only got found out about in some cases years later when we were still trying to release Drupal 8 (like e.g. the entire menu links system which didn't really start getting fixed again until about 2014).
2. More than year later, the system files controller got converted #1987712: Convert file_download() to a new style controller → .
3. PathProcessorFiles got added to deal with the fact that symfony doesn't like arbitrary/path/parts/when/looking/up/potential/routes
4. We then added caching on top of of this, which had to take into account what PathProcessorFiles does to the query string in #2480811: Cache incoming path processing and route matching →
5. I think this is the first time any of 2-4 has been revisited since it was added.

As the issue summary says this is all a bit problematic.

I think my question is:

Could PathProcessFiles set a request attribute instead of a query string, which we then retrieve in FileDownloadController? Then we'd need to save the request attribute(s) in the cache item instead of the query string, but because that's not overwriting information from the request it should be fine. For that specific use case we could almost add a generic 'original request path' request attribute that preserves what came in before path processors run.

If we completely stop adding the query parameters to the cache item, that might break some other code somewhere that's doing something similar to PathProcessorFiles, but could we possibly only do that if the query parameters differ after incoming path processing has run, and trigger a deprecation error if they do? Then we could stop supporting that behaviour altogether in 12.x then

Thanks for the review, @catch. I agree a request attribute would likely fix this - though the BC layer would have to look something like this MR anyway if we want it to actually work with caching. And given D10 is LTS, I think it's still worth "fixing" even if it contains a long-lived deprecation that gets cleaned up in D12. I agree it could be a second issue and worth cleaning up.

TL;DR, I think we still need this bugfix regardless if we decide to change the underlying bug, since it's entirely possible people are using query strings internally as cheapo attributes.

Opened 📌 Use attributes instead of query string in PathProcessorFiles and stop replacing query string in RouteProvider::getRouteCollectionForRequest() Active for the follow-up. Haven't done a line by line review here so leaving this at RTBC.

I just want to chime in here that the proposed PathProcessor is exactly what we implemented in Open Social where I found the problems with query on cacheable requests (which is why I assume the private file system didn't suffer) and moved to attributes instead.

However, one issue we're seeing is that there is a difference between the initial uncached request and secondary requests that utilise the page cache. Specifically we're seeing that for the first request the attribute that's set is properly run through Drupal\Core\PathProcessor\PathProcessorDecode but for subsequent requests that doesn't happen. This causes the attribute to be inconsistently decoded or encoded which makes downstream code brittle and can cause errors.

I'm not sure if that's fixed with the MR proposed here (I have to look at the code) but at least wanted to share that issue because maybe someone already knows how this MR might affect that behaviour.

This was shortly discussed on Slack where Bard added

I would imagine attributes aren't currently cached because they weren't considered at the time the system was first built out and/or weren't thought to be something that would materially affect the cacheability of the response. As solutions have matured there's more of a need for this stuff, but the caching layer hasn't caught up

And catch confirmed

None of this particular code has been touched since about 2013, it was all stop gaps around the routing system which was extremely broken at the time.

So in that sense the private file system will likely need attribute caching in 📌 Use attributes instead of query string in PathProcessorFiles and stop replacing query string in RouteProvider::getRouteCollectionForRequest() Active but that can likely happen as part of that issue and I don't see a reason to block this issue itself. We don't currently see a reason why attribute caching wouldn't work.

Happy to leave this RTBC (also after looking at the code) :D

Reviewed this again. I think we should try to get the follow-ups done so that we can stop caching by query string again, but what's here is good, and the test coverage should ensure that any changes we make in the follow-ups don't break things unexpectedly.

Did my best with issue credit here but it's a very long issue and ctrl-f isn't working to find people's usernames properly for some reason.

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

Thanks!

One quick clarification though:

so that we can stop caching by query string again

We've always cached by query string... the change here is that we are caching based on both the original query string and any changes after processing. Honestly I think this would have to remain even if we change the original reason for this being a bug. For instance, I am setting some "default" query parameters in a path processor on my current project, which I think is a legitimate use of this pattern. (Instead of core stashing away some metadata derived from the query string.) So there's additional discussion to be had about pulling support for this altogether. But yes, follow-ups definitely appropriate. Thanks for closing this out! Bolstering my reputation as a necromancer.

Hi, I'm trying to apply a patch based on MR183 to 9.5.x but it's failing at TestRunnerKernel.php

➜  drupal git:(9.5.x) ✗ patch -p1 < ./patches/183.patch
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/modules/system/tests/modules/router_test_directory/router_test.module'
patching file 'core/modules/system/tests/modules/router_test_directory/router_test.routing.yml'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestServiceProvider.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/TestControllers.php'
patching file 'core/tests/Drupal/FunctionalTests/Routing/RouteCachingQueryAlteredTest.php'
patching file 'core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php'
patching file 'core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/TestControllers.php'
patching file 'core/tests/Drupal/FunctionalTests/Routing/RouteCachingQueryAlteredTest.php'
patching file 'core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php'
patching file 'core/tests/Drupal/FunctionalTests/Routing/RouteCachingQueryAlteredTest.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/lib/Drupal/Core/Test/TestRunnerKernel.php'
1 out of 1 hunks failed--saving rejects to 'core/lib/Drupal/Core/Test/TestRunnerKernel.php.rej'
patching file 'core/modules/system/tests/modules/router_test_directory/src/RouterTestEarlyExceptionSubscriber.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/modules/system/tests/modules/router_test_directory/src/TestControllers.php'
patching file 'core/lib/Drupal/Core/Routing/RouteProvider.php'
patching file 'core/tests/Drupal/KernelTests/Core/Routing/RouteProviderTest.php'
➜  drupal git:(9.5.x) ✗ 

Maybe related to the failed test on this commit?: https://git.drupalcode.org/project/drupal/-/merge_requests/183/diffs?com...

@marcofernandes Commenting on a closed issue is unlikely to get you a response. Also, D9 is unsupported, so the general recommendation will be to upgrade first.

@bradjones1 Yes, I understand that 😉 but my comment was meant as a heads-up for people who are still in the 9.x and comes across this issue. The solution was just remove that rejected hunk.

Automatically closed - issue fixed for 2 weeks with no activity.