Bug 88139

Adam, I can make a patch, but do you think it would break a lot of things in the wild?

I suspect ap has an opinion here. My understanding is that Firefox follows the spec, so I think we should give following the spec a try. This is a security check, so we don't want to be sloppy.

Yup.

The testcase there is pointing to a wrong domain (www.w3c-test.org), it should be no-www. The server should probably be configured in a bit more restrictive way so that it's easier to rely on the domain. Anyway, there's an updated test, that doesn't test for the exception code (which is not that interesting): http://w3c-test.org/webappsec/tests/cors/submitted/opera/staging/origin.htm That URI will also change at one point if it moves to approved.

Created attachment 145633 [details] Patch

This patch LGTM. Let's give folks a chance to comment before landing.

@peter: I wanted to add the WebVisible keyword to this patch, but it doesn't seem to exist yet.

Comment on attachment 145633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > + if (accessControlOriginString != securityOrigin->toString()) { should we check for !securityOrigin->isUnique() or would that be overkill?

(In reply to comment #8) > (From update of attachment 145633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > > + if (accessControlOriginString != securityOrigin->toString()) { > > should we check for !securityOrigin->isUnique() or would that be overkill? That check is already a few lines above the change.

(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 145633 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > > > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > > > + if (accessControlOriginString != securityOrigin->toString()) { > > > > should we check for !securityOrigin->isUnique() or would that be overkill? > > That check is already a few lines above the change. ok, patch looks good then

Comment on attachment 145633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > // FIXME: Access-Control-Allow-Origin can contain a list of origins. > - RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString); > - if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) { > + if (accessControlOriginString != securityOrigin->toString()) { I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon. Is this relying on both accessControlOriginString and securityOrigin->toString() being lowercase? I cannot immediately see why they are. > LayoutTests/http/tests/xmlhttprequest/resources/basic-auth/access-control-auth-basic.php:3 > -header("Access-Control-Allow-Origin: http://127.0.0.1:8000/"); > +header("Access-Control-Allow-Origin: http://127.0.0.1:8000"); Hmm. So all tests using this script used to fail in Firefox?

(In reply to comment #11) > (From update of attachment 145633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > > // FIXME: Access-Control-Allow-Origin can contain a list of origins. > > - RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString); > > - if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) { > > + if (accessControlOriginString != securityOrigin->toString()) { > > I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon. > Does SecurityOrigin::createFromString() create a valid origin for the first url if accessControlOriginString is a list? I assumed the FIXME meant that having a list of origins would just not work at all. I'll look into it. > Is this relying on both accessControlOriginString and securityOrigin->toString() being lowercase? I cannot immediately see why they are. > The url used for the security origin is lowercased when the SecurityOrigin is created. And then the comparison is case-sensitive with the value of Access-Control-Allow-Origin. The test covers this, but i didn't realize the test is run from 127.0.0.1, hmm. Any suggestions? > > LayoutTests/http/tests/xmlhttprequest/resources/basic-auth/access-control-auth-basic.php:3 > > -header("Access-Control-Allow-Origin: http://127.0.0.1:8000/"); > > +header("Access-Control-Allow-Origin: http://127.0.0.1:8000"); > > Hmm. So all tests using this script used to fail in Firefox? Yes, it seems like it.

> The url used for the security origin is lowercased when the SecurityOrigin is created. And then the comparison is case-sensitive with the value of Access-Control-Allow-Origin. The test covers this, but i didn't realize the test is run from 127.0.0.1, hmm. Any suggestions? I misread "case sensitive" in bug description. Anyway, not sure how exactly to test, but perhaps localhost vs. LOCALHOST can be used? >> Hmm. So all tests using this script used to fail in Firefox? > Yes, it seems like it. Can you test to confirm?

(In reply to comment #13) > >> Hmm. So all tests using this script used to fail in Firefox? > > Yes, it seems like it. > > Can you test to confirm? Sorry, i misread the results when i tested it before. The tests that use the script fail outright in Firefox, see https://bugs.webkit.org/show_bug.cgi?id=37781#c9 But Firefox passes all of the tests in the w3c testcase, so theoretically the trailing slash would matter in the tests that use the script.

(In reply to comment #11) > (From update of attachment 145633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145633&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:152 > > // FIXME: Access-Control-Allow-Origin can contain a list of origins. > > - RefPtr<SecurityOrigin> accessControlOrigin = SecurityOrigin::createFromString(accessControlOriginString); > > - if (!accessControlOrigin->isSameSchemeHostPort(securityOrigin)) { > > + if (accessControlOriginString != securityOrigin->toString()) { > > I guess one way this could actually cause a compatibility regression is when Access-Control-Allow-Origin has a list of origins, which we used to mis-parse as a single one. In that case, scheme host and port could reasonably matched securityOrigin for the first item. So maybe we should address the FIXME now or very soon. This indeed happens, but i can't think of any examples of a list of urls that is interpreted as a single url and should still work with the spec as it stands now. The only one example i can think of that comes close would be something like "http://localhost:8000/ http://example.org". But even if we parsed the list correctly, the trailing slash would be there and it wouldn't match. Can you think of a problematic example?

I don't think the list case is a problem for us yet because we never send a list in the Origin header to start with. The FIXME is correct, but it's not something we need to worry about for this patch.

Comment on attachment 145633 [details] Patch Attachment 145633 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12900457 New failing tests: http/tests/xmlhttprequest/origin-exact-matching.html

Created attachment 145746 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #17) > (From update of attachment 145633 [details]) > Attachment 145633 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12900457 > > New failing tests: > http/tests/xmlhttprequest/origin-exact-matching.html The cases that fail are the origins that end with a null character. It doesn't fail for me on chromium-mac, though, not sure what's going on there.

*** Bug 88628 has been marked as a duplicate of this bug. ***

Comment on attachment 145633 [details] Patch One thing we can do is to land this patch without the null character tests and file a bug about the null character issue.

(In reply to comment #21) > (From update of attachment 145633 [details]) > One thing we can do is to land this patch without the null character tests and file a bug about the null character issue. Sounds good.

Created attachment 146653 [details] Patch

(In reply to comment #23) > Created an attachment (id=146653) [details] > Patch Removed the trailing null character tests (and filed bug 88688). Also reworked the test a bit to test case differences properly.

Comment on attachment 146653 [details] Patch Alright. Let's give this a shot.

Comment on attachment 146653 [details] Patch Clearing flags on attachment: 146653 Committed r119911: <http://trac.webkit.org/changeset/119911>

All reviewed patches have been landed. Closing bug.

Comment on attachment 146653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146653&action=review > LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.html:9 > +var urlTemplate = "http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin="; You're not passing any "url" argument to redirect-cors.php. What is the expected behavior in that case? On GTK and EFL ports (both using libsoup), we end up with "Too many redirects" errors because it keep redirecting the same URL (http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=...). Shouldn't we pass some URL to avoid this?

(In reply to comment #28) > (From update of attachment 146653 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146653&action=review > > > LayoutTests/http/tests/xmlhttprequest/resources/origin-exact-matching-iframe.html:9 > > +var urlTemplate = "http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin="; > > You're not passing any "url" argument to redirect-cors.php. What is the expected behavior in that case? On GTK and EFL ports (both using libsoup), we end up with "Too many redirects" errors because it keep redirecting the same URL (http://127.0.0.1:8000/xmlhttprequest/resources/redirect-cors.php?access-control-allow-origin=...). Shouldn't we pass some URL to avoid this? Hmm, i thought that without a $url or $redirect_preflight it wouldn't actually do the redirect. I'll fix the test.

<rdar://problem/11679305>