Use request's current url as base URL in redirects

[annevk]

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.

---

Preview | Diff

[wanderview]

LGTM. Is this covered by existing WPT tests? I think it probably is.

[annevk]

I'm not sure about the various scenarios involving service workers.

[harrishancock]

FWIW, looks like a good fix to me.

[jakearchibald]

LGTM

[annevk]

@jakearchibald do you happen to know if we have opaqueredirect tets?

[wanderview]

FWIW, we check opaqueredirect in a number of WPT tests:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/redirected-response.https.html
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/navigation-redirect-to-http.https.html

Not sure if they cover all cases for this PR, though.

[wanderview]

There is also a fetch API test here:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/fetch-event-redirect.https.html

[annevk]

To test this I think what we need is this:

1. Attempt to navigate to /.
2. The server returns a redirect with Location: x.
3. That opaque redirect is stored in the Cache API.
4. You then attempt to navigate to /y/.
5. The service worker returns the opaque redirect.
6. You end up at /x.

Does that make sense? If someone could do this for me that'd be great, if not I'll have to sort out how to set up HTTPS testing and how to write service worker tests.

[annevk]

So I think with this change we end up doing what I think is the correct thing for opaque redirects, but it doesn't quite work for synthetic redirects not created through Response.redirect() (those are protected because the URL is already parsed and headers are immutable):

const res = new Response(null, { status: 302 });
res.headers.append("Location", "something-relative");

In order to make this work we'd have to parse the Location header if status is a redirect status when it's first stored/used somewhere. Otherwise those responses would always end up being relative to the current request.

But maybe we should live with that inconsistency? Not entirely sure how to fix it.

[annevk]

So there's basically four types of redirect responses:

1. A Response.redirect() response.
2. A new Response() response (with status and Location set appropriately).
3. A fresh network response.
4. A reused network response.

1 is different from 2 because 1 always has an absolute URL and therefore always goes to the same URL.

This PR as it stands ensures 3 and 4 always go to the same URL. This does not match Chrome and Firefox, but does match Safari. Given https://fetch.spec.whatwg.org/#atomic-http-redirect-handling it seems good to me that the client side cannot fiddle with the intent of the server, even if it was weakly stated (with a relative URL) and therefore I'd argue we align with Safari.

One approach we could take here is that 2 becomes a network error or "not a redirect" (just a response that happens to look like one). This would be easy to accomplish using the https://fetch.spec.whatwg.org/#concept-response-location-url field and setting it for 1.

I quite like that idea, but I'd like to solicit feedback on that from you all first. (An alternative is that 2 remains an oddball redirect response whose location varies on who requests it, but that seems a little less clean.)

cc @youennf @yutakahirano @mattto @hober

[wanderview]

One approach we could take here is that 2 becomes a network error or "not a redirect" (just a response that happens to look like one). This would be easy to accomplish using the https://fetch.spec.whatwg.org/#concept-response-location-url field and setting it for 1.

Could we allow (2), but make it throw if the Location URL is relative?

[annevk]

@wanderview the response remains mutable, so that won't work. We could maybe do something like that when first handling such a response, but it'll be rather special.

[annevk]

Arguably even nicer would be throwing a RangeError when constructing a Response with a 3xx status. Is that unrealistic? If it is, how realistic is returning a network error for some of them?

[annevk]

@wanderview @jakearchibald @yutakahirano @ricea @youennf thoughts on the above?

[wanderview]

Arguably even nicer would be throwing a RangeError when constructing a Response with a 3xx status.

Haven't we promoted Response.redirect() as a convenience, but not required to create a redirect? We would probably need data to know if people are manually constructing Response objects with 3xx status codes.

[annevk]

Yeah, that is correct.

[jakearchibald]

I don't think we should prevent users creating redirects with new Response().

In the new Response() case, the developer is setting the header themselves, so it seems fine to revolve that URL later.

[yutakahirano]

If there were no backward-compatibility issues, would we want to disallow a user-created redirect response with a relative location at 3-3 in https://fetch.spec.whatwg.org/#http-fetch?

[annevk]

@yutakahirano I think that would be the most conservative solution that solves the stated problem. I think the ideal solution is not allowing the creation of such responses in the first place, but that's far less likely to be compatible.

[annevk]

@jakearchibald I guess, but it does create a rather weird special case that we'll likely have to take into consideration forever and ever when it comes to redirects. I'm worried that it'll trip us up somehow.

[annevk]

Anyone any further thoughts here? It'd be nice to resolve this.