Allow range header without preflight #1312

jakearchibald · 2021-09-28T09:20:39Z

At least two implementers are interested (and none opposed):
- Chromium
- Mozilla
Tests are written and can be reviewed and commented upon at:
- Add tests for safelisting simple range headers from preflight web-platform-tests/wpt#31058
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1255711
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1733981
- Safari: https://bugs.webkit.org/show_bug.cgi?id=231174
Security review

fetch.bs

annevk · 2021-09-28T10:02:00Z

Can we consolidate with https://wicg.github.io/background-fetch/#extract-content-range-values-algorithm perhaps? (I realize there are delimiter differences, but perhaps they can be abstracted somehow. See also WICG/background-fetch#157.)

The other thing that might help is to use isomorphic decode, so you can operate on a string. That removes the need for state and allows for using Infra's "collect a sequence" algorithms.

fetch.bs

jakearchibald · 2021-09-28T10:20:21Z

@annevk I'm not sure how useful it is to consolidate with Content-Range, since the format is pretty different.

^bytes \d+-\d+/(\*|\d+)$ - simple Content-Range
^bytes=\d+-\d*$ - simple Range

I think it'd be confusing to try and combine the two.

I'll look at defining this using "collect a sequence", or would you rather I used the ABNF style from background-fetch?

annevk · 2021-09-28T10:31:50Z

Fair about combining. They are a bit more different than I thought. Implementers (at least Matt Menke) seem to prefer algorithms over ABNF and that's also the style I've used for the other header parsers thus far so I guess we should stick with that. Also, ABNF often leads to implementations that approximate the parser requirements in my experience.

jakearchibald · 2021-09-28T10:54:21Z

@annevk cool, I've rewritten it using "collecting a sequence of code points", it's much easier to read now.

annevk

Thanks, this looks very readable indeed.

fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>

jakearchibald · 2021-09-28T12:05:22Z

Ugh, I just realised it's bytes= rather than bytes . It's Content-Range that uses bytes . I've fixed up the range-setting algorithm too, but I'm happy to make that a different PR.

jakearchibald · 2021-09-28T12:35:51Z

This PR aims to allow, without requiring a preflight, CORS requests with a Range header similar to those the browser would send when downloading media or a resumable download.

When requesting media, browsers will typically request ranges like bytes=123456-789012, to target things like metadata in the resource, and bytes=0- for the initial media download.

When resuming the download of files, browsers will typically just use the initial range, like bytes=123-.

However, there are a few novel requests that this PR enables:

Ranges outside the length of the resource. This includes cases where the server cannot determine the resources content length (eg it's still being created).
Tiny ranges, eg bytes=123-123. I don't think browsers do this.
Range along with a POST request. Downloads can be initiated via a POST request, but browsers will not send a Range header in these cases.
Range and Origin headers on a GET request. We already include Origin with POST requests, but not GET unless it's a CORS request.
Combining Range with unusual values in other safe-listed headers, such as accept: whatever.

Only "ranges outside the length of the resource" makes me raise an eyebrow, so I'd like a second opinion on that.

annevk

Thanks for the analysis! Modulo some final nits this looks good to me now. I wonder if @mikewest, @yutakahirano, and @youennf can help out with additional security review.

(This also ends up fixing #1265 now. It seems okay to include the fix for that here, as long as we indicate it in the final commit message.)

fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>

jakearchibald · 2021-09-29T07:51:30Z

(This also ends up fixing #1265 now. It seems okay to include the fix for that here, as long as we indicate it in the final commit message.)

Ah, sorry I missed that original issue.

yutakahirano

LGTM

jakearchibald · 2021-09-30T09:27:50Z

@rayankans over to you for the tests

Fetch PR: whatwg/fetch#1312

rayankans · 2021-10-01T13:33:52Z

I created a PR for the tests here: web-platform-tests/wpt#31058

annevk · 2021-10-04T11:48:48Z

I'm comfortable with merging this once we have implementation bugs.

jakearchibald · 2021-10-04T16:12:27Z

Implementation bugs added.

Commit message:

Allow particular Range header values without a preflight.

The allowed format aligns with the values the browser uses when requesting media and resuming downloads.
Spec discussion: #1310
Spec PR: #1312
Tests PR: web-platform-tests/wpt#31058

For whatwg/fetch#1312. Co-authored-by: Jake Archibald <[email protected]>

annevk · 2021-10-05T16:04:39Z

Thanks all!

…eaders from preflight, a=testonly Automatic update from web-platform-tests Fetch: tests for safelisting simple range headers from preflight For whatwg/fetch#1312. Co-authored-by: Jake Archibald <[email protected]> -- wpt-commits: 902e9dceb10d98a646ad77d46df62e0365626fff wpt-pr: 31058

The Fetch standard now safelists 'simple' range headers from preflight checks. (whatwg/fetch#1312) This CL also successfully runs against the new WPT suite (web-platform-tests/wpt#31058) I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/0f1zQ4hjoyQ Bug: 1255711 Change-Id: I06ee27fec586950b7d45e3cba416df1b5090fa4d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3190330 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Rayan Kanso <[email protected]> Cr-Commit-Position: refs/heads/main@{#930108}

For whatwg/fetch#1312. Co-authored-by: Jake Archibald <[email protected]>

The Fetch standard now safelists 'simple' range headers from preflight checks. (whatwg/fetch#1312) This CL also successfully runs against the new WPT suite (web-platform-tests/wpt#31058) I2S: https://groups.google.com/a/chromium.org/g/blink-dev/c/0f1zQ4hjoyQ Bug: 1255711 Change-Id: I06ee27fec586950b7d45e3cba416df1b5090fa4d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3190330 Reviewed-by: Reilly Grant <[email protected]> Commit-Queue: Rayan Kanso <[email protected]> Cr-Commit-Position: refs/heads/main@{#930108} NOKEYCHECK=True GitOrigin-RevId: 7b4dbc8d73cd09187b1501931141871f9d90b30d

jakearchibald added 3 commits September 28, 2021 10:16

Assert range is valid

a39c705

Move early exit… earlier

e0b0a28

Allow 'simple' range header values without preflight

482eaf1

jakearchibald requested a review from annevk September 28, 2021 09:20

jakearchibald commented Sep 28, 2021

View reviewed changes

fetch.bs Show resolved Hide resolved

jakearchibald commented Sep 28, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

jakearchibald added 2 commits September 28, 2021 10:39

No longer need link text

d8eb7ee

Ensure - is included

4ef481d

yutakahirano reviewed Sep 28, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Show resolved Hide resolved

Simplify parsing using code point collection

f987f4a

Add note about missing end value

9db2a65

annevk reviewed Sep 28, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

jakearchibald and others added 5 commits September 28, 2021 12:45

Update fetch.bs

dc0ee1c

Co-authored-by: Anne van Kesteren <[email protected]>

It's bytes=

c54b56f

Check for end of value

59a7380

Wrapping and tweaks

73efad3

Parsing as number change

057361d

annevk reviewed Sep 29, 2021

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

jakearchibald and others added 3 commits September 29, 2021 08:50

Update fetch.bs

fe9e16d

Co-authored-by: Anne van Kesteren <[email protected]>

Update fetch.bs

cf6719a

Co-authored-by: Anne van Kesteren <[email protected]>

Update fetch.bs

ed0a9ca

Co-authored-by: Anne van Kesteren <[email protected]>

yutakahirano approved these changes Sep 30, 2021

View reviewed changes

rayankans added a commit to rayankans/wpt that referenced this pull request Oct 1, 2021

Add tests for safelisting simple range headers from preflight

d939309

Fetch PR: whatwg/fetch#1312

rayankans mentioned this pull request Oct 1, 2021

Add tests for safelisting simple range headers from preflight web-platform-tests/wpt#31058

Merged

jakearchibald mentioned this pull request Oct 5, 2021

Range requests and preflights w3c/IFT#34

Closed

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Oct 5, 2021

Fetch: tests for safelisting simple range headers from preflight

902e9dc

For whatwg/fetch#1312. Co-authored-by: Jake Archibald <[email protected]>

annevk merged commit 6f37b51 into main Oct 5, 2021

annevk deleted the allow-range-header-without-preflight branch October 5, 2021 16:02

Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021

Fetch: tests for safelisting simple range headers from preflight

f0abbbf

For whatwg/fetch#1312. Co-authored-by: Jake Archibald <[email protected]>

annevk mentioned this pull request Apr 4, 2022

Range header 'helper' produces incorrect header value #1265

Closed

bmaranville mentioned this pull request May 9, 2022

Enable ROS3 Driver usnistgov/h5wasm#12

Open

rayankans mentioned this pull request Oct 14, 2022

blob: URLs and range requests #1070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow range header without preflight #1312

Allow range header without preflight #1312

jakearchibald commented Sep 28, 2021 •

edited by pr-preview bot

Loading

annevk commented Sep 28, 2021

jakearchibald commented Sep 28, 2021 •

edited

Loading

annevk commented Sep 28, 2021

jakearchibald commented Sep 28, 2021

annevk left a comment

jakearchibald commented Sep 28, 2021

jakearchibald commented Sep 28, 2021 •

edited

Loading

annevk left a comment

jakearchibald commented Sep 29, 2021

yutakahirano left a comment

jakearchibald commented Sep 30, 2021

rayankans commented Oct 1, 2021

annevk commented Oct 4, 2021

jakearchibald commented Oct 4, 2021 •

edited

Loading

annevk commented Oct 5, 2021

Allow range header without preflight #1312

Allow range header without preflight #1312

Conversation

jakearchibald commented Sep 28, 2021 • edited by pr-preview bot Loading

annevk commented Sep 28, 2021

jakearchibald commented Sep 28, 2021 • edited Loading

annevk commented Sep 28, 2021

jakearchibald commented Sep 28, 2021

annevk left a comment

Choose a reason for hiding this comment

jakearchibald commented Sep 28, 2021

jakearchibald commented Sep 28, 2021 • edited Loading

annevk left a comment

Choose a reason for hiding this comment

jakearchibald commented Sep 29, 2021

yutakahirano left a comment

Choose a reason for hiding this comment

jakearchibald commented Sep 30, 2021

rayankans commented Oct 1, 2021

annevk commented Oct 4, 2021

jakearchibald commented Oct 4, 2021 • edited Loading

annevk commented Oct 5, 2021

jakearchibald commented Sep 28, 2021 •

edited by pr-preview bot

Loading

jakearchibald commented Sep 28, 2021 •

edited

Loading

jakearchibald commented Sep 28, 2021 •

edited

Loading

jakearchibald commented Oct 4, 2021 •

edited

Loading