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

Redesign "extract header values" and "extract header list values" #814

Open
annevk opened this issue Oct 12, 2018 · 4 comments
Open

Redesign "extract header values" and "extract header list values" #814

annevk opened this issue Oct 12, 2018 · 4 comments

Comments

@annevk
Copy link
Member

annevk commented Oct 12, 2018

These APIs had the wrong design and are also incorrectly used at times (#559). "Extract a MIME type" also needs a new design, likely on top of this as a new Content-Type parser: #529.

In particular, other than for Set-Cookie the ideal way to parse a header is to combine first, and then parse, which isn't really what the current operations are doing. How to combine headers is nearly settled: #813. Once that's done my tentative plan is to introduce a new "get" for header lists to solve #752 which would return null or the combined value for a given header name from a header list.

Another problem with the current setup is that the actual parsing is handwavy by referencing ABNF defined "somewhere", which in practice leads to it being poorly tested. We can continue to allow ABNF for headers where that is actually how they are implemented, but we need to be more strict about referencing the ABNF and writing the tests and not allowing as much indirection as is currently allowed for.

Unfortunately this will impact downstream, but I think it's the right thing to do long term and will lead to better considered header parsers (and encourage more reuse as well). I'll also start filing downstream issues now linking this issue to ensure we actually design something that works for everyone.

So I think what we want is basically "get" and then per header name we'll need to define its parser (which could just be an ANBF reference plus tests) or parsers (Content-Type differs between request and response, yay). Thoughts on the details here appreciated as well as on the overall plan of course.

@annevk
Copy link
Member Author

annevk commented Oct 18, 2018

In #818 @mikewest suggested that perhaps it'd be useful if there was a way to always get the "string" value from a header. So basically "get" on a header list and then return the result of isomorphic decoding that (if non-null). We could add "get as string" for this provided there's more than one consumer, which seems likely if we'll continue to define most parsers on top of strings.

@annevk
Copy link
Member Author

annevk commented Oct 19, 2018

Things to define anew within Fetch (and test) to get rid of "extract header values":

For "extract header list values":

annevk added a commit that referenced this issue Nov 1, 2018
And add some of the infrastructure needed to define parsing better for all headers going forward (needed for #814). Fixes #752.

This also fixes an issue with CORB as it simply assumed an X-Content-Type-Options was present.

Tests: web-platform-tests/wpt#13559.
@annevk
Copy link
Member Author

annevk commented Nov 1, 2018

There's also headers that maybe should be defined in Fetch (or HTTP if it wants to tackle error handling):

(I might end up tracking these separately though as these would be additions rather than refactoring the status quo.)

annevk added a commit to web-platform-tests/wpt that referenced this issue Nov 1, 2018
annevk added a commit that referenced this issue Nov 5, 2018
annevk added a commit that referenced this issue Nov 9, 2018
CORS without preflight should not allow all kinds of Content-Type headers to go across the network.

Tests: web-platform-tests/wpt#13921.

Helps with #814.
annevk added a commit that referenced this issue Nov 9, 2018
Also known as "extract a MIME type" down right.

Tests: web-platform-tests/wpt#10525.

Helps with #814.

Fixes #529. Closes whatwg/mimesniff#30.
annevk added a commit that referenced this issue Nov 12, 2018
Also known as "extract a MIME type" down right.

Tests: web-platform-tests/wpt#10525.

Helps with #814.

Fixes #529. Closes whatwg/mimesniff#30.
annevk added a commit to web-platform-tests/wpt that referenced this issue Nov 13, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 19, 2018
…sing, a=testonly

Automatic update from web-platform-testsFetch: Access-Control-Expose-Headers parsing

See whatwg/fetch#814 for context.
--

wpt-commits: 645c0e8a5c028a613e7ad1732834100dbe946fc7
wpt-pr: 13849
@annevk
Copy link
Member Author

annevk commented Nov 23, 2018

In HTML:

annevk added a commit that referenced this issue Nov 27, 2018
Also known as "extract a MIME type" done right.

Tests: web-platform-tests/wpt#10525.

Helps with #814.

Fixes #529. Closes whatwg/mimesniff#30.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…sing, a=testonly

Automatic update from web-platform-testsFetch: Access-Control-Expose-Headers parsing

See whatwg/fetch#814 for context.
--

wpt-commits: 645c0e8a5c028a613e7ad1732834100dbe946fc7
wpt-pr: 13849

UltraBlame original commit: b12ab8efbefe006789483f40036d4b1fe8eab05f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…sing, a=testonly

Automatic update from web-platform-testsFetch: Access-Control-Expose-Headers parsing

See whatwg/fetch#814 for context.
--

wpt-commits: 645c0e8a5c028a613e7ad1732834100dbe946fc7
wpt-pr: 13849

UltraBlame original commit: b12ab8efbefe006789483f40036d4b1fe8eab05f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…sing, a=testonly

Automatic update from web-platform-testsFetch: Access-Control-Expose-Headers parsing

See whatwg/fetch#814 for context.
--

wpt-commits: 645c0e8a5c028a613e7ad1732834100dbe946fc7
wpt-pr: 13849

UltraBlame original commit: b12ab8efbefe006789483f40036d4b1fe8eab05f
pull bot pushed a commit to Alan-love/chromium that referenced this issue Mar 16, 2021
- When failing to parse "access-control-max-age", use the default
  value (5 seconds) in stead of 0.
- Negative value is not a parse error.
- Make the parser robust against integer overflow.

Note the parsing logic is still underspecified. See
whatwg/fetch#814.

Bug: 651743
Change-Id: I4c2dcb3812cf757235c57298dfe17a7e4fe2e489
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2755991
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863191}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
- When failing to parse "access-control-max-age", use the default
  value (5 seconds) in stead of 0.
- Negative value is not a parse error.
- Make the parser robust against integer overflow.

Note the parsing logic is still underspecified. See
whatwg/fetch#814.

Bug: 651743
Change-Id: I4c2dcb3812cf757235c57298dfe17a7e4fe2e489
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2755991
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#863191}
GitOrigin-RevId: e9f67b8e8951bfbf64a83492151d6c42f9be51af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant