Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(214)

Issue 14557011: Fix problems with cross-origin redirects. (Closed)

Created:
7 years, 7 months ago by Ken Russell (switch to Gerrit)
Modified:
7 years, 7 months ago
Reviewers:
abarth-chromium
CC:
blink-reviews, jeez, Nate Chapin, eae+blinkwatch, gavinp+loader_chromium.org, M-A Ruel
Visibility:
Public.

Description

Fix problems with cross-origin redirects. Three problems exist in the current code: 1) If a same-origin request causes a redirect to a different origin, do not enforce access control checks for the redirect response itself, because the request which resulted in the redirect was same-origin. 2) If a same-origin request causes a redirect to a different origin, use the original request's URL as the origin for the new request; do not use a unique security origin. 3) Track whether the client (i.e., XMLHttpRequest) actually requested that credentials be sent in the first place. When a same-origin request redirects to a different origin, the original request will send cookies whether requested or not, because it is same-origin. The new cross-origin request should not send cookies unless they were requested, so that the access control checks on the response will succeed if the server granted "Access-Control-Allow-Origin=*". BUG=226897 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150130

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fixed origin in same-origin-to-cross-origin redirects. Added tests. Rebaselined. #

Patch Set 3 : Fixed typo in numbering in a test. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -61 lines) Patch
M LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html View 1 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html View 1 2 2 chunks +18 lines, -32 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-expected.txt View 1 1 chunk +7 lines, -16 lines 0 comments Download
A LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html View 1 1 chunk +88 lines, -0 lines 0 comments Download
A LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin-expected.txt View 1 1 chunk +27 lines, -0 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt View 1 1 chunk +3 lines, -2 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-2-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-post-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-tripmine-expected.txt View 1 1 chunk +10 lines, -0 lines 0 comments Download
A + LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-allow-no-credentials.cgi View 1 1 chunk +0 lines, -1 line 0 comments Download
M LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/loader/DocumentLoader.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/loader/DocumentThreadableLoader.cpp View 1 3 chunks +12 lines, -4 lines 0 comments Download
M Source/core/loader/ResourceLoaderOptions.h View 1 2 chunks +19 lines, -2 lines 0 comments Download
M Source/core/loader/cache/CachedResourceLoader.cpp View 1 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/page/EventSource.cpp View 1 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/platform/network/ResourceHandleTypes.h View 1 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/xml/XMLHttpRequest.cpp View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Ken Russell (switch to Gerrit)
Adam, Nate, Please take a look at these changes. If the direction looks good, I'll ...
7 years, 7 months ago (2013-05-09 03:03:40 UTC) #1
abarth-chromium
Definitely on the right track. Please add some tests. :) https://codereview.chromium.org/14557011/diff/1/Source/core/loader/DocumentThreadableLoader.cpp File Source/core/loader/DocumentThreadableLoader.cpp (right): https://codereview.chromium.org/14557011/diff/1/Source/core/loader/DocumentThreadableLoader.cpp#newcode199 ...
7 years, 7 months ago (2013-05-09 04:22:06 UTC) #2
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLoaderOptions.h File Source/core/loader/ResourceLoaderOptions.h (right): https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLoaderOptions.h#newcode64 Source/core/loader/ResourceLoaderOptions.h:64: ResourceLoaderOptions() : sendLoadCallbacks(DoNotSendCallbacks), sniffContent(DoNotSniffContent), dataBufferingPolicy(BufferData), allowCredentials(DoNotAllowStoredCredentials), credentialsRequested(ClientDidNotRequestCredentials), crossOriginCredentialPolicy(DoNotAskClientForCrossOriginCredentials), securityCheck(DoSecurityCheck) ...
7 years, 7 months ago (2013-05-09 23:50:02 UTC) #3
Ken Russell (switch to Gerrit)
Please re-review. Compared to the previous version, this one fixes another issue where a same-origin ...
7 years, 7 months ago (2013-05-10 04:20:24 UTC) #4
abarth-chromium
lgtm I thing of beauty is a joy forever.
7 years, 7 months ago (2013-05-10 06:26:50 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kbr@chromium.org/14557011/11001
7 years, 7 months ago (2013-05-10 06:27:00 UTC) #6
commit-bot: I haz the power
Retried try job too often on win_layout for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout&number=1733
7 years, 7 months ago (2013-05-10 12:09:10 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kbr@chromium.org/14557011/11001
7 years, 7 months ago (2013-05-10 20:15:45 UTC) #8
commit-bot: I haz the power
Failed to apply the patch. Sending LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-expected.txt Adding LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin-expected.txt Adding LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async-same-origin.html Sending LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html Sending LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-expected.txt ...
7 years, 7 months ago (2013-05-10 20:31:07 UTC) #9
Ken Russell (switch to Gerrit)
7 years, 7 months ago (2013-05-11 00:41:43 UTC) #10
The CQ screwed up; this patch actually was committed in
https://src.chromium.org/viewvc/blink?view=rev&revision=150130 . Closing
manually.

Powered by Google App Engine
This is Rietveld 408576698