There are several places where this is needed. We currently just assume that any host ending in a digit is an IP address, except in PublicSuffix where platform specific code is used. We could add URL::hostIsIPAddress() platform specific implementations, falling back to current assumption if there isn't an implementation for the platform.
Created attachment 332955 [details] Patch
Created attachment 332956 [details] Patch
Comment on attachment 332956 [details] Patch Attachment 332956 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6330249 New failing tests: http/tests/security/set-domain-remove-subdomain-for-ip-address.html
Created attachment 332958 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 332956 [details] Patch Attachment 332956 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6330271 New failing tests: http/tests/security/set-domain-remove-subdomain-for-ip-address.html
Created attachment 332959 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 332963 [details] Patch
Comment on attachment 332963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review Looks good, but I'll hold off on r+ because I bet Alex will want to review this. > Source/WebCore/page/OriginAccessEntry.cpp:43 > + , m_hostIsIPAddress(URL::hostIsIPAddress(m_host)) Might be better to drop this member variable in favor of direct calls to hostIsIPAddress(m_host)? > Source/WebCore/page/OriginAccessEntry.cpp:70 > - if (m_hostIsIPAddress && m_ipAddressSettings == TreatIPAddressAsIPAddress) > + if (m_ipAddressSettings == TreatIPAddressAsIPAddress && (m_hostIsIPAddress || URL::hostIsIPAddress(origin.host()))) I think this is right, but it's an unrelated functionality change. Previously there was no test to check if origin was an IP address, and now there is.
(In reply to Michael Catanzaro from comment #8) > Comment on attachment 332963 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332963&action=review > > Looks good, but I'll hold off on r+ because I bet Alex will want to review > this. > > > Source/WebCore/page/OriginAccessEntry.cpp:43 > > + , m_hostIsIPAddress(URL::hostIsIPAddress(m_host)) > > Might be better to drop this member variable in favor of direct calls to > hostIsIPAddress(m_host)? Probably out of scope for this patch. > > Source/WebCore/page/OriginAccessEntry.cpp:70 > > - if (m_hostIsIPAddress && m_ipAddressSettings == TreatIPAddressAsIPAddress) > > + if (m_ipAddressSettings == TreatIPAddressAsIPAddress && (m_hostIsIPAddress || URL::hostIsIPAddress(origin.host()))) > > I think this is right, but it's an unrelated functionality change. > Previously there was no test to check if origin was an IP address, and now > there is. It's not unrelated, this patch revealed the bug, that's what caused the http/tests/security/set-domain-remove-subdomain-for-ip-address.html failure. With the previous code 0.0.1 was considered a valid IP, which was wrong, and matchesOrigin() returned false. We should also check that the origin host is not an IP before trying to match domains.
Comment on attachment 332963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review > Source/WebCore/platform/URL.cpp:1042 > + // Assume that any host that ends with a digit is trying to be an IP address. > + return !host.isEmpty() && isASCIIDigit(host[host.length() - 1]); It's awful that this was ever used, but it's good that this is being used on fewer platforms.
Comment on attachment 332963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332963&action=review > Tools/ChangeLog:10 > + * TestWebKitAPI/Tests/WebCore/URL.cpp: There are more IP address formats than tested here, see e.g. Address Representations at https://en.m.wikipedia.org/wiki/IPv4 I think that it’s ok to make an incremental improvement, but I’m concerned that this quite incomplete check may be used for something important.
I think only WinCairo is using the naive implementation after this patch. The GLib impl is here: https://git.gnome.org/browse/glib/tree/glib/ghostutils.c?id=40be86bb0e422247673ccc36fc3c493c882b4390#n683 and is only a hundred lines long; we could copy it into WebKit for use by WinCairo.
I think that it's also the Apple Windows port. Mac does seem right. Is the glib implementation even right? It doesn't seem like it recognizes "0x7f000001" as in "http://0x7f000001:8000/". Either way, these abominations need to be added to the test. Perhaps a separate "extra bonus" test if we don't intend to fix the deficiencies in all ports right away.
Committed r228086: <https://trac.webkit.org/changeset/228086>
<rdar://problem/37227012>
(In reply to Alexey Proskuryakov from comment #13) > I think that it's also the Apple Windows port. > > Mac does seem right. > > Is the glib implementation even right? It doesn't seem like it recognizes > "0x7f000001" as in "http://0x7f000001:8000/". > > Either way, these abominations need to be added to the test. Perhaps a > separate "extra bonus" test if we don't intend to fix the deficiencies in > all ports right away. Maybe we can simply reuse what we have in URLParser and we don't need any platform specific code. Or we could combine both, since URLParser serializes the IP addresses, we could simply create a URL for the given doamin, and pass the parsed host as input of the platform specific entry point.
(In reply to Alexey Proskuryakov from comment #13) > Is the glib implementation even right? It doesn't seem like it recognizes > "0x7f000001" as in "http://0x7f000001:8000/". It is considered a feature in GLib to reject these formats, see https://bugzilla.gnome.org/show_bug.cgi?id=679957 "The additional formats parsed by inet_aton() are not actually part of any standard, were not much used historically, and are only ever used now by phishers trying to obscure the domain name in URLs (eg, http://yourbank.com@1539136771/)"
Wow, we should definitely have some warning about tricks like http://yourbank.com@91.189.93.3/ (which is gnome.org). Firefox pops up a dialog that asks if you are really trying to visit 91.189.93.3 and authenticate with username yourbank.com, warning that someone may be trying to trick you. But we just accept it. That's bad.
This commit introduced a failure in API test URLTest.HostIsIPAddress on all Mac and iOS platforms. Failure: /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebCore/URL.cpp:248 Value of: URL::hostIsIPAddress("00123:4567:89AB:cdef:3210:7654:ba98:FeDc") Actual: true Expected: false https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733/steps/run-api-tests/logs/stdio https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733
Reverted r228086 for reason: This introduced a failure with API test URLTest.HostIsIPAddress. Committed r228118: <https://trac.webkit.org/changeset/228118>
(In reply to Matt Lewis from comment #19) > This commit introduced a failure in API test URLTest.HostIsIPAddress on all > Mac and iOS platforms. > > Failure: > /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/ > WebCore/URL.cpp:248 > Value of: URL::hostIsIPAddress("00123:4567:89AB:cdef:3210:7654:ba98:FeDc") > Actual: true > Expected: false > > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733/steps/run- > api-tests/logs/stdio > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/2733 This commit is introducing that test not the failure, so it isn't a regression. It it's failing is because _web_looksLikeIPAddress() is accepting that wrong IP as a valid one, so there's little we can do. Reverting it doesn't fix anything. EWS didn't fail, so I would need that someone with a mac checks what test cases are failing and add a platform ifdef with a FIXME to disable those (I'm assuming there will be more)
This address is accepted by inet_pton, so the failing subtest is wrong.
(In reply to Alexey Proskuryakov from comment #22) > This address is accepted by inet_pton, so the failing subtest is wrong. Not according to RFC 4291: "x:x:x:x:x:x:x:x, where the 'x's are one to four hexadecimal digits of the eight 16-bit pieces of the address." Our URLParser doesn't accept that one either.
Created attachment 333159 [details] Patch for landing Disable the failing test in Mac.
Comment on attachment 333159 [details] Patch for landing Attachment 333159 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6379848 New failing tests: transitions/transition-display-property.html
Created attachment 333165 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
(In reply to Carlos Garcia Campos from comment #16) > Maybe we can simply reuse what we have in URLParser and we don't need any > platform specific code. Or we could combine both, since URLParser serializes > the IP addresses, we could simply create a URL for the given doamin, and > pass the parsed host as input of the platform specific entry point. This failing test just proves the case for using URLParser instead of platform-specific functions to do work that is not in any way platform-specific.
Comment on attachment 333159 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=333159&action=review > Source/WebCore/platform/mac/URLMac.mm:85 > +bool URL::hostIsIPAddress(const String& host) > +{ > + return [host _web_looksLikeIPAddress]; > +} There's also a semantic problem here, in that you're implementing an "is" function in terms of a "looks like" function.
(In reply to Michael Catanzaro from comment #28) > Comment on attachment 333159 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333159&action=review > > > Source/WebCore/platform/mac/URLMac.mm:85 > > +bool URL::hostIsIPAddress(const String& host) > > +{ > > + return [host _web_looksLikeIPAddress]; > > +} > > There's also a semantic problem here, in that you're implementing an "is" > function in terms of a "looks like" function. Then we would allow the ipv4 addresses in the forms we don't want to accept. I guess we don't want to accept them when loading a URL either, so we would need to add an option to URLParser anyway.
> Our URLParser doesn't accept that one either. This is an interesting observation, but I do not think that matching URLParser is what we are after here, at least not directly. The new function added here just takes a String, so it doesn't have any pre-requisites, like the string being a normalized URL. There are certainly input paths into WebKit where we rely on external components more, such as when taking the initial request from the client, or when handling a redirect. All of these need to be vetted if/when we decide that we don't want to match inet_pton.
Committed r228261: <https://trac.webkit.org/changeset/228261>
Reverted r228261 for reason: This broke an internal build Committed r228282: <https://trac.webkit.org/changeset/228282>
(In reply to Matt Lewis from comment #32) > Reverted r228261 for reason: > > This broke an internal build > > Committed r228282: <https://trac.webkit.org/changeset/228282> Can you provide more info about what broke ? logs, etc
(In reply to Matt Lewis from comment #32) > Reverted r228261 for reason: > > This broke an internal build > > Committed r228282: <https://trac.webkit.org/changeset/228282> How am I supposed to deal with your internal builds?
(In reply to Carlos Alberto Lopez Perez from comment #33) > (In reply to Matt Lewis from comment #32) > > Reverted r228261 for reason: > > > > This broke an internal build > > > > Committed r228282: <https://trac.webkit.org/changeset/228282> > > Can you provide more info about what broke ? logs, etc Sorry for the delay — the original revision (r228261) caused an internal builder to start failing in PublicSuffixMac.mm because it could not find decodeHostName. This appears to be declared in WebCoreNSURLExtras.h, but the line where we #import "WebCoreNSURLExtras.h" was removed from PublicSuffixMac.mm in the original patch. I can take a closer look tomorrow, but hopefully this helps a bit!
(In reply to Wenson Hsieh from comment #35) > (In reply to Carlos Alberto Lopez Perez from comment #33) > > (In reply to Matt Lewis from comment #32) > > > Reverted r228261 for reason: > > > > > > This broke an internal build > > > > > > Committed r228282: <https://trac.webkit.org/changeset/228282> > > > > Can you provide more info about what broke ? logs, etc > > Sorry for the delay — the original revision (r228261) caused an internal > builder to start failing in PublicSuffixMac.mm because it could not find > decodeHostName. This appears to be declared in WebCoreNSURLExtras.h, but the > line where we #import "WebCoreNSURLExtras.h" was removed from > PublicSuffixMac.mm in the original patch. > > I can take a closer look tomorrow, but hopefully this helps a bit! Thanks for the details. It seems to me that simply adding the missing include to fix the internal build is less effort than rolling out the two patches again, but still, I'll submit a new patch with the missing include.
Created attachment 333477 [details] Patch for landing
Comment on attachment 333477 [details] Patch for landing Let's try again.
Comment on attachment 333477 [details] Patch for landing Clearing flags on attachment: 333477 Committed r228323: <https://trac.webkit.org/changeset/228323>
All reviewed patches have been landed. Closing bug.