Bug 214218 – Improve IPv6 detection when setting host/hostname

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Rob Buis]

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.

[Rob Buis]

Created attachment 404049 [details]
Patch

[Rob Buis]

Created attachment 404054 [details]
Patch

[Darin Adler]

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.

[Rob Buis]

Created attachment 404093 [details]
Patch

[Rob Buis]

Created attachment 404094 [details]
Patch

[Rob Buis]

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.

[Rob Buis]

Created attachment 404105 [details]
Patch

[EWS]

Committed r264282: <https://trac.webkit.org/changeset/264282>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404105 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65438944>