HTML Imports: Send credentials for same origin requests

HTML Imports: Send credentials for same origin requests

This captures following spec chagne: https://www.w3.org/Bugs/Public/show_bug.cgi?id=24905
The essential part of the change is in HTMLImportsController.cpp.
Anything else is to make it work with redirect.

The problem here is that allowCredentials flag is held both
by ResourceLoaderOptions and ResourceRequest and these two
can go out-of-sync. This change tries to make them in sync.

Such a state duplication should be resolved eventually, but
that is another story.

BUG=348671
TEST=import-cors-credentials.html
R=dglazkov@chromium.org, japhet@chromium.org, abarth

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169496

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169578

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169700

[Hajime Morrita, 2014-03-11 22:49:28]

PTAL?

[abarth-chromium, 2014-03-12 18:46:43]

typo: HTMLIportsController.cpp

[abarth-chromium, 2014-03-12 18:49:31]

This CL looks like one for japhet.

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
File Source/core/fetch/FetchRequest.cpp (right):

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
Source/core/fetch/FetchRequest.cpp:77: m_options.credentialsRequested =
requested;
What does it mean to call this function with allowCredentials set to "no" and
credential request set to "yes"?

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
File Source/core/fetch/FetchRequest.h (right):

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
Source/core/fetch/FetchRequest.h:63: void
setCrossOriginAccessControl(SecurityOrigin*, StoredCredentials);
Can we remove this function and just supply a default value for the third
argument above?

[Hajime Morrita, 2014-03-12 18:56:10]

Thanks for taking look Adam!
Nate, could you take a look?

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
File Source/core/fetch/FetchRequest.cpp (right):

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
Source/core/fetch/FetchRequest.cpp:77: m_options.credentialsRequested =
requested;
On 2014/03/12 18:49:32, abarth wrote:
> What does it mean to call this function with allowCredentials set to "no" and
> credential request set to "yes"?
There shouldn't be that combination. Will add an ASSERT().

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
File Source/core/fetch/FetchRequest.h (right):

https://codereview.chromium.org/196043002/diff/1/Source/core/fetch/FetchReque...
Source/core/fetch/FetchRequest.h:63: void
setCrossOriginAccessControl(SecurityOrigin*, StoredCredentials);
On 2014/03/12 18:49:32, abarth wrote:
> Can we remove this function and just supply a default value for the third
> argument above?
We cannot. The default for the third parameter depends on the second one.

[Hajime Morrita, 2014-03-13 18:42:40]

Nate: Could you take a look for non-import part?
Then Dimitri can take care of rest.

[dglazkov, 2014-03-13 18:45:20]

lgtm on importsey parts.

[Hajime Morrita, 2014-03-14 20:44:35]

Let me explain this change a bit more:

The goal of this CL is that I want to set credentials only if the request is
sent to the same origin.
This is what the change on HTMLImportController::load() is doing.

However, only with this change, it doesn't work when the response redirects to
different origins.
Blink sends credentials toe the other domain.
Fortunately, Blink already has a mechanism to prevent it.
ResourceLoaderOptions::credentialsRequested is for that.
If this flag is set, the credentials isn't sent cross-origin after redirects.
CrossOriginAccessControl::handleRedirect()
takes care of that.

There was a bug though. The handleRedirect() doesn't work with
multiple-redirects. 
his CL introduces lastResourceReques() to take fix of that.

At last, setting setCrossOriginAccessControl() was confusing and error prone.
This change tries to strengthen it by introducing ResourceLoader::applyOptions()
to cover where ResourceLoader::m_resources is set. 
Now it is more reasonable to see if ResourceRequest::allowStoredCredentials is
reflected to final ResourceRequest.

Conceptually, ResoruceRequest is a lower-level abstraction and should be hidden
behind ResourceLoader or ResourceFetcher,
and setting ResourceLoaderOptions::allowStoreCredentials should be the only way
control the counterpart of ResourceRequest. 
But currently ResourceRequest is leaking out to upper layers so we need to deal
with this abstraction skew.

Hope this helps.

[Hajime Morrita, 2014-03-18 21:40:37]

Nate: ping?

[Nate Chapin, 2014-03-18 21:50:19]

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:243: StoredCredentials
withCredentials = resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials;
I would have thought that lastResourceRequest() was the same as the request
parameter. Is that incorrect?

[Hajime Morrita, 2014-03-18 22:10:39]

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:243: StoredCredentials
withCredentials = resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials;
On 2014/03/18 21:50:20, Nate Chapin wrote:
> I would have thought that lastResourceRequest() was the same as the request
> parameter. Is that incorrect?
The |request| parameter isn't in the Resource::m_redirectChain yet so
lastResourceRequest() is different from |request|. It is added to the chain
later. Probably that is because handleRedirect() could decline the upcoming
redirect.

[Nate Chapin, 2014-03-18 22:15:44]

LGTM

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:243: StoredCredentials
withCredentials = resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials;
On 2014/03/18 22:10:40, morrita1 wrote:
> On 2014/03/18 21:50:20, Nate Chapin wrote:
> > I would have thought that lastResourceRequest() was the same as the request
> > parameter. Is that incorrect?
> The |request| parameter isn't in the Resource::m_redirectChain yet so
> lastResourceRequest() is different from |request|. It is added to the chain
> later. Probably that is because handleRedirect() could decline the upcoming
> redirect.

Maybe we should consider updating the redirect chain, then doing the
handleRedirect() check? Having the extra request in the redirect chain shouldn't
matter if the request gets cancelled.

We don't have to block this patch on that change, though.

[Hajime Morrita, 2014-03-18 22:34:57]

Thanks for the review!

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
File Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
Source/core/fetch/CrossOriginAccessControl.cpp:243: StoredCredentials
withCredentials = resource->lastResourceRequest().allowStoredCredentials() ?
AllowStoredCredentials : DoNotAllowStoredCredentials;
On 2014/03/18 22:15:44, Nate Chapin wrote:
> On 2014/03/18 22:10:40, morrita1 wrote:
> > On 2014/03/18 21:50:20, Nate Chapin wrote:
> > > I would have thought that lastResourceRequest() was the same as the
request
> > > parameter. Is that incorrect?
> > The |request| parameter isn't in the Resource::m_redirectChain yet so
> > lastResourceRequest() is different from |request|. It is added to the chain
> > later. Probably that is because handleRedirect() could decline the upcoming
> > redirect.
> 
> Maybe we should consider updating the redirect chain, then doing the
> handleRedirect() check? Having the extra request in the redirect chain
shouldn't
> matter if the request gets cancelled.

I can do this, but how does it help?
We need secondLastResouceRequest() or something here then, or example.

I agree that though, the redirect handling is confusing as a result of
historically accumulated debt and should be straighten somehow. Currently the
flow goes up and down between layers and hard to follow.

[Hajime Morrita, 2014-03-18 22:35:34]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:35:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/20001

[Nate Chapin, 2014-03-18 22:36:46]

On 2014/03/18 22:34:57, morrita1 wrote:
> Thanks for the review!
> 
>
https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
> File Source/core/fetch/CrossOriginAccessControl.cpp (right):
> 
>
https://codereview.chromium.org/196043002/diff/20001/Source/core/fetch/CrossO...
> Source/core/fetch/CrossOriginAccessControl.cpp:243: StoredCredentials
> withCredentials = resource->lastResourceRequest().allowStoredCredentials() ?
> AllowStoredCredentials : DoNotAllowStoredCredentials;
> On 2014/03/18 22:15:44, Nate Chapin wrote:
> > On 2014/03/18 22:10:40, morrita1 wrote:
> > > On 2014/03/18 21:50:20, Nate Chapin wrote:
> > > > I would have thought that lastResourceRequest() was the same as the
> request
> > > > parameter. Is that incorrect?
> > > The |request| parameter isn't in the Resource::m_redirectChain yet so
> > > lastResourceRequest() is different from |request|. It is added to the
chain
> > > later. Probably that is because handleRedirect() could decline the
upcoming
> > > redirect.
> > 
> > Maybe we should consider updating the redirect chain, then doing the
> > handleRedirect() check? Having the extra request in the redirect chain
> shouldn't
> > matter if the request gets cancelled.
> 
> I can do this, but how does it help?
> We need secondLastResouceRequest() or something here then, or example.
> 
> I agree that though, the redirect handling is confusing as a result of
> historically accumulated debt and should be straighten somehow. Currently the
> flow goes up and down between layers and hard to follow.

Wow. You're right, my thinking was backwards. Sorry about that.

[commit-bot: I haz the power, 2014-03-18 23:15:07]

Change committed as 169496

[Hajime Morrita, 2014-03-19 18:13:13]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-03-19 18:13:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/80001

[commit-bot: I haz the power, 2014-03-19 18:14:34]

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

[commit-bot: I haz the power, 2014-03-19 18:14:34]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Hajime Morrita, 2014-03-19 18:28:51]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-03-19 18:28:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/80001

[commit-bot: I haz the power, 2014-03-19 19:29:46]

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

[commit-bot: I haz the power, 2014-03-19 19:29:47]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Hajime Morrita, 2014-03-19 20:07:13]

Committed patchset #5 manually as r169578 (presubmit successful).

[tkent, 2014-03-20 05:27:25]

A revert of this CL has been created in
https://codereview.chromium.org/199733008/ by tkent@chromium.org.

The reason for reverting is: The following tests crash on all debug bots.
- fast/shapes/shape-inside/shape-inside-image-set.html
- fast/shapes/shape-outside-floats/shape-outside-image-set.html
.

[tkent, 2014-03-20 05:28:22]

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

[Hajime Morrita, 2014-03-20 16:46:58]

On 2014/03/20 05:28:22, tkent wrote:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%25...

Thanks for taking care of it. I thought the failure had been there :-(

[Hajime Morrita, 2014-03-20 20:56:28]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:56:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/100001

[commit-bot: I haz the power, 2014-03-20 21:42:20]

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

[commit-bot: I haz the power, 2014-03-20 21:42:20]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Hajime Morrita, 2014-03-20 21:44:59]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:45:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/100001

[commit-bot: I haz the power, 2014-03-20 21:57:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/morrita@chromium.org/196043002/100001

[commit-bot: I haz the power, 2014-03-20 22:28:20]

Change committed as 169700