-
Notifications
You must be signed in to change notification settings - Fork 325
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
Header value definition needs work #332
Comments
We found an errata today, which defines Did you mean that the header value should match the field-content production in the errata? [0] https://www.rfc-editor.org/errata_search.php?rfc=7230 (Errata ID: 4189) |
How is this for field-content?
The notes from the errata linked above says:
With this re-write, you can:
I guess we then get rid of field-value altogether. |
Thank you for reporting this. I suspect it's a duplicate of either #213 or #115. Clearly something is wrong here. I also didn't know errata was filed for this particular production so thanks for pointing that out too. @hiroshige-g have you looked into this since February? Should we just go back to what we had before HTTP was revised? Or at least something that means the same. This new production has just led to flurry of issues. |
Add the append method for the Headers API <!-- Please describe your changes on the following line: --> This commit adds the append method for the Headers API. @malisas and I are both contributors. There are a few TODOs related: - The script needs to parse the header value for certain header names to decide the header group it belongs - There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332)) There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12467) <!-- Reviewable:end -->
Add the append method for the Headers API <!-- Please describe your changes on the following line: --> This commit adds the append method for the Headers API. @malisas and I are both contributors. There are a few TODOs related: - The script needs to parse the header value for certain header names to decide the header group it belongs - There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332)) There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12467) <!-- Reviewable:end -->
Note that the errata you mention is "Held for Document Update", and the notes say:
@reschke, any comment? |
@mnot --no, we haven't figured out the best fix yet. |
@reschke is there an open issue I can track? This has been tripping up multiple implementers now, I feel like I should add some pointers in Fetch even if it's non-final. |
I opened httpwg/http-core#19 |
I started creating tests to sort this out since it's a rather longstanding issue. My first attempt was to pass all bytes through Firefox fails for 0x00 in the former and for 0x00, 0x0A, and 0x0D in the latter. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1330297 on that since it seems silly. Chrome consistently fails for 0x00, 0x0A, and 0x0D. Safari TP consistently fails for 0x00-0x1F and 0x7F. I guess next is testing what actually arrives at the server. |
Browsers that did that not throw on 0x09, 0x0A, 0x0D, and 0x20 do appear to strip them before sending them to the server (verified through the console in Firefox, not verified in Chrome since the console is crap for that). It seems the testing server is eating 0x0B and 0x0C. Filed w3c/wptserve#111. Chrome also doesn't appear to allow "," to roundtrip, but other browsers do manage that. Not sure what is going on there. |
I think what I recommend is that we strip any leading and trailing 0x09, 0x0A, 0x0D, and 0x20. So a single 0x0A would not make us throw. Then, if value still contains 0x00, 0x0A, or 0x0D, throw. Then pass it through to the server unchanged. So a header value would be a byte sequence that does not have any leading or trailing HTTP whitespace bytes and does not contain 0x00, 0x0A, or 0x0D. |
See whatwg/fetch#332 for context.
@jeenalee @malisas @reschke @tyoshino @hiroshige-g feedback on the above header value proposal is most welcome. web-platform-tests/wpt#4525 has browser tests. PR for the Fetch Standard to follow. |
HTTP is taking too long to sort this out. Tests: web-platform-tests/wpt#4525. Fixes #332.
Also paging @youennf who implemented the relevant bits for WebKit (and made their code stricter than other browsers). |
So the plan is to still refer to the RFC, and until the RFC is updated, add the guidelines you described above. Seems fine to me. |
See whatwg/fetch#332 for context. Once w3c/wptserve#111 is fixed these tests can be further updated.
HTTP is taking too long to sort this out. Tests: web-platform-tests/wpt#4525. Fixes #332.
Looking at it a bit more closely, I think browsers currently forbid non ASCII UTF-8 content. |
See XMLHttpRequest/setrequestheader-bogus-value.htm as an example. |
@youennf I'm not sure I follow. IDL ByteString means that input > U+00FF throws. But input < U+100 becomes bytes. So you could get a UTF-8 byte sequence, if you play by the rules. E.g., if your input to setRequestHeader() is U+00E2 U+0082 U+00AC nobody would throw I believe and the byte sequence would be the UTF-8 sequence for €. |
Ah right, WebKit related IDLs need to be fixed. |
I'm sorry but can someone just say Yes or No whether empty header values are legal HTTP? |
Are they allowed by HTTP? Yes. Are they allowed / defined to have meaning for an individual header field? It depends on the field's definition. That said, it's not good practice to do it. |
e.g. |
… jeenalee:jeena-headersAPI); r=jdm <!-- Please describe your changes on the following line: --> This commit adds the append method for the Headers API. malisas and I are both contributors. There are a few TODOs related: - The script needs to parse the header value for certain header names to decide the header group it belongs - There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332)) There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 03fa7f0ba533acc44100639ad85625810618df3a UltraBlame original commit: 9a892b1c7c7b04a342e32ba1e0a035a5cfcb328a
… jeenalee:jeena-headersAPI); r=jdm <!-- Please describe your changes on the following line: --> This commit adds the append method for the Headers API. malisas and I are both contributors. There are a few TODOs related: - The script needs to parse the header value for certain header names to decide the header group it belongs - There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332)) There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 03fa7f0ba533acc44100639ad85625810618df3a UltraBlame original commit: 9a892b1c7c7b04a342e32ba1e0a035a5cfcb328a
… jeenalee:jeena-headersAPI); r=jdm <!-- Please describe your changes on the following line: --> This commit adds the append method for the Headers API. malisas and I are both contributors. There are a few TODOs related: - The script needs to parse the header value for certain header names to decide the header group it belongs - There are possible spec bugs that could change what a valid header value looks like (related: [issue page](whatwg/fetch#332)) There are WPT tests already written for the Headers API, but they will fail as the Headers API is not fully implemented. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [X] These changes do not require tests because tests for the Headers API already exists, but this commit does not implement the interface fully. The tests will fail. <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 03fa7f0ba533acc44100639ad85625810618df3a UltraBlame original commit: 9a892b1c7c7b04a342e32ba1e0a035a5cfcb328a
The spec defines header value as "a byte sequence that matches the field-content token production" [0, 1].
Field-content is defined as
field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
[2]. This seems to say that field-content is at most a field-vchar followed by some whitespace and another field-vchar. It seems wrong that it would be at most 2 vchars.Should the Fetch spec define header value as "a byte sequence that matches the field-value token production"? Field-value token production is defined as
field-value = *( field-content / obs-fold )
[2].Thank you.
[0] https://fetch.spec.whatwg.org/#concept-header-name
[1] https://github.com/whatwg/fetch/blob/master/Overview.html#L440
[2] https://tools.ietf.org/html/rfc7230#section-3.2
The text was updated successfully, but these errors were encountered: