Remove location URL as field on responses #1149

annevk · 2021-01-29T09:19:36Z

It does not need to be stored on a response and therefore resulted in confusion.

Also clarify that synthetic responses need to have an absolute URL in the Location header field value (Response.redirect() does this automatically).

Corresponding HTML PR: TODO.

Tests: TODO.

Closes #631, closes #633, closes #958, and closes #1146. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)

At least two implementers are interested (and none opposed):
- Chrome
- Firefox
Tests are written and can be reviewed and commented upon at:
- https://chromium-review.googlesource.com/c/chromium/src/+/2665871
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1170379
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1690286
- Safari: N/A

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

annevk · 2021-01-29T09:20:59Z

cc @davidben @wanderview

Complements whatwg/fetch#1149.

domenic

IMO this should either be "extract a location URL" and take a header list (like "extract a MIME type"), or it should be "response's location URL" (like "response's/request's MIME type"). I prefer the latter.

domenic · 2021-01-29T18:19:03Z

fetch.bs

+ <!-- https://github.com/whatwg/fetch/issues/814#issuecomment-431366126 -->
+
+ <li>
+  <p>If <var>location</var> is a <a for=header>value</a>, then set <var>location</var> to the result


"extracting header list values" returns a list, not a value, if I am reading this right?

Yeah, but I'm not fixing this here.

fetch.bs

davidben

Thanks for doing this!

fetch.bs

annevk · 2021-01-29T18:40:17Z

A request never has a location URL so that seems kinda redundant.

domenic · 2021-01-29T19:19:41Z

I'm just suggesting changing the phrasing from "To extract a location URL given a response response" to "The location URL of a response response is the value given by the following algorithm:". This lets the call sites be nicer, more like a "getter" instead of a "function call".

annevk · 2021-01-30T07:14:45Z

@davidben if you have a changeset up for Chromium could you link it here? I would like to use that to satisfy the test criteria. I also take it web-platform-tests/wpt#10449 can be closed in favor of that?

It does not need to be stored on a response and therefore resulted in confusion. Also clarify that synthetic responses need to have an absolute URL in the Location header field value (Response.redirect() does this automatically). Corresponding HTML PR: TODO. Tests: TODO. Closes #631, closes #633, closes #958, and closes #1146. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)

davidben · 2021-02-02T04:58:08Z

@davidben if you have a changeset up for Chromium could you link it here? I would like to use that to satisfy the test criteria. I also take it web-platform-tests/wpt#10449 can be closed in favor of that?

Here you are. Sorry about the delay. I tried to get a test working for relative script-constructed redirects. I... only sort of succeeded. It relies on network errors triggering iframe onload which seems to only be true in Chromium. (And, even then, It's Complicated.) Is there's a better trick for that sort of thing?
https://chromium-review.googlesource.com/c/chromium/src/+/2665871
https://chromium-review.googlesource.com/c/chromium/src/+/2665871/1/third_party/blink/web_tests/external/wpt/service-workers/service-worker/navigation-redirect-resolution.https.html

annevk · 2021-02-02T07:48:54Z

It seems you could do a fetch() and assert it throws a TypeError, with a service worker intercepting it and returning a synthetic "faulty" redirect response (i.e., where Location is not an absolute URL, but everything else is in order)? (iframe not firing a load event for network errors is a known interop issue. Chrome's behavior there is being standardized upon.)

davidben · 2021-02-02T14:52:58Z

Ah, yeah that'll work. Similar to how I previously misread it, I thought only navigations followed SW-constructed redirects, but that's not quite true since HTTP fetch step 3 still continues on to step 6, and step 3.2.3 only forbids opaqueredirect on non-manual requests. Synthetic ones are still fine. I'll switch it to that.

We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d

Complements whatwg/fetch#1149.

We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d

We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Commit-Queue: David Benjamin <[email protected]> Cr-Commit-Position: refs/heads/master@{#850874}

…n the response, a=testonly Automatic update from web-platform-tests Resolve Service Worker redirects based on the response We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Commit-Queue: David Benjamin <[email protected]> Cr-Commit-Position: refs/heads/master@{#850874} -- wpt-commits: 020c59f0ae3ce0a5649c8e811faca2101d947d63 wpt-pr: 27444

…n the response, a=testonly Automatic update from web-platform-tests Resolve Service Worker redirects based on the response We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu <shimazuchromium.org> Reviewed-by: Kinuko Yasuda <kinukochromium.org> Reviewed-by: Ben Kelly <wanderviewchromium.org> Commit-Queue: David Benjamin <davidbenchromium.org> Cr-Commit-Position: refs/heads/master{#850874} -- wpt-commits: 020c59f0ae3ce0a5649c8e811faca2101d947d63 wpt-pr: 27444 UltraBlame original commit: f5404d5590f52e0ccf7100a37b1429363ca23c0c

…n the response, a=testonly Automatic update from web-platform-tests Resolve Service Worker redirects based on the response We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Commit-Queue: David Benjamin <[email protected]> Cr-Commit-Position: refs/heads/master@{#850874} -- wpt-commits: 020c59f0ae3ce0a5649c8e811faca2101d947d63 wpt-pr: 27444

* Editorial: remove redundant "the" * Meta: default branch rename Also correct a broken link. Not even w3.org URLs are that cool. Helps with whatwg/meta#174. * Editorial: clean up calls to "parse a URL" It actually takes a string, so calls should be clear about that. * Review Draft Publication: January 2021 * Simplify <link>s In particular, remove their activation behavior, stop them from matching :link and :visited, and stop suggesting that they be focusable areas. This also includes a slight expansion and rearrangement of the link element's section to make it clearer what hyperlinks created by <link> are meant for, contrasting them to <a> and <area> hyperlinks. Closes whatwg#4831. Closes whatwg#2617. Helps with whatwg#5490. * Meta: remove demos/offline/* (whatwg#6307) These are no longer needed as of e4330d5. * Meta: minor references cleanup Use more HTTPS and drop obsolete HTML Differences reference. * Editorial: anticlockwise → counterclockwise We use en-US these days. Spotted in https://twitter.com/iso2022jp/status/1352601086519955456. * Use :focus-visible in the UA stylesheet See w3c/csswg-drafts#4278. * Editorial: align with WebIDL and Infra * Fix "update a style block" early return The new version matches implementation reality and CSSWG resolution. The algorithm was also inconsistent, as it looked at whether the element was in a shadow tree or in the document tree, but it was only specified to be re-run if the element becomes connected or disconnected. The CSSWG discussed this in w3c/csswg-drafts#3096 (comment) and http://wpt.live/shadow-dom/ShadowRoot-interface.html tests this. This also matches closer the definition of <link rel="stylesheet">, which does use connectedness (though it uses "browsing-context connected", which is a bit different): https://html.spec.whatwg.org/#link-type-stylesheet * Modernize and refactor simple dialogs This contains a small bug fix, in that confirm() and prompt() said "return" in some cases instead of "return false" or "return null" as appropriate. Other notable changes, all editorial, are: * Factoring out repeated "cannot show simple dialogs" steps, which will likely expand over time (see e.g. whatwg#6297). * Separating out and explaining the no-argument overload of alert(). * Passing the document through to the "printing steps", instead of just having them talk about "this Window object". * Meta: add definition markup for MessageEvent * Remove <marquee> events They are only supported by one engine (Gecko). Closes whatwg#2957. * Clarify when microtasks happen * Ignore COEP on non-secure contexts Fixes whatwg#6328. * Editorial: update URL Standard integration * Editorial: only invoke response's location URL once Complements whatwg/fetch#1149. * Track the incumbent settings and active script in Promise callbacks Closes whatwg#5213. * createImageBitmap(): stop clipping sourceRect to source's dimensions It has been found in whatwg#6306 that this was an oversight at the time of its introduction. Current behavior goes against author expectations and no implementer has opposed the change to "no-clip". Tests: web-platform-tests/wpt#27040. Closes whatwg#6306. * Remove CSP plugin-types blocking With Flash not being supported anymore, the CSP directive plugin-types has lost its main reason for being and is being removed from the Content Security Policy specification: w3c/webappsec-csp#456. This change removes references to the relevant algorithm from the Content Security Policy spec. * Meta: set more dfn types A follow-up to: * whatwg#5694 * whatwg#5916 * Editorial: occuring → occurring * Make all plugin-related APIs no-ops Part of whatwg#6003. * Disallow simple dialogs from different-origin domain iframes Closes whatwg#5407. * Revive @@iterator for PluginArray/MimeTypeArray/Plugin @@iterator is implicitly installed by defining an indexed property getter. Since there is no other way to define it exclusively, this restores some methods back to being indexed getters. This fixes an inadvertent observable behavior change in d4f07b8. * Adjust web+ scheme security considerations to account for FTP removal Also, network scheme is now reduced to HTTP(S) scheme. Helps with whatwg#5375, but form submission issue remains. See whatwg/fetch#1166 for context. * Meta: export pause Nobody but XMLHttpRequest take a dependency on this please. You have been warned. Context: whatwg/xhr#311. * Fix typo: ancestor → accessor Fixes whatwg#6374. Co-authored-by: Dominic Farolino <[email protected]> Co-authored-by: Anne van Kesteren <[email protected]> Co-authored-by: Domenic Denicola <[email protected]> Co-authored-by: Emilio Cobos Álvarez <[email protected]> Co-authored-by: Momdo Nakamura <[email protected]> Co-authored-by: Jake Archibald <[email protected]> Co-authored-by: Yutaka Hirano <[email protected]> Co-authored-by: Shu-yu Guo <[email protected]> Co-authored-by: Kaiido <[email protected]> Co-authored-by: Antonio Sartori <[email protected]> Co-authored-by: Michael[tm] Smith <[email protected]> Co-authored-by: Ikko Ashimine <[email protected]> Co-authored-by: Carlos IL <[email protected]> Co-authored-by: Kagami Sascha Rosylight <[email protected]> Co-authored-by: Simon Pieters <[email protected]>

…n the response, a=testonly Automatic update from web-platform-tests Resolve Service Worker redirects based on the response We currently resolve Service-Worker-forwarded location headers using the request. While this matches Firefox, this does not match the spec or Safari's behavior. Instead, the spec says to resolve the location header based on the response's URL. This comes up if the FetchEvent was for /, but the Service Worker responded with ev.respondWith(fetch("/foo/", {redirect: "manual"})). In that case, a Location: bar.html header would result in /bar.html by our version and /foo/bar.html by the spec's version. Align with the spec. This makes the redirect go where it would have gone under {redirect: "follow"}. This has two platform-visible behavior changes: - First, cases like the above will result in a different URL. - Second, script-constructed Response objects do not have a URL list. If the URLs are absolute, this works fine. If they are relative, those fetches will now result in a network error. Note Response.redirect() internally constructs absolute URLs, so those continue to work. This only affects ev.respondWith(new Response(... location: "bar.html"}})). Both of these changes match Safari. Note that, as of writing, the Fetch spec describes this behavior in terms of a location URL property on the response object. This would require computing the location URL earlier and preserving it across many layers, including persisting in CacheStorage. See https://chromium-review.googlesource.com/c/chromium/src/+/2648648. Instead, this CL uses the equivalent formulation in whatwg/fetch#1149. See also discussion in whatwg/fetch#1146. Bug: 1170379 Change-Id: Ibb6b12566244fd259029e67787dd7f08edeece9d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2665871 Reviewed-by: Makoto Shimazu <[email protected]> Reviewed-by: Kinuko Yasuda <[email protected]> Reviewed-by: Ben Kelly <[email protected]> Commit-Queue: David Benjamin <[email protected]> Cr-Commit-Position: refs/heads/master@{#850874} -- wpt-commits: 020c59f0ae3ce0a5649c8e811faca2101d947d63 wpt-pr: 27444

annevk added the topic: redirects label Jan 29, 2021

annevk requested a review from domenic January 29, 2021 09:19

annevk added a commit to whatwg/html that referenced this pull request Jan 29, 2021

Adopt Fetch's extract a location URL

aefcde2

Complements whatwg/fetch#1149.

annevk mentioned this pull request Jan 29, 2021

Editorial: only invoke response's location URL once whatwg/html#6340

Merged

3 tasks

domenic reviewed Jan 29, 2021

View reviewed changes

davidben reviewed Jan 29, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

annevk force-pushed the annevk/location-url branch from 154ede8 to e63e41a Compare February 1, 2021 12:23

domenic approved these changes Feb 1, 2021

View reviewed changes

annevk merged commit 89cfc1d into main Feb 2, 2021

annevk deleted the annevk/location-url branch February 2, 2021 15:31

chromium-wpt-export-bot mentioned this pull request Feb 2, 2021

Resolve Service Worker redirects based on the response web-platform-tests/wpt#27444

Merged

annevk added a commit to whatwg/html that referenced this pull request Feb 3, 2021

Editorial: only invoke response's location URL once

4d832b0

Complements whatwg/fetch#1149.

Remove location URL as field on responses #1149

Remove location URL as field on responses #1149

Uh oh!

Conversation

annevk commented Jan 29, 2021 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Jan 29, 2021

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

domenic Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

annevk Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

davidben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

annevk commented Jan 29, 2021

Uh oh!

domenic commented Jan 29, 2021

Uh oh!

annevk commented Jan 30, 2021

Uh oh!

davidben commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk commented Feb 2, 2021

Uh oh!

davidben commented Feb 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

annevk commented Jan 29, 2021 •

edited by pr-preview bot

Loading

davidben commented Feb 2, 2021 •

edited

Loading