Solidus stripping in DOMURL::setHostname seems wrong

[Maciej Stachowiak]

This code in setHostname (also in HTMLAnchorElement) seems dubious and is probably wrong:

void DOMURL::setHostname(const String& value)
{
    // Before setting new value:                                                                                                 
    // Remove all leading U+002F SOLIDUS ("/") characters.                                                                       
    unsigned i = 0;
    unsigned hostLength = value.length();
    while (value[i] == '/')
        i++;

[Timothy Gu]

Worth noting that setHost() doesn't do the solidus stripping. Equivalent Chromium bug is https://crbug.com/1212318

[Alex Christensen]

This code has changed quite a bit since this bug was reported in 2014.  It is now URLDecomposition::setHost which doesn't have the solidus removal.  If there's a behavior problem related to this, feel free to reopen or file a new bug.

[Timothy Gu]

This is still an issue, but it only affects setHostname(), not setHost(). I don't have permissions to reopen this bug, but consider

    u = new URL('http://abc/');
    u.hostname = '////def';
    console.log(u.href);

versus

    u = new URL('http://abc/');
    u.host = '////def';
    console.log(u.href);

In the first case, the resultant URL is http://def/, while in the second it is http://abc/. The code that does this has indeed moved to URLDecomposition:

https://github.com/WebKit/WebKit/blob/4d78e56eb5b61d8f62844ab855e411477d031870/Source/WebCore/html/URLDecomposition.cpp#L152

Search for "removeAllLeadingSolidusCharacters".

[Alexey Proskuryakov]

Reopening per the above comment.

[Chris Dumez]

Created attachment 429665 [details]
Patch

[Chris Dumez]

Comment on attachment 429665 [details]
Patch

The issue with making this change is that I think we would stop matching other browsers.

[Timothy Gu]

Hi Chris, contrary to the commit message, this change would actually _increase_ compatibility with the spec and Firefox. You can tell from the WPT rebaseline that it increases spec compliance, as it passes more tests than before. I am planning to make the change in Chromium as well soon, after some related changes get released first.

[Chris Dumez]

(In reply to Timothy Gu from comment #7)
> Hi Chris, contrary to the commit message, this change would actually
> _increase_ compatibility with the spec and Firefox. You can tell from the
> WPT rebaseline that it increases spec compliance, as it passes more tests
> than before. I am planning to make the change in Chromium as well soon,
> after some related changes get released first.

I am not sure what you mean by increasing compatibility. Matching the spec does not improve compatibility if no browser matches the spec.

As far as I can tell, NO major browser passes this particular WPT test (including the stable version of Firefox I tested). As a result, I believe the spec and WPT test may need to be updated to match browsers instead?

[Alex Christensen]

Firefox follows the spec here but fails that test because it doesn't do anything when setting a hostname that is only solidus characters.  Chrome has the same behavior as WebKit here.  Because of that I don't think this is the right time for us to make this change.

[Timothy Gu]

2023 update: both Chrome and Firefox now follow the spec. Given the following, Chrome and Firefox both print "http://x/". Safari is the only one that prints "http://abc/".

    u = new URL('http://x/');
    u.hostname = '/abc';
    console.log(u.href);

[Chris Dumez]

Ok, I'll take a look.

[Chris Dumez]

Pull request: https://github.com/WebKit/WebKit/pull/9062

[EWS]

Committed 259366@main (865cbc8abfc5): <https://commits.webkit.org/259366@main>

Reviewed commits have been landed. Closing PR #9062 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/104650674>