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.
<rdar://problem/5443521>
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.
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.
(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).
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.
Created attachment 16149 [details] Added use of default http/https port for comparison if no port was specified in the url.
(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
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.
(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.
(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).
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.
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.
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.
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.
> 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).
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.
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.
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.
Created attachment 21674 [details] Patch, Step 3: Fix default ports This patch leverages our SecurityOrigin technology to get the default ports correct.
Comment on attachment 21672 [details] Patch, Step 1: Clean up extra dependencies LGTM.
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+.
Comment on attachment 21673 [details] Patch, Step 2: Test that we handle document.domain properly r=me
Comment on attachment 21674 [details] Patch, Step 3: Fix default ports r=me
Fixed in r34532.