Url only outputs the last value of a query parameter

It makes sense to have this configurable, though it's important to point out that neither one nor the other interpretation is an industry standard. There simply is no standard. Every platform handles this in a different way, see this overview from 2009:

(source: https://www.owasp.org/images/b/ba/AppsecEU09_CarettoniDiPaola_v0.8.pdf)

Please note that some hacker tricks make use of non-consistent interpretation of repeated query parameters, a behaviour that is known as "HTTP Parameter Pollution" (https://www.owasp.org/images/b/ba/AppsecEU09_CarettoniDiPaola_v0.8.pdf). This doesn't mean that abandoning parse_str() for a more flexible solution is a bad idea, it just means:

• we need to be very careful changing anything
• we need extensive test coverage.
• both on the parsing and the output side this probably has to remain configurable to allow for consistent setups in heterogenous architectures.

Please also note that W3C recommends

that HTTP server implementors, and in particular, CGI implementors support the use of ";" in place of "&"

As we're currently in line with PHP, while there is no clear web / industry standard, I can't see how this is a bug. Maybe a task.

Found another good overview with some interesting references at the bottom: https://www.owasp.org/index.php/Testing_for_HTTP_Parameter_pollution_(OTG-INPVAL-004)

This is also interesting: https://cs.brown.edu/~vpk/papers/arc.acns12.pdf

@blazey

It really is a bug.

Urls fed into Drupal are correct but the output is broken.

Just to make it clear, I already said I'm sympathetic to the idea of allowing different parsing strategies for query parameters. And I'm sure you have a proper use-case and need it to work that way.

However, while the fact that you need this feature might make it an important feature, it doesn't make it a bug. There's nothing wrong with our current parsing strategy. It's just not the one you happen to need for your external web app.

If something's broken, then it's the expectation that different platforms would parse query parameters the same way.
I'm in favor of allowing other parsing strategies as a feature but not at the cost of breaking the expectations of other sitebuilders. And certainly not at the cost of unconsidered HPP risks.

It doesn't seem you read my links, and/or understand how sensitive this issue is. I'm leaving it there though and not going to argue with you further. I basically support your idea, so good luck with it.

Marking this RTBC because it works :)

To reduce the security risk we could use a well known library instead of maintaining custom code.
Something like https://github.com/thephpleague/uri-query-parser could work. But this one requires PHP >= 7.2. Does this mean it can't be used with Drupal 8?

1. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -419,4 +419,60 @@ public static function isValid($url, $absolute = FALSE) {
   +   * @return array
   

   we need a comment here

2. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -419,4 +419,60 @@ public static function isValid($url, $absolute = FALSE) {
   +  public static function parseString($string) {
   

   we definitely should be adding extensive unit tests for this

3. +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php
   @@ -419,4 +419,60 @@ public static function isValid($url, $absolute = FALSE) {
   +    // http://php.net/manual/en/function.parse-str.php#76792
   

   should this be @see?

Doesn't apply

Are we sure we want to override such a low-level function of PHP.

After discussing this issue with a long-term prolific contributor, the opinion was broached that perhaps this is something that belongs lower down the stack, e.g. in Guzzles PSR7 library (they already have parse_query implementation that covers a lot of the use cases here, but is not as thorough as parse_str).

Said contributor also pointed out that the current implementation doesn't handle nesting such as

parse_str('foo[1][2]=bar&foo[2][1]=baz', $result); print_r($result);

The test fails because parse_query returns null if the key has no value, and parse_str returns an empty string. Let's change that value to an empty string for BC, and also update the test to ensure the expected values matches type as well.

Sorry for pushing this back, but I have found few issues.

I tested the patch with some random query strings. The test code was:

$ drush eval '$s = "query=string"; parse_str($s, $r); var_dump($r); var_dump(\Drupal\Component\Utility\UrlHelper::parseString($s));'

Results:

array(1) {
  'arr' =>
  array(2) {
    [0] =>
    string(1) "1"
    [1] =>
    string(1) "2"
  }
}

array(1) {
  'arr' =>
  array(1) {
    [0] =>
    string(5) "Array"
  }
}

array(1) {
  'arr' =>
  array(2) {
    [0] =>
    array(1) {
      [0] =>
      string(1) "1"
    }
    [1] =>
    array(1) {
      [0] =>
      string(1) "2"
    }
  }
}

array(1) {
  'arr' =>
  array(1) {
    [0] =>
    array(1) {
      [0] =>
      string(5) "Array"
    }
  }
}

array(1) {
  'arr' =>
  array(1) {
    [0] =>
    string(1) "2"
  }
}

array(1) {
  'arr' =>
  string(1) "1"
}

array(2) {
  'a' =>
  string(1) "1"
  'b' =>
  array(1) {
    [0] =>
    string(1) "2"
  }
}

array(2) {
  'b' =>
  array(1) {
    [0] =>
    string(1) "2"
  }
  'a' =>
  string(1) "1"
}

We faced the same issue and applying patch #27 worked for us.

Is this problem only being hit when trying to render external URLs entered into menus and link fields? Because if so I think there might be a less invasive solution. We can short circuit the \Drupal\Core\Utility\UnroutedUrlAssembler::buildExternalUrl() method and do something like:

  /**
   * {@inheritdoc}
   */
  protected function buildExternalUrl($uri, array $options = [], $collect_bubbleable_metadata = FALSE) {
    // Early return to not unnecessarily alter external URLs.
    if (empty($options['query']) && empty($options['fragment']) && !isset($options['https'])) {
      return $collect_bubbleable_metadata ? (new GeneratedUrl())->setGeneratedUrl($uri) : $uri;
    }

We only need to process the URL if we changing the scheme, query string or fragments and this is not at all the usual case. With this code links with a value of https://example.com/page?tag=one&tag=two are outputted correctly for both link fields in content and in menus. That way this change would become a performance optimisation for displaying external links instead of being some that moves low level code from C to PHP.

Re-rolling #27 patch for Drupal 9

fixed the test cases.

I'm facing an issue when rendering a link when it contains more than one value for the same query param (e.g. industry_sector below)

So, for example, this URL (/search-jobs?opportunity_types=Job&industry_sectors=1&industry_sectors=2&industry_sectors=3&start=0&default=1) added to a Link field is displayed like: /search-jobs?opportunity_types=Job&industry_sectors%5B0%5D=1&industry_sectors%5B1%5D=2&industry_sectors%5B2%5D=3&start=0&default=1

Any ideas where we could fix this part?

Added the requested change by @alexpott.

Fixed the coding violations.

Removed the following test from the patch:

diff --git a/core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php b/core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php
index 3157a63dc7..b658195d19 100644
--- a/core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Url/LinkGenerationTest.php
@@ -62,4 +62,12 @@ public function testHookLinkAlter() {
     $this->assertRaw('<strong>Test!</strong>');
   }
 
+  /**
+   * Tests how hook_link_alter() can affect escaping of the link text.
+   */
+  public function testMultipleGetArguments() {
+    $url = Url::fromUri('http://example.com/page?tag=one&tag=two');
+    $this->assertEquals('http://example.com/page?tag%5B0%5D=one&tag%5B1%5D=two', $url->toString());
+  }
+
 }

This test fails because we added the code as requested by @alexpott in comment #31.

Guys, just a quick update that even with the new patch provided on #40, the issue (#36) when rendering a link when it contains more than one value for the same query param still happens.

For external URLs, I think this would address it: https://www.drupal.org/project/drupal/issues/3007243

However, how should we deal with internal URLs? Any ideas?

Re-rolled #40 for 9.3 support -- patch and diff is attached.

Re-rolled #40 for 9.4.12

Remade the patch from #48

Re-rolled #40 for 9.4.12

Re-rolled #49 for 10.2.3

It looks like there's an unanswered question of ticket scope: are we only interested in external URLs, or should we also deal with internal? Personally I'd suggest that if we can't simply solve both, then it makes sense to at least support external URLs along the lines of #31 and if we want to handle internal links do that in a separate ticket. The only issue with #31 that I can see is the slight WTF you'd get from adding an entirely separate query param (or even a fragment) to such an external URL: I don't think you'd expect that to change how the duplicate query parameters are handled. ie...

// Assuming we're using a solution that only implements #31.
\Drupal\Core\Url::fromUri('https://example.com/page?tag=one&tag=two')->toString();
// Returns https://example.com/page?tag=one&tag=two
\Drupal\Core\Url::fromUri('https://example.com/page?tag=one&tag=two', ['query' => ['foo' => 'bar']])->toString();
// Returns https://example.com/page?tag=two&foo=bar

@carolpettirossi could you give any extra detail on why/how you're using local links with duplicate query parameter names? Thanks!

I can confirm patch #51 works on Drupal 10.3.

Hello, I have tried using the patch at #51 on a 10.2.11 project.
This patch applies and retains the duplicate query parameter, following the proposed solution pattern (?tag[0]=one&tag[1]=two) but with an error clicking the link: A client error happened

```
Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Input value "keyword" contains a non-scalar value. in Symfony\Component\HttpKernel\HttpKernel->handle() (line 83 of /var/www/html/vendor/symfony/http-kernel/HttpKernel.php).
```

The link field is using an internal url going to a filtered by keyword search.
`/collection/search?keyword%5B0%5D=Ryakuga&keyword%5B1%5D=haya-oshie`
When there are more than one of the same query parameter, only the last value is outputting.

I've tried the same patch on a 10.3.10 project, again patch applies, retains the duplicate parameter with the pattern of (?tag[0]=one&tag[1]=two),however clicking the link strips off all parameters completely.

Wondering if anyone else has seen anything similar (also trying to dig into if this is just something in our internal setup).

Just... one more for the grinder... we have an external system (Daxko) that is providing links with even stranger keys... 😭

date_ranges%5B0%5D.start=&date_ranges%5B0%5D.end

decoded

date_ranges[0].start=&date_ranges[0].end

Core's link formatter is mashing those together into

date_ranges[0]=

which... the external system doesn't like.

Re-rolled #51 for 11.x

I would encourage people to review #3554522: Only parse and mess with the URI in \Drupal\Core\Utility\UnroutedUrlAssembler::buildExternalUrl() if we have to as that issue extracts the minimum change from this issue as recommended by me back in #3038774-31: Url only outputs the last value of a query parameter.

If we do want to do this I recommend we investigate the use of https://uri.thephpleague.com/ and do not use our own code from URI and URL parsing.

Rerolled patch for 11.3

Updated from 10.5.3 to 10.6.1: can't reproduce the issue anymore.