Introduce new security restrictions in FetchEvent.respondWith().

Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249
Committed: https://crrev.com/82298e59225ddfab0402b77c265624acb29632a4
Cr-Commit-Position: refs/heads/master@{#438063}

[horo, 2016-11-25 10:35:46]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-25 10:36:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-25 12:11:26]

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

[commit-bot: I haz the power, 2016-11-25 12:11:27]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-28 01:49:35]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 01:49:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 03:14:50]

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

[commit-bot: I haz the power, 2016-11-28 03:14:51]

Dry run: This issue passed the CQ dry run.

[horo, 2016-11-29 06:52:13]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 06:52:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-11-29 06:53:55]

Patchset #4 (id:60001) has been deleted

[horo, 2016-11-29 06:53:58]

Patchset #2 (id:20001) has been deleted

[horo, 2016-11-29 06:53:59]

Patchset #2 (id:40001) has been deleted

[horo, 2016-11-29 06:54:03]

Patchset #1 (id:1) has been deleted

[horo, 2016-11-29 07:55:10]

Description was changed from

==========
Introduce a new security restriction in handling navigation request FetchEvent.

BUG=
==========

to

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Disallows responding to requests which redirect mode is not 'follow' with
   redirected responses. But not to suddenly break existing sites we allow
   responding to navigation requests with redirected responses and show two
   warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249
==========

[commit-bot: I haz the power, 2016-11-29 08:56:43]

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

[commit-bot: I haz the power, 2016-11-29 08:56:44]

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

[horo, 2016-11-29 09:54:24]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 09:54:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 11:26:40]

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

[commit-bot: I haz the power, 2016-11-29 11:26:41]

Dry run: This issue passed the CQ dry run.

[horo, 2016-12-12 07:48:47]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 07:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-12 07:49:14]

Patchset #1 (id:80001) has been deleted

[horo, 2016-12-12 07:49:38]

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

[horo, 2016-12-12 07:49:39]

falken@
Could you please review?

[falken, 2016-12-12 08:40:18]

looks good

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html
(right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html:1:
<!DOCTYPE html>
In CL description, part #2 could be more clear this is just adding a deprecation
message, not yet disallowing anything. Maybe the first sentence should be "Add a
deprecation warning for responding to requests..."

part #1 I haven't seen discussed in the intent to ship or the bug. Is this just
a small bug you noticed while making this patch and consider trivial (i.e.,
doesn't warrant intent to ship)?

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html:9:
function assert_rejects(promise, description) {
Can this use testharness's promise_rejects instead?

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Deprecation.cpp:420: "redirected response.
This will be resulted in an error in %s.",
"This will result in an error"

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:322: // Shows the
deprication message and measures the impact of the new security
deprecation

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:412: // The following
code will create an opaque-redirect filtered response.
A little confusing that the following code is "break". Maybe:
// Break; the code below creates an opaque-redirect filtered response.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:72:
"request which redirect mode is not \"manual\".";
s/which/whose

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:96:
"redirect mode is not \"follow\".";
s/which/whose

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:114:
"of the response was: [\"" + responseURLList[0].getString() + "\"";
Maybe tighten this: "In Chrome 59, the navigation to requestURL.getString() will
result in a network error, because FetchEvent.respondWith() was called with a
redirected response. See https://crbug.com/658249. The url list of the response
was..."

[horo, 2016-12-12 09:01:06]

Description was changed from

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Disallows responding to requests which redirect mode is not 'follow' with
   redirected responses. But not to suddenly break existing sites we allow
   responding to navigation requests with redirected responses and show two
   warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249
==========

to

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249
==========

[horo, 2016-12-12 09:29:53]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 09:30:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-12-12 09:31:07]

Thank you.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html
(right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html:1:
<!DOCTYPE html>
On 2016/12/12 08:40:17, falken wrote:
> In CL description, part #2 could be more clear this is just adding a
deprecation
> message, not yet disallowing anything. Maybe the first sentence should be "Add
a
> deprecation warning for responding to requests..."

Done.


> part #1 I haven't seen discussed in the intent to ship or the bug. Is this
just
> a small bug you noticed while making this patch and consider trivial (i.e.,
> doesn't warrant intent to ship)?

Yes. I think this is a trivial bug and we don't need intent to ship for it.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/redirected-response.html:9:
function assert_rejects(promise, description) {
On 2016/12/12 08:40:17, falken wrote:
> Can this use testharness's promise_rejects instead?

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/Deprecation.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/Deprecation.cpp:420: "redirected response.
This will be resulted in an error in %s.",
On 2016/12/12 08:40:17, falken wrote:
> "This will result in an error"

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:322: // Shows the
deprication message and measures the impact of the new security
On 2016/12/12 08:40:18, falken wrote:
> deprecation

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchManager.cpp (right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchManager.cpp:412: // The following
code will create an opaque-redirect filtered response.
On 2016/12/12 08:40:18, falken wrote:
> A little confusing that the following code is "break". Maybe:
> // Break; the code below creates an opaque-redirect filtered response.

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp
(right):

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:72:
"request which redirect mode is not \"manual\".";
On 2016/12/12 08:40:18, falken wrote:
> s/which/whose

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:96:
"redirect mode is not \"follow\".";
On 2016/12/12 08:40:18, falken wrote:
> s/which/whose

Done.

https://codereview.chromium.org/2526343003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.cpp:114:
"of the response was: [\"" + responseURLList[0].getString() + "\"";
On 2016/12/12 08:40:18, falken wrote:
> Maybe tighten this: "In Chrome 59, the navigation to requestURL.getString()
will
> result in a network error, because FetchEvent.respondWith() was called with a
> redirected response. See https://crbug.com/658249. The url list of the
response
> was..."

Done.

[commit-bot: I haz the power, 2016-12-12 11:20:42]

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

[commit-bot: I haz the power, 2016-12-12 11:20:43]

Dry run: This issue passed the CQ dry run.

[falken, 2016-12-13 04:43:39]

sorry for the delay, slipped my attention this morning. lgtm.

https://codereview.chromium.org/2526343003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h
(right):

https://codereview.chromium.org/2526343003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/serviceworkers/RespondWithObserver.h:75: const
WebURLRequest::RequestContext m_requestContext;
like the consts

[horo, 2016-12-13 04:48:32]

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

[horo, 2016-12-13 04:48:32]

haraken@
Could you please review third_party/WebKit/Source/core/* and
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp?

[horo, 2016-12-13 04:48:35]

haraken@
Could you please review third_party/WebKit/Source/core/* and
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp?

[haraken, 2016-12-13 05:13:27]

LGTM

[horo, 2016-12-13 05:15:40]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2016-12-13 05:16:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 05:21:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481606140201350,
"parent_rev": "aed096709ab0996089d1536f6b5314f03e90b0a9", "commit_rev":
"97e1733e5534661949f291dfde76b01a2d08b349"}

[commit-bot: I haz the power, 2016-12-13 05:22:07]

Description was changed from

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249
==========

to

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249

Review-Url: https://codereview.chromium.org/2526343003
==========

[commit-bot: I haz the power, 2016-12-13 05:22:09]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2016-12-13 05:28:53]

Description was changed from

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249

Review-Url: https://codereview.chromium.org/2526343003
==========

to

==========
Introduce new security restrictions in FetchEvent.respondWith().

This CL introduces two changes in the restriction of FetchEvent.respondWith().

1. Allow responding to non-navigation requests which redirect mode is 'manual'
   with opaque-redirect responses.
   Ex:
     SW: self.onfetch = evt => { evt.respondWith(fetch(evt.request)); };
     Page: fetch(new Request("/redirect-url", {redirect: 'manual'}));
     Server: Returns a redirect response to somewhere.
     Before this CL: fetch() fails.
     After this CL: fetch() returns the opaque-redirect response.

2. Add a deprecation warning for responding to requests which redirect mode is
   not 'follow' with redirected responses. Not to suddenly break existing sites
   we allow responding to navigation requests with redirected responses and show
   two warning messages. One in the DevTools attached to the service worker from
   RespondWithObserver::responseWasFulfilled(). And one in the DevTools attached
   to the page tab from DocumentLoader::finishedLoading().

BUG=658249

Committed: https://crrev.com/82298e59225ddfab0402b77c265624acb29632a4
Cr-Commit-Position: refs/heads/master@{#438063}
==========

[commit-bot: I haz the power, 2016-12-13 05:28:55]

Patchset 3 (id:??) landed as
https://crrev.com/82298e59225ddfab0402b77c265624acb29632a4
Cr-Commit-Position: refs/heads/master@{#438063}