Expose and check origin of request in response for foreign fetch.

Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5813bd1852b2fe7e

And implements the checks for the |origin| in the ForeignFetchResponse that is
passed to ForeignFetchEvent.respondWith, as specced in steps 16.8 through 16.11 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on-foreign-fetch-request-algorithm
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1acd9f4010365f19

BUG=540509
Committed: https://crrev.com/1e4e767bf4fa7722bc5e9943d7f1446d1b1bdf61
Cr-Commit-Position: refs/heads/master@{#394163}

[Marijn Kruisselbrink, 2016-05-12 23:00:02]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2016-05-12 23:03:34]

Description was changed from

==========
WIP implement origin attribute in ForeignFetchEvent

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

to

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse
that is passed to ForeignFetchEvent.respondWith, as specced in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-05-12 23:07:06]

Description was changed from

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse
that is passed to ForeignFetchEvent.respondWith, as specced in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

to

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse
that is passed to ForeignFetchEvent.respondWith, as specced in steps 16.8
 through 16.11 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-05-12 23:07:20]

Description was changed from

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse
that is passed to ForeignFetchEvent.respondWith, as specced in steps 16.8
 through 16.11 of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

to

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse that is
passed to ForeignFetchEvent.respondWith, as specced in steps 16.8 through 16.11
of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

[Marijn Kruisselbrink, 2016-05-12 23:12:07]

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

[Marijn Kruisselbrink, 2016-05-12 23:12:08]

(the layouttests in this CL are maybe a bit more complicated than needed to test
just this change, that's because this entire CL was split off of a larger CL
that also adds support for the other foreign fetch specific response parameter
(https://codereview.chromium.org/1965013002). But that larger CL isn't quite
ready for review yet).

[horo, 2016-05-16 05:28:39]

This cl lgtm.

But please add LayoutTest to check that ForeignFetchEvent.origin is "null" when
"no-referrer" is set in the referrer page.
<meta name="referrer" content="no-referrer">

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html
(right):

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:63:
.then(() => promise_rejects(t, new TypeError(), fetch(url, {mode: 'no-cors'})))
Please wrap at 80 columns.

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:144:
description: 'Same origin fetch without CORS headers, exposed to wrong origin',
ditto

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:214:
description: 'Cross origin fetch with CORS headers, exposed to wrong origin',
ditto

[Marijn Kruisselbrink, 2016-05-16 23:35:44]

On 2016/05/16 at 05:28:39, horo wrote:
> But please add LayoutTest to check that ForeignFetchEvent.origin is "null"
when "no-referrer" is set in the referrer page.
> <meta name="referrer" content="no-referrer">

Added tests to verify origin and referrer in no-referrer requests. Also filed
https://github.com/slightlyoff/ServiceWorker/issues/899 since the spec as
written doesn't actually currently require the origin to be "null" in that case,
although I do agree that it really should be.

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html
(right):

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:63:
.then(() => promise_rejects(t, new TypeError(), fetch(url, {mode: 'no-cors'})))
On 2016/05/16 at 05:28:39, horo wrote:
> Please wrap at 80 columns.

Done

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:144:
description: 'Same origin fetch without CORS headers, exposed to wrong origin',
On 2016/05/16 at 05:28:39, horo wrote:
> ditto

Done

https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:214:
description: 'Cross origin fetch with CORS headers, exposed to wrong origin',
On 2016/05/16 at 05:28:39, horo wrote:
> ditto

Done

[horo, 2016-05-17 00:16:42]

On 2016/05/16 23:35:44, Marijn Kruisselbrink wrote:
> On 2016/05/16 at 05:28:39, horo wrote:
> > But please add LayoutTest to check that ForeignFetchEvent.origin is "null"
> when "no-referrer" is set in the referrer page.
> > <meta name="referrer" content="no-referrer">
> 
> Added tests to verify origin and referrer in no-referrer requests. Also filed
> https://github.com/slightlyoff/ServiceWorker/issues/899 since the spec as
> written doesn't actually currently require the origin to be "null" in that
case,
> although I do agree that it really should be.
> 
>
https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html
> (right):
> 
>
https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:63:
> .then(() => promise_rejects(t, new TypeError(), fetch(url, {mode:
'no-cors'})))
> On 2016/05/16 at 05:28:39, horo wrote:
> > Please wrap at 80 columns.
> 
> Done
> 
>
https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:144:
> description: 'Same origin fetch without CORS headers, exposed to wrong
origin',
> On 2016/05/16 at 05:28:39, horo wrote:
> > ditto
> 
> Done
> 
>
https://codereview.chromium.org/1969403004/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/foreign-fetch-cors.html:214:
> description: 'Cross origin fetch with CORS headers, exposed to wrong origin',
> On 2016/05/16 at 05:28:39, horo wrote:
> > ditto
> 
> Done

lgtm.

Thank you!

[Marijn Kruisselbrink, 2016-05-17 00:19:43]

mek@chromium.org changed reviewers:
+ japhet@chromium.org

[Marijn Kruisselbrink, 2016-05-17 00:19:44]

+japhet for WebKit/Source/web OWNERS

[Nate Chapin, 2016-05-17 17:39:22]

Source/web/ LGTM, but a concern in modules...

https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp:43:
} else if (m_requestOrigin->toString() != foreignFetchResponse.origin()) {
I don't know the spec well enough to know exactly which check you want here, but
I'm pretty sure the canonical way to do an origin check is with a function on
SecurityOrigin, rather than via direct string compare. Maybe
SecurityOrigin::isSameSchemeHostPort?

[Marijn Kruisselbrink, 2016-05-17 17:43:16]

https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
(right):

https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp:43:
} else if (m_requestOrigin->toString() != foreignFetchResponse.origin()) {
On 2016/05/17 at 17:39:22, Nate Chapin wrote:
> I don't know the spec well enough to know exactly which check you want here,
but I'm pretty sure the canonical way to do an origin check is with a function
on SecurityOrigin, rather than via direct string compare. Maybe
SecurityOrigin::isSameSchemeHostPort?

I actually had that check first (and then realized that the comment for
isSameSchemeHostPort is wrong and there isn't currently a isSameOrigin
equivalent method at all). But more importantly, the way regular CORS checks if
an origin received in a Access-Control-Allow-Origin header is valid is by
comparing the serialized origins, rather than comparing the origins themselves.
And this code is very similar to CORS so it makes sense to do the same thing
here.

I don't know exactly why the fetch/cors spec (and the blink implementation of
CORS) were written this way (where it matters is with what happens with unique
origins; which are serialized as "null" and then always compare as equal with
eachoterh), but for now this is definitely the correct check, and consistent
with our CORS implementation.

[Marijn Kruisselbrink, 2016-05-17 17:43:28]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2016-05-17 17:43:59]

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

[Nate Chapin, 2016-05-17 17:51:31]

On 2016/05/17 17:43:16, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp
> (right):
> 
>
https://codereview.chromium.org/1969403004/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/modules/serviceworkers/ForeignFetchRespondWithObserver.cpp:43:
> } else if (m_requestOrigin->toString() != foreignFetchResponse.origin()) {
> On 2016/05/17 at 17:39:22, Nate Chapin wrote:
> > I don't know the spec well enough to know exactly which check you want here,
> but I'm pretty sure the canonical way to do an origin check is with a function
> on SecurityOrigin, rather than via direct string compare. Maybe
> SecurityOrigin::isSameSchemeHostPort?
> 
> I actually had that check first (and then realized that the comment for
> isSameSchemeHostPort is wrong and there isn't currently a isSameOrigin
> equivalent method at all). But more importantly, the way regular CORS checks
if
> an origin received in a Access-Control-Allow-Origin header is valid is by
> comparing the serialized origins, rather than comparing the origins
themselves.
> And this code is very similar to CORS so it makes sense to do the same thing
> here.
> 
> I don't know exactly why the fetch/cors spec (and the blink implementation of
> CORS) were written this way (where it matters is with what happens with unique
> origins; which are serialized as "null" and then always compare as equal with
> eachoterh), but for now this is definitely the correct check, and consistent
> with our CORS implementation.

Fair enough. I don't love that sometimes we compare strings and sometimes we go
to SecurityOrigin, but that's a cleanup to consider another day, not something
that should block this patch.

[commit-bot: I haz the power, 2016-05-17 18:04:31]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-17 18:06:13]

Description was changed from

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse that is
passed to ForeignFetchEvent.respondWith, as specced in steps 16.8 through 16.11
of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509
==========

to

==========
Expose and check origin of request in response for foreign fetch.

Adds the |origin| attribute in ForeignFetchEvent that was specced in
https://github.com/slightlyoff/ServiceWorker/commit/10df242c45757ccf83e2b3ab5...

And implements the checks for the |origin| in the ForeignFetchResponse that is
passed to ForeignFetchEvent.respondWith, as specced in steps 16.8 through 16.11
of
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#on...
as got added in
https://github.com/slightlyoff/ServiceWorker/commit/d2fdb147c9c769ac6338117c1...

BUG=540509

Committed: https://crrev.com/1e4e767bf4fa7722bc5e9943d7f1446d1b1bdf61
Cr-Commit-Position: refs/heads/master@{#394163}
==========

[commit-bot: I haz the power, 2016-05-17 18:06:14]

Patchset 4 (id:??) landed as
https://crrev.com/1e4e767bf4fa7722bc5e9943d7f1446d1b1bdf61
Cr-Commit-Position: refs/heads/master@{#394163}