Create a new reserved environment for cross-origin redirects. #3891

mfalken · 2018-08-08T07:35:51Z

Previously, a navigation would create a reserved environment once,
and use it for all redirects. This commit changes that so
a new environment is created on a cross-origin redirect.

This also runs CSP for each redirect, which is probably more
correct.

Per w3c/ServiceWorker#1316

/browsing-the-web.html ( diff )
/infrastructure.html ( diff )

mfalken · 2018-08-08T07:37:49Z

@annevk would you be able to review this?
@mikewest for CSP.

mfalken · 2018-08-10T08:44:41Z

Hum, there's a problem here. Is the intent of creation URL to be after redirects? The service worker spec seems to be using creation URL as the current URL. Maybe that's mistake of the service worker spec though.

The current PR will update creation URL after cross-origin redirects but not same-origin ones, which would be too inconsistent.

@wanderview @jungkees @jakearchibald what do you think?

wanderview · 2018-08-10T14:21:26Z

I think the most sane thing is to set the creation URL at the time the settings object is marked execution ready. That would be after all redirects. (FWIW, this is what I implemented in firefox.)

I think we discussed this a bit in regard to SharedWorker here:

w3c/ServiceWorker#1289

Also a bit in here regarding documents:

w3c/ServiceWorker#1091

I'm not sure, though, if the current spec says anything like that.

wanderview · 2018-08-10T14:23:30Z

I think the most sane thing is to set the creation URL at the time the settings object is marked execution ready. That would be after all redirects. (FWIW, this is what I implemented in firefox.)

I guess since we don't expose reserved clients it may not matter if the creation URL is set earlier. Any redirects should be complete before execution ready is set and causes the client to be exposed. Changing the reserved environment on redirect should update the creation URL prior to execution ready IMO.

mfalken · 2018-08-13T01:52:41Z

@wanderview Thanks, that makes sense. I'll update the creation URL on redirects. I agree it shouldn't matter if we do that right after the redirect on the reserved client, or right before execution ready, since reserved clients aren't exposed. Setting it after redirect seems good to me because it's where most of the reserved client is created, and the SW spec might try to access the creation URL of the reserved client rather than the request URL.

Do you know off-hand if we have WPT for this (Client#url after redirects)?

mfalken · 2018-08-13T02:11:46Z

Revised. This will need WPT for the service worker spec. I'll work on those soon but would prefer to merge this change in parallel of that if it looks good..

wanderview · 2018-08-13T16:07:12Z

Do you know off-hand if we have WPT for this (Client#url after redirects)?

I don't see one. And of course we also don't have a WPT test that showing history.pushState() does not change the client.url either. Both of those tests would be great to clarify behavior here.

mfalken · 2018-08-13T21:00:35Z

Thanks. Another quick question while you're around: do you recall if Client.url for windows is intended to reflect the final response URL (i.e., including respondWith(fetch(url)) or just the request URL after redirects? I know workers are supposed to reflect the response URL (#3771). There was some talk at w3c/ServiceWorker#1031 too.

Yea.. WPT would be good for all this.

wanderview · 2018-08-13T21:28:49Z

Oh, interesting question. Since window navigation is a special case and does not reflect the final URL provided to respondWith(), I think it should also not be reflected in the client.url value.

I think that is what we implemented in firefox. See the navigation check here:

https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/dom/serviceworkers/ServiceWorkerEvents.cpp#713-731

(The comment isn't making sense to me right now, though.)

The client.url is then set to nsIDocument::GetOriginalURI() which is the after-redirect URL of the document:

https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/dom/clients/manager/ClientSource.cpp#266

In preparation for extending or modifying the test to test Client.url and resultingClientId for whatwg/html#3891. Bug: 876223 Change-Id: I50e8b8c21c5f8639a24a9c2ec00df3ccafdc8ac8

In preparation for extending or modifying the test to test Client.url and resultingClientId for whatwg/html#3891. Bug: 876223 Change-Id: I50e8b8c21c5f8639a24a9c2ec00df3ccafdc8ac8 Reviewed-on: https://chromium-review.googlesource.com/1184656 Commit-Queue: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Cr-Commit-Position: refs/heads/master@{#585738}

annevk

I'm unclear why we need request cloning here and manipulation of request's URL list. It seems too much of Fetch is creeping into the navigate algorithm.

annevk · 2018-08-28T07:08:02Z

source

-    <span data-x="concept-environment-creation-url">creation URL</span> matches a service worker
-    registration. <ref spec="SW"></p>
-   </li>
+   <li><p>Repeat the following steps until <var>done</var> is true:</p>


While done is false*

Also, since this has more than one nested element the <p> needs to be newlined

annevk · 2018-08-28T07:08:15Z

source

@@ -2830,10 +2830,13 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
      data-x-href="https://fetch.spec.whatwg.org/#concept-request">request</dfn> and its associated:
      <ul class="brief">
       <li><dfn data-x="concept-request-url" data-x-href="https://fetch.spec.whatwg.org/#concept-request-url">url</dfn></li>
+       <li><dfn data-x="concept-request-url-list" data-x-href="https://fetch.spec.whatwg.org/#concept-request-url-list">url list</dfn></li>


annevk · 2018-08-28T07:08:25Z

source

       <li><dfn data-x="concept-request-method" data-x-href="https://fetch.spec.whatwg.org/#concept-request-method">method</dfn></li>
       <li><dfn data-x="concept-request-header-list" data-x-href="https://fetch.spec.whatwg.org/#concept-request-header-list">header list</dfn></li>
       <li><dfn data-x="concept-request-body" data-x-href="https://fetch.spec.whatwg.org/#concept-request-body">body</dfn></li>
       <li><dfn data-x="concept-request-client" data-x-href="https://fetch.spec.whatwg.org/#concept-request-client">client</dfn></li>
+       <li><dfn data-x="concept-request-clone" data-x-href="https://fetch.spec.whatwg.org/#concept-request-clone">clone</dfn></li>
+       <li><dfn data-x="concept-request-current-url" data-x-href="https://fetch.spec.whatwg.org/#concept-request-current-url">current url</dfn></li>


current URL

annevk · 2018-08-28T07:09:15Z

source

-   client</span> to <var>reservedEnvironment</var>.</p></li>
+    <ol>
+     <li><p>Let <var>newRequest</var> be a <span data-x="concept-request-clone">clone</span> of
+     <var>request</var>.</li>


Closing </p>

annevk · 2018-08-28T07:09:34Z

source

-    <ref spec="CSP"></p>
+     <li><p>If <var>response</var> is not null, append <var>response</var>'s <span
+     data-x="concept-response-location-url">location URL</span> to <var>newRequest</var>'s
+     <span data-x="concept-request-url-list">url list.</span></p></li>


Dot outside the span element.

annevk · 2018-08-28T07:24:57Z

source


-    <p>Otherwise:</p>
+     <li>
+      <p>If <var>reservedEnvironment</var> is not null, and <var>newRequest's</var>


's outside the var element

annevk · 2018-08-28T07:25:31Z

source


+     <li><p>If <var>reservedEnvironment</var> is null, let <var>reservedEnvironment</var> be a new


s/let/then set/
s/be/to/

annevk · 2018-08-28T07:26:16Z

source


+     <li><p>If <var>reservedEnvironment</var> is null, let <var>reservedEnvironment</var> be a new
+     <span>environment</span>, and set its <span data-x="concept-environment-id">id</span> to a new


"a new environment whose id is ... and target browsing context is ..."

annevk · 2018-08-28T07:44:35Z

source

+     <var>browsingContext</var>, then set <var>response</var> to a network error, and set
+     <var>done</var> to true. <ref spec="CSP"></p></li>
+
+     <li><p>Otherwise:</p>


Newline <p>

annevk · 2018-08-28T07:44:50Z

source

+     <li><p>If the <span>Should navigation request of type from source in target be blocked by Content
+     Security Policy?</span> algorithm returns "<code data-x="">Blocked</code>" when executed upon
+     <var>newRequest</var>, <var>navigationType</var>, <var>sourceBrowsingContext</var>, and
+     <var>browsingContext</var>, then set <var>response</var> to a network error, and set


link network error

no need for comma before "and set" as there's only two actions.

annevk · 2018-08-28T07:57:19Z

(Apologies for the delay here by the way. I overlooked this somehow.)

… navigation-redirect.https.html., a=testonly Automatic update from web-platform-testsWPT: service worker: Refactor and format navigation-redirect.https.html. In preparation for extending or modifying the test to test Client.url and resultingClientId for whatwg/html#3891. Bug: 876223 Change-Id: I50e8b8c21c5f8639a24a9c2ec00df3ccafdc8ac8 Reviewed-on: https://chromium-review.googlesource.com/1184656 Commit-Queue: Matt Falkenhagen <falken@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Makoto Shimazu <shimazu@chromium.org> Cr-Commit-Position: refs/heads/master@{#585738} -- wpt-commits: 1bc6d9975ef38c3ade2986dff737b4421c6352df wpt-pr: 12610

mfalken · 2018-08-31T08:15:27Z

Thank you for the review!

Sorry I'm not good at git... looks like I messed up this PR trying to sync with upstream changes.

mfalken · 2018-08-31T08:34:03Z

I'm unclear why we need request cloning here and manipulation of request's URL list. It seems too much of Fetch is creeping into the navigate algorithm.

Hum, I think I needed this for the "Should navigation request of type from source in target be blocked by Content Security Policy?" step. I thought it should use request with the new URL added to it. But we can't modify request directly here since the Fetch spec also modifies it in HTTP-redirect fetch.

However, it looks like https://w3c.github.io/webappsec-csp/#should-block-navigation-request uses "request's URL". There are two problems here:

URL links to the "response URL" concept in the Fetch spec, which looks wrong
It probably means to link to "request URL" which seems to be the first URL of the redirect chain, in which case appending the request to the list won't help.

I guess I can keep CSP as is for this change (keep passing request), and can probably forgo the cloning.

mfalken · 2018-08-31T09:13:52Z

@annevk thanks, I addressed your feedback.

(I think I was able to recover the PR by using git push -f to cancel the previous bad push)

mfalken · 2018-09-05T07:40:01Z

@annevk: ping. I also have a WPT at https://chromium-review.googlesource.com/c/chromium/src/+/1206054

annevk

This looks a lot better, thanks. Did you file issues against CSP for the issues you found?

source

mfalken · 2018-09-06T09:36:00Z

Thanks! I will be away for some time but plan to get back to this ASAP.

Previously, a navigation would create a reserved environment once, and use it for all redirects. This commit changes that so a new environment is created on a cross-origin redirect. This also runs CSP for each redirect, which is probably more correct. Per w3c/ServiceWorker#1316

mfalken · 2018-09-28T05:54:45Z

Thanks! I'm preparing a patch in response to your comments.

Did you file issues against CSP for the issues you found?

Filed w3c/webappsec-csp#343

mfalken · 2018-09-28T06:58:28Z

@annevk I believe I addressed your comments. Please take another look.

annevk

I think this is good, modulo nits. You should also ensure there are two newlines between any </p> and <ol>.

I'd love it if @wanderview could have a look as well, given he's somewhat familiar with all this and might be able to spot more.

source

mfalken · 2018-10-03T08:13:59Z

Thanks @annevk! Updated. @wanderview did you want to look as well?

wanderview · 2018-10-03T14:17:47Z

LGTM

annevk

Thanks! Tests already landed? And presumably there's cross-browser support since this is a bug fix.

mfalken · 2018-10-05T13:45:59Z

Thanks! Yes, tests landed in web-platform-tests/wpt@2df7f9f#diff-f23ebe2bdee04a9c0b8ab094be59f318

And yes, I think this was the consensus from previous F2Fs and in the latest issue:
w3c/ServiceWorker#1031 (comment)
w3c/ServiceWorker#1316

annevk · 2018-10-05T14:47:48Z

Great, are there implementation bugs? If those are filed this is ready to land.

mfalken · 2018-10-05T15:24:52Z

There are bugs against resultingClientId (which the WPT depends on) at:

Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=778497
Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1264177
WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=190313

annevk · 2018-10-08T07:35:19Z

Thanks!

… navigation-redirect.https.html., a=testonly Automatic update from web-platform-testsWPT: service worker: Refactor and format navigation-redirect.https.html. In preparation for extending or modifying the test to test Client.url and resultingClientId for whatwg/html#3891. Bug: 876223 Change-Id: I50e8b8c21c5f8639a24a9c2ec00df3ccafdc8ac8 Reviewed-on: https://chromium-review.googlesource.com/1184656 Commit-Queue: Matt Falkenhagen <falkenchromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Makoto Shimazu <shimazuchromium.org> Cr-Commit-Position: refs/heads/master{#585738} -- wpt-commits: 1bc6d9975ef38c3ade2986dff737b4421c6352df wpt-pr: 12610 UltraBlame original commit: 3da52ed85a2cb6a6c6b61cb230bd008bb397e629

chromium-wpt-export-bot mentioned this pull request Aug 22, 2018

WPT: service worker: Refactor and format navigation-redirect.https.html. web-platform-tests/wpt#12610

Merged

domenic requested a review from annevk August 28, 2018 03:29

annevk reviewed Aug 28, 2018

View reviewed changes

mfalken force-pushed the reserved branch from f3f8309 to 5c64801 Compare August 31, 2018 08:20

annevk reviewed Sep 6, 2018

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

source Show resolved Hide resolved

mfalken added 6 commits September 28, 2018 14:24

Add CSP.

4c14464

Try to fix Travis error.

12ac2aa

set creation URL every time

c1c37ec

Add <var> for newRequest.

1a8fee6

review comments

cc68b29

more

adca630

mfalken force-pushed the reserved branch from 6946861 to adca630 Compare September 28, 2018 05:25

fix

5ec123a

annevk reviewed Oct 2, 2018

View reviewed changes

source Show resolved Hide resolved

source Show resolved Hide resolved

fix

93b767e

annevk approved these changes Oct 5, 2018

View reviewed changes

annevk merged commit 45ddbd2 into whatwg:master Oct 8, 2018

mfalken mentioned this pull request Jul 2, 2019

How to create registration objects w3c/ServiceWorker#1445

Merged


		<li><p>If <var>reservedEnvironment</var> is null, let <var>reservedEnvironment</var> be a new


		<li><p>If <var>reservedEnvironment</var> is null, let <var>reservedEnvironment</var> be a new
		<span>environment</span>, and set its <span data-x="concept-environment-id">id</span> to a new

Create a new reserved environment for cross-origin redirects. #3891

Create a new reserved environment for cross-origin redirects. #3891

Conversation

mfalken commented Aug 8, 2018 • edited by pr-preview bot Loading

mfalken commented Aug 8, 2018

mfalken commented Aug 10, 2018

wanderview commented Aug 10, 2018

wanderview commented Aug 10, 2018

mfalken commented Aug 13, 2018

mfalken commented Aug 13, 2018

wanderview commented Aug 13, 2018

mfalken commented Aug 13, 2018

wanderview commented Aug 13, 2018

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Aug 28, 2018

mfalken commented Aug 31, 2018

mfalken commented Aug 31, 2018

mfalken commented Aug 31, 2018

mfalken commented Sep 5, 2018

annevk left a comment

Choose a reason for hiding this comment

mfalken commented Sep 6, 2018

mfalken commented Sep 28, 2018 • edited Loading

mfalken commented Sep 28, 2018

annevk left a comment

Choose a reason for hiding this comment

mfalken commented Oct 3, 2018

wanderview commented Oct 3, 2018

annevk left a comment

Choose a reason for hiding this comment

mfalken commented Oct 5, 2018

annevk commented Oct 5, 2018

mfalken commented Oct 5, 2018

annevk commented Oct 8, 2018

mfalken commented Aug 8, 2018 •

edited by pr-preview bot

Loading

mfalken commented Sep 28, 2018 •

edited

Loading