Bug 57600 (sam)

<rdar://problem/9359014>

also doesn't work with async XHRs: http://samples.msdn.microsoft.com/ietestcenter/CORS/CORS_014.htm

I'd propose to make ResourceHandle::loadResourceSynchronously take a ResourceHandleClient, and have FrameLoader implement ResourceHandleClient That way, FrameLoader is notified of redirects and can do the access control checks wdyt?

(In reply to comment #3) > I'd propose to make ResourceHandle::loadResourceSynchronously take a ResourceHandleClient, and have FrameLoader implement ResourceHandleClient > > That way, FrameLoader is notified of redirects and can do the access control checks > > wdyt? I'd rather add a ResourceLoader::loadResourceSynchronously() and have FrameLoader call that than make FrameLoader a ResourceHandleClient. FrameLoader implemeneting ResourceHandeClient feels like a layering violation to me, but maybe others disagree.

Created attachment 122489 [details] Patch

I added a test for starters

Comment on attachment 122489 [details] Patch Clearing flags on attachment: 122489 Committed r105009: <http://trac.webkit.org/changeset/105009>

All reviewed patches have been landed. Closing bug.

not yet fixed, that was just a test

Looking at the code, it seems like the natural place to check access control after a redirect would be in DocumentThreadableLoader. There's a FIXME there that states: bool DocumentThreadableLoader::isAllowedRedirect(const KURL& url) { if (m_options.crossOriginRequestPolicy == AllowCrossOriginRequests) return true; // FIXME: We need to implement access control for each redirect. This will require some refactoring though, because the code // that processes redirects doesn't know about access control and expects a synchronous answer from its client about whether // a redirect should proceed. return m_sameOriginRequest && securityOrigin()->canRequest(url); } In reading the CORS standard, it sounds like redirects should be transparent under access control, which means we would not call the synchronous client callback. In fact, we would hide the fact that the redirect happened at all. Hopefully, that would reduce the amount of refactoring required to make this work. (In reply to comment #9) > not yet fixed, that was just a test

Created attachment 122819 [details] work in progress for fixing CORS redirects I made an attempt at fixing this a couple of months ago (see attachment). I didn't clean it up and post it for review because chromium is missing some plumbing for this to work correctly (see codereview.chromium.org/8589032) and I got distracted in the middle of fixing the chromium issues.

ok, assigning to you then, Nate

Created attachment 123030 [details] Proposed Patch

I've added a proposed patch that modifies DocumentThreadableLoader to allow redirects. The basic approach is to intercept the redirect and start the load over with the new request. One of the issues is that this request is assembled by the browser, and may contain headers that aren't allowed by CORS. I added methods to remove these headers to ResourceRequestBase, but it might be better to have a method that removes all forbidden headers. However, that seemed like it might not be appropriate to implement on ResourceRequestBase.

Comment on attachment 123030 [details] Proposed Patch Please include xhr-cors-redirect.html in the patch. I suspect that one test is likely not enough. A good - although extremely time consuming - way to come up with more tests is to look through working group discussions about changes in the spec. If something was not obvious to spec authors, it should be explicitly tested by us.

I think the first patch on this bug contains xhr-cors-redirect.html and was reviewed, landed, and closed. I agree that more tests are needed, and will expand the patch. I may also have to change the existing test / expectations. I would be interested in general comments about the suitability of the approach, potential problems, and security concerns.

Indeed - not sure why it got landed separately, but it confused me. What was the reason to choose this design? If possible, it would be much better to let low level machinery take care of generating a new request, as it already does for non-CORS requests. There are many tricky cases, e.g. for whether credentials should be carried over.

The new request is generated at a lower level and passed up the loader stack. DocumentThreadableLoader only removes some headers so the request can pass the CORS request checks, and then restarts the load using the new request as if it was the original. The header that causes problems in my manual testing was "User-Agent", added by FrameLoader. I could reduce the patch to only add a 'clearHttpUserAgent' method to ResourceRequestBase, but it seems safer to add methods for all browser-added headers and remove them all before kicking off the new request (only when using access control). It seems natural to me that this redirect logic be in DocumentThreadableLoader, since that is where URL loads with access control are made.

The first patch for this bug committed layout tests for this functionality that don't work. The redirect response has the access control headers, but the final response for the redirected request doesn't, so it will fail. I've added some .php and .cgi files and modified xmlhttprequest/access-control-and-redirects.html to test my patch. In the process, I noticed that there are tests (xmlhttprequest/redirect-xxx.html) that seem to expect CORS redirects to fail in cases where the spec says they should succeed. Alexey, could I get more guidance from you on what work needs to be done to get a solid test suite for this new feature? Are there tests that should be deleted, rewritten, or re-baselined? I'm thinking a good approach is to use redirect.php and write a bunch of redirect-xxx.html tests to request URLs that redirect to existing .php and .cgi resources. From my reading of the spec, it seems like we want the following: 1) Simple requests that redirect should succeed or fail as if the redirect never happened (the loader just restarts when it gets the redirect, with the new URL). The redirect should be transparent to the XHR. 2) CORS requests with preflight that redirect should fail (either preflight or actual request). I'd like to approach this conservatively, opening up redirects only where the spec is clear.

> In the process, I noticed that there are tests (xmlhttprequest/redirect-xxx.html) that > seem to expect CORS redirects to fail in cases where the spec says they should succeed. If you have a specific example you'd like me to comment on, I can look into its history. > Alexey, could I get more guidance from you on what work needs to be done to get > a solid test suite for this new feature? Are there tests that should be deleted, > rewritten, or re-baselined? I'm pretty sure that we don't have a complete test suite. For example, it's likely completely untested what happens with credentials on redirect. I do encourage you to take at least a brief look at working group discussions. Existing tests that disagree with new behavior should be deleted or updated, depending on whether the case they're testing (as explained in each test's description) is still meaningful. Just changing results from "PASS" to "FAIL" would not be a correct course of action.

I haven't been able to find "working group discussions" on CORS using web search. Can you provide a link? Also, while looking at the CORS standard, I noticed there is "Latest Editor Version" which appears to contain changes to the standard: http://www.w3.org/TR/2009/WD-cors-20090317/ In particular, there is a "follow redirects flag" which allows the client to control this behavior. There is no corresponding new value on XMLHttpRequest. I'm wondering if I should try to add this to ThreadableLoaderOptions.

Try search like <http://www.google.com/search?q=XHR+redirect+public-webapps>, e.g. <http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/0655.html> or <http://old.nabble.com/-XHR2--Redirects-(was%3A-Re%3A--whatwg--unexpected-use-of-the-CORS--specification)-td26372946.html>. Generally, latest editor's version is what we want to implement.

Comment on attachment 123030 [details] Proposed Patch What's the status of this patch? Is this ready for review?

I think that we're waiting for it to be reconciled with editor's draft, and for more test coverage.

The code needs to be updated and I am still working on the tests. There are a bunch of existing tests that expect redirects to fail that need to be modified or converted to test the redirect steps as described in the editor's working draft.

Comment on attachment 123030 [details] Proposed Patch Ok. I'm going to clear the review flag from this patch then.

Created attachment 128009 [details] Proposed Patch Updating the patch. This patch is closer to how I read the CORS editor's draft as it stands today. It passes all current layout tests except access-control-and-redirects.html of course. I am working on a test plan.

> Updating the patch. This patch is closer to how I read the CORS editor's draft as it stands today. It passes all current layout tests except access-control-and-redirects.html of course. I am working on a test plan. The shape of this patch looks good. I haven't done a detailed comparison with the spec. Would you like me to do that now, or should I wait for a future version of this patch?

I think it's ready for checking against the spec (editor's draft). I don't know of any divergence, so it would be great if you'd have a look.

Created attachment 128267 [details] Preliminary Patch The previous patch incorrectly allows non-simple cross-origin requests to redirect (after the preflight). Added a m_simpleCrossOriginRequest field and test this rather than that m_actualRequest in non-null.

Created attachment 133157 [details] Proposed Patch Ready for review comments. I still need to rebaseline the existing access-control-and-redirects.html tests for the async case.

Attachment 133157 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1 Source/WebCore/loader/DocumentThreadableLoader.cpp:192: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 133166 [details] Proposed Patch Added test expectations and fixed style. The existing access-control-and-redirects.html test doesn't appear to need rebaselining. Sync loads still fail and async ones appear to fail because the redirect doesn't pass the access control check as it has no CORS headers. We might still want to update the test. An earlier attachment to this issue added some tests but we should probably remove those now.

Comment on attachment 133166 [details] Proposed Patch Attachment 133166 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12070796 New failing tests: http/tests/security/xhr-cors-redirect.html

Comment on attachment 133166 [details] Proposed Patch Attachment 133166 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12070809 New failing tests: http/tests/security/xhr-cors-redirect.html

Comment on attachment 133166 [details] Proposed Patch Why do you mark these r+?

Created attachment 133245 [details] Proposed Patch Sorry, my mistake. I meant to select R? of course. I updated the patch to remove the previously committed test, as what it intends to test is more thoroughly covered by the new test.

Comment on attachment 133245 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133245&action=review > Source/WebCore/ChangeLog:4 > + DocumentThreadableLoader follows the CORS redirect steps when asynchronously loading a > + cross origin request with access control. You might consider adding a link to the spec in the ChangeLog. Traditionally, we use the line above the bug URL for the title of the bug and then put a description like this under the bug URL (after a blank line), but this way works too. > Source/WebCore/loader/DocumentThreadableLoader.cpp:185 > + allowRedirect = > + SchemeRegistry::shouldTreatURLSchemeAsCORSEnabled(request.url().protocol()) WebKit would usually merge these two lines. > Source/WebCore/loader/DocumentThreadableLoader.cpp:201 > + if (!originalOrigin.get()->isSameSchemeHostPort(requestOrigin.get())) This line is pretty subtle. One complication that we need to worry about that the spec doesn't need to worry about is "universal" origins that can access any URL... However, in that case, we won't be in the UseAccessControl codepath, so I think this is actually correct. nit: originalOrigin.get()->isSameSchemeHostPort can be written as originalOrigin->isSameSchemeHostPort because RefPtr overloads operator->

Created attachment 133508 [details] Proposed Patch Addressed Adam's comments and redid the ChangeLogs. One interesting result of this patch is that same origin XHRs which receive cross origin redirects still fail. This is because XHRs have 'withCredentials' set to 'true' when they begin to load a same origin request. Once they receive a redirect to a cross origin URL, they always fail, since the security origin gets set to a globally unique id and that plus 'allowCredentials' causes the access control check to fail. We could fix this by clearing this flag (on the loader) but that might be confusing. A surprising (to me) result of this work is that of the 9 test cases, only 1 succeeds.

Comment on attachment 133508 [details] Proposed Patch This patch looks good to me. Let's give Alexey a chance to comment before we land it.

Comment on attachment 133508 [details] Proposed Patch Rejecting attachment 133508 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12146303

Comment on attachment 133508 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133508&action=review > Source/WebCore/ChangeLog:5 > + cross-origin XMLHttpRequest doesn't work with redirect > + https://bugs.webkit.org/show_bug.cgi?id=57600 > + You need to leave the "Reviewed by NOBODY (OOPS!)." line in the ChangeLog for the bots to be smart enough to fill it out for you.

Created attachment 133983 [details] Patch for landing

Comment on attachment 133983 [details] Patch for landing Clearing flags on attachment: 133983 Committed r112217: <http://trac.webkit.org/changeset/112217>

All reviewed patches have been landed. Closing bug.

This broke the H&R Block tax website. See <https://bugs.webkit.org/show_bug.cgi?id=82964> for details.