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

Woah, interesting that you found the need to modify core's routing system for this! Will try to look into this soonish.

Here I have added re-roll of patch #41.

Hi @bradjones1,

I just added a re-roll of the patch.

I'd encountered the same issue. While I wasn't using subrequests in my case, I did encounter the same outcome.
The patch from #41 when applied to Drupal 8.8.2 seems to have helped! Will advise if anything new crops up on this front.

Thank you @bradjones1 for the deep dive on this -- diagnosing it and proposing a solution.

Hi @bradjones1,

I think I'm fighting with the same issue in this issue.

To test the patch or even trying to debug the issue, can you provide the steps how to reproduce the error?
I have no idea how to proceed and this really seems to be a complex issue.

And if it's even the same issue, we can close it as a duplicate.

I closed my issue because it tested the patch from #41 of course in production and it seems to fix the issue in 8.8.5 core. No idea why, but no errors appeared in the log during couple of days now.

Hi all.

I have reproduced this issue via an Ajax call, on a site using my config_entity_revisions module, which calls \Drupal::service('router')->matchRequest(\Drupal::request(), TRUE) from a form alter hook, triggering the issue. (Line 42 of webform_revisions.module, if it's any help).

Regards,

Nigel

I'm saying it fixed the issue, thanks.

Trying to fix testcases in #65.

Adding fix for the coding standards violation.
Regarding test case failure => It looks like the test case is fine but the fix seems to need to check for the query parameter.

I'm on Drupal core 8.9.6 and have applied patch #41 and am still seeing the error described in the description, specifically a dynamic page cached 400 response from /jsonapi/user/{entity} with message "CacheableBadRequestHttpException: The following query parameters violate the JSON:API spec: 'destination'". I am unable to reliably reproduce the issue.

I think I've identified steps to reproduce:

1. Make an unauthorized GET request to the user endpoint /jsonapi/user/{entity}
2. Notice the response is a 401 with Content-Type text/html
3. Make an authorized GET request to the exact same user endpoint /jsonapi/user/{entity}
4. Notice the response is a 400 with Content-Type application/json

I've also identified this: if I get the request to bypass lines 138-142 in docroot/core/modules/dynamic_page_cache/src/EventSubscriber/DynamicPageCacheSubscriber.php for one request, the next inbound request will render correctly. I assume this is simply because the dynamic page cache gets rebuilt on the request that bypasses it.

This leads me to conclude the request that gets the 401 response (steps 1 and 2 in my previous comment) is building a dynamic page cache value behind-the-scenes that gets returned as a 400 when a valid request comes in (steps 3 and 4 in my previous comment).

I'm assuming what I'm describing here still aligns with the intention of this ticket.

I've isolated the cache to "route:local.example.com:en:/jsonapi/user/123456ff-123f-123f-123f-123f123f123f:". Whatever is generating this route cache is doing so in a way that is adding "destination", here is an example cache value:

a:3:{s:4:"path";s:50:"/jsonapi/user/123456ff-123f-123f-123f-123f123f123f";s:5:"query";a:2:{s:11:"destination";s:1:"/";s:21:"_exception_statuscode";i:401;}s:6:"routes"....morestuff

And, I've isolated that issue as well. That route cache gets populated when metatag module is installed. I noticed this by the fact that if I put a return statement in metatag_preprocess_html and metatag_page_attachments the problem goes away. To be absolutely certain, I did an additional test of disabling the module entirely and noticed the problem went away.

Here's how I'm testing this locally:

1. Clear out relevant caches

mysql> delete from cache_data where cid like '%jsonapi%user%';
mysql> delete from cache_dynamic_page_cache where cid like '%user%';

2. Make a GET request to /jsonapi/user/123456ff-123f-123f-123f-123f123f123f with an invalid authorization, e.g. "Authorization: Bearer 123"
3. Start a step debug session and put a breakpoint at line 182 of docroot/core/lib/Drupal/Core/Routing/RouteProvider.php in the getRouteCollectionForRequest() methodwhere it populates the route cache and make sure $query_parameters is empty.

Alternatively, you can simply query the route cache table to see if you get a result that has "destination" and "_exception_statuscode" specified as query params:

mysql> select * from cache_data where cid like '%jsonapi%user%';

Important to note that I have simple oauth module installed.

I can't outright disable metatag module. That module is important to site functionality. So, my next step here would be to figure out the following: Why does an invalid authorization header to /jsonapi/user/{entity} generate an incorrect route cache entry in cache_data when metatag is installed, but when it is not installed no route cache entry is generated at all? The route cache entry is only generated on a successful request.

I confirmed that by just enabling metatag the issue does not occur. However, it does occur if I enable metatag AND add the metatag field to the basic page content type. The basic page content type is the node type that renders the 401 result. Maybe the path I need to go down here is to ensure /jsonapi/ routes do not return Content-Type text/html on a 401, i.e. don't render a node?

Ok, I have more insight.

The issue described in this ticket seems to be reproducible any time a request is made to /jsonapi/user/{entity} that results in an exception being thrown in the SimpleOauthAuthenticationProvider here: https://git.drupalcode.org/project/simple_oauth/-/blob/8.x-4.x/src/Authe.... And, metatag must be enabled with a field on the node that gets rendered on a 401.

When an exception happens, the DefaultExceptionHtmlSubscriber from core will add "destination" and "_exception_statuscode" to the sub-request it makes, see https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/lib/Drupal/C....

It's this sub-request that seems to be generating the invalid cache. Specifically, the logic hits core's RouteProvider here https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/lib/Drupal/C.... Which, sees the "destination" and "_exception_statuscode" in the query parameters and stores them in the cached route definition in the "cache_data" table.

Then, on a subsequent request that is valid, the route definition loads from cache with the "destination" and "_exception_statuscode" query parameters which then gets run through the JSON API validation and declared as a violation here: https://git.drupalcode.org/project/drupal/-/blob/8.9.x/core/modules/json....

I'm not sure where to go from here. Is the issue I'm seeing the same root cause described by the ticket? Is the issue actually with metatag module? Is it with JSON API module? Is it with route caching? This all seems highly interconnected to me; I can't quite pinpoint what I would declare as the exact issue that needs fixed.

Side note: When testing, its important to make sure dynamic page cache and the route cache are both cleared. If you just clear the route cache, but still have a dynamic page cache, you won't reproduce the error.

I've tested and confirmed that this hook will avoid the issue:

function mymodule_page_attachments(array &$attachments) {
  $request = \Drupal::request();
  if (substr($request->getPathInfo(), 0, 8) == '/jsonapi') {
    $metatag_attachments = &drupal_static('metatag_attachments');
    $metatag_attachments = FALSE;
  }
}

This avoids the route cache being poisoned with the destination and _exception_statuscode query parameters from a 401 request. I don't like this workaround, so I'd like to keep digging.

I see the token metatag is generating is for [current-page:url]. This line at 761 in tokens.tokens.inc gets called:

$url = Url::createFromRequest($request)->setOptions($url_options);

A question I keep coming back to is why JSON API doesn't handle the exception? I see that stepping through core/modules/jsonapi/src/EventSubscriber/DefaultExceptionSubscriber.php the isJsonApiExceptionEvent() returns false. So, maybe the root issue is how simple_oauth is throwing exceptions on JSON API routes? I see core/modules/jsonapi/src/Routing/Routes.php::routes() will set the JSON_API_ROUTE_FLAG_KEY flag and set _format to api_json, and these are missing from the request. Maybe simple_oauth is doing something to prevent this from happening?

I have found what, at this point, I would consider the root problem: Authentication runs before route filters. This means that Simple OAuth can throw an exception before the inbound request gets labelled as JSON API format. This, then, means the \Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase `onException` subscriber will catch the exception and render an HTML response.

I discovered this by observing that when the request gets to SimpleOauthAuthenticationProvider, the request "_format" attribute is not set. This means, $request->getRequestFormat() will return 'html'.

I'm thinking a solution to this problem could be focused on one of three areas in the code, each would result in a different change and philosophy about this problem:
1 - The issue is with \Drupal\jsonapi\Routing\EarlyFormatSetter. Determining the request as a "JSON API" request or not should not happen in a route filter. It should happen sooner in the request stack.
2 - The issue is with \Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber. It should not rely on determination of whether the request is a "JSON API" request solely based on whether the inbound request has the format of "api_json" set.
3 - The issue is not in how we're determining the request format. The issue is that JSON API needs its own authentication exception handler that catches anything on a JSON API route. Assumedly, this exception handler would know that route filters had not been run, therefore it would need to do its own determination as to whether the incoming request is JSON API or not.

Options 1 and 2 would require some sincere thought about the architecture of request handling specific to authentication and route filters.

Option 3 would be straightforward to implement, either in core or a separate module like JSON API extras.

Since I need a quick solution here, I'm going to focus on 3 and submit a proof-of-concept patch for that just in case someone else comes along and encounters this issue.

I worked on the above investigation with Joe

Yes, to @crasx' point, it was talking through with him that we figured out that requests get flagged as JSON API in a route filter, and that route filters run after authorization. MANY many thanks, @crasx!

And, I have a working solution! Here it goes:

mymodule.services.yml

services:
  mymodule.exception_subscriber:
    class: Drupal\mymodule\EventSubscriber\JsonApiExceptionSubscriber
    tags:
    - { name: event_subscriber }
    arguments: ['@jsonapi.serializer', '%serializer.formats%']

src/EventSubscriber/JsonApiExceptionSubscriber.php
(see attached file for exact example code)

// ...event class that extends JSON API's DefaultExceptionSubscriber
  protected function isJsonApiExceptionEvent(GetResponseForExceptionEvent $exception_event) {
    $request = $exception_event->getRequest();
    $router = \Drupal::service('router.no_access_checks');
    $router->matchRequest($request);

    if ($request->getRequestFormat() !== 'api_json') {
      return FALSE;
    }

    return TRUE;
  }

I'm not so sure I've got the logic here written in the best way, but it is working, so I'll consider this an OK proof-of-concept.

I've said LOTS of words on this ticket. I hope they are helpful. Does anyone else know enough about the original issue described in this ticket to confirm if everything I've been writing about aligns with this ticket? And, does anyone agree with the diagnosis or have a thought on a permanent solution?

Thanks @josephdpurcell for all your hard work on this issue. I managed to get your suggestion from #80 to work, which is a great help for me.

I changed a few things, because the GetResponseForExceptionEvent is deprecated:

<?php

namespace Drupal\mymodule\EventSubscriber;

use Drupal\jsonapi\EventSubscriber\DefaultExceptionSubscriber;
use Symfony\Component\HttpKernel\Event\ExceptionEvent;

/**
 * Serializes exceptions any time the request is JSON API.
 *
 * @link https://www.drupal.org/project/drupal/issues/3085360
 */
class JsonApiExceptionSubscriber extends DefaultExceptionSubscriber {

  /**
   * {@inheritdoc}
   */
  protected static function getPriority() {
    // Ensure this runs before JSON API's version of this runs.
    // @see parent::getPriority().
    return parent::getPriority() + 1;
  }

  /**
   * {@inheritdoc}
   */
  protected function getHandledFormats() {
    return ['html', 'api_json'];
  }

  /**
   * {@inheritdoc}
   */
  protected function isJsonApiExceptionEvent(ExceptionEvent $exception_event) {
    // Get the request and run a route match to ensure route filters are run.
    // The route filters will set the appropriate format on the request. If the
    // request format is "api_json", we know this is a JSON API request.
    $request = $exception_event->getRequest();
   
    /** @var \Drupal\Core\Routing\Router $router */
    $router = \Drupal::service('router.no_access_checks');
    $router->matchRequest($request);
    if ($request->getRequestFormat() !== 'api_json') {
      return FALSE;
    }

    return TRUE;
  }

}

VERY IMPORTANT detail: the $router->matchRequest($request) call may throw an exception. Why is this important? It may turn all would-be-404 pages into 500 errors.

I would love to see a proper fix for this so that I'm not using a patch. I'll try to circle back on this if I can, but for now wanted to note that about the matchRequest call causing 500s.

I haven't tested this patch, but I think this represents the concept of what we're thinking here.

Clarity! Sorry, I definitely didn't explain.

Ok, the comments from #80 and #81 have "code samples" of how you can work around the issue described by this ticket in a custom module.

The patch from #84 replaces the need for those code samples by fixing the problem in the JSON API module directly. So, if you are using the code samples, you don't need the patch, and vice versa. And, if you have both applied, it shouldn't cause an error or anything, it would just be redundant.

At this point, perhaps the proper state of this ticket is Needs Review? I'll set it to that. I will point out that there is no test written at this point. I did end up testing this locally and will be using this in my project.

Whoops! I made the patch in the wrong place. Ok, this patch is for Drupal core 9.1.0.

The patch from #87 is quite different than the patch from #41-69. I think what #41-69 was addressing was different than what #87 is fixing. Do these need to be merged together into 1 patch that addresses both?

I agree this needs a test. I'll put back to Needs work and add needs test tag.

There is something about calling $router->matchRequest($request); here that is causing the existing tests to fail. I think we can work around that by duplicating what \Drupal\jsonapi\Routing\EarlyFormatSetter is doing (which is what is required to determine whether the inbound request is JSON API or not). I don't like this approach because it results in duplicate code, but it works, and hopefully helps us triangulate a better approach.

Ok, I *think* I have a working test. In order to test, I created an auth provider that throws an auth exception. Without the fix in place that we're talking about, the test should error saying that the returned content-type is "text/html; charset=UTF-8", when it should be "application/vnd.api+json". And, this aligns with what we're seeing: the DefaultExceptionSubscriber isn't saying the exception should be JSON when it should!

I've created two patches, one with just the test, the other the test and the patch. So, we should see the test-only patch fail, and the test+code patch succeed.

Agreed! And, I'm happy to take any advice and guidance! Ultimately, I'm just hoping to get the issue I'm observing resolved in Drupal core.

In my previous comments I had tried to identify the root cause, and I think I've boiled it down to:
A) The fact that route filters do not run before authentication, which causes JSON API's EarlyFormatSetter to not be run, which causes the inbound request format to not be set when there is an exception during authentication.
B) The fact that JSON API's DefaultExceptionSubscriber assumes EarlyFormatSetter had already been run.
C) Some other generalized problem in the routing system I haven't identified yet.

I agree to let this ticket be for discussing A and C (any other generalized root problem in the routing system), and split B out into its own ticket since the solution is JSON API specific. I think that makes sense to me!

I'll work to pull out comments and patches from #70 onward to a new ticket. This ticket should then roll back to comment #69 for the most recent / relevant conversation. I'm so sorry to have cluttered this ticket with JSON API specific things!

Ok, I moved the issue I was describing over to #3187763: Use api_json format on exception responses for api_json resources. It's worth noting that the pre-#70 patches and the patch I'm attaching now does not also solve #3187763.

Looking back through the pre-#70 comment patches, I see this:

* #65 has the latest reroll, which is 3085360-route-provider-cache-query-string-65.patch
* #67 fixes the patch from #65, specifically the need for the return type declaration on setUp, this is 3085360-67.patch
* #69 fixes a code style issue from #65/#65, this is 3085360-69.patch

It took a few iterations over the tests to get them passing locally. I will point out that it looks like the cache ID has the query string duplicated. For example, instead of [query_parameters]=foo=bar it has [query_parameters]=foo=bar:foo=bar. I didn't know if there was a special reason for that so I left it.

I am setting the status to NW for an issue summary update. I will add the tag for that. While I am at it, I will remove the tag for a reroll, since that seems to be out of date.

I like this line from the Proposed resolution section of the current summary:

Adjust the route collection cache cid to include the query parameters stored on the Request object after early processing.

but I am not sure what to make of the rest of it.

Based on the line I quoted, and a quick look at the non-test portion of the patch, I think this issue is about cache IDs that are not specific enough, so we are fetching the wrong data from the cache.

Currently, when you create a new issue, the template includes a "Steps to reproduce" subsection, and the instructions include

If you are reporting a bug, it needs to consist of three things:

• What are the steps required to reproduce the bug?
• What behavior were you expecting?
• What happened instead?

From reading the current issue description, I cannot tell what the steps are to reproduce. In principle, I could read through the test portion of the MR and figure it out. But I am much more likely to review this issue if you do that work for me. I think the same is true of other potential reviewers. In principle, the steps for this issue should be pretty simple: make one request, then a second one, and get the wrong results.

If you can give two scenarios (form request and JSON:API endpoint), that is even better.

If the background information is complicated, then add additional subsections to the Problem/Motivation section.

I often quip that writing documentation is easy: provided that the only person reading it will be the person who wrote it. The same idea applies to bug reports.

Remaining tasks

Subsystem maintainer/core committer review.

Opinions differ on what belongs in this section. Some people routinely add "Patch, Review, Commit" as a bulleted list when creating a new issue. My opinion is that this is not helpful: it duplicates the information in the issue status. I typically leave this section blank for a new ticket, and update it as work progresses to show what needs to be done to get the issue ready for review.

Also, subsystem maintainers are not the only ones who can review issues. The "C" in RTBC stands for Community. Anyone who feels competent to review an issue (and who has not contributed to it) can promote an issue to RTBC. And my parenthetical caveat is not an absolute rule.

@bradjones1 can you (or a core committer!) edit the MR to be based on 9.3.x? Thanks!

DrupalCI is failing to upgrade PHPUnit, so none of those tests are running:

04:38:21 > Drupal\Core\Composer\Composer::upgradePHPUnit
04:38:21 > Drupal\Composer\Composer::ensureComposerVersion
04:38:21 Loading composer repositories with package information
04:38:22 Dependency "phpspec/prophecy" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.
04:38:22 Updating dependencies
04:38:22 Your requirements could not be resolved to an installable set of packages.
04:38:22 
04:38:22   Problem 1
04:38:22     - Root composer.json requires drupal/core 9.3.x.dev, found drupal/core[9.3.x.dev] but the package is fixed to 9.4.x-dev (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
04:38:22   Problem 2
04:38:22     - Root composer.json requires drupal/core-project-message 9.3.x.dev, found drupal/core-project-message[9.3.x.dev] but the package is fixed to 9.4.x-dev (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
04:38:22   Problem 3
04:38:22     - Root composer.json requires drupal/core-vendor-hardening 9.3.x.dev, found drupal/core-vendor-hardening[9.3.x.dev] but the package is fixed to 9.4.x-dev (lock file version) by a partial update and that version does not match. Make sure you list it as an argument for the update command.
04:38:22 
04:38:22 Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.
04:38:22 Script @composer update phpunit/phpunit --with-dependencies --no-progress handling the drupal-phpunit-upgrade event returned with error code 2
04:38:22 Script Drupal\Core\Composer\Composer::upgradePHPUnit handling the drupal-phpunit-upgrade-check event terminated with an exception
04:38:22   ERROR: PHPUnit testing framework version 9 or greater is required when running on PHP 7.4 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.

Would testing on PHP 7.3 avoid those errors?

Not really sure that's the problem, it seems DrupalCI thinks it is installing against 9.3.x, but the branch is set to 9.4.x?

Root composer.json requires drupal/core 9.3.x.dev, found drupal/core[9.3.x.dev] but the package is fixed to 9.4.x-dev

At least, with PHP 7.3, the run test are 28971.

Thanks for the steps to reproduce! Tagging for testing.

Next steps:

1. Code review.

2. Manual testing.

Patch applies cleanly to 9.3 and 9.4 and with offsets for 10. It still needs testing per the steps to reproduce in the issue summary.

[drupal-10.0.x-dev/10.0.x] [drupal-10.0.x-dev]$ patch -p1 < 183.diff 
patching file core/lib/Drupal/Core/Routing/RouteProvider.php
Hunk #1 succeeded at 451 (offset -53 lines).
Hunk #2 succeeded at 461 (offset -53 lines).
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

Agree this doesn't need manual testing, the automated test coverage is comprehensive and covers it better than a manual tester would.

I reviewed the MR and there's a number of bits of feedback that need addressing.

Thanks @longwave and @bradjones1. I'm just in the habit of tagging for testing if a bug can be tested but I defer to @longwave on this one :)

I tried to address @longwave comments on the MR.

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.

@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 ;))

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.

This is potentially all for a follow-up rather than here, but it feels like if the system files path processor had used a request attribute instead of a query string in the first place, we'd never have had this problem.

Opened #3450279: Use attributes instead of query string in PathProcessorFiles and stop replacing query string in RouteProvider::getRouteCollectionForRequest() 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.

EDIT: Looking at the code I don't really expect this MR to change anything for the above. I don't know whether there would be a blocker for converting the private file system's path processor.

EDIT2: I think it will become a blocker for Private File system.

Using query to transfer the path:
The filepath query item is generated from the decoded URL which has %20 replaced with " ". This query item is cached. The path is also cached and does not contain the original filepath since that's not part of the route match (it was captured in the processor as excess and moved into the query).

On the next request the path is matched and the path (without filepath) is restored along with the URL decoded query which contained the filepath.

Using the attributes to transfer the path:
The filepath attribute item is generated from the decoded URL which has %20 replaced with " ". This attribute is _not_ cached. The path is cached but does not contain the original filepath since it's not part of the route match.

One the next request the path is matched and the path (without filepath) is restored. The URL decoded query is not present because the file processors haven't run and the attribute is not part of the cache. The attribute will no longer be provided to the controller. To work around this it'll have to reprocess the original URL. This was something I didn't understand before. This uses the raw URL which hasn't passed through any path processors so it must decode the URL itself.

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 #3450279: Use attributes instead of query string in PathProcessorFiles and stop replacing query string in RouteProvider::getRouteCollectionForRequest() 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!

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...

Here's the rejected hunk:

@@ -84,7 +84,6 @@
     $this->getContainer()->get('stream_wrapper_manager')->register();
 
     // Create the build/artifacts directory if necessary.
-    $realdir = realpath('public://');
     if (!is_dir('public://simpletest') && !@mkdir('public://simpletest', 0777, TRUE) && !is_dir('public://simpletest')) {
       throw new \RuntimeException('Unable to create directory: public://simpletest');
     }

@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.