Use the request URL as the URL of the resources which are fetched ServiceWorker [1/3 blink]

Use the request URL as the URL of the resources which are fetched ServiceWorker [1/3 blink]

In current implementation the response's URL is used as the URL of the resources. But it is not correct.


1/3 blink: https://codereview.chromium.org/732963003 [THIS PATCH]
 - Intorduce originalURLViaServiceWorker in ResourceResponse and WebURLResponse
 - Change CSP check to use originalURLViaServiceWorker if exists in ResourceFetcher::didReceiveResponse().
2/3 chromium: https://codereview.chromium.org/731983003
 - Stop updating the request's URL in ResourceDispatcher::OnReceivedResponse() and WebURLLoaderImpl::Context::OnReceivedResponse().
 - Set WebURLResponse's originalURLViaServiceWorker in WebURLLoaderImpl::PopulateURLResponse().
3/3 blink: https://codereview.chromium.org/732003002
 - Change CSP check to use originalURLViaServiceWorker.
 - Add LayoutTest for this bug.

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

[horo, 2014-11-17 07:09:03]

Patchset #1 (id:1) has been deleted

[horo, 2014-11-17 08:11:34]

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

[horo, 2014-11-17 08:11:35]

yhirano@

[horo, 2014-11-17 08:11:47]

yhirano@
Could you please review?

[yhirano, 2014-11-19 04:06:32]

lgtm

[horo, 2014-11-19 04:12:25]

The CQ bit was checked by horo@chromium.org

[horo, 2014-11-19 04:12:48]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-11-19 06:30:53]

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

[horo, 2014-11-19 06:30:54]

mkwst@
Could you please review this?

[Mike West, 2014-11-19 10:00:28]

Hrm. I'm a little bit worried about the security implications of pretending that
a resource is actually hosted at a different URL. This is especially true for
Flash, where the resource URL determines it's effective origin, but also
interesting for canvas tainting, mixed content/csp checks, etc.

Have these considerations been addressed in the design of this aspect of SW?

[horo, 2014-11-19 13:12:49]

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

[horo, 2014-11-19 13:20:50]

On 2014/11/19 10:00:28, Mike West wrote:
> Hrm. I'm a little bit worried about the security implications of pretending
that
> a resource is actually hosted at a different URL. This is especially true for
> Flash, where the resource URL determines it's effective origin, but also
> interesting for canvas tainting, mixed content/csp checks, etc.
> 
> Have these considerations been addressed in the design of this aspect of SW?

The HTTP requests from plugins are not handled by the ServiceWorker.
(crbug.com/413094)
For the canvas tainting, we check serviceWorkerResponseType() in
ImageResource::isAccessAllowed().
For mixed content/csp checks, we checks originalURLViaServiceWorker() in
ResourceFetcher::didReceiveResponse().

There is a discussion about this issue.
https://github.com/slightlyoff/ServiceWorker/issues/560

But according to the "current" ServiceWorker Spec the URL should be the
requested URL.
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#respond-with...
> The final URL of all successful (non network-error) responses is the requested
URL

[Mike West, 2014-11-19 16:19:45]

The GitHub bug you point to basically says that we should be doing the CSP check
on the response URL after the response comes back to the document. That seems
reasonable. Do we have a path towards that world?

Basically, I don't want us to land this and then forget about the thing we
actually want to do. :)

[horo, 2014-11-19 21:18:01]

On 2014/11/19 16:19:45, Mike West wrote:
> The GitHub bug you point to basically says that we should be doing the CSP
check
> on the response URL after the response comes back to the document. That seems
> reasonable. Do we have a path towards that world?

Yes. We check the response URL in ResourceFetcher::didReceiveResponse.
|originalURL| is the response URL.

> Basically, I don't want us to land this and then forget about the thing we
> actually want to do. :)

But I think we need this change to fix this relative URL bug.
https://github.com/slightlyoff/ServiceWorker/issues/560#issuecomment-63272361

[Mike West, 2014-11-20 07:57:54]

Alright. LGTM.

[horo, 2014-11-20 11:04:21]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-11-20 11:05:03]

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

[commit-bot: I haz the power, 2014-11-20 11:23:24]

Committed patchset #1 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185659