Allow range header to be set by APIs

[jakearchibald]

This is part of #144.

The aim is to allow APIs to use the range header for no-cors requests, and allow them to pass through a service worker, but disallow modification of these requests, and disallow developers creating their own no-cors ranged requests.

---

Preview | Diff

[annevk]

Couple nits. I'll have more time to study the impact next week. The idea is that media APIs will set this flag?

[annevk]

Was this intentional?

[jakearchibald]

Yeah, "immutable" is also used for fetch() responses. I could add the comment back and include the other places "immutable" is used.

[annevk]

Newline after <li> as it contains multiple children.

[annevk]

Newline due to multiple children again.

[annevk]

No dot before e.g.

[jakearchibald]

The idea is that media APIs will set this flag?

And downloads, yeah.

I'm not quite sure how to spec that bit yet, I guess there'll be a section like "this API may issue range requests. To fetch a range, run the following steps…"

Those steps will detail how to handle ranges that don't match what was requested, and ensuring all parts come from the same source if any opaque response is involved. Also how to handle an HTTP 200 response.

For downloads I want to ensure that browsers restart the download if the length, etag or last-modified has changed. Firefox is the only browser doing a decent job here, the others happy merge the chunks together even though the content has changed.

[foolip]

I've looked over the changes and it seems to make sense to me. @annevk, any parts in particular you were looking for feedback on?

[annevk]

@foolip mostly if these changes are compatible with eventual changes for the media API in HTML.

[annevk]

@jakearchibald I guess we can't really test this change without defining some of the other parts? I wonder if we just want to land this (assuming it's editorially okay) and test later or wait until we have defined some of the integration bits.

[jakearchibald]

@annevk I can write tests for the cases that must produce a network error. Eg returning a ranged response to a request that didn't have a range header.

Aside from that, given that the intent is to prevent script-created no-cors requests with range headers, it might not be possible to automate the tests. Seeking an audio element should produce enough range requests for testing, but the browser doesn't have to make range requests to be spec-compliant.

What's best for me to do next? Make a couple of tests for this PR, or start on the HTML changes?

[annevk]

@jakearchibald if you're okay with doing HTML first before landing this I think that'd be ideal. HTML will require tests as well so you could also start there. I don't really have advice how to handle the case where a UA would opt not do range requests. Maybe we can make that non-conforming? Not sure there's a good reason for that.

[foolip]

@foolip mostly if these changes are compatible with eventual changes for the media API in HTML.

Probably. I think the non-obvious things that need to work for media elements are:

• Requesting the range 0- must be possible.
• If the response to that is 200, that's not an error, but it is a piece of information that's needed to avoid interrupting the fetch later, as resuming is impossible.
• If more data than requested is returned, that's probably OK, as long as it starts at the right place.
• Cache behavior is strange and probably inconsistent. At the very least, with no cache-related headers, I think implementations assume it is unchanging as long as the resource size doesn't change, or perhaps even a growing resource is OK. Needs testing.

[foolip]

Media fragments #t=30,60 as actually implemented doesn't actually interact with the fetch layer at all. Rather, you just try to load the file and then seek to time offset 30. In other words, there's no existing behavior beyond byte range requests that fetch needs to explain. (Disclaimer: I haven't read the whole discussion.)

[jakearchibald]

@foolip cheers – that matches my mental model too.

[foolip]

new Audio("/path/to/media#t=30,60") makes range request, yes?

It does, but the requests should look roughly the same as when you do this:

var audio = new Audio("/path/to/media")
audio.currentTime = 30;
audio.play();
audio.ontimeupdate = function() {
  if (audio.currentTime >= 60)
    audio.pause();
}

That's actually fairly close to how this is implemented. The media fragment is handled in the media element code, not at any lower level.

If you want the server to do some of the work in slicing out 30-60 seconds, then https://www.w3.org/TR/media-frags/#URIquery-media-fragments might be what you're looking for, but that then requires a clever server to interpret "/path/to/media?t=30,60".

[annevk]

@guest271314 it's a little bit unclear how your feedback thus far relates to this PR. I'd appreciate it if you put it someplace else. This is really only meant for review of the proposed changes.

[jakearchibald]

It seems like browsers will allow a 206 partial response to a <script src>. As in, it will execute the script, which this PR would prevent.

The current behaviour seems weird to me. The level of risk is unclear, but if a server could be tricked into thinking your request is a range request (via query string params), and produces a partial response, it could result in data leaking.

With a service worker involved, it means you could take an opaque partial response (from a request generated by a media element) and use it in response to a script fetch. Again, there's a potential for data leak.

Should I try to find a way to preserve browser behaviour here, or go ahead and change it?

[mnot]

A server that generates a 206 based upon query parameters is clearly broken and intentionally so; I'd suggest trying to prevent brokenness in that case is a lost cause.

OTOH, a 206 response clearly has an application-visible semantic, as NOT being the whole response.

AFAIK there isn't any existing markup that allows a <script> to specify a range. That says to me that it should specify that a partial response shouldn't be processed.

Relevant spec seems to be here:
https://html.spec.whatwg.org/#prepare-a-script

... around step 21, which does not appear to talk about success or completeness of the result of fetching.

@annevk?

[jakearchibald]

@mnot This PR says the fetch must fail if the request does not have a Range header, and the response is 206 partial. Are you saying you disagree with that?

[jakearchibald]

Chrome's security team would prefer 206s to fail if a range wasn't requested. However, @ericlaw1979 says there are servers that do this, but the range covers the whole resource https://fiddler.ideas.aha.io/ideas/FID-I-191.

It'd be pretty easy to work around that case in the spec.

[mnot]

@jakearchibald ah, sorry, I didn't realise this was a PR :)

That's fine by me, with one caveat; it's also legal to send a 206 in response to a request with If-Range.

[jakearchibald]

@mnot oh wow, I totally forgot about If-Range. Thank you! I've never seen a browser make an If-Range request, are you aware of any cases where they do?

[ericlaw]

Aren't we still okay here? "A client MUST NOT generate an If-Range header field in a request that does not contain a Range header field. A server MUST ignore an If-Range header field received in a request that does not contain a Range header field."

[jakearchibald]

@ericlaw correct! I thought that If-Range was an alternative to Range, but as you say, it isn't, so we're fine.

It would make sense for browsers to use If-Range when resuming downloads, but none do.

[jakearchibald]

I need to take #144 (comment) into account when I pick this up again.

[wanderview]

I'm a little concerned that this may not match the gecko implementation at all. Currently we add Range headers at the http cache level. We do this because we use the http cache to known whether we are resuming a previously seen resource or if the resource has changed. If the resource has changed we don't use a range request.

@mcmanus Is my reading of range requests in gecko correct here? Are there other ways that we generate range requests? For example, do media channels do something different here compared to resuming a download?

[wanderview]

Oh, we do set it manually in other places:

https://searchfox.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1264

[jakearchibald]

Re-reading this, I'm not happy with:

const request = new Request(event.request);
request.headers.set('accept', 'foo');

Now the request has the range header from event.request, and the modified accept header.

The range header will be ditched when it gets copied within fetch(), but it feels like privileged headers should be removed if the headers are tampered with, and the guard is "request-no-cors".

[annevk]

(There appear to be some issues with the various servers involved in building, so it might not be properly published until tomorrow.)

[jakearchibald]

Whoop! Thanks for dragging me through this one. Maybe one day I'll get a PR through without 100 changes 😀

[youennf]

Nice to see this happening, this might be especially useful in WebKit context where audio/video relies a lot on range requests.
Based on https://bugs.webkit.org/show_bug.cgi?id=184447, it seems that some servers might require 'Accept-Encoding : identity' to serve range responses from range requests.
I don't know how much this is a bug that servers should fix and how much this can impact deployment of this feature.

[annevk]

@youennf could you file a new issue on that Accept-Encoding thing? Seems like something we should research further.

[youennf]

Filed #747