Bug 211358 - A URL cannot have a username/password/port if its host is null
Summary: A URL cannot have a username/password/port if its host is null
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-03 13:26 PDT by Rob Buis
Modified: 2020-06-01 03:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (9.75 KB, patch)
2020-05-03 13:33 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.38 KB, patch)
2020-05-04 01:39 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2020-05-04 09:43 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (11.83 KB, patch)
2020-05-04 10:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2020-05-05 08:29 PDT, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-05-03 13:26:59 PDT
A URL cannot have a username/password/port if its host is null [1], so
adjust URL.cpp accordingly.

Behavior matches Chrome and Firefox.

[1] https://url.spec.whatwg.org/#cannot-have-a-username-password-port
Comment 1 Rob Buis 2020-05-03 13:33:19 PDT
Created attachment 398327 [details]
Patch
Comment 2 Daniel Bates 2020-05-03 20:16:07 PDT
Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

Patch looks good.

> Source/WTF/wtf/URL.cpp:453
> +    if (hostStart() == m_hostEnd || protocolIs("file"))

Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.
Comment 3 Daniel Bates 2020-05-03 20:21:53 PDT
Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

>> Source/WTF/wtf/URL.cpp:453
>> +    if (hostStart() == m_hostEnd || protocolIs("file"))
> 
> Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.

Also optimal solution is to move this check into a private member  function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated.
Comment 4 Rob Buis 2020-05-04 01:39:15 PDT
Created attachment 398355 [details]
Patch
Comment 5 Rob Buis 2020-05-04 09:43:21 PDT
Created attachment 398379 [details]
Patch
Comment 6 Rob Buis 2020-05-04 09:55:38 PDT
Comment on attachment 398327 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review

>>> Source/WTF/wtf/URL.cpp:453
>>> +    if (hostStart() == m_hostEnd || protocolIs("file"))
>> 
>> Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug.
> 
> Also optimal solution is to move this check into a private member  function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated.

Thanks for the review! Today I found out cannot-be-a-base-URL is already handled in URLDecomposition, so I moved my code there.
Comment 7 Rob Buis 2020-05-04 10:32:53 PDT
Created attachment 398388 [details]
Patch
Comment 8 Darin Adler 2020-05-04 11:15:23 PDT
Comment on attachment 398388 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398388&action=review

> Source/WTF/wtf/URL.h:169
> +    WTF_EXPORT_PRIVATE bool hasEmptyHost() const;

This is not significantly more efficient than writing:

    fullURL.host().isEmpty()

That’s one of the benefits of StringView. Not sure we need to add the new function.

The name seems to imply some difference between having "no host" and "empty host".

> Source/WebCore/html/URLDecomposition.cpp:61
> +    if (fullURL.hasEmptyHost() || fullURL.cannotBeABaseURL() || fullURL.protocolIs("file"))

Starting to think we need a URL::protocolIsFile function. Also irritated that there’s no concept here, like "cannot be base URL".
Comment 9 Rob Buis 2020-05-05 08:29:33 PDT
Created attachment 398510 [details]
Patch
Comment 10 EWS 2020-05-05 09:09:09 PDT
Committed r261173: <https://trac.webkit.org/changeset/261173>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398510 [details].
Comment 11 Radar WebKit Bug Importer 2020-05-05 09:10:18 PDT
<rdar://problem/62889969>