Problem/Motivation

#2955685: Unrouted URLs cannot have have overridden query or fragments has introduced a serious regression in 8.6.x. Steps to reproduce:

  1. Install standard
  2. Go to admin/structure/menu/manage/footer?destination=/admin/structure/menu
  3. Add a menu link with the URL http://google.com?news
  4. View the footer menu on the site - the URL will be http://google.com/?news= - the extra equals will break the link.

This was reported to me via github - see https://github.com/drupal/drupal/commit/a7fdfb64693d00bd7e094ccda45c3e58...

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#4 2987114-4.patch1.6 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2955685: Unrouted URLs cannot have have overridden query or fragments
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

This is caused by the parse_str() usage in \Drupal\Component\Utility\UrlHelper::parse().

For an input of "news&foo" it will do 

  $options['query'] = [
     "news" => "",
     "foo" => "",
   ];

If the values here were NULL everything would be good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Related issues: +#578520: Make $query in url() only accept an array
StatusFileSize
new 2987114-4.patch1.6 KB

The other possibility is to not add the = for an empty string. This NULL check was added in #578520: Make $query in url() only accept an array. Let's see what breaks.

Status: Needs review » Needs work

The last submitted patch, 4: 2987114-4.patch, failed testing. View results

kakar’s picture

kakar commented

Thanks, Alex Pott, Confirmed Patch #4 works on Drupal 8.5.5

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

In the github issue that caused me to open this one the reporter stated]

I think we are missing one use case, I have seen many URLs with the QueryString key without value, i.e http://www.google.com?news, Drupal change the url to http://www.google.com?news= which actually breaks the link.
We don't have this issue on Drupal 8.5.3

The odd this is if I go to http://www.google.com?news I actually get redirected to https://www.google.com/?news=&gws_rd=ssl

Are we sure this is a real bug?

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

Is there something other than https://tools.ietf.org/html/rfc3986#section-3.4 that defined this behavior?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@mpdonadio I couldn't find anything more either. I still want to know what's broken - ie. the answer to #7

madsciencepro’s picture

madsciencepro commented

Also confirming Patch #4 works on Drupal 8.5.5.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@MadSciencePro can you give examples of links that are broken? Thanks :)

madsciencepro’s picture

madsciencepro commented

@alexpott Mine was linking to Maxient reporting forms.

This:
https://cm.maxient.com/reportingform.php?UNCPembroke

Would become this:
https://cm.maxient.com/reportingform.php?UNCPembroke=

I can unpatch my dev server and grab some screenshots if you them.

catch’s picture

catch
he/him
Primary language English
commented

Looks like this might be a duplicate of #1464244: Rewrite as URL adding equals sign to end of url. which has been open since 2012 and touches the same code.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Closed (duplicate)
Related issues: +#1464244: Rewrite as URL adding equals sign to end of url.

Marking duplicate.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@catch well it's a sort of duplicate because we caused this behaviour by doing #2955685: Unrouted URLs cannot have have overridden query or fragments which means we're having changing user input (an external menu link) from something they know is correct to something that's not. This is similar to #2984272: Dots in query parameter names converted to underscores. I'm getting more an more convinced that the change to using UrlHelper::parse() and thereby parse_str() was not right.

herzbube’s picture

herzbube commented

As @alexpott already mentioned in #3, the problem is caused by using parse_str() in UrlHelper::parse(). But maybe a little bit more context analysis could be helpful.

parse_str() produces an array where the value for a query parameter is an empty string even if the "=" character is missing for that query parameter (i.e. a valueless query parameter).

Later on in UnroutedUrlAssembler::buildExternalUrl() the URI is reassembled by invoking the function UrlHelper::buildQuery(). This function in fact contains correct handling for valueless query parameters: It expects that the value for such a query parameter in the supplied array is NULL, and checks accordingly with isset($value).

The bug therefore arises because the result of parse_str() (empty string value) does not meet the expectations of UrlHelper::buildQuery() (NULL value).

I believe parse_str() is the wrong function to use, because it does not distinguish between valueless query parameters (?foo) and query parameters whose value is an empty string (?foo=) - it produces the same result for both. As long as parse_str() is used to break down a URI into its parts, it is therefore simply impossible to correctly reassemble the URI later on.

Last but not least, in case another example is needed, here's the case why I have stumbled upon this issue in the first place:

* Add a link field to a content type
* The user enters this URL: http://www.isfdb.org/cgi-bin/title.cgi?1580
* Drupal outputs the URL like this: http://www.isfdb.org/cgi-bin/title.cgi?0=

The problem that Drupal changes "1580" to "0" is covered in a separate issue that I just submitted. But even if that issue were fixed, Drupal would still output the link as http://www.isfdb.org/cgi-bin/title.cgi?1580=, which is clearly wrong and causes the ISFDB site to respond with an error.

karlshea’s picture

karlshea
he/him
Primary language English
Location Minneapolis 🇺🇸
commented

I'm running into the same problem as @MadSciencePro, and it's actually exactly the same maxient form link I'm trying to get working. @alexpott I agree, parse_str() is absolutely not the right answer if it's going to be mangling correct URLs. It seems that it's only meant to parse query params in the way that PHP is expecting them, not as a general solution for any valid URL.

madsciencepro’s picture

madsciencepro commented

I keep using the fix in #4 every time I update core. It works for my site and doesn't appear to break anything else we're doing. Is there a reason why that's not a good permanent fix?

herzbube’s picture

herzbube commented

@MadSciencePro No, it's not a good fix. Why?

As I've said in #16, it is possible to have valueless query parameters (?foo) and query parameters whose value is an empty string (?foo=). Drupal currently breaks the "valueless" case by adding an extraneous "=" character, but handles the "value is an empty string" case correctly because in that case the "=" character at the end is required.

The fix in #4 would lead to a reversal of the situation - the "valueless" case would now be handled correctly (making you happy ☺) but would break the "value is an empty string" case (making other people unhappy) by stripping away the "=" character although it is required.

To reiterate from #16: Using parse_str() makes it impossible to later reconstruct the original URL correctly in all cases, because the function loses information about the original URL. A solution can only work if parse_str() is no longer used (or a future release of PHP changes the behaviour of parse_str(), which is highly unlikely).