Private files access may break with certain config

I cannot reproduce this problem.

An issue about private files has been fixed recently, see #3222717: Redirecting to a file with spaces in the name results in 404 and #3200138: Leading slash for destination param should not be stripped, causes URL encoding issues.

Can you retry on latest dev to see if you still reproduce ?

I could reproduce this exact problem and the patch with the additional 'user' cache context fixed the problem for me.

This patch also worked for us (the problem started occurring around Drupal 9.2.9, FYI). +1 for RTBC.

These adjustments to the test should demonstrate the issue. This patch only includes the test adjustments, so testbot should confirm the issue exists by coming back with a failure.

And here is the test adjustments + a fix.

This fix is different than the initial proposal of adding the 'user' cache context. There are a couple reasons for the adjustment:

1. That may work for the case where the logged in user is having a problem, but there would still be issues for 2 unauthenticated users trying to access files should still run into issues because they're both "the same user".
2. The 'user' cache context makes for so much variation that cacheability may be fairly low. 'session.exists' is just a boolean for whether you're logged in or not, so there should be a higher hit rate. Also including 'url.path' since the response for file 1 is not necessarily the same as for file 2.

We are experiencing similar issues with private files and r4032login redirects.

I think the addition/tweaking of cache contexts makes sense but I would disagree that session.exists is best. Anonymous users can have sessions but I can't think of scenario for the same URL where two anonymous users should get a different response from this module or where an anonymous-user-with-session should get the same response as an authenticated user

Indeed by default Drupal won't render/dynamic-page cache anything with the user cache context.
https://git.drupalcode.org/project/drupal/-/blob/9.4.x/sites/default/def...

But user.role:anonymous should allow some caching but with the least amount cache fragmentation.

Additionally, I think the url.path cache context should be changed to just url so that the query string is also considered. In the case of private files, we've observed that sometimes Drupal will rewrite URLs like /system/files/path/to/a/file to /system/files?file=/path/to/a/file for some reason (in our case it happens when specifying a private file as a Redirect module destination).

Sounds reasonable to me. Here are those tweaks.

I spent again a lot of time trying to reproduce this until I realize that the problem occurs only when the Dynamic Page Cache module is enabled.

The provided test helped me a lot to better undestand the situation, thanks for that !

So there is 2 problem actually :

Problem 1 : Dynamic Page Cache cache the redirection without considering the access of the user since we do not provide access cache context

So if a user accede to a page he has not access to, the redirection will be set in cache for all users, authenticated or not.

Problem 2 : Dynamic Page Cache manage its cache per route and not per url

So when we return our redirection, we return it for all url corresponding to this route.
For example in the case of this issue we make a redirection for the "system.files" route for the "private" scheme.
So as soon as we make a redirection for a private file, all private files become inaccessibles.

This is a more general problem actually that juste private files access.

We could try to determine our own cache tags and contexts but there is too much cases to considere so probably we should better try to follow what the core do.

An easy way would be to base our class on the default 403 event subscriber and retrieve the cache metadata for the original response provided by the core.
We just need to add the client error tag then, as the core will not do it since our response status code is not necessary a 4xx code.

With this solution we are mostly sure that our redirection is cached exactly as a default 403 would be so I hope this will solve all caching issues.

Here is the new patch containing the test and the new cache tag approach.

If test pass I will merge it directly.

The tests don't pass but after thinking about it I wonder if it is really necessary to get the cache metadata from the 403 page itself.

We could more simply apply the cache metadata of the initial response and thus those of the access result.

In the case of a private file, the behavior of the core is to not cache the response in Dynamic Page Cache so by reusing the same cache metadata we will have the same behavior and therefore our redirection will not be cached either.

This avoids that our module applies a different behavior than the one the core would have applied if it had displayed the 403.

If later the core evolves and a 403 on a private file is finally cached with a variation by url or by role, then this module will automatically follow this evolution.

I re-upload the 2 patches in this simplified version with test only and the patch to see if everything is ok now.

Thanks to all !

Committed to 2.x-dev, will be released in 2.1.1.