Improve handling of CORS redirects for some resource loads.

Improve handling of CORS redirects for some resource loads.

To align with what CORS requires for redirects, have CORS-enabled
resource loading perform access control checks on redirects.

The ResourceLoader delegates the access control check to its host's
canAccessRedirect() implementation:

 bool ResourceLoaderHost::canAccessRedirect(Resource*, ResourceRequest&, const ResourceResponse&, ResourceLoaderOptions&);

which is passed the redirect request + response along with other
arguments needed to make a yes/no decision on following the redirect.

To correctly handle redirects to another origin, the
canAccessRedirect() predicate is also responsible for updating the
'source origin' as the redirect is followed. This and other redirect
steps are taken care of by the helper method

  CrossOriginAccessControl::handleRedirect()

The included tests cover redirects over <img> and <script>, for the
various redirect responses possible (wrt CORS.) Rely on existing
redirect tests for HTML imports.

Notice that the redirect handling added here does not apply to
resource types and loaders that implement their special (and extended)
handling of CORS + redirects -- e.g., XMLHttpRequest fetches.

R=abarth@chromium.org
BUG=274843
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166508

[sof, 2014-01-31 22:41:50]

No particular rush (with the weekend looming and all), but when you have a spare
moment next, please take a look.

@morrita: added you for the HTML Imports related changes, please check that this
meets expected behavior (I had to split up the htmlimports redirect tests to
avoid flakiness wrt console message output.)

[abarth-chromium, 2014-01-31 23:42:40]

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.h:59: bool
checkCrossOriginAccessRedirectionUrl(const KURL&, String& errorDescription);
Url -> URL

Rather than using the verb "check", it would be better to explain what the
function is checking.  For example,
crossOriginAccessControlRedirectForbiddenForURL

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1313: bool
ResourceFetcher::canAccessRedirect(Resource* resource, ResourceRequest& request,
const ResourceResponse& redirectResponse, ResourceLoaderOptions& options)
The logic you're adding to this function looks way too complicated.  Is this
really what the spec says?  Can you write the code in a way that machines the
spec line-by-line and cites each step in the spec?

[sof, 2014-02-01 14:39:18]

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/Resour...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/Resour...
Source/core/fetch/ResourceFetcher.cpp:1313: bool
ResourceFetcher::canAccessRedirect(Resource* resource, ResourceRequest& request,
const ResourceResponse& redirectResponse, ResourceLoaderOptions& options)
On 2014/01/31 23:42:40, abarth wrote:
> The logic you're adding to this function looks way too complicated.  Is this
> really what the spec says?  Can you write the code in a way that machines the
> spec line-by-line and cites each step in the spec?

I've tried to address this by factoring out the redirect steps into a separate
helper, including spec cross-references. Easier to comprehend/reconciliate?

More can be done to unify that method with the redirect steps that the
DocumentThreadableLoader performs. I would like to address that, but as
follow-on work.

[sof, 2014-02-04 22:01:49]

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/149643003/diff/80001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.h:59: bool
checkCrossOriginAccessRedirectionUrl(const KURL&, String& errorDescription);
On 2014/01/31 23:42:40, abarth wrote:
> Url -> URL
> 
> Rather than using the verb "check", it would be better to explain what the
> function is checking.  For example,
> crossOriginAccessControlRedirectForbiddenForURL

CrossOriginAccessControl::isLegalRedirectLocation() is what it was named in the
patchset uploaded over the weekend.

[abarth-chromium, 2014-02-05 08:20:44]

LGTM

Thanks for iterating on this CL.  I'm very happy with the direction you're
taking this code.  Thanks again for working on it.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:229: const KURL& originalUrl =
redirectResponse.url();
s/Url/URL/

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:250: if
(!originalOrigin->isSameSchemeHostPort(requestOrigin.get())) {
we should probably use:

originalOrigin->canRequest(requestUrl)

In spec-land, this check is symmetric, but in implementation-land, we need to
deal with privileged origins that can request URLs from a whitelist of origins.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.h:51: class CrossOriginAccessControl
{
Thanks for creating this class.  Over time, we probably should migrate the test
of the functions in this header into here.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:563: if (!sourceOrigin ||
sourceOrigin->canRequest(url))
The !sourceOrigin case here cannot occur.  document()->securityOrigin() is
always non-null.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1322: return false;
This can't ever occur.  documet()->securityOrigin() is always non-null.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1325: if (frame() && frame()->document())
frame()->document() will always be non-null in this case.  In fact, it will
always be equal to document()

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1326:
frame()->document()->addConsoleMessage(JSMessageSource, ErrorMessageLevel,
errorMessage);
Why not just call addCnosoleMessage on document() directory?  There's no way to
get here after the document detaches from the frame.  You can add an
ASSERT(frame()); if you like.

[sof, 2014-02-05 10:05:42]

Thanks for finding the time, much appreciated.

I'll follow up on where to direct the console output separately, if we want to
change that.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:229: const KURL& originalUrl =
redirectResponse.url();
On 2014/02/05 08:20:44, abarth wrote:
> s/Url/URL/

Alright, I'll follow that convention -- done.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Cross...
Source/core/fetch/CrossOriginAccessControl.cpp:250: if
(!originalOrigin->isSameSchemeHostPort(requestOrigin.get())) {
On 2014/02/05 08:20:44, abarth wrote:
> we should probably use:
> 
> originalOrigin->canRequest(requestUrl)
> 
> In spec-land, this check is symmetric, but in implementation-land, we need to
> deal with privileged origins that can request URLs from a whitelist of
origins.

I see; we do want to avoid "degenerating" to unique origin for those, so
switched to using that predicate.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1322: return false;
On 2014/02/05 08:20:44, abarth wrote:
> This can't ever occur.  documet()->securityOrigin() is always non-null.

Just being conservative, good to know. Gone.

https://codereview.chromium.org/149643003/diff/240001/Source/core/fetch/Resou...
Source/core/fetch/ResourceFetcher.cpp:1325: if (frame() && frame()->document())
On 2014/02/05 08:20:44, abarth wrote:
> frame()->document() will always be non-null in this case.  In fact, it will
> always be equal to document()

I did have it as "if (document())" initially (like in canAccessResource()), but
noticed that in a HTML Imports testcase
(import-script-block-crossorigin-dynamic) which generates CORS errors from
within an imported document, the errors weren't being propagated out (=> console
errors not being displayed in the test's output.)

Where do we want to report such errors to?

[sof, 2014-02-05 11:00:57]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-05 11:01:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/149643003/340001

[commit-bot: I haz the power, 2014-02-05 14:04:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 14:04:26]

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

[sof, 2014-02-05 14:05:37]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-02-05 14:05:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/149643003/340001

[commit-bot: I haz the power, 2014-02-05 16:07:46]

Change committed as 166508