RESOLVED FIXED 15100
XMLHttpRequest::urlMatchesDocumentDomain raises error if port information does not match exactly
https://bugs.webkit.org/show_bug.cgi?id=15100
Summary XMLHttpRequest::urlMatchesDocumentDomain raises error if port information doe...
Christian Mittendorf
Reported 2007-08-28 08:54:29 PDT
The framework that we are using creates Ajax links where it adds the port to an URL. As an example, a site runs under "https://www.foo.com" and the automatically generated JavaScript link looks like "https://www.fool.com:443/Bar/app?abc=123". The current WebCore (Rev 25269) XMLHttpRequest does refuse to make this request with an "Access denied" error because the site url does not include the explicit port information ("documentURL.port() == url.port()" is false). In my example the documentURL.port() would be 0 and url.port() would be 443. Although using https without any specific port would result in using port 443 anyway. IE and Firefox do allow these links and perform the Ajax request without an error. I would suggest to either remove the test that the port must match in both urls or to add more logic to the test method "bool XMLHttpRequest::urlMatchesDocumentDomain(const KURL& url) const" in /WebKit/WebCore/xml/XMLHttpRequest.cpp.
Attachments
Added use of default http/https port for comparison if no port was specified in the url. (1.21 KB, patch)
2007-08-28 15:19 PDT, Christian Mittendorf
mjs: review-
Patch for better compliance with RFC 2616 Section 3.2.3 (1003 bytes, patch)
2008-06-10 20:48 PDT, Travis Vachon
sam: review-
Patch, Step 1: Clean up extra dependencies (1.17 KB, patch)
2008-06-12 22:59 PDT, Adam Barth
eric: review+
Patch, Step 2: Test that we handle document.domain properly (1.67 KB, patch)
2008-06-12 23:00 PDT, Adam Barth
darin: review+
Patch, Step 3: Fix default ports (5.05 KB, patch)
2008-06-12 23:02 PDT, Adam Barth
darin: review+
David Kilzer (:ddkilzer)
Comment 1 2007-08-28 10:00:02 PDT
Alexey Proskuryakov
Comment 2 2007-08-28 10:44:01 PDT
Did this work in 10.4.x Safari/WebKit? If it did, this problem would be a P1, although I'm not quite sure what the correct fix would be. IIRC, the protocol<->port relationship is just a default that can be overridden via DNS, so it's not possible to know that https corresponds to port 443 without making a network connection.
Christian Mittendorf
Comment 3 2007-08-28 13:39:09 PDT
It's already a problem with 10.4.x, although Safari won't display a pop-up showing an error message (Access denied). It simply doesn't work. The error popup comes with Safari 3 Beta.
Christian Mittendorf
Comment 4 2007-08-28 13:46:01 PDT
(In reply to comment #2) > IIRC, the protocol<->port relationship is just a default that can be overridden via DNS, > so it's not possible to know that https corresponds to port 443 without making > a network connection. The DNS doesn't have anything to do with the protocol - port, the port being used is just a convention (see RFC 2616).
Christian Mittendorf
Comment 5 2007-08-28 13:56:26 PDT
There is also an interesting chapter entitled "3.2.3 URI Comparison" (http://www.faqs.org/rfcs/rfc2616.html) in RFC 2516 (HTTP/1.1): "A port that is empty or not given is equivalent to the default port for that URI-reference;" Even though W3C does not define how "constitutes as non same-origin" (http://www.w3.org/TR/XMLHttpRequest/) is defined, refering to RFC 2516 does in my eyes make sense.
Christian Mittendorf
Comment 6 2007-08-28 15:19:01 PDT
Created attachment 16149 [details] Added use of default http/https port for comparison if no port was specified in the url.
David Kilzer (:ddkilzer)
Comment 7 2007-08-28 15:45:37 PDT
(In reply to comment #6) > Created an attachment (id=16149) [edit] > Added use of default http/https port for comparison if no port was specified in > the url. Please set the "review?" flag if you would like this patch reviewed. Thanks! http://webkit.org/coding/contributing.html
Darin Adler
Comment 8 2007-08-28 16:01:20 PDT
Comment on attachment 16149 [details] Added use of default http/https port for comparison if no port was specified in the url. Is this change really appropriate? Do other browsers consider this when comparing ports for security reasons? I'm not sure the actual port matters. All that matters is the security rule. The formatting is wrong (spaces before the url) and performance-wise we don't want to call lower multiple times on the same string.
Alexey Proskuryakov
Comment 9 2007-08-28 20:04:02 PDT
(In reply to comment #4) > The DNS doesn't have anything to do with the protocol - port, the port being > used is just a convention (see RFC 2616). Unfortunately, it's not that simple: per RFC 2782, this information is stored in SRV records. On the other hand, it's not clear if this RFC is ever going to be implemented.
Boris Zbarsky
Comment 10 2007-08-28 20:18:30 PDT
(In reply to comment #8) > Is this change really appropriate? Do other browsers consider this when > comparing ports for security reasons? The relevant code in Gecko is <http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.321&mark=326-328#319>. This is the code that is used for all same-origin checks (including cross-origin script access).
Maciej Stachowiak
Comment 11 2007-08-28 20:55:22 PDT
Comment on attachment 16149 [details] Added use of default http/https port for comparison if no port was specified in the url. I think we should match Mozilla and IE on this. Some technicalities about the current patch: 1) All these steps are done twice: + int document_port = documentURL.port(); + + if (document_port == 0) { + if(documentURL.protocol().lower() == "http") + document_port = 80; + if(documentURL.protocol().lower() == "https") + document_port = 443; + } It would be nice to factor this work out into a common function, especially so we can reuse it anywhere that an XSS same origin check is needed. Also, I'd suggest adding the default port for "ftp". And finally, we probably need to eventually add a way to ask the network layer for the default ports for various protocols. r- for these technicalities. I will gladly r+ a version that improves these things. I also asked Sam Weinig to look at this, to see if we can apply the same fix to scripting checks.
Sam Weinig
Comment 12 2007-08-28 21:21:58 PDT
A few comments on this itteration, Please capitalize the first word in the comment. + // make sure that the default http / https port 80 / 443 is used for comparison Our style dictates that these variables should be named documentPort and urlPort. + int document_port = documentURL.port(); + int url_port = url.port(); Please put a space after the if. This goes for a few other places as well. In addition, a slightly better way to do this comparison is to use the equalIgnoringCase(stringA, stringB) function. + if(documentURL.protocol().lower() == "http") If we make this change, we definitely need to make it in the isSafeScript check as well.
Travis Vachon
Comment 13 2008-06-10 20:48:53 PDT
Created attachment 21617 [details] Patch for better compliance with RFC 2616 Section 3.2.3 Take protocol default port into account when comparing URL ports.
Travis Vachon
Comment 14 2008-06-10 20:56:10 PDT
I've added a new patch, based on the original, rejected patch, to fix this bug. I'm not particularly fluent in C++, so please excuse any obvious mistakes. I'd also just like to add to this conversation by pointing out a real world bug caused in part by the current (unpatched) behavior: https://bugzilla.osafoundation.org/show_bug.cgi?id=12118 Bugs like this can be fairly subtle and difficult to track down, and can be particularly frustrating due to "correct" behavior in Firefox and IE. This is all to say that I'm personally invested in getting this patch included in trunk, so please don't hesitate to let me know how this process can be sped along.
Adam Barth
Comment 15 2008-06-11 00:24:41 PDT
> I've added a new patch, based on the original, rejected patch, to fix this bug. > I'm not particularly fluent in C++, so please excuse any obvious mistakes. We have some good technology for doing these kinds of comparisons. You should take the document's securityOrigin property and call it's isSameSchemeHostPort method to compare with the target URL (which you should parse into a SecurityOrigin).
Sam Weinig
Comment 16 2008-06-11 00:30:41 PDT
Comment on attachment 21617 [details] Patch for better compliance with RFC 2616 Section 3.2.3 This should be done with SecurityOrigins. I have a patch to change this.
Adam Barth
Comment 17 2008-06-12 22:59:06 PDT
Created attachment 21672 [details] Patch, Step 1: Clean up extra dependencies Saw some things worth changing while fixing this bug. Here's the first step.
Adam Barth
Comment 18 2008-06-12 23:00:34 PDT
Created attachment 21673 [details] Patch, Step 2: Test that we handle document.domain properly Step 2. We handle document.domain properly for XMLHttpRequest, but we don't have a test for it. Let's make sure we don't regress.
Adam Barth
Comment 19 2008-06-12 23:02:08 PDT
Created attachment 21674 [details] Patch, Step 3: Fix default ports This patch leverages our SecurityOrigin technology to get the default ports correct.
Eric Seidel (no email)
Comment 20 2008-06-12 23:14:34 PDT
Comment on attachment 21672 [details] Patch, Step 1: Clean up extra dependencies LGTM.
Eric Seidel (no email)
Comment 21 2008-06-12 23:15:43 PDT
Comment on attachment 21673 [details] Patch, Step 2: Test that we handle document.domain properly I don't really know the background here. This looks sane to me though. Best to let sam or mjs give the official r+.
Darin Adler
Comment 22 2008-06-13 14:09:19 PDT
Comment on attachment 21673 [details] Patch, Step 2: Test that we handle document.domain properly r=me
Darin Adler
Comment 23 2008-06-13 14:10:37 PDT
Comment on attachment 21674 [details] Patch, Step 3: Fix default ports r=me
Adam Barth
Comment 24 2008-06-13 23:53:52 PDT
Fixed in r34532.
Note You need to log in before you can comment on or make changes to this bug.