Skip to content
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

Resolve Service Worker redirects based on the response #27444

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Feb 2, 2021

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 <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#850874}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2665871 branch 3 times, most recently from 0a937af to 4f2ab38 Compare February 4, 2021 18:47
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 <shimazu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ben Kelly <wanderview@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#850874}
@davidben
Copy link
Contributor

davidben commented Feb 5, 2021

(@annevk FYI)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants