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

Clarify response's url list state when parsing a 3xx response's Location header value #631

Closed
harrishancock opened this issue Nov 15, 2017 · 7 comments · Fixed by #1149
Closed

Comments

@harrishancock
Copy link
Contributor

I'm trying to figure out the algorithm Fetch specifies for following redirects, but keep stumbling over whether or not the 3xx response's Location header is parsed relative to a base url. The Fetch spec implies it is parsed with the response's url as its base:

Fetch section 4.3, step 5:

  1. If actualResponse’s status is a redirect status, then run these substeps:
    1. [...]
    2. Let location be the result of extracting header list values given Location and actualResponse’s header list.
    3. If location is a value, then set location to the result of parsing location with actualResponse’s url.

where "actualResponse's url" means the last response url in the response's url list. However, at the point in the algorithm I'm describing, I don't see how the response's url list can be anything other than null, which seems to imply that we're supposed to parse the Location header as an absolute url. That seems to contradict RFC 7231's definition of the Location header value:

RFC7231, section 7.1.2:

The field value consists of a single URI-reference. When it has the
form of a relative reference ([RFC3986], Section 4.2), the final
value is computed by resolving it against the effective request URI
([RFC3986], Section 5).

The situation I'm considering involves fetches originating from a service worker script, requesting resources from the same origin as the service worker's registration. The Location header parsing described above happens in the HTTP fetch algorithm Fetch section 4.3 which is ultimately called from step 5 of the main fetch algorithm (Fetch section 4.1). However, the response's url list is only ever set in step 9 of the main fetch algorithm ("If internalResponse’s url list is empty, then set it to a copy of request’s url list."), long after the response's Location header is supposed to have been parsed. Fetch section 2.2.6 says about the url list, "Unless stated otherwise, it is the empty list."

Am I interpreting something incorrectly? Or perhaps there's another specification which covers when a response's url list is set? I'd appreciate any clarification. Thanks!

@annevk
Copy link
Member

annevk commented Nov 15, 2017

Thanks, you found a bug. 😊

If we used the request's current url instead I think we'd be okay, except when the service worker returns an "opaqueredirect", but in that case I think we should be returning early and skip the redirect processing as it has already happened.

So I think we need to make two changes:

  1. Use request's current url as base URL for redirects.
  2. Add step 3.2.5: 'If response's type is "opaqueredirect", then return response.'

Would you like to be acknowledged as Harris Hancock?

@wanderview @yutakahirano @jakearchibald could you please review the above?

@wanderview
Copy link
Member

Does using the Request.url handle the multiple redirect case? I think perhaps a second redirect should be relative to the first redirect URL?

Maybe we should continue using the Response.url, but add a step where we set Response.url to the Request.url for the first redirect.

@annevk
Copy link
Member

annevk commented Nov 15, 2017

It does, request's current url is updated upon a redirect. See step 13 of https://fetch.spec.whatwg.org/#http-redirect-fetch.

@harrishancock
Copy link
Contributor Author

Would you like to be acknowledged as Harris Hancock?

Hey, that'd be great! Thank you for investigating and confirming this, @annevk.

@wanderview
Copy link
Member

It does, request's current url is updated upon a redirect.

r+

annevk added a commit that referenced this issue Nov 20, 2017
As the response's url list is not set at this point using that doesn not work. We could potentially set it earlier, but that brings along other complications that this setup avoid.

This also returns opaque redirect responses earlier, to avoid having them processed twice, which was simply wrong (and would become extra wrong here).

Fixes #631.
@annevk
Copy link
Member

annevk commented Nov 20, 2017

Posted a PR, review appreciated.

annevk added a commit that referenced this issue Apr 23, 2018
As the response's url list is not set at this point using that doesn not work. We could potentially set it earlier, but that brings along other complications that this setup avoid.

This also returns opaque redirect responses earlier, to avoid having them processed twice, which was simply wrong (and would become extra wrong here).

Fixes #631.
annevk added a commit that referenced this issue Aug 23, 2018
As the response's url list is not set at this point using that doesn not work. We could potentially set it earlier, but that brings along other complications that this setup avoid.

This also returns opaque redirect responses earlier, to avoid having them processed twice, which was simply wrong (and would become extra wrong here).

Fixes #631.
@annevk
Copy link
Member

annevk commented Jan 28, 2021

See #1146 for some updates to this issue. We are getting closer to being able to finally close this.

annevk added a commit that referenced this issue Jan 29, 2021
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.)
annevk added a commit that referenced this issue Feb 1, 2021
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.)
annevk added a commit that referenced this issue Feb 2, 2021
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: whatwg/html#6340.

Tests: https://chromium-review.googlesource.com/c/chromium/src/+/2665871.

Closes #631, closes #633, closes #958, closes #1146, and closes web-platform-tests/wpt#10449. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants