Bug 46151 – XMLHttpRequest: setRequestHeader() does not throw SYNTAX_ERR exception if the header field name is empty

• Component: XML
• Status: RESOLVED FIXED

[Jian Li]

XMLHttpRequest: setRequestHeader() does not throw SYNTAX_ERR exception if the header field name is empty

Per the XHR spec:
  client . setRequestHeader(header, value)
    Throws a SYNTAX_ERR exception if header is not a valid HTTP header field name or if value is not a valid HTTP header field value.

Test: http://tc.labs.opera.com/apis/XMLHttpRequest/setrequestheader-bogus-header.htm

Note that this test is not 100% correct because it checks against INVALID_STATE_ERR instead of SYNTAX_ERR. However, after fixing this, we do have one bug with not throwing SYNTAX_ERR exception if the header field name is empty

[Luke Macpherson]

The fix appears to be changing XMLHttpRequest::isValidToken() to match http://www.ietf.org/rfc/rfc2616.txt section 2.2 by rejecting zero-length tokens.

The question is: will throwing an exception here as mandated by the spec break existing web sites?

[Alexey Proskuryakov]

> The fix appears to be changing XMLHttpRequest::isValidToken()

Note that this function is also used to check HTTP method, so this will also fix part of bug 46008.

I think that this is unlikely to break web sites. Seems fine to make the change, as long as at least one of IE/Firefox matches the spec here.

Given that we're going to import the whole test suite sooner or later, and that this is such an inconsequential change, it may even be OK to land it without a regression test.

[Luke Macpherson]

Created attachment 68490 [details]
Patch

[David Levin]

Does IE or Firefox match the spec here?

[Luke Macpherson]

Firefox and IE did both throw an exception in the test case in the patch attached to this bug. (once I changed it to use innerHTML)
The conversion from the caught exception to a string did give different results on each browser though, so I'm not sure if they are returning SYNTAX_ERR or some other exception.

[Alexey Proskuryakov]

Comment on attachment 68490 [details]
Patch

Code changes look fine, but you should test both issues fixed here - or for neither.

[Luke Macpherson]

@Alexey, is the second issue you want tested https://bugs.webkit.org/show_bug.cgi?id=46008, or something else?

[Alexey Proskuryakov]

Yes, the empty HTTP method one. Even though it was originally split into a different bug, it makes most sense to land regression tests in the same commit as code changes.

[Luke Macpherson]

Created attachment 68502 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 68502 [details]
Patch

Clearing flags on attachment: 68502

Committed r68124: <http://trac.webkit.org/changeset/68124>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.