Closed Bug 302809 Opened 17 years ago Closed 17 years ago

AJAX regression: POST setRequestHeader causes NS_ERROR_ILLEGAL_VALUE for invalid headers

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.8beta4

People

(Reporter: bgrupe, Assigned: darin.moz)

References

()

Details

(4 keywords)

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730 Firefox/1.0+

Checkin of bug 302263 added a check for invalid headers. If one of them is
found, it throws an exception rather then discarding them silently or outputting
a JavaScript warning.

Since the requests would work without those headers in most cases, and this
checkin breaks almost any AJAX post requests I know, the behaviour should change.


Reproducible: Always

Steps to Reproduce:
1. Go to the bug url
2. click on the POST test
3. check javascript console

Actual Results:  
Exception is thrown.

Expected Results:  
An alert window with the response pops up.

content-length is corpus delicti in this case.
I see no error in the JS Console after clicking on "Click here to run the POST
test!". 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050730
Firefox/1.0+ ID:2005073013
Ria: The testcase for POST appears to have
setRequestHeader("Content-Length",...) commented out.

Bastian: So, you are saying that it is very common on the web to call
setRequestHeader("Content-Length",...) even though it has never been needed?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 302263
Severity: normal → major
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta4
Flags: blocking1.8b4?
I accidently left the commented version online, the exception should be back in
place now.

Darin:
When I first learned doing AJAX stuff, I read it in at least 2 tutorials/guides
I think and then I assumed that it was needed and added it to my guide as well.

My point is that if it is not required, why isn't Gecko just silently ignoring it?
> My point is that if it is not required, why isn't Gecko just silently ignoring it?

The assumption I made is that only someone with malicious intent would set it,
but that is clearly a bad assumption.
If we don't return some kind of error web authors will assume the call worked,
making web app debugging more difficult. On the other hand bgrupe is right: lots
of pages, including msdn, show setting the content-length.

The channel will still error on invalid characters in headers and values, right?
I was mostly concerned about those and they should remain covered. In that case
simply dropping unwanted headers by returning NS_OK might be the most compatible
thing (add an NS_WARNING, or even a PR_LOG warning if we support that in this
module).
Keywords: regression
Flags: blocking1.8b4? → blocking1.8b4+
dveditz: yup, invalid characters will still throw.  patch coming up to implement
the NS_OK return w/ NS_WARNING.  no NSPR logging in xmlhttprequest land.
Attached patch v1 patchSplinter Review
Attachment #191736 - Flags: superreview?(dveditz)
Attachment #191736 - Flags: review?(dveditz)
Comment on attachment 191736 [details] [diff] [review]
v1 patch

I'm not sure that this so great. the NS_WARNING will really only be seen by
mozilla developers, who probably don't care so much about what web pages do.
maybe this should show a JS Console warning instead?
hrm... yeah, i agree.  a JS console warning would be better.  i don't think we
should necessarily hold this patch for that though.  i have higher priority
things to work on, and i probably won't be able to write JS console logging code
until after 1.5 beta.  patches welcome :)
You mean something like this?

Warning: Refusing to set request header Content-length
Source File: http://bazzinet.info/tutorials/js-request/example0_3.js
Line: 14

I can't actually tell if it really helps this example, though, as it still does
nothing, but I can be sure the JS code continues past the disregarded
setRequestHeader as requested.
That's a lot of code for logging... is there no utility function that can be
called that does all of that for us?
I can find the following that do similar stuff:
  http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#1121
  http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsJSUtils.cpp#59

The former is hard-wired for DOM errors, only allows JS warnings, and only gets
messages from the DOM properties file (with no substitution, too), and the
latter only gets the location information for use with other code (irronically,
it's only used in one place).

If you want a slightly more useful utility method somewhere, just tell me where
and I'll see what I can do. Note, however, that I'm away from my computer from
not until Friday, so you'd have to wait until then (or find someone else).
That is only half-useful, since it only wraps the actual reporting - not the bit
that gets the location.
Comment on attachment 191736 [details] [diff] [review]
v1 patch

r/sr=dveditz for this band-aide. Mystified developers can turn on http header
logging.
Attachment #191736 - Flags: superreview?(dveditz)
Attachment #191736 - Flags: superreview+
Attachment #191736 - Flags: review?(dveditz)
Attachment #191736 - Flags: review+
Comment on attachment 191736 [details] [diff] [review]
v1 patch

Let's add better console logging after we get this initial patch checked in.
Attachment #191736 - Flags: approval1.8b4?
Attachment #191736 - Flags: approval1.8b4? → approval1.8b4+
fixed-on-trunk

I'll file a follow-up bug about improving the logging.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Bug 302263 has been checked in on the aviary branch, I think this should this
should be done with attachment 191736 [details] [diff] [review] as well.
Flags: blocking-aviary1.0.7?
Indeed.
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7?
Flags: blocking-aviary1.0.7+
Attachment #191736 - Flags: approval1.7.12?
Attachment #191736 - Flags: approval-aviary1.0.7?
Keywords: fixed1.8
Comment on attachment 191736 [details] [diff] [review]
v1 patch

Yeah, we need to take this.  Sorry for missing it the first time around.
Attachment #191736 - Flags: approval1.7.12?
Attachment #191736 - Flags: approval1.7.12+
Attachment #191736 - Flags: approval-aviary1.0.7?
Attachment #191736 - Flags: approval-aviary1.0.7+
Checked in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.
Bastian - can you do final verification?
Verified for 1.8 branch
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.