Skip to content

/ servo

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 values no longer have to be ASCII or UTF-8 #25358

Merged
merged 1 commit into from Jan 3, 2020
Merged

Header values no longer have to be ASCII or UTF-8 #25358

merged 1 commit into from Jan 3, 2020
+56 −124

Conversation

@pshaughn
Copy link
Member

pshaughn commented Dec 21, 2019 •
edited

This passes some failed tests related to header validity when handling ByteStrings outside the printable ASCII range. A few failures remain because the HeaderValue class is stricter than WHATWG/WPT, disallowing various control-code bytes that the spec and tests expect to be allowed.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix some of the test cases described in #24903
  • There are tests for these changes OR
@highfive
Copy link

highfive commented Dec 21, 2019

Heads up! This PR modifies the following files:
@pshaughn pshaughn mentioned this pull request Dec 23, 2019
Open
@CYBAI
Copy link
Collaborator

CYBAI commented Dec 24, 2019

@bors-servo try=wpt
bors-servo added a commit that referenced this pull request Dec 24, 2019
@bors-servo
Auto merge of #25358 - pshaughn:looser_header_validation, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
3928c74 
Header values no longer have to be ASCII or UTF-8

<!-- Please describe your changes on the following line: -->
This passes some failed tests related to header validity when handling ByteStrings outside the printable ASCII range. A few failures remain because the HeaderValue class is stricter than WHATWG/WPT, disallowing various control-code bytes that the spec and tests expect to be allowed.

---
<!-- 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
- [X] These changes fix some of the test cases described in #24903

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2019

⌛ Trying commit 0901fce with merge 3928c74...
@bors-servo
Copy link
Contributor

bors-servo commented Dec 24, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Dec 24, 2019 

  ▶ Unexpected subtest result in /fetch/api/response/response-init-002.html:
  └ PASS [expected FAIL] Testing empty Response Content-Type header
  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Testing empty Request Content-Type header
@pshaughn
Headers no longer have to be ASCII or UTF-8, though there is some dis…
Loading status checks…
0c08849 
…agreement with http::header::HeaderValue
@pshaughn pshaughn force-pushed the pshaughn:looser_header_validation branch from 0901fce to 0c08849 Dec 24, 2019
@pshaughn
Copy link
Member Author

pshaughn commented Dec 24, 2019

A couple notes about this:

  • I think I've taken out all the places where a header value gets parsed as a UTF-8 string without sanity-checking, but I don't know for sure. Hyper HeaderMap does a lot of the sanity-checking for us; for most "typed" headers we can safely cast the value that comes out of the map to a string. Conversion from a header value to a DOMString for Javascript consumption happens via ByteString, which should give a weird but safe "isomorphic decode" from bytes to UTF-16 values 0-255 instead of a UTF-8 parse.
  • The "Servo thinks "{:?}" is a valid HTTP header value but HeaderValue doesn't." codepath represents a case where different parties still haven't really converged on a spec; Hyper is currently following the RFC rather than WHATWG. In practice I don't think any legitimate website is going to send a header where this particular disagreement matters.
@pshaughn
Copy link
Member Author

pshaughn commented Jan 1, 2020

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2020

⌛ Trying commit 0c08849 with merge de99d20...
bors-servo added a commit that referenced this pull request Jan 1, 2020
@bors-servo
Auto merge of #25358 - pshaughn:looser_header_validation, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
de99d20 
Header values no longer have to be ASCII or UTF-8

<!-- Please describe your changes on the following line: -->
This passes some failed tests related to header validity when handling ByteStrings outside the printable ASCII range. A few failures remain because the HeaderValue class is stricter than WHATWG/WPT, disallowing various control-code bytes that the spec and tests expect to be allowed.

---
<!-- 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
- [X] These changes fix some of the test cases described in #24903

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True
@jdm
Copy link
Member

jdm commented Jan 2, 2020

@bors-servo r+
Thanks for sorting this out!
@bors-servo
Copy link
Contributor

bors-servo commented Jan 2, 2020

📌 Commit 0c08849 has been approved by jdm
@highfive highfive assigned jdm and unassigned Manishearth Jan 2, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2020

⌛ Testing commit 0c08849 with merge 97f0f5e...
bors-servo added a commit that referenced this pull request Jan 3, 2020
@bors-servo
Auto merge of #25358 - pshaughn:looser_header_validation, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
97f0f5e 
Header values no longer have to be ASCII or UTF-8

<!-- Please describe your changes on the following line: -->
This passes some failed tests related to header validity when handling ByteStrings outside the printable ASCII range. A few failures remain because the HeaderValue class is stricter than WHATWG/WPT, disallowing various control-code bytes that the spec and tests expect to be allowed.

---
<!-- 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
- [X] These changes fix some of the test cases described in #24903

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Jan 3, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2020

⌛ Testing commit 0c08849 with merge 097a911...
bors-servo added a commit that referenced this pull request Jan 3, 2020
@bors-servo
Auto merge of #25358 - pshaughn:looser_header_validation, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
097a911 
Header values no longer have to be ASCII or UTF-8

<!-- Please describe your changes on the following line: -->
This passes some failed tests related to header validity when handling ByteStrings outside the printable ASCII range. A few failures remain because the HeaderValue class is stricter than WHATWG/WPT, disallowing various control-code bytes that the spec and tests expect to be allowed.

---
<!-- 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
- [X] These changes fix some of the test cases described in #24903

<!-- Either: -->
- [X] There are tests for these changes OR

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 097a911 to master...
@bors-servo bors-servo merged commit 0c08849 into servo:master Jan 3, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@pshaughn @highfive @CYBAI @bors-servo @jdm @Manishearth
You can’t perform that action at this time.