Use TrustedRedirectResponse Error on Multilingual Setup

Hi all,

Tested on 9.4.5 and patch in #31 still applies cleanly and works as expected, prevent the error message "Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it". Also, I have tested using the new approach from #37, but still getting the same issue.

Tested on site which is multilingual, and the detection is done using url (Domain url). When adding for example a menu item and the domain url is set for example in DE and the select list field for language is set for example in FR, then Im getting the error message. The menu is added, but the screen just displays the error, which is confusing for the end user.
Trusted hosts patterns in setting.php is set as "*".

Fixing the new tests.

#44: Did you use the patch from #37 or or #39? #39 should work with .* I tested that with the unit test.

That said, this shows a problem with this approach. Maybe you just did that local or for testing, but you should _not_ use that, because it completely subverts the protection that this is supposed to provided, as any URL is then trusted and will be redirected to.

I don't think this needs a change record, it's a bug and it's now fixed. Only thing would be a reminder that you should property define the trusted host patterns :)

Removing CR tag per #46

Verified the test fails without the fix
Data set #8 Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.

Verified following the steps in the IS
Patch #46 seems to solve it.

Thanks as well.

I am a bit concerned about what I said in #46. This means that not having the trusted host patterns configured properly (which gives you a warning/error on status page but works just fine) now also completely bypasses the outgoing projection. We might need a security review here.

The implementation is based on #22, two options I can think of is explicitly check for the full/default wildcard and then disallow this, or going back to an earlier implementation with its own hook/extension point.

xjm → credited catch → .

xjm → credited cilefen → .

xjm → credited lauriii → .

xjm → credited star-szr → .

Adding credits for the triage discussion mentioned in #22 (note: Cottser has changed his username since the comment was posted.)

#48 also sounds like this is not actually RTBC. I added #46 to the remaining tasks in the IS.

Yes, I'm more and more convinced that this is a bad idea. See for example the template for platform.sh projects: https://github.com/platformsh-templates/drupal10/blob/master/web/sites/d.... That will completely disable the protection for redirects to external hosts.

Just wondering what was actually wrong with the approach in #8? Ah #32 points out the redirect can happen in other places too.

I agree with @Berdir - the security implications of falling back on the trusted hosts setting are large when you consider the default config of platform.sh. Also re-using trusted hosts here repurposes/extends its meaning which, on reflection, doesn't feel like a good idea given its role in securing your site and the role of LocalRedirectResponse.

So this brings us back to #31 as the best (or maybe least worst) solution we have.

#31 applies cleanly to D10.0.11.

I encountered a similar issue, but when deleting a translation of a node. I've detailed reproduction steps in another issue before I found this. Adding that issue as related.

https://www.drupal.org/project/drupal/issues/3255337 🐛 "Redirects to external URLs are not allowed by default" in detection configuration by domain Active

When testing the scenario of deleting a translation and receiving the error, giving the browser a refresh, after encountering the error and pressing continue (on form resubmission) will result in the other translation being deleted as well.

So for example for a node with nid 4 on a EN/NL site with domain detection enabled.....

• * https://en.d10-ml.ddev.site/node/add/page and save
• * Go to https://en.d10-ml.ddev.site/node/4/translations
• * Press add under NL. You will go to https://nl.d10-ml.ddev.site/node/4/translations/add/en/nl
• * Add NL translation and save
• * Go to https://en.d10-ml.ddev.site/node/4/translations
• * Delete NL translation - You will go to https://nl.d10-ml.ddev.site/node/4/delete
• * Press "Delete dutch translation"
• * You will see "Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it."
• * The dutch translation is now deleted (check by going to https://en.d10-ml.ddev.site/node/4/translations in another browser tab, hit F5 to clear browser cache if you still see the dutch node)
• * Now go to the other browser tab with the error message and hit refresh of F5
• * Press "continue" on the popup "Confirm form resubmission"
• * Notice that https://en.d10-ml.ddev.site/node/4 does not exist and is also deleted.

This is probably logical because the second form submission goes to https://en.d10-ml.ddev.site/node/4/delete? But still it leads to data loss when a user perfoms these actions.

Reroll for Drupal 10.3.x

Updated the issue summary and a MR for the fix.

A simple test case might be useful, however, one thing I've noticed for a while now from other commits and MRs is that most of existing Functional tests are failing randomly in Gitlab CI, do we have an idea as to why that is?

gitlab shows that there is a conflict in .module file, please rebase and fix

Rebased for 11.x, adding support for the 📌 [ignore] Convert everything everywhere all at once Active work.

Version of the patch still using the procedural approach is available here for reference purposes.

Removed the needs test tag, and used a more appropriate title.

Attached a patch for 10.3.x