Bug 214218 - Improve IPv6 detection when setting host/hostname
Summary: Improve IPv6 detection when setting host/hostname
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-07-11 02:43 PDT by Rob Buis
Modified: 2020-07-15 00:10 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.93 KB, patch)
2020-07-11 02:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2020-07-11 08:50 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.33 KB, patch)
2020-07-12 02:57 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.33 KB, patch)
2020-07-12 04:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2020-07-12 10:53 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-07-11 02:43:26 PDT
Improve IPv6 detection when setting host/hostname by checking for '[' and ']' as
well as using reverse find for ':' separators, to ensure we are not finding a
separator within the IPv6 section of the url.
Comment 1 Rob Buis 2020-07-11 02:48:57 PDT
Created attachment 404049 [details]
Patch
Comment 2 Rob Buis 2020-07-11 08:50:47 PDT
Created attachment 404054 [details]
Patch
Comment 3 Darin Adler 2020-07-11 12:09:53 PDT
Comment on attachment 404054 [details]
Patch

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

Looks good, but I am not sure it’s perfect.

> Source/WTF/wtf/URL.cpp:464
> +        if (hostName.find(':') != notFound && !hostName.startsWith('['))

find != notFound is the same thing as contains; contains is preferred.

Unless you meant to say != colonIndex here? If you did, then do we have enough test coverage, because somehow we missed that error?

> Source/WebCore/html/URLDecomposition.cpp:110
> +    size_t ipv6separator = value.reverseFind(']');

ipv6separator should not be all lowercase. I think the "s" should be capitalized.
Comment 4 Rob Buis 2020-07-12 02:57:09 PDT
Created attachment 404093 [details]
Patch
Comment 5 Rob Buis 2020-07-12 04:54:31 PDT
Created attachment 404094 [details]
Patch
Comment 6 Rob Buis 2020-07-12 06:55:34 PDT
Comment on attachment 404054 [details]
Patch

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

>> Source/WTF/wtf/URL.cpp:464
>> +        if (hostName.find(':') != notFound && !hostName.startsWith('['))
> 
> find != notFound is the same thing as contains; contains is preferred.
> 
> Unless you meant to say != colonIndex here? If you did, then do we have enough test coverage, because somehow we missed that error?

Yeah, contains is better. But I do not want != colonIndex here, since the hostName has been adjusted to not include the ':' separator for the port in the line above.

>> Source/WebCore/html/URLDecomposition.cpp:110
>> +    size_t ipv6separator = value.reverseFind(']');
> 
> ipv6separator should not be all lowercase. I think the "s" should be capitalized.

Done.
Comment 7 Rob Buis 2020-07-12 10:53:22 PDT
Created attachment 404105 [details]
Patch
Comment 8 EWS 2020-07-12 11:47:35 PDT
Committed r264282: <https://trac.webkit.org/changeset/264282>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404105 [details].
Comment 9 Radar WebKit Bug Importer 2020-07-12 11:48:12 PDT
<rdar://problem/65438944>