3.49 KB, patch
|Details | Diff | Splinter Review|
Test cases where wrong exceptions are being thrown: http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-name.htm http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-bogus-value.htm
> void setRequestHeader(ByteString header, ByteString value); http://xhr.spec.whatwg.org/#xmlhttprequest > If the value of any element of x is greater than 255, then throw a TypeError. http://dev.w3.org/2006/webapi/WebIDL/#es-ByteString Can you fix the test?
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
I can certainly fix the test, but I note that all (two) other testable implementations (OpPresto and Chrome) pass now, and start failing if we require a TypeError instead..?
Since those are incorrect, that seems fine with me ;)
Yes, fixing the test seems to be the right thing here. Other implementations aren't validating the arguments at all and just fall through to 3 and 4 of http://xhr.spec.whatwg.org/#dom-xmlhttprequest-setrequestheader ?
Let's see if I get this right.. so setRequestHeader() can throw two different exceptions for invalid arguments. 1) If either argument 'name' or 'value' contains a character with a code point higher than U+00FF, IDL argument validation of ByteString kicks in and a TypeError is thrown 2) If IDL validation deems all characters valid, the strings are checked against header name/value syntax rules, if the check fails a SyntaxError is thrown. If I got that right, it seems that the tests are mostly correct as-is since most of them do not test > U+00FF characters. A couple of the tests do, and should be fixed. (Although IMHO it is a bit confusing to spec two different errors for invalid input here - maybe the spec should just call the input a DOMString and rely on the HTTP header syntax check? This is of course nitpicking though..)
We still haven't figured out the exact exception story. Just using TypeError here works for me.
(I was sort of suggesting using SyntaxError for everything.. ;)). Would be good to figure out this now to finalize test suite and spec.. I'd still like Ms2ger to confirm that I got the (ccurrent) logic right in comment 15.
Yes, comment 15 is correct.
OK, makes sense. I'll go through and fix the relevant tests accordingly when you've changed the spec (might also be able to close some of the [XHR2] bugs here in the process ;)).
So, if Anne wants everything to throw TypeError we still have work to do in this bug - reopening. This is what the tests are going to look like: http://w3c-test.org/web-platform-tests/submissions/343/XMLHttpRequest/setrequestheader-bogus-name.htm http://w3c-test.org/web-platform-tests/submissions/343/XMLHttpRequest/setrequestheader-bogus-value.htm
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
There are two tests still failing in http://w3c-test.org/XMLHttpRequest/setrequestheader-bogus-value.htm One has a tab (\t) in the header's value and the other has the DEL character (\x7f). The function being called to validate the string is nsHttp::IsReasonableHeaderValue(), which only invalidates if it finds an \r \n or \0. This doesn't follow the RFC 7230 productions for field-values, which the XHR spec says should be used to decide whether to throw a SyntaxError: field-value = *( field-content / obs-fold ) field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] field-vchar = VCHAR / obs-text VCHAR = any visible [USASCII] character obs-text = %x80-FF obs-fold = CRLF 1*( SP / HTAB ) However, I'm not certain if fixing IsReasonableHeaderValue() is the right choice, or if an similar alternative method should be made just for XHRs specifically. Plus, Chrome also fails these two tests, so I'm left wondering what the best course of action here is.
See https://github.com/whatwg/fetch/issues/115 and https://github.com/whatwg/fetch/issues/100. This was changed based on changes made to WebKit, but perhaps that's too strict? If we change this we should do so in a way that affects XMLHttpRequest, fetch(), and networking in general. If we cannot change this we should probably open a new issue against Fetch.
It looks like the Blink devs want more investigation before they take this: https://bugs.chromium.org/p/chromium/issues/detail?id=455099 They don't seem inclined to investigate it with telemetry, though. Regardless, here's a candidate patch which implements the same proposed changes, just in case we do want to make these changes sometime (should affect networking in general, not just XHRs).
Thomas, shall we close this? I think we pass the relevant tests now so this got resolved somehow, probably by changing the standard around.
Yes, I don't see any reason to keep this open if the tests are passing now. We can always reopen if later consensus is reached on using RFC 7230 productions for field values.
Status: NEW → RESOLVED
Last Resolved: 6 years ago → 2 years ago
Resolution: --- → INVALID
I forgot, but I filed bug 1330297 on the remaining issues, but the patch here is not applicable either way.
You need to log in before you can comment on or make changes to this bug.