WebFonts: Send credentials for same origin requests

WebFonts: Send credentials for same origin requests

This fixes a regression from Blink r199364 where credentials are not
sent for same-origin webfont requests. This patch basically does the
same thing as HTMLImportsController::load(); set AllowStoredCredentials
flag only when the request is same-origin.

BUG=516192
TEST=http/tests/webfont/same-origin-credentials.html

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

[Kunihiko Sakamoto, 2015-08-05 04:47:16]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2015-08-05 04:47:21]

Patchset #1 (id:20001) has been deleted

[Kunihiko Sakamoto, 2015-08-05 04:47:27]

Patchset #1 (id:40001) has been deleted

[Kunihiko Sakamoto, 2015-08-06 10:42:00]

Patchset #1 (id:60001) has been deleted

[Kunihiko Sakamoto, 2015-08-06 11:02:55]

ksakamoto@chromium.org changed reviewers:
+ horo@chromium.org, japhet@chromium.org, tyoshino@chromium.org

[Kunihiko Sakamoto, 2015-08-06 11:02:56]

CORS experts, could you take a look? I'm not 100% sure if my spec read is
correct.

[tyoshino (SeeGerritForStatus), 2015-08-11 06:22:27]

Sorry for delay. I'm reviewing this now. There're many call sites for this. I'm
checking if there's any caller that is expecting the behavior or not. Basically
I agree with you that for same-origin cases, clearing credentials is not needed,
and even if it's required, that should be done not by using the
setCrossOriginAccessControl().

[tyoshino (SeeGerritForStatus), 2015-08-11 09:19:13]

FYI, regarding the meaning of the Origin header attached to same-origin
requests, there's an active discussion on the Fetch Standard issue tracker. See
http://crbug.com/512836 and https://github.com/whatwg/fetch/issues/91.

I'll just check that we're not going to break anything nor introducing any new
security issue.

----

First, checked call sites. All the callers are referring to one of the Fetch
Standard, the fetch definition in the WHATWG HTML spec or the W3C CORS spec. So,
we just need to make sure we're following the idea behind the specs.

CSS shape loaded by StyleResourceLoader::loadPendingImages()
- Spec: https://drafts.csswg.org/css-shapes/#valdef-shape-outside-image
  - Referring to:
http://www.w3.org/TR/html5/infrastructure.html#cors-enabled-fetch
    - CORS enabled fetch

Other images loaded by StyleResourceLoader::loadPendingImages()
- Impl: CORS not enabled. So, setCrossOriginAccessControl() is not called. Not
affected.

CSSCursorImageValue
- Impl: CORS not enabled. So, setCrossOriginAccessControl() is not called. Not
affected.

CSS font face
- Spec: https://drafts.csswg.org/css-fonts/#font-fetching-requirements
  - Referring to:
http://www.w3.org/TR/html5/infrastructure.html#cors-enabled-fetch
    - CORS enabled fetch

(PreloadScanner on) <link rel=import ... >
- Spec: http://w3c.github.io/webcomponents/spec/imports/#fetching-import
  - Referring to: https://fetch.spec.whatwg.org/#concept-fetch
    - fetch with mode=CORS credentials_mode=same-origin. Affected

<link rel=prefetch>, <link rel=subresource> and <link rel=stylesheet>
- Spec: https://html.spec.whatwg.org/multipage/semantics.html#the-link-element
  - Potentially CORS-enabled fetch
- Spec: https://w3c.github.io/resource-hints/

ImageLoader (for src in HTMLEmbedElement, HTMLImageElement, HTMLObjectElement,
HTMLPluginElement, HTMLVideoElement, ImageInputType, SVGImageElement)
- Spec: https://html.spec.whatwg.org/multipage/embedded-content.html
  - For img, video: potentially CORS-enabled fetch
  - For object, embed: fetch 
  - <input type=image ...>
    - Spec:
https://html.spec.whatwg.org/multipage/forms.html#image-button-state-(type=im...
    - fetch
  - SVG. Couldn't find the spec. It's reasonable to use potentially CORS-enabled
fetch.

ScriptLoader (<script src=... >)
- Spec: https://html.spec.whatwg.org/multipage/scripting.html#prepare-a-script
  - Potentially CORS-enabled fetch

----

As you described in the CL description, it's considered to be wrong to set up
some cross-origin specific (standardized or de-facto). One issue is that
ResourceFetcher::canAccessRedirect() doesn't call setCrossOriginAccessControl()
on same-origin to cross-origin redirect unlike
DocumentThreadableLoader::redirectReceived(). Seems
CrossOriginAccessControl::handleRedirect() should do it.

It seems applying setCrossOriginAccessControl() for same-origin requests is
mitigating this if it's really problematic.

Setting allowed stored credentials should be done even for same-origin requests,
I think. In case there's no redirect, the ResourceFetcher doesn't consult the
value, so it's fine to set it. But when redirect happens, if we haven't set it,
canAccessRedirect() may see a wrong value. The same about fetch credentials
mode... maybe.

----

So, ... in summary, I think we should:
- fix ResourceFetcher::canAccessRedirect() or
CrossOriginAccessControl::handleRedirect() (maybe the former) to remove
credentials and set origin when we proceed after transition from same-origin to
cross-origin
- stop removing credentials and setting origin for the initial same-origin
request as you did in the current patch set but keep setting the credentials
flags

See also http://crbug.com/512836.

[tyoshino (SeeGerritForStatus), 2015-08-11 09:19:57]

tyoshino@chromium.org changed reviewers:
+ sigbjornf@opera.com

[tyoshino (SeeGerritForStatus), 2015-08-11 09:20:08]

+sof

[Kunihiko Sakamoto, 2015-08-13 07:25:43]

Thanks so much for the thorough call sites review!

I've changed CrossOriginAccessControl::handleRedirect() to remove credentials
and set origin when transitioning from same-origin to cross-origin. But it seems
not reflected to actual network request - cross origin request after redirect
still has credentials and no origin header. (New test
http/tests/webfont/auth-then-redirect.html fails after redirection)

Is this related to the sof@'s comment at
https://code.google.com/p/chromium/issues/detail?id=427429#c2 ?
> handleRedirect() doesn't handle cross-origin redirects correctly; updating and
clearing Origin: is ignored by the network layer upon return from
willSendRequest() -- its ResourceRequest reference argument isn't really fully
updatable.
>
> That together with how to handle credentials when redirects go out-of-origin
needs to be reworked and/or all CORS redirect handling be delegated to net/.

There is some code to set Origin header to "null" upon cross-origin redirect, in
net/url_request/url_request.cc (!). Perhaps we need a similar hack? I hope not.
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

[sof, 2015-08-13 17:52:15]

On 2015/08/13 07:25:43, Kunihiko Sakamoto wrote:
> Thanks so much for the thorough call sites review!
> 
> I've changed CrossOriginAccessControl::handleRedirect() to remove credentials
> and set origin when transitioning from same-origin to cross-origin. But it
seems
> not reflected to actual network request - cross origin request after redirect
> still has credentials and no origin header. (New test
> http/tests/webfont/auth-then-redirect.html fails after redirection)
> 
> Is this related to the sof@'s comment at
> https://code.google.com/p/chromium/issues/detail?id=427429#c2 ?
> > handleRedirect() doesn't handle cross-origin redirects correctly; updating
and
> clearing Origin: is ignored by the network layer upon return from
> willSendRequest() -- its ResourceRequest reference argument isn't really fully
> updatable.
> >
> > That together with how to handle credentials when redirects go out-of-origin
> needs to be reworked and/or all CORS redirect handling be delegated to net/.
> 
> There is some code to set Origin header to "null" upon cross-origin redirect,
in
> net/url_request/url_request.cc (!). Perhaps we need a similar hack? I hope
not.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

It looks related to issue 427429; that support for "null" origin transitions is
a fairly recent addition to url_request.cc ( issue 465517 )

[tyoshino (SeeGerritForStatus), 2015-08-14 14:36:39]

On 2015/08/13 17:52:15, sof wrote:
> On 2015/08/13 07:25:43, Kunihiko Sakamoto wrote:
> > Thanks so much for the thorough call sites review!
> > 
> > I've changed CrossOriginAccessControl::handleRedirect() to remove
credentials
> > and set origin when transitioning from same-origin to cross-origin. But it
> seems
> > not reflected to actual network request - cross origin request after
redirect
> > still has credentials and no origin header. (New test
> > http/tests/webfont/auth-then-redirect.html fails after redirection)
> > 
> > Is this related to the sof@'s comment at
> > https://code.google.com/p/chromium/issues/detail?id=427429#c2 ?
> > > handleRedirect() doesn't handle cross-origin redirects correctly; updating
> and
> > clearing Origin: is ignored by the network layer upon return from
> > willSendRequest() -- its ResourceRequest reference argument isn't really
fully
> > updatable.
> > >
> > > That together with how to handle credentials when redirects go
out-of-origin
> > needs to be reworked and/or all CORS redirect handling be delegated to net/.
> > 
> > There is some code to set Origin header to "null" upon cross-origin
redirect,
> in
> > net/url_request/url_request.cc (!). Perhaps we need a similar hack? I hope
> not.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> 
> It looks related to issue 427429; that support for "null" origin transitions
is
> a fairly recent addition to url_request.cc ( issue 465517 )

Thanks for pointing to the bug! Yeah, in
blink::ResourceLoader::willSendRequest(), the |passedNewRequest| is modified
through |newRequest|. ResourceFetcher::canAccessRedirect() calls
CrossOriginAccessControl::handleRedirect() and it makes modifications such as
addition of origin header for cross origin redirect on the ResourceRequest.

The modified ResourceRequest is set to ResourceLoader::m_request in
willSendRequest(), and it's also WebURLLoaderImpl::request_ in
WebURLLoaderImpl::Context::OnReceivedRedirect(). But it seems the changes are
not reflected to the URLRequest. After invoking
WebURLLoaderImpl::Context::OnReceivedRedirect(),
ResourceDispatcher::OnReceivedRedirect() just sends the
ResourceHostMsg_FollowRedirect IPC message. It just results in resuming the
corresponding content::ResourceLoader to call URLRequestJob::FollowRedirect().
It calls URLRequest::Redirect() and it tries to adjust the origin header as
described in the CL of the bug, but when I dumped
extra_request_headers_.GetHeader(HttpRequestHeaders::kOrigin, ...), it has
nothing.

[Kunihiko Sakamoto, 2015-08-17 06:54:40]

Thanks for the investigation.

> but when I dumped
extra_request_headers_.GetHeader(HttpRequestHeaders::kOrigin, ...), it has
nothing.

Because it sets Origin:null only if original extra_request_headers_ had an
Origin header, while our initial same-origin request does not have it?
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

So it seems like we need to add logic in net/ (maybe URLRequest::Redirect())
that detects SO->CO redirect and set Origin / clear credentials?  In the long
term, this redirect handling should be reworked (crbug.com/427429), but we need
a short term solution that will at least fix crbug.com/516192.

I think it's ok not doing SO->CO redirect 100% correctly in the short term
though, as long as we're not introducing any security/privacy issue.

WDYT?

[tyoshino (SeeGerritForStatus), 2015-08-19 07:03:22]

On 2015/08/17 06:54:40, Kunihiko Sakamoto wrote:
> Thanks for the investigation.
> 
> > but when I dumped
> extra_request_headers_.GetHeader(HttpRequestHeaders::kOrigin, ...), it has
> nothing.
> 
> Because it sets Origin:null only if original extra_request_headers_ had an
> Origin header, while our initial same-origin request does not have it?
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> 
> So it seems like we need to add logic in net/ (maybe URLRequest::Redirect())
> that detects SO->CO redirect and set Origin / clear credentials?  In the long
> term, this redirect handling should be reworked (crbug.com/427429), but we
need
> a short term solution that will at least fix crbug.com/516192.
> 
> I think it's ok not doing SO->CO redirect 100% correctly in the short term
> though, as long as we're not introducing any security/privacy issue.
> 
> WDYT?

Sorry for delay. I'm almost convinced that this is not so harmful and the
benefit wins the breakage of the CO after redirect from SO case.

[tyoshino (SeeGerritForStatus), 2015-08-19 07:22:01]

On 2015/08/19 07:03:22, tyoshino wrote:
> On 2015/08/17 06:54:40, Kunihiko Sakamoto wrote:
> > Thanks for the investigation.
> > 
> > > but when I dumped
> > extra_request_headers_.GetHeader(HttpRequestHeaders::kOrigin, ...), it has
> > nothing.
> > 
> > Because it sets Origin:null only if original extra_request_headers_ had an
> > Origin header, while our initial same-origin request does not have it?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...
> > 
> > So it seems like we need to add logic in net/ (maybe URLRequest::Redirect())
> > that detects SO->CO redirect and set Origin / clear credentials?  In the
long
> > term, this redirect handling should be reworked (crbug.com/427429), but we
> need
> > a short term solution that will at least fix crbug.com/516192.
> > 
> > I think it's ok not doing SO->CO redirect 100% correctly in the short term
> > though, as long as we're not introducing any security/privacy issue.
> > 
> > WDYT?
> 
> Sorry for delay. I'm almost convinced that this is not so harmful and the
> benefit wins the breakage of the CO after redirect from SO case.

I should have had understood the background. Had offline chat with Kunihiko.
This basically lg.

[Kunihiko Sakamoto, 2015-08-19 07:50:06]

Sorry I should have explained the background first. This is to fix a regression
by r199364.

As we discussed offline, I updated the patch so that it only changes behavior
for webfont resources, instead of trying to fix CORS for all resource types.

Please take another look, thanks!

[tyoshino (SeeGerritForStatus), 2015-08-20 08:39:47]

lgtm

[Kunihiko Sakamoto, 2015-08-20 08:56:47]

Nate, sof,

PTAL for owners. Thanks!

[sof, 2015-08-20 14:56:58]

https://codereview.chromium.org/1267023004/diff/140001/LayoutTests/http/tests...
File LayoutTests/http/tests/webfont/resources/cookie-match.cgi (right):

https://codereview.chromium.org/1267023004/diff/140001/LayoutTests/http/tests...
LayoutTests/http/tests/webfont/resources/cookie-match.cgi:1: #!/usr/bin/perl -wT
nit: is the use of perl crucial? (I thought we were trying not to extend our
reliance on perl for tests also.)

https://codereview.chromium.org/1267023004/diff/140001/Source/core/css/CSSFon...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/1267023004/diff/140001/Source/core/css/CSSFon...
Source/core/css/CSSFontFaceSrcValue.cpp:83: bool sameOriginRequest =
securityOrigin->canRequestNoSuborigin(request.url());
I see what this is based on, but should we rephrase it to be more Blink-like?
Something like:

    StoredCredentials allowCredentials = DoNotAllowStoredCredentials;
    bool sameOriginRequest =
securityOrigin->canRequestNoSuborigin(request.url());
    // Include credentials for same origin requests (and assume that
    // redirects out of origin will be handled per Fetch spec.)
    if (sameOriginRequest)
        allowCredentials = AllowStoredCredentials;
    request.setCrossOriginAccessControl(securityOrigin, allowCredentials,
ClientDidNotRequestCredentials);

[Kunihiko Sakamoto, 2015-08-21 01:37:07]

https://codereview.chromium.org/1267023004/diff/140001/LayoutTests/http/tests...
File LayoutTests/http/tests/webfont/resources/cookie-match.cgi (right):

https://codereview.chromium.org/1267023004/diff/140001/LayoutTests/http/tests...
LayoutTests/http/tests/webfont/resources/cookie-match.cgi:1: #!/usr/bin/perl -wT
On 2015/08/20 14:56:58, sof wrote:
> nit: is the use of perl crucial? (I thought we were trying not to extend our
> reliance on perl for tests also.)

Rewrote it with php.

https://codereview.chromium.org/1267023004/diff/140001/Source/core/css/CSSFon...
File Source/core/css/CSSFontFaceSrcValue.cpp (right):

https://codereview.chromium.org/1267023004/diff/140001/Source/core/css/CSSFon...
Source/core/css/CSSFontFaceSrcValue.cpp:83: bool sameOriginRequest =
securityOrigin->canRequestNoSuborigin(request.url());
On 2015/08/20 14:56:58, sof wrote:
> I see what this is based on, but should we rephrase it to be more Blink-like?
> Something like:
> 
>     StoredCredentials allowCredentials = DoNotAllowStoredCredentials;
>     bool sameOriginRequest =
> securityOrigin->canRequestNoSuborigin(request.url());
>     // Include credentials for same origin requests (and assume that
>     // redirects out of origin will be handled per Fetch spec.)
>     if (sameOriginRequest)
>         allowCredentials = AllowStoredCredentials;
>     request.setCrossOriginAccessControl(securityOrigin, allowCredentials,
> ClientDidNotRequestCredentials);

Done.

[sof, 2015-08-21 05:39:43]

thanks, lgtm.

[sof, 2015-08-21 05:39:47]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-08-21 05:39:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org
Link to the patchset: https://codereview.chromium.org/1267023004/#ps160001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-08-21 05:40:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267023004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267023004/160001

[commit-bot: I haz the power, 2015-08-21 09:00:28]

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

[commit-bot: I haz the power, 2015-08-21 09:00:29]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Kunihiko Sakamoto, 2015-08-21 09:01:50]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2015-08-21 09:02:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267023004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267023004/160001

[commit-bot: I haz the power, 2015-08-21 09:07:08]

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

[commit-bot: I haz the power, 2015-08-21 09:07:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kunihiko Sakamoto, 2015-08-21 09:14:52]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2015-08-21 09:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
tyoshino@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1267023004/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-21 09:15:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267023004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267023004/180001

[commit-bot: I haz the power, 2015-08-21 10:50:57]

Committed patchset #6 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200980