| Summary: | KURL should remove default port numbers when canonicalizing urls | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||
| Component: | Platform | Assignee: | Eric Seidel (no email) <eric> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, brettw, commit-queue | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | Other | ||||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 66017 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Eric Seidel (no email)
2011-02-09 04:35:41 PST
Created attachment 81790 [details]
Patch
Comment on attachment 81790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81790&action=review > Source/WebCore/platform/KURL.cpp:1105 > +static inline bool isDefaultPortForScheme(const char* port, size_t portLength, const char* scheme, size_t schemeLength) Where did this list of schemes/ports come from? I think Firefox has some sort of extensibility model where each URL provider annonces a scheme and a default port. This is probably a good step though. > Source/WebCore/platform/KURL.cpp:1122 > + switch (schemeLength) { > + case 4: > + return !strncmp(scheme, "http", schemeLength) && !strncmp(port, "80", portLength); > + case 5: > + return !strncmp(scheme, "https", schemeLength) && !strncmp(port, "443", portLength); > + case 3: > + if (!strncmp(scheme, "ftp", schemeLength)) > + return !strncmp(port, "43", portLength); > + if (!strncmp(scheme, "wss", schemeLength)) > + return !strncmp(port, "443", portLength); > + break; > + case 6: > + return !strncmp(scheme, "gopher", schemeLength) && !strncmp(port, "70", portLength); > + case 2: > + return !strncmp(scheme, "ws", schemeLength) && !strncmp(port, "80", portLength); > + } Can you put these in numerical origin? Also, consider adding an explicit default case. Let me find a dupe, I think there were some interesting comments there... The duplicate is in Radar, <rdar://problem/7908207>. An interesting thing is that we should care about what's sent over HTTP more that we should care about DOM access to components - and the most common practical case where this occurs is slightly misconfigured servers that redirect to :80. The code in ResourceHandleMac.mm that (I think) makes redirects work correctly is super subtle. Specifically, here is how one broken site behaved: 1. Open http://www.rambler.ru/dict Expected results: redirected to http://www.rambler.ru/dict/ Actual results: redirected to http://www.rambler.ru:80/dict/ Here, the server was adding :80 at the same time it was adding a trailing slash, so we ended up sending www.rambler.ru:80 as Host, as well as in Safari address bar. The server has been since fixed. -FAIL canonicalize('//c:\\foo') should be http://c/foo. Was http://c:/foo. +PASS canonicalize('//c:\\foo') is 'http://c/foo' I think that you have an unrelated change in your patch that's affecting this test. +// FIXME: This function is a horrible mess of copy/paste, but I don't know +// of a better way to write it. +static inline bool isDefaultPortForScheme(const char* port, size_t portLength, const char* scheme, size_t schemeLength) Please don't wrap the comment. I personally don't think that code quality deserves a FIXME comment - a person capable of improving the code won't need a comment to see that. FIXMEs are more useful for subtle behavior bugs or questions. + const char *portEndPtr = url + portEnd; Misplaced star. I strongly encourage you to add HTTP test cases for this change. Not strongly enough to formally request that and mark the patch r-, but close to that :) > -FAIL canonicalize('//c:\\foo') should be http://c/foo. Was http://c:/foo. > +PASS canonicalize('//c:\\foo') is 'http://c/foo' > > I think that you have an unrelated change in your patch that's affecting this test. You should also add a test for //example.com:/foo //example.com: http://example.com:/foo http://example.com: I suspect you're canonicalizing away empty port number too, which seems beneficial (but should be verified). When I put "//c:\\foo" in Firefox address bar (yeah, on Mac), it doesn't remove the colon. Perhaps this change should be discussed on its own merits in a separate bug. (In reply to comment #6) > When I put "//c:\\foo" in Firefox address bar (yeah, on Mac), it doesn't remove the colon. Perhaps this change should be discussed on its own merits in a separate bug. The address bar isn't a reliable way to test this behavior. Also, note that because we're using shouldBe, the \\ gets unescaped to \. I've collated a bunch of data points on the compatibility of URL parsing here: https://github.com/abarth/url-spec/blob/master/tests/gurl-results/by-browser.txt Although, I don't seem to have that particular test on that list. (In reply to comment #2) > (From update of attachment 81790 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81790&action=review > > > Source/WebCore/platform/KURL.cpp:1105 > > +static inline bool isDefaultPortForScheme(const char* port, size_t portLength, const char* scheme, size_t schemeLength) > > Where did this list of schemes/ports come from? I think Firefox has some sort of extensibility model where each URL provider annonces a scheme and a default port. This is probably a good step though. This came from GURL. Since GURL is BSD and Google copyright, the copyright line didn't end up changing. > Can you put these in numerical origin? Also, consider adding an explicit default case. I assumed Brett did that for perf. Not sure. (In reply to comment #4) > -FAIL canonicalize('//c:\\foo') should be http://c/foo. Was http://c:/foo. > +PASS canonicalize('//c:\\foo') is 'http://c/foo' > > I think that you have an unrelated change in your patch that's affecting this test. Nah. That's just that our parser sees (correctly) "c" as the host name and the ":" as the : for port. Its just like if someone wrote: http://www.apple.com:/ I think we'd expect to remove the trailing colon there too. > +// FIXME: This function is a horrible mess of copy/paste, but I don't know > +// of a better way to write it. > +static inline bool isDefaultPortForScheme(const char* port, size_t portLength, const char* scheme, size_t schemeLength) > > Please don't wrap the comment. I personally don't think that code quality deserves a FIXME comment - a person capable of improving the code won't need a comment to see that. FIXMEs are more useful for subtle behavior bugs or questions. I removed it and replace it with one which pointed out where I got the list from. > + const char *portEndPtr = url + portEnd; > > Misplaced star. Fixed. > I strongly encourage you to add HTTP test cases for this change. Not strongly enough to formally request that and mark the patch r-, but close to that :) What specific tests do you have in mind? I'm not sure what your asking for, but happy to entertain the request. Created attachment 81888 [details]
Fixed ap and abarth's comments
> What specific tests do you have in mind? I'm not sure what your asking for, but happy to entertain the request.
Sorry, I now realize that we can't have http tests on port 80.
Btw, I checked FF, Chrome and Opera and they all do the colon-removal thing as tested in my most recent patch. I don't have IE. Comment on attachment 81888 [details]
Fixed ap and abarth's comments
Ok. Please check the HTTP redirect case manually before landing.
Happy to. Can you explain how I do that? I don't fully understand, but consider an HTTP redirect with the following Location header: Location: http://example.com:80/ I think he's worried that Safari will show the ":80" in the address bar or in the Host header when following the redirect. (In reply to comment #15) > I don't fully understand, but consider an HTTP redirect with the following Location header: > > Location: http://example.com:80/ > > I think he's worried that Safari will show the ":80" in the address bar or in the Host header when following the redirect. That seems the opposite of this change, since it's designed to remove :80 from urls. This would be done with some sort of Apache settings or some custom php? I would just use netcat, but that's how I roll. 1. Add main.php to LayoutTests/http/tests:
<?php
header('Location: http://localhost:80/foobar.html');
header('HTTP/1.0 302 Found');
?>
2. run-webkit-httpd
3. sudo nc -l 80
4. Open http://127.0.0.1:8000/main.php in Safari.
5. Observe that a request that you get in nc has "localhost" in Host header field (not localhost:80).
6. Type back:
HTTP/1.1 200 OK
Hello world!
7. Observe that address in Safari address bar doesn't have the :80.
Repeat with and without your patch, in Safari, Chrome, and Firefox :)
(In reply to comment #18) > 1. Add main.php to LayoutTests/http/tests: > <?php > header('Location: http://localhost:80/foobar.html'); > header('HTTP/1.0 302 Found'); > ?> > > 2. run-webkit-httpd > 3. sudo nc -l 80 > 4. Open http://127.0.0.1:8000/main.php in Safari. > 5. Observe that a request that you get in nc has "localhost" in Host header field (not localhost:80). > 6. Type back: > HTTP/1.1 200 OK > > Hello world! > > 7. Observe that address in Safari address bar doesn't have the :80. > > Repeat with and without your patch, in Safari, Chrome, and Firefox :) Done. Seems to work fine. I never saw a :80. Created attachment 81894 [details]
Patch for landing
Created attachment 81899 [details]
Patch for landing
Comment on attachment 81899 [details] Patch for landing Rejecting attachment 81899 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2 Last 500 characters of output: .. fast/dom/Document ................... fast/dom/Document/CaretRangeFromPoint .... fast/dom/Element ................................. fast/dom/EntityReference . fast/dom/Geolocation ................................... fast/dom/HTMLAnchorElement .. fast/dom/HTMLAnchorElement/set-href-attribute-host.html -> failed Exiting early after 1 failures. 7245 tests run. 154.29s total testing time 7244 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 4 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7884258 Two real failures. I'll investigate after some sleep. Created attachment 82107 [details]
Patch for landing
Comment on attachment 82107 [details] Patch for landing Rejecting attachment 82107 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1 Last 500 characters of output: Headers/wtf/CryptographicallyRandomNumber.h r78321 = ba712d32d0ea52b479cd2b68230b941090e5296f (refs/remotes/trunk) M Source/WebCore/dom/Document.h M Source/WebCore/dom/Element.cpp M Source/WebCore/dom/Element.h M Source/WebCore/ChangeLog M Source/WebCore/css/CSSStyleSelector.cpp M Source/WebCore/css/CSSStyleSelector.h r78322 = 61ac637b95e13bf64bf5aba239900de89a6ece25 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/7867605 Comment on attachment 82107 [details] Patch for landing Rejecting attachment 82107 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1 Last 500 characters of output: cfd4132944da0f11ed4b684f3403b2e9e3737719 r78320 = 3043e530d84fb64e5a4521811d9c344b9d122625 r78321 = ba712d32d0ea52b479cd2b68230b941090e5296f r78322 = 61ac637b95e13bf64bf5aba239900de89a6ece25 r78323 = aa5a9e22db2a1749c225bbc65d2ae02ed526ba8d r78324 = 4d78e0888b2e0e58a821f1cb10cfd1b52b9080a6 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://queues.webkit.org/results/7870604 Created attachment 82122 [details]
Patch for landing
Comment on attachment 82122 [details] Patch for landing Rejecting attachment 82122 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1 Last 500 characters of output: CoreSupport/FrameLoaderClientGtk.cpp M Source/WebKit/gtk/ChangeLog M Source/WebCore/ChangeLog M Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp r78331 = f47dc69db31dd8a1237a2f20563fb23636968430 (refs/remotes/trunk) M Source/WebCore/ChangeLog M Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp r78332 = 1f1d3b758799641fac7067bfe7013e9a837282a2 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/7867647 Comment on attachment 82122 [details]
Patch for landing
Ha. It's missing a reviewer. What a dumb message from the cq. I guess it got lost along the way.
@abarth: would you r+ again?
Comment on attachment 82122 [details] Patch for landing Clearing flags on attachment: 82122 Committed r78383: <http://trac.webkit.org/changeset/78383> All reviewed patches have been landed. Closing bug. The commit-queue encountered the following flaky tests while processing attachment 82122 [details]: http/tests/security/xssAuditor/script-tag-post-control-char.html bug 54324 (author: dbates@webkit.org) The commit-queue is continuing to process your patch. This patch broke mobile GMail authentication in some cases. I've filed bug 66017 about that. Also likely broke visited link coloring when port number is present, see bug 78358. *** Bug 21979 has been marked as a duplicate of this bug. *** |