[regression] Inaccessible language switcher links are removed before alternatives can be provided

Here's a start to get the ball rolling, which just moves the access filtering to after the alter hook. I don't quite follow what went on with the $this->negotiatedLanguages property in the original core change for the security fix, so someone that does, might want to check that I've moved that around as needed too.

I can imagine that whoever was involved with the original security fix might want to review the direction being taken in this ticket, as we need to ensure we don't accidentally re-open a security vulnerability. I'm guessing that as long as the new access filtering is still performed after the hook, even any information disclosure vulnerabilities in non-core implementations of hook_language_switch_links_alter() (e.g. adding links that would disclose hidden information as before the security fix) would be resolved.

patch #4 at 3349111 does the job, and it's only one line of code
#3349111-04: [regression] Core language switcher disappeared in Drupal 9.5.5 →

With that said, I'm not sure which is the better approach.

Without patching core we have no language switcher.

patch #2 does not work for us
Warning: Undefined array key "en" in Drupal\wxt_library\Plugin\Block\LanguageBlock->build() (line 161 of modules/contrib/wxt_library/src/Plugin/Block/LanguageBlock.php).

patch #4 from 3349111 does work for us.

It looks like efforts are focused on 🐛 [regression] Core language switcher disappeared in Drupal 9.5.5 Closed: works as designed .

@joseph.olstad - since the core security fix, we can't assume language links will be set any more. That's regardless of this patch - which is actually what aims to allow modules to provide alternative links, when the links would be for inaccessible URLs. 🐛 [regression] Core language switcher disappeared in Drupal 9.5.5 Closed: works as designed appears to be in a state of flux but 🐛 [regression] Language switcher block throws exception when no route is matched Fixed looks like the one that's got the most useful progress on it for now.

From my point of view, ultimately two things need solving:
1) Since the core security fix, core's language switcher doesn't always show links to all languages, whether because their links are inaccessible or unrouted. It's right that links to inaccessible pages shouldn't be shown... but should links to something (e.g. the home page) be shown in their place?
2) Contrib modules should be given opportunity to alter what links are shown, especially if it can offer a more suitable link than the home page in place of inaccessible links.

🐛 [regression] Language switcher block throws exception when no route is matched Fixed could be the place to resolve (1), whilst this ticket might be for (2). But really I suspect they should be addressed together. If 🐛 [regression] Language switcher block throws exception when no route is matched Fixed progresses to a point where (2) is being skipped over, I'll reopen this one.

🐛 [regression] Language switcher block throws exception when no route is matched Fixed has been fixed; there's still a need to allow alternative links to be provided since core will still remove inaccessible links without replacements. I still believe this is a critical regression, as the core language block will no longer show links for all languages like it did before. (Even if the targets of those links needs to be different to avoid reopening the security vulnerability.)

Given 🐛 [regression] Language switcher block throws exception when no route is matched Fixed used a link to the site homepage for the cases when an exception would otherwise occur, I'm tempted to say that perhaps the core language block should link to the site homepage instead of entirely removing inaccessible links too?

Let's see what the bot says, the patch seems reasonable

Here's a patch that's a fresh start, which uses a link to the homepage for any languages which had inaccessible links. This means that all languages have a link again, so the alter hook doesn't need moving, as the array of links is intact. Moving the alter hook only helps sites with modules that implement it, which won't be many. I believe the deeper problem for many is that links to some languages are no longer present at all, which is why I've adjusted the issue title. I've updated the issue summary too.

I've added to the tests for the security issue that were committed as part of 🐛 [regression] Language switcher block throws exception when no route is matched Fixed , to check that all languages get a link in the switcher block, in all the cases those tests were already testing. I think that's reasonable? I'm happy to take advice if it might not be!

Goodness me, those were completely the wrong patches. Sorry! Try these...

Thanks for filing the issue and for working on a patch!

I have some projects where the home page is not accessible in certain languages, mostly because the language is in the process of being setup by the editors, but not really ready for publication yet. Is it worth checking for such a case in the patch? I haven't yet tested the patch on one of those projects and I haven't done a really thorough review, but just from a quick glance at the patch it looks like this would then return the link to the home page even though it is inaccessible, so it would then be a security issue again, wouldn't it?

Personally, I also think that the new approach introduced in #10 is a step to much, I'd rather have used the original approach in #2. But I can see why some people, maybe even the majority might want such a behavior, so maybe this is indeed the best way to do it for the 80% use case.

@FeyP Thanks :) I figured that since 🐛 [regression] Language switcher block throws exception when no route is matched Fixed used the homepage as a fallback for the scenario where an exception could be thrown, it's quite reasonable to link to the homepage. The key with the security issue was that unpublished pages commonly have a path that contains their page title (e.g. when using pathauto), so putting the link on a page exposes that. The homepage shouldn't have that though, as its path is omitted in the links. I don't think linking to an unpublished homepage, using a link that doesn't expose anything about its unpublished contents, can be considered a security issue? (Again, I'm happy to be corrected.)

I would think that you'd need to use hook_language_switch_links_alter() if you want to hide links to the untranslated homepages; but that would have been the case before SA-CORE-2023-003 too?

The last submitted patch, 10: drupal-language-switcher-links-alternative-3348686-9.test-only.patch, failed testing. View results →

The last submitted patch, 10: drupal-language-switcher-links-alternative-3348686-9.patch, failed testing. View results →

I figured that since #3348592: [regression] Language switcher block throws exception when no route is matched used the homepage as a fallback for the scenario where an exception could be thrown, it's quite reasonable to link to the homepage.

That's a good point indeed. Then I think we can keep that.

The key with the security issue was that unpublished pages commonly have a path that contains their page title (e.g. when using pathauto), so putting the link on a page exposes that. The homepage shouldn't have that though, as its path is omitted in the links.

I have actually seen some pages where the home page link now (after the original security patch) links to a path provided by pathauto or even an internal path that has been configured to use as the home page when it linked directly to the front page before. This is not the case on all pages, though, and I didn't yet have time to investigate whether this is caused by the patch or maybe by some custom code and if the former, in what exact configuration. That's why I didn't file an issue for that yet. But that's not of concern here, really.

I would think that you'd need to use hook_language_switch_links_alter() if you want to hide links to the untranslated homepages; but that would have been the case before SA-CORE-2023-003 too?

That's true. I don't actually have a very strong opinion about that, I can add that code I just removed back, that's fine with me ;).

@FeyP thankfully the tests now in core for the security issue should catch if the aliases for unpublished content are showing up in links. Of course, if there's some particular circumstance which they're not covering, that might mean another security issue!

Unfortunately those tests in my last patch were bogus, sorry. Here's the fix for the test, included in both the test-only and test+fix patches.

For me the expected behaviour is, that links that are not accessable are not shown on my page. Therefore I think the apporach of the patch is correct. In every menu we have the same behaviour.

The problem is, that we mixed up an API call (getLanguageSwitchLinks()) and the block for languageswitcher.

I think, we need a 3rd parameter for getLanguageSwitchLinks() with checkAccess=TRUE. Then everyone can get all languageswitch-links via API and can write their own code and show what their want.

For the languageswitch-block I think the expected default behaviour is not to show unaccessable links. It could be a solution to add an option "show link to homepage for missing or unaccessable links". Than the sitebuilder can decide what to do with missing links. But to do this by default like in patch #10/16 seems to invasive for me.

To fix this for current installations I would prefer patch #2, so they can change inaccessable links by their own before access check. But in the long way we should differentiate between API-call and block - and they should write their own block.

Your patch is good for everyone who needs this functionality as soon as possible back! Thank you for this, I am sure the patch is a big help for them. But I dont think this should merged in core.

The last submitted patch, 16: drupal-language-switcher-links-alternative-3348686-16.test-only.patch, failed testing. View results →

The last submitted patch, 16: drupal-language-switcher-links-alternative-3348686-16.patch, failed testing. View results →

Ugh, I wish writing & running tests was easier... I shouldn't have included the quotes in my xpath string. Another attempt...

---

@jan kellermann - hook_language_switch_links_alter() exists to allow people to change the default behaviour. 🐛 [regression] Language switcher block throws exception when no route is matched Fixed already set a precedent for core to use the homepage as a basic fallback when the initial language link can't be shown - is that a problem for you? I agree that some sites may have different use cases, but I believe linking to the homepage is a reasonable fallback for what might be a majority of sites, and linking to the homepage also makes it easier for the few sites that would use that alter hook to provide their own logic/fallbacks.

Adding a 3rd parameter to getLanguageSwitchLinks() would be an API change, but the alter hook is already intended as the API for these links, so I don't think that's necessary. I do see the point that the logic for the block is conflated with the logic for the translated links of a given URL; but that's nearly always been the case. I suggest we aim to fix this regression of links for languages having disappeared since the security fix, and you could always pursue separating the logic in a follow-up?

The last submitted patch, 20: drupal-language-switcher-links-alternative-3348686-18.test-only.patch, failed testing. View results →

Finally the tests show what they were supposed to!

My 2c: I would like the links to disappear as they do in the current core, but only after it has been decided (by the alter hook implementations) that there is no access to them. I'm basing this on 0 evidence, just a personal feeling of not wanting to overrule the decision made by core developers.

After all, an alter hook implementation can switch the inaccessible links into a home page link if it so chooses.

OK so it might be a small electorate here, but I'm outvoted!

The approach from the first patch (in comment 2) could be used instead then, to simply allow the alter hook to run first, where sites could choose to use the homepage as the alternative. That's still better than the current state of core in my point of view, so I'm happy enough to rewind back to that :) So I've reverted the IS back to what it was accordingly.

Here's a patch that adds a check in the existing test, to assert that every language has a link in the set that is passed to the alter hook. I think that's the right thing to test! No interdiff provided; as this is just the patch from comment 2 with the added tests, which are all in the 'test-only' version.

The last submitted patch, 24: drupal-language-switcher-links-alternative-3348686-24.patch, failed testing. View results →

Aw, that test didn't work out as intended.

Anyway; having spoken some more with FeyP on slack, I now recognise that implementations of hook_language_switch_links_alter() may struggle to identify which links need alternatives, if the current access checking is only done after the hook is invoked. Access checking on links doesn't usually take into account the language of a link, instead access is assumed to be for the current page language. So the 'magic' of the security fix was to do the access checking within the ConfigurableLanguageManager, which could switch out its own properties that store the negotiated language. I had jumped in with the assumption

So perhaps the better solution would be as outlined in #3348592-8: [regression] Language switcher block throws exception when no route is matched → , to do the access check before the alter hook, but store the property so that implementations of the hook can spot which links might want alternatives provided. Then ConfigurableLanguageManager::getLanguageSwitchLinks() should still check access again for all the links, so that any changes applied during the alter hook are still filtered for access. (If it's possible to easily check an individual link has definitely not changed in any way since the first access check, the second access check on it could be skipped, but it's probably easier to just check again. Assuming that's not too much of a performance problem... so profiling might be needed.) i.e. something like the following:

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

As this would just be an addition to the existing array, hopefully it wouldn't count as a BC break.

(@FeyP and @kekkis also suggested checking & storing the access before invoking the alter hook in this kind of way, on slack.)

I've probably been a bit keen to jump in on this, plus it's the weekend now, so I'm going to tap out for now!

The failed patch attempts to move the filtering of links for access to them after the hook_language_switch_links_alter() has run. It seems the placement of code in the patch might be causing the issue.

Please review.

Thank you for the interest @kunal_sahu. It is expected to include an interdiff between patches and to check the patch before uploading so removing credit for the 2nd part. Also that patch removed test coverage.

@james.williams if the proposed solution changes if you could update the issue summary please.

Still moving to NW as #24 had test failures in the main patch.

Hopefully this one is close!

W/r/t the test fail in #24:

+    $languages = $this->container->get('language_manager')->getNativeLanguages();

In tests, the container property is initialized during setup. In this case, the test method makes changes to the container, which are relevant to our testing, so we can't just call $this->container at that point, because it is outdated. We would have to update the container first, e.g. by calling $this->rebuildContainer();, preferably in the test method since it should be enough to update it once. In this case however I think we know for certain that our expected languages will be English and French, so I think we can skip this call entirely and just assert those.

W/r/t #26:

I have performed a GitLab code search in the project group and looked at various implementations of the hook in contrib. I think we can just add a boolean access key to the $links array for modules to check, if they want to replace inaccessible links.

I made some changes to the issue summary to that effect and updated the patch accordingly. We now check link access before we pass the results to the hook and add an access key for each link. After the hook, we remove the access key and do the actual filtering of the (potentially changed) results.

Alternatively, we could still just fix the test in #24 and go with that. Let me know, what you think.

Sorry, forgot the interdiff...

The last submitted patch, 30: 3348686-30-tests-only.patch, failed testing. View results →

Okay, test results not as expected. The regression test only should have failed. We need to look into that.

Doh! Looked at this some more and it is again due to the outdated container... Using \Drupal::state() instead of $this->container->get('state') I got the expected results. So I added a call to $this->rebuildContainer() after installation of node module in the test and locally it looks like it works now. Attached is a new set of patches and an interdiff.

The last submitted patch, 34: 3348686-34-regression-tests-only.patch, failed testing. View results →

The last submitted patch, 34: 3348686-34-tests-only.patch, failed testing. View results →

@FeyP that's ace, thanks! Thanks for sharing your learnings about the cached container too. I'd just copied use of $this->container from other tests without thinking too hard. I hope I remember about that for future work!

I think that makes this RTBC from my point of view!

+++ b/core/modules/language/src/ConfigurableLanguageManager.php
@@ -408,27 +408,26 @@ public function getLanguageSwitchLinks($type, Url $url) {
+              return $link + [
+                'access' => $this->isLanguageSwitchLinkAccessible($link),
+              ];

We need to update hook documentation to include info about this key.

I'm not convinced about #26 and the need to call isLanguageSwitchLinkAccessible() twice on the array. I think it is up to the implementation of language_switch_links to determine that it is providing accessible links and it should not be called twice here. Otherwise we're imposing an unnecessary performance penalty on everyone. Even sites which do not implement the hook.

I think we should go back to #24.

OK, back to #24 then, this time with tests that should be fixed (thanks to @FeyP's discovery)

Thanks for your reviews @james.williams and @alexpott and for the updated patch. I think the interdiff is reversed, otherwise the code looks good. Now waiting for test results before I RTBC this...

In the meantime, I updated the IS once more.

Oops, sorry, yes, the interdiff was reversed. Can I blame it being a Monday morning?

The last submitted patch, 40: drupal-language-switcher-links-alternative-3348686-40.test-only.patch, failed testing. View results →

Can I blame it being a Monday morning?

Yes, you can :) Also not as bad as me mentioning the problem with the first use of the container property and then forgetting there is another one further below... Then I even forgot the whole interdiff for the first iteration. And that was all on a Sunday ;).

Test results as expected. I already did the code review and a test run on my dev system while I was waiting for CI to complete. I think this is RTBC!

1. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -505,7 +505,8 @@ public function testLanguageSessionSwitchLinks() {
   +    $this->rebuildContainer();
   +    $entity_type_manager = $this->container->get('entity_type.manager');
   
   @@ -612,6 +613,12 @@ protected function assertLinkMarkup(string $path, int $status, string $marker, s
      /**
   

   Why was this change made?

   Using $this->container in functional tests is not recommended

2. +++ b/core/modules/language/tests/src/Functional/LanguageSwitchingTest.php
   @@ -612,6 +613,12 @@ protected function assertLinkMarkup(string $path, int $status, string $marker, s
   +    $languages = $this->container->get('language_manager')->getNativeLanguages();
   +    $links_for_alter = $this->container->get('state')->get('language_test.language_switch_link_ids');
   

   I think this should use \Drupal rather than $this->container

Couple of questions about things I didn't expect to see in the test changes

Thanks for your review @larowlan.

Why was this change made?

Since the test method installs node module, the container property is outdated at that point. Looks like both @james.williams and me thought that using $this->container to access services was the recommended way to access services in tests. That's why we had to rebuild the container after installation of node module, because otherwise the calls in the assert method would not return the expected services. Since we rebuild the container anyway, we could also get the entity type manager from there... If the recommended way is to use \Drupal::service(), we should just change that.

Yeah according to #2066993: Use \Drupal consistently in tests → at least for functional tests

OK, thanks for that! Now I'll have to remember to learn that I've unlearned what I just learned, haha...

New patches attached; replacing $this->container with the \Drupal equivalents, and therefore no longer rebuilding the container.

The last submitted patch, 49: drupal-language-switcher-links-alternative-3348686-49.test-only.patch, failed testing. View results →

The last submitted patch, 49: drupal-language-switcher-links-alternative-3348686-49.patch, failed testing. View results →

The test fail mentioned in comment 51 was an unrelated (CKEditor) fail. I simply retested the patch again under the same parameters and it now passes as expected.

Thanks for the updated patch @james.williams!

Feedback from #46 addressed, test results as expected => back to RTBC.

Updating issue credits

Committed to 10.1.x and backported to 10.0.x and 9.5.x - thanks all!

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