Processing paths in LanguageNegotiationUserAdmin causes 404s while accessing private files

Hi @drikc,
Did you solve this problem. I am also getting 404 page not found when try to access private files for all users include admin user also.
Please let me know do you have any suggestions regarding this problem.

Thank you.

Discussed something very similar with @esolitos in IRC we discovered a problem with \Drupal\user\Plugin\LanguageNegotiation\LanguageNegotiationUserAdmin::isAdminPath(). Breaking private files is at least a major.

The reason why the path processor is not re-entrant is \Drupal\image\PathProcessor\PathProcessorImageStyles::processInbound()

Here's a failing test.

Cloning the request seems the simplest solution.

This is a straight up bug and should be fixed in 8.3.x

+++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
@@ -127,10 +127,14 @@ protected function isAdminPath(Request $request) {
+          // Some inbound path processors make changes to the request. Make a
+          // copy as we're not actually routing the request so we do not want to
+          // make changes.
+          $cloned_request = clone $request;

This is something which really should be kept in mind all the time. Good to know!

This looks great for me!

Thinking about other places that call ->processInbound()

\Drupal\Core\Path\PathValidator::getPathAttributes()
This one is okay because earlier it does:

    $request = Request::create('/' . $path);
    $attributes = $this->getPathAttributes($path, $request, $access_check);

The same pattern is in \Drupal\system\PathBasedBreadcrumbBuilder::getRequestForPath() too

I think we probably should update the documentation on \Drupal\Core\PathProcessor\InboundPathProcessorInterface::processInbound() to make this consideration known. So that people can discover this gotcha.

Better docs.

Thanks @alexpott. #10 patch worked perfectly for me.

Hi, I wanted to work on this yesterday, but i was sick as hell, provided patch looks and works great, tested on 8.2.6, thanks alexpott for the quick work!

@drikc
Don't worry you were not the only one, I spent 1 day figuring out the issue and then in half day (mostly) thanks to alexpott (and the other peeps in #drupal-contribute) we were able to narrow it down to the LanguageNegotiationUserAdmin::isAdminPath() issue.

+++ b/core/lib/Drupal/Core/PathProcessor/InboundPathProcessorInterface.php
@@ -12,10 +12,17 @@
+   *   The HttpRequest object representing the request to process. Note, if this
+   *   method is being called via the path_processor_manager service and is not
+   *   part routing, the request then the current request object must be cloned

The sentence starting at 'Note, if this ..' is a bit weird if you read this out load.

As far as i know this works in english, it's quite common writing "Please note, foo bar baz".

Well, it's more the second part: "and is not part routing, the request then the current request object must be" - the 'then' doesn't seem to belong there at all ..

@swentel I realized that after a while.
Here's a rerolled patch on 8.3.x with the comment change and updated to the latest commit.

Reroll for 8.4

There's another side effect due to the next line in LanguageNegotiationUserAdmin:

$attributes = $this->router->match($path);

When visiting a node translation (e.g. /node/fr/123) the content appears in the site's default language.
When commenting it out and returning TRUE or FALSE instead, the proper language is shown: french when FALSE, the users admin language when TRUE.

See match() in AccessAwareRouter:

  /**
   * {@inheritdoc}
   *
   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   *   Thrown when access checking failed.
   */
  public function match($pathinfo) {
    return $this->matchRequest(Request::create($pathinfo));
  }

The request object is created statically so I assume something's still going wrong within matchRequest():

  /**
   * {@inheritdoc}
   *
   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
   *   Thrown when access checking failed.
   */
  public function matchRequest(Request $request) {
    $parameters = $this->router->matchRequest($request);
    $request->attributes->add($parameters);
    $this->checkAccess($request);
    // We can not return $parameters because the access check can change the
    // request attributes.
    return $request->attributes->all();
  }

Bumping this to critical alongside #2802403: Combination of language negotiation and path aliasing can cause a corrupted route cache, 404s - visitor-facing 404s make the site unusable and there's no workaround.

re #26 / @mpp that sounds like a separate issue.

Cut out an unnecessary then.

Recently, after updated Drupal Core to 8.5 all my private files became unreachable randomly. Really disturbing.
The cause seems to be that the route, at some point, /system/files/DOCUMENT was not rewrite by the PathProcessorFiles to /system/files (or may be the original path was added after the PathProcessorFiles ran on the path)

Tried patch #29 which fix this main critical issue. But this bugs occurs just after updating to 8.5 so I have some doubt if this is the same issue.

I recently updated to Drupal Core 8.5.4, and now links on my site that are in the form of http://www.somesite.com/index?sometoken=XXXX are having the "?sometoken=XXXX" part of the URL stripped out. The result can be failure to be recognized as an authorized user at the somesite.com site. Is this problem related to this thread? It seems that the problem occurs for URLs that are using the Redirect module, but the problem began occurring right after I updated Core to 8.5.4.

after applied the patch 25 then the patch 29， the private files read ok

thanks for you guys！

Patch from #29 should be the way to go.

Fixed on commit

+++ b/core/lib/Drupal/Core/PathProcessor/InboundPathProcessorInterface.php
@@ -12,10 +12,17 @@
+   * other that the current request.

%s/that/than

Committed as bfed684 and pushed to 8.7.x.

Cherry-picked as 8480003 and pushed to 8.6.x.