[regression] Language switcher block throws exception when no route is matched

Thanks for reporting, marking as critical as this is a regression in the security release.

https://www.drupal.org/node/3156530

Several contrib modules that integrate with the language switcher have found they had to change... of course if a fix can be produced in core, it's possible they'll have to change back?!

Here's a fix that falls back to the old behaviour if we don't have a route match.

Not sure if this is a separate issue, but because the inaccessible links are now filtered out by core, implementations of hook_language_switch_links_alter() can no longer easily provide alternative links, e.g. to the site home page (which is a common requirement). Does that need treating as a separate regression issue?

The fix looks good, but this needs a test.

@james.williams Is this an actual case in an a contrib module (or custom module that you know of)? Should we consider moving the access check to after the alter hook is invoked?

@longwave It's certainly the case for the work that has been going on in #2816701: D9 Language Switcher, which was looking to change links to untranslated pages, to instead point at the homepage. Those links to untranslated links have now (understandably) been removed. Moving the alter hook to after the filtering would work. Alternatively, could the unfiltered links be passed to the alter hook, or the links simply get an 'access' property set on them perhaps?
e.g.

<?php
$result = $this->negotiator->getNegotiationMethodInstance($method_id)->getLanguageSwitchLinks($this->requestStack->getCurrentRequest(), $type, $url);
foreach ($result as $language_id => $link) {
  // Perform access checks, then simply set:
  $link['access'] = $access;
}
// then do the alter hook
// *Finally*, remove links that have a falsey $link['access'].

And I've also got client sites where the language switcher block has been similarly altered, to change links to untranslated content, to link to the homepage instead. So the switcher block always contains links to all the available languages, just linking to different things. The switcher block can no longer be relied upon to display which languages a site offers, only the languages the current page is available in. Given that language switcher links are often displayed quite prominently in things like headers, I suggest this common use case has been broken.

@james.williams we try not to add API as part of a security release, so we would prefer not to add an 'access' key here. But if moving the access check after the alter hook would work for all the cases you know of, then maybe that is a good option? I think this is worth discussing in a separate issue, given that this one is focussed on the missing block in the 404 casse.

Thanks; I've opened #3348686: [regression] Inaccessible language switcher links are removed before alternatives can be provided, based on this one, to suggest we move that alter hook then. It's possible that that one might not count as critical like this one, since it probably only affects sites that have implementations of the alter hook from outside core. Though it's critical for those sites!

So interestingly this problem is caused by bigpipe - if you disable the module then 404s on standard have the language switcher. For some reason with bigpipe 404s don't have the system.404 route when they really should.

Language block is uncacheable and gets placeholdered, so it is rendered in the context of the main request

Language block is uncacheable and gets placeholdered, so it is rendered in the context of the main request

So this is a problem right? Because the block should know it's on a 404 or 403.

Like I wonder what this means for #2245767: Allow blocks to be configured to show/hide on 200/403/404 response pages.

Also it's not just that is being rendered in the context of the main request - it is being rendered without a route match - which should be impossible. In the context of rendered a block I think Url::fromRouteMatch(\Drupal::routeMatch()) should always work.

We can do something like

    $exception = \Drupal::requestStack()->getCurrentRequest()->attributes->get('exception');
    if ($exception instanceof HttpExceptionInterface) {
      $url = Url::fromRoute('system.404');
    }
    else {
      $url = Url::fromRouteMatch(\Drupal::routeMatch());
    }

(obviously we'd need to do something a bit better to work out which route - but it feels a bit off that the language block has to concern itself with what to do here.

Submitting a new patch for this issue. Please review.

Thanks

Here's a test that proves we fix this in the way it worked before the recent security change. I don't think the big_pipe behaviour is correct but I think it is okay to re-instate the previous behaviour in this fix and then we can address the wider issue of the current route match during block rendering for logged-in users during big_pipe rendering.

1. +++ b/core/modules/language/src/Plugin/Block/LanguageBlock.php
   @@ -84,7 +84,9 @@ protected function blockAccess(AccountInterface $account) {
   +    $route_match = \Drupal::routeMatch();
   +    $url = $route_match->getRouteObject() ? Url::fromRouteMatch($route_match) : Url::fromRoute('<front>');
   

   We should add a comment to detail why this is necessary.

2. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -181,6 +190,53 @@ protected function doTestLanguageBlockAnonymous($block_label) {
   +    // Assert that the language switching block is displayed on the frontpage.
   

   Needs updating.

So interestingly this problem is caused by bigpipe - if you disable the module then 404s on standard have the language switcher. For some reason with bigpipe 404s don't have the system.404 route when they really should.

Language block is uncacheable and gets placeholdered, so it is rendered in the context of the main request

So this is a problem right? Because the block should know it's on a 404 or 403.
...
Also it's not just that is being rendered in the context of the main request - it is being rendered without a route match - which should be impossible.

We need to decide whether we commit a workaround here, or spin the bigpipe problem off to its own issue.

I think this is kind of BigPipe's fault but also kind of our weird handling of 404 and 403 requests.

DefaultExceptionHtmlSubscriber::makeSubRequest() is where it all happens.

We clone the main request, and keep everything identical, except for faking a route match for system/403 or system/404. The parent request doesn't have a route, because there isn't one, because nothing was matched.

So... a possible solution would be to also fake the route match on the main request, then BigPipe or anything else that runs in the main request content would have that routing information to go on too.

I think this is kind of BigPipe's fault but also kind of our weird handling of 404 and 403 requests.

I take this back, BigPipe was unfairly maligned and can't be held responsible, it's all the weird 404/403 handling.

Here's a patch that implements #21.

The 'interdiff' is not really an interdiff, it's just the code for the new approach and a one line change to the bigpipe part of the test which shows identical handling for anon and auth users now. The workaround added in previous patches is no longer necessary at all.

New test passes locally for me, see if it breaks something else though. I tried to rewrite the comment to explain the new approach, but it might need further work.

I think the consistency that #22 brings is good. If you make a request for /does-not-exist - then it is the system.404 route that will be making the response. So adding this information to the main request feels correct. I do wonder about unintended impacts of making this change.

Test failures for #22 show it's not that simple, predictable but it was worth a try anyway.

I think we need to open a spin-off to refactor the custom 404/403 handling - IMO it should not be a route at all, we could use a configurable entity reference and/or provide an easy way to swap the behaviour from a module. But pointing to a route and doing a subrequest is janky and this bug shows how much.

That means we might want to go ahead here with #18. I think the bigpipe part of the test should make it clear that the behaviour being tested is a bug.

FYI

Another patch, one line code change, fixes the issue for us here:

#3349111-04: [regression] Core language switcher disappeared in Drupal 9.5.5

Patch #18 above also works for us! Thanks for this! With that said, a far simpler patch is working and found here: #3349111: [regression] Core language switcher disappeared in Drupal 9.5.5

I think we should try the patch from #3349111: [regression] Core language switcher disappeared in Drupal 9.5.5 with the tests from here, approach there seems like less of a workaround.

@catch @amaria,

Still reviewing.

Funny enough, I actually got better results with the simpler patch as mentioned
#3349111-04: [regression] Core language switcher disappeared in Drupal 9.5.5

Patch 18 only worked on our home page, not on other pages. Whereas the patch 3349111 works on all pages I've looked at so far.

FYI: I combined tests from #18 into the 3349111 patch, running tests now.
#3349111-11: [regression] Core language switcher disappeared in Drupal 9.5.5

This passed tests in 3349111-11 , @cilefen asked to concentrate on this issue instead.

This patch works for us, however #18 only works for us on our <front> page. With this new patch number 34, it seems to work on all the pages I've tested.

So uploading interdiff and new patch (#34)

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -418,7 +418,7 @@ public function getLanguageSwitchLinks($type, Url $url) {
-              return $url instanceof Url && $url->access();
+              return $url instanceof Url && $url->access(NULL, TRUE);

There's no test coverage of this part of the change and I have absolutely no idea why this is necessary. It's going to break the security fix because it will return TRUE when it should be returning FALSE.

Here's #18 with #19 fixed and the comment @catch wants added. I really hope people are not deploying #34 to any production sites as this approach reverts the security fix.

@alexpott, yes it looks wrong to me,
however shouldn't access be checking for current user instead of NULL?
I'm working on a patch , trying to dependency inject current user into that access call much like line 655 of modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
How can checking access on NULL be of any use to us in this case, it's going to be broken.

should be something like this instead: $url->access($this->currentUser)

1. +++ b/core/modules/language/src/Plugin/Block/LanguageBlock.php
   @@ -84,7 +84,11 @@ protected function blockAccess(AccountInterface $account) {
   -    $links = $this->languageManager->getLanguageSwitchLinks($type, Url::fromRouteMatch(\Drupal::routeMatch()));
   +    $route_match = \Drupal::routeMatch();
   

   From memory an earlier version of the security patch used this:

   \Drupal\Core\Url::createFromRequest(\Drupal::requestStack())->getMainRequest()

   Is that more accurate than the front page, or is it the same issue because for big pipe that'd be the big pipe JS request?

2. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -181,6 +193,52 @@ protected function doTestLanguageBlockAnonymous($block_label) {
   +  protected function doTestLanguageBlock404(string $block_label, string $system_path) {
   

   We have tests in the private issue, it would be good to run them with this patch applied to make sure there's no regression on the security side of things.

I'll continue working on this later but just dropping this:

our contrib language switcher:
https://git.drupalcode.org/project/wxt_library/-/blob/8.x-7.x/src/Plugin...

@larowlan re #38.1 this would result in a regression for 404 pages for anonymous users and sites without big_pipe as the language switcher would then always have links to the front page because the main request for a 404 does not have a route.

re #38.2 the patch in #36 does not change the security part of the patch - and will only result in the front page being used when there is no routing. And access checking that is unnecessary.

@joseph.olstad see the code... the docs say

   * @param \Drupal\Core\Session\AccountInterface|null $account
   *   (optional) Run access checks for this account. NULL for the current user.

passing NULL as the first argument will use the current user - any effort to inject the current user is unnecessary. And see \Drupal\Core\Access\AccessManager::check()

  public function check(RouteMatchInterface $route_match, AccountInterface $account = NULL, Request $request = NULL, $return_as_object = FALSE) {
    if (!isset($account)) {
      $account = $this->currentUser;
    }

@joseph.olstad the reason your custom language switch is not working is that the Url object you are passing in (https://git.drupalcode.org/project/wxt_library/-/blob/8.x-7.x/src/Plugin...) is not fully routed and therefore cannot be access checked properly. You need to make the same changes to your language switcher as core did. So change

    $route_name = $this->pathMatcher->isFrontPage() ? '<front>' : '<current>';

    $path_elements = explode('/', trim($front, '/'));
    foreach ($this->languageManager->getLanguages() as $language) {
      if (!empty($path_elements[0]) && $path_elements[0] == $language->getId()) {
        array_shift($path_elements);
        if (implode($path_elements) == trim($frontAlias, '/')) {
          $route_name = '<front>';
        }
      }
    }

to

    $route_match = \Drupal::routeMatch();
    // If there is no route match, for example when creating blocks on 404 pages
    // for logged-in users with big_pipe enabled using the front page instead.
    $url = $route_match->getRouteObject() ? Url::fromRouteMatch($route_match) : Url::fromRoute('<front>');

    $path_elements = explode('/', trim($front, '/'));
    foreach ($this->languageManager->getLanguages() as $language) {
      if (!empty($path_elements[0]) && $path_elements[0] == $language->getId()) {
        array_shift($path_elements);
        if (implode($path_elements) == trim($frontAlias, '/')) {
          $url = Url::fromRoute('<front>');
        }
      }
    }

And then everything will work for you.

@alexpott, thanks very much for this, I made a patch from your snippet, it worked, just had one other line to change.

#3349214-02: language switcher fix related to Core Security update.

With this change, no longer need the core patch.

@joseph.olstad your language switcher will be fixed for 404-big-pipe-logged-in-users before core :)

I removed #34 for security reasons, since there's no way to stop sites from applying a specific patch without realizing it's basically rolling back a security advisory, but at least we can keep them from thinking it's a legit WIP.

@larowlan does #41 fully address your concerns?

Manually tested and confirmed the issue and that the patch resolves it. Happy with the code changes.

Combining the embargoed tests from the private issue. If this passes I'll commit - as these tests were previously RTBC on the private issue.

Back to RTBC, going to commit, all I did was stitch the existing RTBC embargoed tests with the existing patch

Committed to 10.1.x, 10.0.x, 9.5.x and 9.4.x

***Edit*** the commit looks good, thanks ***edit***
minor nit, just my 2 cents:
From the interdiff, I suggest replacing
publié
with
publié

james.williams just pushed in a patch with test case here related to the language switcher:
#3348686: [regression] Inaccessible language switcher links are removed before alternatives can be provided

Note, that at the start of "testRestrictedPaths()" we are still logged in as the created "admin_user", the test only succeeds, because this user is not a "real" admin user, since he only has the following permissions:

      'administer blocks',
      'administer languages',
      'access administration pages',
      'access content',

This makes the test quite hard to understand, since later in this test we log in as a "privileged user" even if the admin should be already privileged with the required permissions.

Furthermore, I do not understand the assertion in line 560:

    // Visit a restricted user page.
    // Assert that the language switching block is displayed on the
    // access-denied page, but it does not contain the path alias.
    $this->assertLinkMarkup('/user/1', 403, $block->label(), 'peter-parker');

Why would a block be displayed on a 403 page? Am I missing something here?

Feel free to set this as fixed again, if the maintainers think this is too minor to reopen the issue again, thx!

EDIT: We could also create a follow-up issue for this, if needed.

@Grevil - thanks for reporting, can you please open a new issue for that, it can be a cleanup/followup task.

@larowlan the changes from the patch above cause another regression with front page links. I created a new issue: [regression] Language switcher block returns links to node on the frontpage

@larowlan, I added a follow-up issue for #55: #3351806: Partially refactor Language switcher block tests.