New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the URL from the response, if it has one #146

Merged
merged 1 commit into from Nov 4, 2015

Conversation

4 participants
@annevk
Copy link
Member

annevk commented Oct 28, 2015

This mostly affects the way stylesheets work in service workers. See
w3c/ServiceWorker#757 for details.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Oct 28, 2015

@wanderview, I'd appreciate review.

@annevk annevk force-pushed the networkurlwins branch from c0897b0 to 9effdfc Nov 2, 2015

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 3, 2015

I don't actually see where Response's url list is set with the URL from a network load. Is it implied in step 4 of HTTP-network fetch?

Let response be the result of making an HTTP request over connection using request, following the relevant requirements from HTTP, and waiting until all the headers are transmitted or fetch is being terminated with reason reason. If fetch is being terminated, set response's termination reason to reason.

Since we're making this logic dependent on response url list being set, we should be explicit about where it is it I think.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Nov 4, 2015

Okay, so the way to solve this would indeed be to set response's url list to a copy of request's url list in HTTP-network fetch. That way only synthetic responses end up with an empty url list in main fetch.

Use the URL from the response, if it has one
Instead of always overriding response's url list with request's url
list, even after service workers, only do so for synthetic responses.

This mostly affects the way stylesheets work in service workers. See
w3c/ServiceWorker#757 for details.

@annevk annevk force-pushed the networkurlwins branch from 9effdfc to ed37f5e Nov 4, 2015

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Nov 4, 2015

@wanderview should be fixed now.

I guess to make it even better I could create some sharing of code between service worker responses and network responses. Not sure where to place that though. Ideas?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 4, 2015

LGTM, although it would be nice if github provided a way to see the final document as a rendered html page I can view in the browser. Reading HTML diffs makes me a bit cross-eyed.

@annevk annevk merged commit ed37f5e into master Nov 4, 2015

@annevk annevk deleted the networkurlwins branch Nov 4, 2015

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 5, 2015

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Nov 9, 2015

annevk added a commit that referenced this pull request Jun 9, 2016

Make sure all responses have a URL list
In #146 I tried to let service worker responses that were not synthetic
to keep their url list. However, the way I fixed that regressed url
lists for a number of responses: HTTP cache responses, about URL
responses, etc.

This fix instead tries to keep the old setup and only overwrites the
url list of a response if it’s empty.

Fixes #312.

annevk added a commit that referenced this pull request Jun 9, 2016

Make sure all responses have a URL list
In #146 I tried to let service worker responses that were not synthetic
to keep their url list. However, the way I fixed that regressed url
lists for a number of responses: HTTP cache responses, about URL
responses, etc.

This fix instead tries to keep the old setup and only overwrites the
url list of a response if it’s empty.

Fixes #312.
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Oct 26, 2017

Blink bug https://code.google.com/p/chromium/issues/detail?id=553535

@mattto @horo-t Has this been implemented in chrome? This issue still says "assigned", but this glitch test suggests the final Response.url is being synthesized across to the intercepted request:

https://sw-passthrough-redirect.glitch.me/

In firefox I currently get:

==> SW fetch() got Response url:https://sw-passthrough-redirect.glitch.me/final.txt redirected:true
==> window fetch() got Response url:https://sw-passthrough-redirect.glitch.me/redirect.txt redirected:false

In chrome I get:

==> SW fetch() got Response url:https://sw-passthrough-redirect.glitch.me/final.txt redirected:true
==> window fetch() got Response url:https://sw-passthrough-redirect.glitch.me/final.txt redirected:true

Note that the window fetch() is seeing "final.txt".

Is this a fetch() only thing right now in chrome, or have you implemented it for all intercepted network requests?

I'm happy if this is implemented in chrome, because I'd like to fix:

https://bugzilla.mozilla.org/show_bug.cgi?id=1222008

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Oct 26, 2017

I am able to test edge 16 service workers as well, and they also show the final URL.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 1, 2017

@jakearchibald pointed me at this which suggests chrome is not using the synthetic Response.url for the CSS case:

https://css-redirect-test.glitch.me/

So it seems chrome is sometimes using synthetic Response.url and sometimes not.

I can't test edge on the CSS case yet because its hitting some error even without a SW.

Edit: link to the correct glitch

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Nov 1, 2017

It seems that Edge doesn't update the base URL on redirected with CSS. URLs remain relative to the request URL, not the response URL.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Nov 1, 2017

I'm happy for you to explore this further here, but please do escalate eventually to either an open (new) issue or web-platform-tests for better visibility.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 1, 2017

I'm happy for you to explore this further here, but please do escalate eventually to either an open (new) issue or web-platform-tests for better visibility.

We're currently implementing this spec change and will fix any WPT tests that need fixing as part of that effort. I keep asking here in case someone has an objection.

@mattto

This comment has been minimized.

Copy link

mattto commented Nov 2, 2017

Sorry not ignoring this, I just don't recall what we did off-hand and have been meaning to investigate commits/bugs. @horo-t do you remember what we've changed?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 2, 2017

No problem. I was just trying to understand the current situation a bit better in case there were lurking problems. Tom is making good progress on implementing it in firefox.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Nov 3, 2017

Note, we may have a run into a thorny spec question with this change here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1222008#c38

Consider:

var xmlDoc = document.implementation.createDocument(null, null, null);
xmlDoc.load('load_cross_origin_xml_document_cors.xml');
// SW intercepts and passes a cross-origin CORS response to respondWith()

In this case the outer network request is set up as same-origin (at least we make that restriction for XMLDocument in gecko). What should we do in the case where the service worker synthesizes a cross-origin CORS Response? Should we expose the cross-origin URL for the DocumentURI? That seems somewhat surprising and potentially breaking.

This problem exists for other same-origin requests as well such as Worker() top level scripts, same-origin fetch(), etc.

How would people feel if we rejected the outer network request if you synthesized a cross-origin Response onto a same-origin Request?

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 5, 2018

WPT: service worker: fix test for CSS base URL.
The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 6, 2018

WPT: service worker: fix test for CSS base URL.
The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 6, 2018

WPT: service worker: fix test for CSS base URL.
The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b
Reviewed-on: https://chromium-review.googlesource.com/c/1362776
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614277}

aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 6, 2018

Matt Falkenhagen Commit Bot
WPT: service worker: fix test for CSS base URL.
The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b
Reviewed-on: https://chromium-review.googlesource.com/c/1362776
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614277}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 6, 2018

WPT: service worker: fix test for CSS base URL.
The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b
Reviewed-on: https://chromium-review.googlesource.com/c/1362776
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614277}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 12, 2018

service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 12, 2018

service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2018

service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed

aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 13, 2018

Matt Falkenhagen Commit Bot
service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed
Reviewed-on: https://chromium-review.googlesource.com/c/1372109
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616248}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2018

service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed
Reviewed-on: https://chromium-review.googlesource.com/c/1372109
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616248}

chromium-wpt-export-bot added a commit to web-platform-tests/wpt that referenced this pull request Dec 13, 2018

service worker: XSLT: Use response URL for the base URL.
This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed
Reviewed-on: https://chromium-review.googlesource.com/c/1372109
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616248}

aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 14, 2018

Matt Falkenhagen Commit Bot
CSS: Use the response URL for base URL and type for security decisions.
Use ResourceResponse::ResponseUrl() to set the base URL, and use
ResponseResponse::GetType() to determine whether the resonse is
CORS-same-origin.

This CL has three web-exposed changes.

1. Use the response URL rather than the last request URL as the base URL
   of the stylesheet. This aligns with the standard. See
   whatwg/fetch#146 and WPT results indicate
   Firefox, Edge, and Safari use the response URL. This only matters if
   the response came from a service worker, as the URLs only differ
   when the service worker intercepts the request and responds with a
   different URL via respondWith(fetch(other_url)).

   This is covered by the WPT:
   service-workers/service-worker/fetch-request-css-base-url.https.html

   The test doesn't completely pass yet because the search query part of
   the URL gets chopped off for FetchEvent.request.referrer, but the base
   URL is correct.

   Chrome Status: https://www.chromestatus.com/feature/5642183499579392

2. Consider A->B->A redirects to be cross-origin rather than
   same-origin. Previously, this was considered same-origin. See the
   discussion in whatwg/fetch#737 and change
   whatwg/fetch#834.

   This change makes the following WPT test pass:
   css/cssom/stylesheet-same-origin.sub.html

   It also affects the web test:
   http/tests/security/cannot-read-cssrules-redirect.html

   This test is updated to match the behavior change. It can be removed
   later since it is redundant with the WPT test.

3. Consider load failures to be cross-origin rather than same-origin.
   That is, accessing |styleSheet.rules| throws a SecurityError if the
   load failed.  This aligns with the specification:
   - cssRules checks the `origin-clean` flag:
     https://drafts.csswg.org/cssom/#dom-cssstylesheet-cssrules
   - This is set to true iff CORS-same-origin:
     https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet
   - CORS-same-origin is false on kError:
     https://html.spec.whatwg.org/multipage/urls-and-fetching.html#cors-same-origin

   This change makes the following WPT test pass:
   css/cssom/stylesheet-same-origin.sub.html

   It also affects the web tests:
   register-bypassing-scheme-partial.html
   require-sri-for-style-blocked.php

   These tests are updated to match the behavior change.

Chrome Status: https://www.chromestatus.com/feature/5642183499579392
Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/7OSy00oxVpk/siufiQVBBwAJ

Bug: 911974
Change-Id: I9add3162596963eee66f60f339cfd9911bc151cd
Reviewed-on: https://chromium-review.googlesource.com/c/1367331
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616580}

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 14, 2018

Bug 1512134 [wpt PR 14373] - WPT: service worker: fix test for CSS ba…
…se URL., a=testonly

Automatic update from web-platform-tests
WPT: service worker: fix test for CSS base URL.

The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b
Reviewed-on: https://chromium-review.googlesource.com/c/1362776
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614277}

--

wpt-commits: 3e60f8f48ebed7895b6d58a88d35ccb5a80ed3c6
wpt-pr: 14373

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 15, 2018

Bug 1512134 [wpt PR 14373] - WPT: service worker: fix test for CSS ba…
…se URL., a=testonly

Automatic update from web-platform-tests
WPT: service worker: fix test for CSS base URL.

The existing test was asserting the opposite of the standard,
which is to use the response URL. See
whatwg/fetch#146.

This CL does the following:
- Tests that a CSS stylesheet fetched via respondWith(fetch(responseUrl)
  uses responseUrl as its base URL.
- Tests that a CSS stylesheet fetched via respondWith(new Response())
  uses the response URL (which is the request URL) as its base URL.
- Changes the test to not test cross-origin stylesheets. That is more
  complex than needed for this test, and there is talk of making
  subresource requests from opaque stylesheets skip the service worker,
  which would render the test ineffective for testing base URL.
- Changes the test to use waitUntil() in the message event to try
  to ensure the service worker stays alive between the message and
  fetch events.

Bug: 911974
Change-Id: I167dfe86986ec718a50d512f862f1eb49889608b
Reviewed-on: https://chromium-review.googlesource.com/c/1362776
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614277}

--

wpt-commits: 3e60f8f48ebed7895b6d58a88d35ccb5a80ed3c6
wpt-pr: 14373

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 23, 2019

Bug 1513462 [wpt PR 14474] - service worker: XSLT: Use response URL f…
…or the base URL., a=testonly

Automatic update from web-platform-tests
service worker: XSLT: Use response URL for the base URL.

This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed
Reviewed-on: https://chromium-review.googlesource.com/c/1372109
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616248}

--

wpt-commits: bc5122da94555fdf1b82b520599d860e722d8c05
wpt-pr: 14474

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 23, 2019

Bug 1513462 [wpt PR 14474] - service worker: XSLT: Use response URL f…
…or the base URL., a=testonly

Automatic update from web-platform-tests
service worker: XSLT: Use response URL for the base URL.

This aligns with the standard. See
whatwg/fetch#146

Chrome Status: https://www.chromestatus.com/feature/4857368017895424

No intent to ship because it's a trivial change.

Bug: 914135
Change-Id: I229aec6f8473bb6b7cdc88429afa830bc6eb80ed
Reviewed-on: https://chromium-review.googlesource.com/c/1372109
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616248}

--

wpt-commits: bc5122da94555fdf1b82b520599d860e722d8c05
wpt-pr: 14474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment