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

XMLHttpRequest: response header value containing 0x00 #10424

Merged
merged 2 commits into from
Aug 28, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented Apr 11, 2018

As discussed in whatwg/xhr#165 these should turn the response into a network error.

As discussed in whatwg/xhr#165 these should turn the response into a network error.
@annevk
Copy link
Member Author

annevk commented Apr 11, 2018

(Note that various cors/ tests use 0x00 in header values too, but those already expect a network error due to the nature of CORS.)

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test code LGTM but fetch() should not be tested in the XHR test folder.

matchHeaderValue(" ");
matchHeaderValue("\t");
matchHeaderValue("");

promise_test(t => {
return promise_rejects(t, new TypeError(), fetch("resources/parse-headers.py?my-custom-header="+encodeURIComponent("x\0x")));
}, "Ensure fetch() rejects too")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty out of place here; another separate file would be much better, in case people are testing fetch() and XHR in isolation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would they do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they're working on their fetch() or XHR implementation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to support both and they both need to fail in the same way due to both operating on the same low-level primitive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And it's not like both are tested in the same test so it'd be easy to ignore some results.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I actually think we might want to merge these directories at some point due to the worry of missing test coverage in either API.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not comfortable testing fetch in the XHR directory, sorry. It makes maintainers lives harder when they don't know where a given API's tests are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's already the case though for many cross-purpose tests. I guess we could move this into the http/ directory...

@wpt-pr-bot wpt-pr-bot requested review from domfarolino, mnot, youennf and yutakahirano and removed request for hallvors, ronkorving and kangxu August 24, 2018 14:40
@annevk
Copy link
Member Author

annevk commented Aug 24, 2018

I moved it to a separate file for now. I think we already have this cross-dependency allowed elsewhere, but I want to get this landed.

@annevk annevk requested a review from domenic August 24, 2018 14:40
@annevk annevk merged commit 3d172bc into master Aug 28, 2018
@annevk annevk deleted the annevk/response-header-value-null branch August 28, 2018 06:07
annevk added a commit that referenced this pull request Jan 3, 2020
Tests to complement those written in #10424.
annevk added a commit that referenced this pull request Jan 3, 2020
Tests to complement those written in #10424.
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jan 8, 2020
Automatic update from web-platform-tests
HTTP: 0x00 in a header value

Tests to complement those written in web-platform-tests/wpt#10424.
--

wpt-commits: 38ecde806a5f1710d9e5beba700cef7352f7570e
wpt-pr: 21019
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 8, 2020
Automatic update from web-platform-tests
HTTP: 0x00 in a header value

Tests to complement those written in web-platform-tests/wpt#10424.
--

wpt-commits: 38ecde806a5f1710d9e5beba700cef7352f7570e
wpt-pr: 21019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Jan 11, 2020
Automatic update from web-platform-tests
HTTP: 0x00 in a header value

Tests to complement those written in web-platform-tests/wpt#10424.
--

wpt-commits: 38ecde806a5f1710d9e5beba700cef7352f7570e
wpt-pr: 21019

UltraBlame original commit: 2df4f38122005c353cf34c665c293fcb975fbdb7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Jan 11, 2020
Automatic update from web-platform-tests
HTTP: 0x00 in a header value

Tests to complement those written in web-platform-tests/wpt#10424.
--

wpt-commits: 38ecde806a5f1710d9e5beba700cef7352f7570e
wpt-pr: 21019

UltraBlame original commit: 2df4f38122005c353cf34c665c293fcb975fbdb7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Jan 11, 2020
Automatic update from web-platform-tests
HTTP: 0x00 in a header value

Tests to complement those written in web-platform-tests/wpt#10424.
--

wpt-commits: 38ecde806a5f1710d9e5beba700cef7352f7570e
wpt-pr: 21019

UltraBlame original commit: 2df4f38122005c353cf34c665c293fcb975fbdb7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants