[ServiceWorker] Fix ResourceFetcher::isControlledByServiceWorker to support HTMLImports.

[ServiceWorker] Fix ResourceFetcher::isControlledByServiceWorker to support HTMLImports.

While loading the resources from the imported HTML, ResourceFetcher's m_documentLoader is NULL.
In such cases whether the request is controlled by ServiceWorker or not is determined by the document loader of the frame.
But in current code ResourceFetcher::isControlledByServiceWorker() always returns false.

This patch fixes this by checking the document loader of the frame.

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

[horo, 2014-10-27 07:14:34]

horo@chromium.org changed reviewers:
+ michaeln@chromium.org

[horo, 2014-10-27 07:14:34]

michaeln@
Could you please review this?

[michaeln, 2014-10-27 21:11:48]

The changes to ResourceFetcher::isControlledByServiceWorker() lgtm in light of
how HTMLImports are loaded.

But it doesn't make much sense that FetchRequestModeCORS is set for this
same-origin request where CORS is not in the picture.

[horo, 2014-10-28 00:51:02]

> The changes to ResourceFetcher::isControlledByServiceWorker() lgtm in light of
> how HTMLImports are loaded.

Thank you.

> But it doesn't make much sense that FetchRequestModeCORS is set for this
> same-origin request where CORS is not in the picture.

I think the request mode of HTMLImports must be CORS.
Otherwise the malicious site can read any document in other origin.
In the page:
  var link = document.createElement('link');
  link.rel = 'import';
  link.href = 'foo.html'
  link.onload = function(e) { console.log(link.import.body.innerHTML); };
  document.head.appendChild(link);

In the ServiceWorker:
  self.addEventListener('fetch', function(event) {
    if (event.request.url.search('foo.html') != -1) {
     
event.respondWith(fetch('http://target.example.com/target_document.html'));
    }
  });

We prevent this by checking the request mode and the response type.
https://fetch.spec.whatwg.org/#http-fetch
> If either response's type is opaque and request's mode is not no CORS or
response's type is error, return a network error.

[horo, 2014-10-28 00:51:22]

horo@chromium.org changed reviewers:
+ mkwst@chromium.org

[horo, 2014-10-28 00:51:22]

mkwst@
Could you please review this?

[horo, 2014-10-28 02:33:08]

Patchset #2 (id:20001) has been deleted

[michaeln, 2014-10-28 02:52:02]

> I think the request mode of HTMLImports must be CORS.

That the request mode is set differently depending on whether the page
isControlledByServiceWorker is odd. It seems like the mode should be independent
of whether a serviceworker is in the loop.

[horo, 2014-10-28 03:41:03]

On 2014/10/28 02:52:02, michaeln wrote:
> > I think the request mode of HTMLImports must be CORS.
> 
> That the request mode is set differently depending on whether the page
> isControlledByServiceWorker is odd. It seems like the mode should be
independent
> of whether a serviceworker is in the loop.

HTMLImportsController::load() calls FetchRequest::setCrossOriginAccessControl().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
So the request mode of HTMLImports is always CORS regardless of whether the page
is controlled by the ServiceWorker or not.

[Mike West, 2014-10-28 08:42:47]

LGTM.

https://codereview.chromium.org/655373003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/655373003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:627:
ASSERT(localFrame->loader().documentLoader());
Nit: I'd suggest moving this (and the comment) up to the top as a precondition
for the method, e.g. `ASSERT(m_documentLoader ||
localFrame->loader().documentLoader())`.

[Mike West, 2014-10-28 08:42:49]

LGTM.

[horo, 2014-10-28 14:36:37]

https://codereview.chromium.org/655373003/diff/1/Source/core/fetch/ResourceFe...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/655373003/diff/1/Source/core/fetch/ResourceFe...
Source/core/fetch/ResourceFetcher.cpp:627:
ASSERT(localFrame->loader().documentLoader());
On 2014/10/28 08:42:47, Mike West wrote:
> Nit: I'd suggest moving this (and the comment) up to the top as a precondition
> for the method, e.g. `ASSERT(m_documentLoader ||
> localFrame->loader().documentLoader())`.

Done.

[horo, 2014-10-28 15:34:03]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-28 15:34:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/655373003/40001

[commit-bot: I haz the power, 2014-10-28 16:36:19]

Committed patchset #2 (id:40001) as 184535

[michaeln, 2014-10-28 20:28:31]

> > > I think the request mode of HTMLImports must be CORS.
> > 
> > That the request mode is set differently depending on whether the page
> > isControlledByServiceWorker is odd. It seems like the mode should be
> > independent of whether a serviceworker is in the loop.
> 
> So the request mode of HTMLImports is always CORS regardless of whether the
page
> is controlled by the ServiceWorker or not.

Ah, good!

Apologies for the distracting questions. I think i'm making non sequitur
comments here in response to a somewhat related chunk of code that's not
directly involved in the HTMLImports loading case. This block of code from
DocumentLoader::DocumentLoader() is what i'm wondering about. That looks like
the it sets the request mode differently depending.

    // If the fetch request will be handled by the ServiceWorker, the
    // FetchRequestMode of the request must be FetchRequestModeCORS or
    // FetchRequestModeCORSWithForcedPreflight. Otherwise the ServiceWorker can
    // return a opaque response which is from the other origin site and the
    // script in the page can read the content.
    //
    // We assume that ServiceWorker is skipped for sync requests by content/
    // code.
    if (m_async && !request.skipServiceWorker() &&
m_document.fetcher()->isControlledByServiceWorker()) {
        ResourceRequest newRequest(request);
        // FetchRequestMode should be set by the caller. But the expected value
        // of FetchRequestMode is not speced yet except for XHR. So we set here.
        // FIXME: When we support fetch API in document, this value should not
        // be overridden here.
        if (options.preflightPolicy == ForcePreflight)
           
newRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORSWithForcedPreflight);
        else
            newRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS);

        m_fallbackRequestForServiceWorker = adoptPtr(new
ResourceRequest(request));
        m_fallbackRequestForServiceWorker->setSkipServiceWorker(true);

        loadRequest(newRequest, m_resourceLoaderOptions);
        return;
    }

From the looks of it, if the request is destined for processing by a
serviceworker, the fetch mode will always be one of the two CORs variants. The
comment suggests that this is to prevent the sw from responding to a same-origin
request with data sourced from another origin.

[horo, 2014-10-29 06:17:51]

On 2014/10/28 20:28:31, michaeln wrote:
> > > > I think the request mode of HTMLImports must be CORS.
> > > 
> > > That the request mode is set differently depending on whether the page
> > > isControlledByServiceWorker is odd. It seems like the mode should be
> > > independent of whether a serviceworker is in the loop.
> > 
> > So the request mode of HTMLImports is always CORS regardless of whether the
> page
> > is controlled by the ServiceWorker or not.
> 
> Ah, good!
> 
> Apologies for the distracting questions. I think i'm making non sequitur
> comments here in response to a somewhat related chunk of code that's not
> directly involved in the HTMLImports loading case. This block of code from
> DocumentLoader::DocumentLoader() is what i'm wondering about. That looks like
> the it sets the request mode differently depending.
> 
>     // If the fetch request will be handled by the ServiceWorker, the
>     // FetchRequestMode of the request must be FetchRequestModeCORS or
>     // FetchRequestModeCORSWithForcedPreflight. Otherwise the ServiceWorker
can
>     // return a opaque response which is from the other origin site and the
>     // script in the page can read the content.
>     //
>     // We assume that ServiceWorker is skipped for sync requests by content/
>     // code.
>     if (m_async && !request.skipServiceWorker() &&
> m_document.fetcher()->isControlledByServiceWorker()) {
>         ResourceRequest newRequest(request);
>         // FetchRequestMode should be set by the caller. But the expected
value
>         // of FetchRequestMode is not speced yet except for XHR. So we set
here.
>         // FIXME: When we support fetch API in document, this value should not
>         // be overridden here.
>         if (options.preflightPolicy == ForcePreflight)
>            
>
newRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORSWithForcedPreflight);
>         else
>            
newRequest.setFetchRequestMode(WebURLRequest::FetchRequestModeCORS);
> 
>         m_fallbackRequestForServiceWorker = adoptPtr(new
> ResourceRequest(request));
>         m_fallbackRequestForServiceWorker->setSkipServiceWorker(true);
> 
>         loadRequest(newRequest, m_resourceLoaderOptions);
>         return;
>     }
> 
> From the looks of it, if the request is destined for processing by a
> serviceworker, the fetch mode will always be one of the two CORs variants. The
> comment suggests that this is to prevent the sw from responding to a
same-origin
> request with data sourced from another origin.

FetchRequestMode is used only when the request goes to the ServiceWorker.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
So when the request will not go to the ServiceWorker we don't need to set
FetchRequestMode.

But it is better to set FetchRequestMode in the caller of
DocumentThreadableLoader regardless of whether the page is controlled by the
ServiceWorker or not.