Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error

That would be a bug in the Domain module then; it's stripping a query argument from a redirect URL when it shouldn't. Should be pretty easy to fix!

This is the relevant code:

    if (!$request->query->has('destination')) {
      throw new HttpException(400, 'The original location is missing.');
    }

i.e. the destination URL query argument is being stripped.

@Wim

I'm not sure this is Domain module related. Core language negotiation by domain prefix is the likely issue here.

There is nothing in this very thin error report to indicate that Domain module is involved.

Needs more information from the original reporter.

@Wim-

If this is Domain related, where is that code snippet from?

The code snippet is from \Drupal\big_pipe\Controller\BigPipeController::setNoJsCookie().

It's totally possible the problem is not in Domain, but in core's language negotiation. What's certain is that something is stripping that query string. I read Domain in the issue title and assumed the OP meant domain module. I now see that it totally could've been domain-based language negotiation in core :) Sorry!

No worries. But we still need more context from the original reporter.

I can reproduce the bug only with core. The problem is that core/lib/Drupal/Core/Security/RequestSanitizer.php is being too aggressive.

Here's how to reproduce:

1. Install standard
2. Install the language module
3. Add second language
4. Go to Configuration > Regional and language > Languages > Detection, click on Domain and make up some configuration and save it.
5. Visit /admin/structure/block
6. Click the seven link in local tasks
7. Click remove on the first block
8. Click click the remove button
9. You should be on admin/structure/block/list/seven but you're on admin/structure/block

This is being caused by \Drupal\Core\Security\RequestSanitizer::processParameterBag() stripping all external destinations. However all destination URLs will be external when domain language negotiation is configured.

#3018942: Domain URL language detection - InvalidArgumentException: The user-entered string must begin with a '/', '?', or '#' adds a test similar to #9 it'll need fixing when we fix this.

Here's a fix.

We're too early for $GLOBALS['base_url'] - that's done in \Drupal\Core\DrupalKernel::initializeRequestGlobals() which means we're not setting the base URL completely as expected but I think this check is good enough - if there's an insecure and malicious site available at the same domain then you've got more problems then redirects.

Fixed the tests and made the code a bit more robust if the Request object doesn't have all the information. Added a test for the case when the destination and the request are for the same domain.

Improved the issue summary and the comment in the patch.

After adding new parameter $request we do not need parameter $bag ($bag = $request->$bag_name).

-  protected static function processParameterBag(ParameterBag $bag, $whitelist, $log_sanitized_keys, $bag_name, $message, Request $request) {
+  protected static function processParameterBag(Request $request, $whitelist, $log_sanitized_keys, $bag_name, $message) {
+    $bag = $request->$bag_name;

@Krzysztof Domański yep that looks good. Nice one.

The patch in #15 is working for me on Drupal 8.7.3 - thanks a lot.

This has sufficient testcoverage and I can't see anything at all that should change for this patch.

Wow! Excellent investigative work in #9, @alexpott!

Fixed test failure #15.

--- a/core/modules/block/tests/src/Functional/BlockUiTest.php
+++ b/core/modules/block/tests/src/Functional/BlockUiTest.php
@@ -337,9 +337,7 @@ public function testBlockPlacementIndicator() {
     // Removing a block will remove the block placement indicator.
     $this->clickLink('Remove');
     $this->submitForm([], 'Remove');
-    // @todo https://www.drupal.org/project/drupal/issues/2980527 this should be
-    //   'admin/structure/block/list/classy' but there is a bug.
-    $this->assertSession()->addressEquals('admin/structure/block');
+    $this->assertSession()->addressEquals('admin/structure/block/list/classy');

1. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -105,11 +105,23 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
   +        try {
   +          $is_local = UrlHelper::externalIsLocal($destination, $request->getSchemeAndHttpHost());
   +        }
   +        catch (\InvalidArgumentException $e) {
   +          $is_local = FALSE;
   +        }
   

   There needs to be a comment as to why catching the exception is required. Why at this point would either $destination or $request->getSchemeAndHttpHost() fail

       $url_parts = parse_url($url);
       $base_parts = parse_url($base_url);
   
       if (empty($base_parts['host']) || empty($url_parts['host'])) {
         throw new \InvalidArgumentException('A path was passed when a fully qualified domain was expected.');
       }
   

   Or is this being ultra defensive?

2. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -48,7 +47,7 @@ public static function sanitize(Request $request, $whitelist, $log_sanitized_key
   -        if (static::processParameterBag($request->$bag, $whitelist, $log_sanitized_keys, $bag, $message)) {
   +        if (static::processParameterBag($request, $whitelist, $log_sanitized_keys, $bag, $message)) {
   
   @@ -78,9 +77,10 @@ public static function sanitize(Request $request, $whitelist, $log_sanitized_key
   -  protected static function processParameterBag(ParameterBag $bag, $whitelist, $log_sanitized_keys, $bag_name, $message) {
   +  protected static function processParameterBag(Request $request, $whitelist, $log_sanitized_keys, $bag_name, $message) {
   
   @@ -105,11 +105,23 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
   +          $is_local = UrlHelper::externalIsLocal($destination, $request->getSchemeAndHttpHost());
   

   Let's pass in $request->getSchemeAndHttpHost() as an additional argument rather than changing the parameters.

Lol I added the try catch in #13

made the code a bit more robust if the Request object doesn't have all the information

So the answer is more defensive. I guess as this is security code that makes sense. So let's ignore #22.1

So once upon a time I thought the parameter change was a good idea too but looking at the loop that calls processParameterBag() I'm now not so sure.

rerolled patch #21.

created MR for 9.5

The patch 2980527-29.patch did not work for 9.4.5 because classy has been replaced with stark. This patch should do the trick.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Simplier solution might be to change this line (61) in big_pipe.module,
method big_pipe_page_attachments to set destination parametter to local uri instead of absolute
by the way it will not be detected as external. The right host be already set by the route of big_pipe.nojs .

'content' => '0; URL=' . Url::fromRoute('big_pipe.nojs', [], ['query' => \Drupal::service('redirect.destination')->getAsArray()])->toString(),

#37 worked for me, thanks

This issue is resolved if solution in #3424701-30: Domain-based language negotiation should retain "destination" URL query argument is accepted.

Think this would be a good plan. Lets postpone this one for the fix in #3424701: Domain-based language negotiation should retain "destination" URL query argument then we can reopen this one for expanding test coverage and removing the todo in the code.

So the fix in #44 needed to update the tests too. So going to close as duplicate and move over credit.

I just committed #3424720: LanguageNegotiationUrl unnecessarily adds domain to outbound URL's which fixes this issue. Credit wasn't transferred over originally, but we can assign credit for duplicate issues now, so doing that here.