Issue 2062 - Preserve trailing question mark when converting URL into string

The problem is that the URL object doesn't distinguish between no query string and an empty query string, e.g:

new URL("http://foobar").search == new URL("http://foobar?").search == ""

Hence filters that expect a "?" to be given don't match anymore if there is an empty query string. First I considered handling this particular case, writing following code:

function stringifyURL(url)
{
  let protocol = url.protocol;
  if (protocol != "http:" && protocol != "https:")
    return url.href;

  let host = getDecodedHostname(url);
  if (url.port)
    host += ":" + url.port;

  let search = url.search;
  if (!search)
  {
    let href = url.href;
    let queryPos = href.indexOf("?");
    if (queryPos != -1)
    {
      let hashPos = href.indexOf("#");
      if (hashPos == -1 || hashPos > queryPos)
        search = "?";
    }
  }

  return protocol + "//" + host + url.pathname + search;
}

However, this would make the logic even more complex than it already is, and might still not consider all corner cases. So I went with a more naive approach, which is way simpler and also faster (in particular in the common case, without IDN and no hash). Basically, I reduced the assumptions to a minimum, by just replacing IDN domains and stripping the hash part from URL.href.

[Sebastian Noack, 2015-02-27 20:01:59]

http://codereview.adblockplus.org/5919468974768128/diff/5629499534213120/quni...
File qunit/tests/url.js (left):

http://codereview.adblockplus.org/5919468974768128/diff/5629499534213120/quni...
qunit/tests/url.js:85:
testNormalizedURL("http://user:password@example.com/","http://example.com/",
"stripped auth credentials");
The behavior regarding credentials changed. For simplicity they are no longer
stripped. We don't care about credentials in URLs anyway. They aren't even
supported on Chrome (for security reasons).

[Sebastian Noack, 2015-02-27 20:06:29]

http://codereview.adblockplus.org/5919468974768128/diff/5741031244955648/quni...
File qunit/tests/url.js (left):

http://codereview.adblockplus.org/5919468974768128/diff/5741031244955648/quni...
qunit/tests/url.js:85:
testNormalizedURL("http://user:password@example.com/","http://example.com/",
"stripped auth credentials");
The behavior regarding credentials changed. For simplicity they are no longer
stripped. We don't care about credentials in URLs anyway. They aren't even
supported on Chrome (for security reasons).

[kzar, 2015-03-02 15:04:13]

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname",
"pathname", "search"];
Do we not need to check the port for a URL anywhere else in the code?

[Sebastian Noack, 2015-03-02 15:10:37]

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname",
"pathname", "search"];
On 2015/03/02 15:04:13, kzar wrote:
> Do we not need to check the port for a URL anywhere else in the code?

Not as far as I know. But feel free to double check. ;)

[kzar, 2015-03-02 15:14:30]

On 2015/03/02 15:10:37, Sebastian Noack wrote:
>
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
> File lib/url.js (right):
> 
>
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
> lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname",
> "pathname", "search"];
> On 2015/03/02 15:04:13, kzar wrote:
> > Do we not need to check the port for a URL anywhere else in the code?
> 
> Not as far as I know. But feel free to double check. ;)

I already did :p I could see adblockplus/lib/contentPolicy.js uses it for the
getSitekey function but wasn't sure if that code was relevant/effected in
Chrome. Otherwise I can't see it being used.

[Sebastian Noack, 2015-03-02 15:18:20]

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname",
"pathname", "search"];
On 2015/03/02 15:14:30, kzar wrote:
> On 2015/03/02 15:10:37, Sebastian Noack wrote:
> >
>
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
> > File lib/url.js (right):
> > 
> >
>
http://codereview.adblockplus.org/5919468974768128/diff/5724160613416960/lib/...
> > lib/url.js:21: let URLProperties = ["href", "protocol", "host", "hostname",
> > "pathname", "search"];
> > On 2015/03/02 15:04:13, kzar wrote:
> > > Do we not need to check the port for a URL anywhere else in the code?
> > 
> > Not as far as I know. But feel free to double check. ;)
> 
> I already did :p I could see adblockplus/lib/contentPolicy.js uses it for the
> getSitekey function but wasn't sure if that code was relevant/effected in
> Chrome. Otherwise I can't see it being used.

contentPolicy.js is Firefox-specific code, also it doesn't use the URL class,
but nsURI.

[kzar, 2015-03-02 15:23:06]

LGTM

[Wladimir Palant, 2015-03-02 15:27:18]

In the end, it is up to you to support this code. As I said, IMHO this is a
flawed approach. But - LGTM.

[Sebastian Noack, 2015-03-02 15:35:33]

For reference, we discussed this on Friday on IRC, and Wladimir prefers the
alternative approach I pasted in the description above, where I also outlined
why I prefer the simpler and IMO more robust approach.

Wladimir however thinks this is a footgun, because it includes to much implicit
logic. I however think the other approach is a footgun, because it involves too
many assumptions about the scheme and the URL object's behavior, plus it is way
more complex, and slightly slower.

So what do you think Dave?

[kzar, 2015-03-02 15:41:12]

Well I love all my children equally... no but seriously I actually prefer your
approach here Sebastian. It's simpler and I found it easy to understand. (It's
possible we're missing some corner cases but I couldn't think of any and the
same could be said for the other approach.)