Bug 46151 - XMLHttpRequest: setRequestHeader() does not throw SYNTAX_ERR exception if the header field name is empty
Summary: XMLHttpRequest: setRequestHeader() does not throw SYNTAX_ERR exception if the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 18:03 PDT by Jian Li
Modified: 2010-09-22 23:59 PDT (History)
8 users (show)

See Also:

Attachments
Patch (2.87 KB, patch)
2010-09-22 18:34 PDT, Luke Macpherson 		no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2010-09-22 23:02 PDT, Luke Macpherson 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2010-09-20 18:03:37 PDT 
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
Comment 1 Luke Macpherson 2010-09-22 00:24:47 PDT 
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?
Comment 2 Alexey Proskuryakov 2010-09-22 00:43:05 PDT 
> 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.
Comment 3 Luke Macpherson 2010-09-22 18:34:15 PDT 
Created attachment 68490 [details]
Patch
Comment 4 David Levin 2010-09-22 21:56:48 PDT 
Does IE or Firefox match the spec here?
Comment 5 Luke Macpherson 2010-09-22 22:39:58 PDT 
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.
Comment 6 Alexey Proskuryakov 2010-09-22 22:45:42 PDT 
Comment on attachment 68490 [details]
Patch

Code changes look fine, but you should test both issues fixed here - or for neither.
Comment 7 Luke Macpherson 2010-09-22 22:50:55 PDT 
@Alexey, is the second issue you want tested https://bugs.webkit.org/show_bug.cgi?id=46008, or something else?
Comment 8 Alexey Proskuryakov 2010-09-22 23:01:13 PDT 
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.
Comment 9 Luke Macpherson 2010-09-22 23:02:43 PDT 
Created attachment 68502 [details]
Patch
Comment 10 WebKit Commit Bot 2010-09-22 23:59:28 PDT 
Comment on attachment 68502 [details]
Patch

Clearing flags on attachment: 68502

Committed r68124: <http://trac.webkit.org/changeset/68124>
Comment 11 WebKit Commit Bot 2010-09-22 23:59:34 PDT 
All reviewed patches have been landed.  Closing bug.