Script-created redirects responses have no location URL

[davidben]

Edit: I misread the Fetch spec. See #1146 (comment) and #1146 (comment). Though instead we've identified an editorial change that is hopefully clearer and less complex to implement.

---

(CC @jakearchibald @wanderview who I talked to about this recently.)

Fetch responses have a location URL, computed from the Location header, when they're constructed from the network:
https://fetch.spec.whatwg.org/#ref-for-concept-response-location-url%E2%91%A0

And then the HTTP-redirect fetch algorithm never looks at the Location header and instead follows the location URL value. I also don't see any reference to setting the location URL in the Service Worker spec.
https://fetch.spec.whatwg.org/#concept-http-redirect-fetch

From what I can tell, the main platform-visible consequence of this that, if a Service Worker responds to a navigation FetchEvent of / with ev.respondWith(fetch("/foo/", {redirect: "manual"}) and the server responds with Location: bar.html, the redirect resolves to /foo/bar.html rather than /bar.html.

Looks like Chrome and Firefox don't implement this (resulting in /bar.html) while Safari does. The spec's behavior seems preferable, especially given atomic HTTP redirect handling, so I'm looking at fixing that. In doing so, two fun cases came up:

• Response.redirect sets the Location header, but not the location URL. That means, if I understand the spec correctly, respondWith(Response.redirect(...)) does not actually work. We could fix that by adding an extra step to the algorithm, but...
• It's also possible to construct a redirect with new Response("", {status: 302: headers: {location: "bar.html"}}). Since you can manipulate the headers after construction, there isn't a very clear place to stick in the location URL computation.

I can see two options here:

1. Add an extra step in Response.redirect, but leave new Response(...302...) broken.
2. Update HTTP-redirect fetch (or FetchEvent.respondWith) to say, if the response doesn't have a location URL, you rerun the steps to construct it from the status code and Location header and use that instead.

Interestingly, Response.redirect works in Chrome/Firefox/Safari, but new Response(...302...) only works in Chrome/Firefox. Safari seems to fail the fetch when you respondWith(new Response(...302...)). [Edit: it only fails for relative Location headers. Absolute Location headers are treated as redirects.]

(1) does mostly match one browser. I say mostly because I don't see anything in the spec that supports failing the fetch. It seems the spec would want you to actually commit the response, which is really weird. But, relative to Chrome/Firefox, it's a breaking change and I'm a little nervous about compatibility. It also seems generally confusing for new Response(...302...) to silently fail. (2) would avoid this.

I think (2) is thus preferable, even though repeating the Location resolution in two places is a little odd.

[wanderview]

I like option (2).

An alternative would be to always resolve the redirect URL at respondWith(), but prefer the response.url as the base URL if its present and only fall back to request URL if there is no response.url.

[davidben]

Oh huh. I missed that was a thing. In that case, we potentially don't even need a notion of location URL? We could drop it and just say that HTTP-redirect fetch does:

• base URL = response's URL if it exists, otherwise request's URL (URL list??)
• Resolve Location header

Shall we call this option 3?

(Chromium's lower-level net stack bits already surface out a pre-resolved redirect URL. A lot of fixing the location URL handling was preserving that up a few more layers, rather than dropping and recomputing it. But they should always match when they come from the net stack, so shrug)

[davidben]

Actually, do options 2 and 3 have any platform-visible differences? I don't see an obvious way to set either the location URL or the URL list in a script-constructed Response.

[wanderview]

I think maybe there is a difference in the case where a network created response later has its location header changed. In theory the spec today says to freeze URL Location at the time the response was created and with (3) we would be moving it to recomputing that at respondWith() time. That would be consistent with recent changes to recompute mime type, though, instead of freezing on the response.

[davidben]

Good point. Though I think that may be impossible: the only script-accessible network-created redirect responses are opaqueredirects, which I hope can't be updated like that.

Still, it's a point towards option 3 in that the model is clearer. @annevk, what do you think?

[wanderview]

Oh, thats true. You can't modify headers on an opaqueredirect. So yea, probably not web observable between the two.

[davidben]

Safari seems to fail the fetch when you respondWith(new Response(...302...)).

Oh huh. A correction: Safari fails the fetch if you respondWith(new Response(...302...)) with a relative Location header, but an absolute Location header is processed as a redirect. I wonder if they just resolve Location relative to response.url, and if that's null, so be it. That is consistent with what the spec says for network-constructed responses, except those (I think?) always have a response URL.

[annevk]

See #633, #883, and also some tests I created at web-platform-tests/wpt#10449. It'd be great to be able to revive that. I sorta dropped working on it due to lack of input.

[davidben]

#883 seems an orthogonal encoding issue, though yeah redirects are quite a mess. Maybe I'll take a stab at fixing that so the test no longer needs to be speculative. I think it's just a matter of adding a sentence to define the conversion from byte sequence to string. The conversion is a little goofy, but that goofiness does seem necessary for web compatibility.

#633 is interesting. It sounds like it's saying the response's URL list is not set by the time location URL is computed? If so, that would complicate option (3). Although, is that still the case? Looks like a response's URL list is set here:
https://fetch.spec.whatwg.org/#ref-for-concept-response-url-list%E2%91%A0%E2%93%AA

That's called here, which happens before the location URL is computed. So I think that means #633 is no longer necessary?
https://fetch.spec.whatwg.org/#ref-for-concept-http-network-or-cache-fetch%E2%91%A0

Regarding your WPT PR, if I'm reading it right, it captures the place where Chrome/Firefox currently diverge from the spec. It doesn't seem to capture the script-constructed redirects question in this issue. I independently wrote a test myself as part of fixing this in Chrome. Mine avoids the evilGlobalState variable, reuses fetch-rewrite-worker.js, and also tests the CacheResponse round-trip. I'm thinking I'll just check those tests in with my fix.

The issue is I need to pick a behavior for script-constructed redirects for the fix. There are a few options above. I like (2) or (3), with I think a slight preference for (3), but I don't feel strongly between the two. What do you think?

[annevk]

Ah yeah, #1030 changed that, but we forgot that that this resolved some issues for redirects. It doesn't address the service worker returning non-network redirects however as you note. Still exciting though, thanks for spotting that.

I rather like Safari's approach here I have to say of requiring an absolute URL (which can we call option (4)?). Did you check exactly when Safari parses? In particular I wonder if they always parse late or do it perhaps when the Response object is constructed.

const response = new Response(null, { status: 302 });
response.headers.set("Location", "https://example.com/");

vs

const response = new Response(null, { status: 302, headers: { Location: "https://example.com/" } });

If they parse late (which I think is what we want and would also align with a recent change to how we determine the MIME type of a response) this would require no changes to the specification (response's URL would be null and passing null into the URL parser for the base URL does the right thing).

(Though I do want to make an editorial change makes it clear that all responses have the location URL field. Optional fields are rather messy.)

[davidben]

This seems to work in Safari:

const response = new Response(null, { status: 302 });
response.headers.set("Location", "https://example.com/");

So, yeah, I agree with you that Safari's behavior seems to be just a later resolution.

If they parse late [...] this would require no changes to the specification (response's URL would be null and passing null into the URL parser for the base URL does the right thing).

Would it? We'd at least need to move the location resolution logic from the end of HTTP fetch to the top of HTTP-redirect fetch, right? (At which point we don't need to store a location URL on the response at all.) Or do you just mean the actual steps wouldn't have the change?

Anyway, that seems reasonable to me. There is a compat risk for Chrome/Firefox in that script-constructed relative redirects would now break, but hopefully that risk is low. And if we're worried, we can start by implementing option (3), to be almost spec-compliant, measure the delta to option (4), and then flip that switch.

(I'm not very worried about the delta between Chrome/Firefox's current behavior to option (3). That only matters for really pathological cases if you rewrote one network fetch to another network fetch and somehow expected the relative redirect to still work.)

[davidben]

I guess simply removing the location URL is slightly tricky because we need a replacement for "If response does not have a location URL or the location URL is not a URL whose scheme is an HTTP(S) scheme" in step 9.6.8 here.
https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch

In particular, we need to decide whether a response with a redirect status code but whose location URL couldn't be extracted is an error or treated as a non-redirect response. Looking at the code, I believe Chrome's behavior, outside of service workers, is:

• If it's not a redirect status code, it's not a redirect.
• If there is no Location header or there are multiple ones, it's not a redirect.
• Otherwise, it is a redirect to whatever string was in that Location header (after goofy escaping per Handling of invalid Location header characters doesn't match browsers #883). If that URL fails to resolve or we're otherwise not allowed to navigate to it, it's an error.

(This is mostly an artifact of our net stack's IsRedirect function returning either true with an unresolved location string or false.)

[annevk]

Oh, I think I misunderstood part from OP, but I also think you might have misunderstood Fetch. In particular https://fetch.spec.whatwg.org/#concept-http-fetch does call into service workers so step 6 there deals with both network and service worker responses alike. I don't think Fetch has a way to return a response where location URL would not be set if the response has a redirect status. Does that makes sense?

How exactly Location headers are turned into a location URL is something that is probably best tackled separately. It's unfortunate if we have to treat separate headers differently from values containing commas, especially in the face of naïvely combining middleware, but there's probably no way around that at this point.

[davidben]

Ohhhh, yes I did misunderstand it! I think I assumed step 3 was an early return and forgot about it. I agree with you now there's no spec change needed to get to Safari's behavior. What I thought was a spec bug works just fine.

WDYT about an editorial change to switch location URL from state to a computed property anyway? It's a little weird that (unless I'm still confused), when a SW is juggling response objects, they sometimes have the internal location URL attached, and sometimes don't (depending on if it was script- or network-constructed). It doesn't matter because when the SW forwards the response object up to HTTP fetch, we'll compute or recompute the location URL anyway. But the spec models it as state, so it's not obvious it doesn't actually need to be, say, persisted in CacheStorage.

[annevk]

That sounds reasonable, we can make it similar to https://fetch.spec.whatwg.org/#concept-body-mime-type (except that we can define the algorithm directly as there's only one "class" involved). Though if we did that we might want to refactor HTTP-redirect fetch (and HTML's navigate) as well to invoke it only once, even though the end result should be indistinguishable that seems a bit cleaner.

[wanderview]

Note, we had to revert this change due to a web compat issue. It broke a large site that was using new Response() with a relative Location header. They expected it to be resolved to an absolute location at respondWith() time using FetchEvent.request.url as the base.

Is it possible to support this expectation? If not we will need to do a more careful deprecation and its unclear right now if we will be able to achieve.

[annevk]

It should be possible, but I'm not sure it's great? And as I understand it that site would already be broken in Safari. Let's track it in a new issue if it's needed as closed issues tend to get forgotten.

(I really wish we would have forbidden the creation of 3xx synthetic responses except through Response.redirect().)

[wanderview]

Yes, but safari can add support and we may not be able to remove support.

[wanderview]

Also, the partner said they expected it to work based on:

"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)."
https://tools.ietf.org/html/rfc7231#section-7.1.2

Given the FetchEvent.request.url sure looks like a "request URI" I don't think its a completely unreasonable expectation.

[annevk]

It's not completely, but it's also subtle as it would make synthetic responses behave differently from server responses. (If you replay the former to different request URLs the location URL would change and it would not with the latter.)

[wanderview]

If we are able to follow-through on this deprecation in chrome, I think we might also want to make new Response() throw if it gets passed a redirect status code and a relative Location header. This would make it an immediate error instead of depending on a later operation to fail in a more subtle way.

[annevk]

I don't think we should make new Response() parse header values. That also wouldn't help if the header got added later through append(). If we can make new Response() throw for a 3xx status code that would be ideal, but also seems unlikely?

[davidben]

I agree with @annevk that throwing based on headers in new Response() doesn't seem reasonable. I wouldn't object to doing it via status code, but then we risk other compatibility issues if folks were constructing absolute redirects. The next earliest time to report an error would be in resolving the redirect, which I believe is what already happens?

As for RFC7231, that talks about normal HTTP. Synthetic response from service workers are weird enough that Fetch would need to be consulted for what the "effective request URI" is, and right now it's defined to not have one. We could say to use the response URL if it exists, and otherwise the request URL, but that seems weirdly special-case-y.