Fix problems with cross-origin redirects.

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

[Ken Russell (switch to Gerrit), 2013-05-09 03:03:40]

Adam, Nate,

Please take a look at these changes. If the direction looks good, I'll work on
writing tests for them.

Without changes on Epic's servers, they don't make the demo run. However, with
the following patch to ResourceResponse.cpp, the demo loads and runs
successfully on 64-bit Linux, at least.

diff --git a/Source/core/platform/network/ResourceResponse.cpp
b/Source/core/platform/network/ResourceResponse.cpp
index 826d7ea..344dda4 100644
--- a/Source/core/platform/network/ResourceResponse.cpp
+++ b/Source/core/platform/network/ResourceResponse.cpp
@@ -308,11 +308,21 @@ void ResourceResponse::setHTTPHeaderField(const
AtomicString& name, const String
 
 void ResourceResponse::addHTTPHeaderField(const AtomicString& name, const
String& value)
 {
+    String value2 = value;
+
+    if (name == "Access-Control-Allow-Origin" &&
+        m_url.host() == "cdn.unrealengine.com" &&
+        (m_url.lastPathComponent() == "UDKGame-Browser-Shipping.js.mem" ||
+         m_url.lastPathComponent() == "UDKGame_Data.data")) {
+        fprintf(stderr, "Expanding Access-Control-Allow-Origin privileges for
%s\n", m_url.string().ascii().data());
+        value2 = "*";
+    }
+
     updateHeaderParsedState(name);
 
-    HTTPHeaderMap::AddResult result = m_httpHeaderFields.add(name, value);
+    HTTPHeaderMap::AddResult result = m_httpHeaderFields.add(name, value2);
     if (!result.isNewEntry)
-        result.iterator->value.append(", " + value);
+        result.iterator->value.append(", " + value2);
 }
 
 const HTTPHeaderMap& ResourceResponse::httpHeaderFields() const

[abarth-chromium, 2013-05-09 04:22:06]

Definitely on the right track.  Please add some tests.  :)

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/DocumentTh...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/DocumentTh...
Source/core/loader/DocumentThreadableLoader.cpp:199: && (m_sameOriginRequest ||
passesAccessControlCheck(redirectResponse, m_options.allowCredentials,
securityOrigin(), accessControlErrorDescription));
Ah, I see.  I was mis-reading the implementation of isAllowedRedirect.

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLo...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLo...
Source/core/loader/ResourceLoaderOptions.h:64: ResourceLoaderOptions() :
sendLoadCallbacks(DoNotSendCallbacks), sniffContent(DoNotSniffContent),
dataBufferingPolicy(BufferData), allowCredentials(DoNotAllowStoredCredentials),
credentialsRequested(ClientDidNotRequestCredentials),
crossOriginCredentialPolicy(DoNotAskClientForCrossOriginCredentials),
securityCheck(DoSecurityCheck) { }
Can you explode this onto a bunch of lines so that it's easier to read?

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLo...
Source/core/loader/ResourceLoaderOptions.h:79: CredentialRequest
credentialsRequested; // Whether the client (e.g. XHR) wanted credentials in the
first place.
It's confusing to have these two closely related fields.  Can we add more values
to the StoredCredentials enum to cover these cases?

https://codereview.chromium.org/14557011/diff/1/Source/core/platform/network/...
File Source/core/platform/network/ResourceHandleTypes.h (right):

https://codereview.chromium.org/14557011/diff/1/Source/core/platform/network/...
Source/core/platform/network/ResourceHandleTypes.h:33:
DoNotAllowStoredCredentials
For example, AllowStoredCredentialsWhileSameOrigin?  Maybe all four states
exists and having separate enums is better.

[Ken Russell (switch to Gerrit), 2013-05-09 23:50:02]

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLo...
File Source/core/loader/ResourceLoaderOptions.h (right):

https://codereview.chromium.org/14557011/diff/1/Source/core/loader/ResourceLo...
Source/core/loader/ResourceLoaderOptions.h:64: ResourceLoaderOptions() :
sendLoadCallbacks(DoNotSendCallbacks), sniffContent(DoNotSniffContent),
dataBufferingPolicy(BufferData), allowCredentials(DoNotAllowStoredCredentials),
credentialsRequested(ClientDidNotRequestCredentials),
crossOriginCredentialPolicy(DoNotAskClientForCrossOriginCredentials),
securityCheck(DoSecurityCheck) { }
On 2013/05/09 04:22:06, abarth wrote:
> Can you explode this onto a bunch of lines so that it's easier to read?

Will do.

https://codereview.chromium.org/14557011/diff/1/Source/core/platform/network/...
File Source/core/platform/network/ResourceHandleTypes.h (right):

https://codereview.chromium.org/14557011/diff/1/Source/core/platform/network/...
Source/core/platform/network/ResourceHandleTypes.h:33:
DoNotAllowStoredCredentials
On 2013/05/09 04:22:06, abarth wrote:
> For example, AllowStoredCredentialsWhileSameOrigin?  Maybe all four states
> exists and having separate enums is better.

It should be a separate enum. There are multiple APIs (EventSource, XHR) which
give the user the option of turning off the sending of credentials, but the
implementation overrides that decision if the request is same-origin. If a
redirect causes the request to go cross-origin, then DocumentThreadableLoader
needs both pieces of information in order to reconsider its decision.

[Ken Russell (switch to Gerrit), 2013-05-10 04:20:24]

Please re-review.

Compared to the previous version, this one fixes another issue where a
same-origin request redirecting to a different origin should use the original
request's origin as the Origin header.

The changes in this CL cause some redirect-related access control errors to be
caught later; rather than being caught while handling the redirect, they are
caught during access control checks on the response. This causes some console
errors to be displayed, because didFail is called on the
DocumentThreadableLoaderClient instead of didFailRedirectCheck. The tests
affected by this change have been rebaselined.

Expanded the tests in
http/tests/xmlhttprequest/access-control-and-redirects-async.html, and split off
the same-origin test cases into
access-control-and-redirects-async-same-origin.html to avoid the possibility of
timeouts. Modified expectations for one case in
access-control-and-redirects.html which should now pass. There's one case in
access-control-and-redirects-async.html which was failing and still is, and I've
added a FIXME for it.

Re-verified that with these changes, Chrome on 64-bit Linux runs the Epic
Citadel demo.

[abarth-chromium, 2013-05-10 06:26:50]

lgtm

I thing of beauty is a joy forever.

[commit-bot: I haz the power, 2013-05-10 06:27:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/14557011/11001

[commit-bot: I haz the power, 2013-05-10 12:09:10]

Retried try job too often on win_layout for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-10 20:15:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kbr@chromium.org/14557011/11001

[commit-bot: I haz the power, 2013-05-10 20:31:07]

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
Sending       
LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects.html
Sending       
LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-2-expected.txt
Sending       
LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-expected.txt
Sending       
LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-post-expected.txt
Sending       
LayoutTests/http/tests/xmlhttprequest/redirect-cross-origin-tripmine-expected.txt
Adding        
LayoutTests/http/tests/xmlhttprequest/resources/access-control-basic-allow-no-credentials.cgi
Sending       
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt
Sending        Source/core/loader/DocumentLoader.cpp
Sending        Source/core/loader/DocumentThreadableLoader.cpp
Sending        Source/core/loader/ResourceLoaderOptions.h
Sending        Source/core/loader/cache/CachedResourceLoader.cpp
Sending        Source/core/page/EventSource.cpp
Sending        Source/core/platform/network/ResourceHandleTypes.h
Sending        Source/core/xml/XMLHttpRequest.cpp
Transmitting file data ...................

[Ken Russell (switch to Gerrit), 2013-05-11 00:41:43]

The CQ screwed up; this patch actually was committed in
https://src.chromium.org/viewvc/blink?view=rev&revision=150130 . Closing
manually.