Last Comment Bug 308484 - Extensions can't set Content-Length header for XMLHttpRequest
: Extensions can't set Content-Length header for XMLHttpRequest
Status: RESOLVED FIXED
: fixed-aviary1.0.7, fixed1.7.12, fixed1.8, regression
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: All All
: -- major (vote)
: ---
Assigned To: Darin Fisher
: Ashish Bhatt
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 307769 310048
  Show dependency treegraph
 
Reported: 2005-09-14 07:11 PDT by Wladimir Palant
Modified: 2008-01-19 02:51 PST (History)
14 users (show)
mtschrep: blocking1.7.12+
mtschrep: blocking‑aviary1.0.7+
dveditz: blocking1.8b5+
dveditz: blocking1.9a1+
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch: for 1.7 / 1.0.1 branches (2.32 KB, patch)
2005-09-15 14:16 PDT, Darin Fisher
dveditz: review+
jst: superreview+
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
asa: approval1.8b5+
Details | Diff | Splinter Review
mochitest compatible testcase (1.98 KB, text/html)
2006-11-29 08:02 PST, Wladimir Palant
bzbarsky: review+
Details
mochitest testcase with bz's comments addressed (1.98 KB, text/html)
2006-11-30 02:00 PST, Wladimir Palant
trev.moz: review+
Details

Description Wladimir Palant 2005-09-14 07:11:52 PDT
Checkin for bug 302263 doesn't allow the Content-Length header field to be set
in XMLHttpRequest POST request. While I can't see any security related reason
for this (other than servers handling wrong Content-Length values badly, which
isn't our problem), this definitely breaks existing functionality. The
Content-Length header will only be set automatically when a string is sent, not
when you send a stream. So if you try to send a stream to the server, you will
get a "411 Length Required" response now. The code sending the request looks
like this:

var data = "test";

var stream = Components.classes['@mozilla.org/io/string-input-stream;1']
                       .createInstance(Components.interfaces.nsIStringInputStream);
stream.setData("\r\n" + data, -1);

var r = new XMLHttpRequest();
r.open('POST', 'http://www.google.com/');
// r.setRequestHeader('Content-Length', data.length);
r.send(stream);
r.onload = function() {
  alert(r.status)
};

Why I use this in a browser extension: it seems to be the only way to send data
to the server that isn't encoded as UTF-8.
Comment 1 Benjamin Smedberg [:bsmedberg] 2005-09-14 07:17:06 PDT
See https://bugzilla.mozilla.org/show_bug.cgi?id=302263#c3
Comment 2 Darin Fisher 2005-09-14 08:49:31 PDT
Hmm... this requires some investigation.  From my reading of the XMLHttpRequest
code, it should always send a Content-Length header along with the HTTP request.
 I'm surprised that you are getting a 411 error response.
Comment 3 Darin Fisher 2005-09-14 09:28:27 PDT
I can't reproduce the bug.  The HTTP request in your example has a
"Content-Length: 6" request header as expected.

Marking WORKSFORME.  Please reopen this bug if you have evidence that the
problem still exists.  If you can provide a trace of the headers to support the
bug report that would be helpful.  Thanks!
Comment 4 Wladimir Palant 2005-09-14 10:12:24 PDT
Yes, my fault, I should've done a better investigation. Yes, Content-Length is
set automatically for streams - but only on trunk. The problem exists on the
aviary branch though, tested with a Firefox 1.0.7 2005091320 build. Note that
bug 302809 isn't fixed there as well.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-09-14 23:34:12 PDT
Note that on trunk and 1.8 branch this only works because of bug 307769 at the
moment...
Comment 6 Jesse Ruderman 2005-09-15 00:12:25 PDT
> While I can't see any security related reason for this (other than servers
> handling wrong Content-Length values badly, which isn't our problem), this 
> definitely breaks existing functionality.

Servers handling Content-Length "badly" *is* our problem.  Allowing content to
set a bogus Content-Length (one that doesn't match the size of the content)
would be a security hole.

bz, what are you proposing to do in bug 307769 comment 4?
Comment 7 Jesse Ruderman 2005-09-15 00:22:34 PDT
If you're only requesting the ability to change Content-Length from an
extension, that wouldn't be a security hole, and the summary should be changed
to reflect that.
Comment 8 Wladimir Palant 2005-09-15 02:35:48 PDT
I finally got it why setting Content-Length was forbidden (I'm not allowed to
read bug 302263). At the moment I see three solutions:

1. Make sure Content-Length is set automatically for streams on the aviary
branch (looking at bug 307769 this is probably not possible).
2. Allow setting the Content-Length header for priviledged content only - that
should be the easiest thing to do, unpriviledged content can't use streams
anyways IMO.
3. Generally allow setting the Content-Length header but make sure that exactly
the specified amount of data is sent - probably the best solution, but more
difficult to implement.
Comment 9 Wladimir Palant 2005-09-15 09:24:41 PDT
I was told that this is important, so:

1. The extension that is broken by this problem is Xpoint sidebar:
https://addons.mozilla.org/extensions/moreinfo.php?id=486,
http://xpoint.mozdev.org/installation.html. Xpoint.ru is a major russian web
developer site with a high percentage of Gecko users - this extension is one of
the reasons for it.
2. The version on addons.mozilla.org is not updated yet, currently all Firefox
versions up to 1.5 Beta 1 are supported. If this problem is not fixed before the
1.0.7 release, I will have to advise against upgrading or suggest using Firefox
1.5 *Beta* - both bad solutions. As far as I can see there is no workaround for
the problem.
3. A short search on the web brought up some pages describing how to send
multipart POST requests, they are also affected by the problem:
http://xulfr.org/wiki/ApplisWeb/Request,
http://xpoint.ru/forums/programming/XUL/thread/27174.xhtml,
http://www.privosoft.com/pblog/index.php?entry=entry050604-072301. So this is
definitely not an uncommon pattern.
Comment 10 Darin Fisher 2005-09-15 11:16:49 PDT
OK, I'm working on a fix for the 1.7 and aviary-1.0.1 branches.
Comment 11 Darin Fisher 2005-09-15 11:31:50 PDT
One solution that might work well for extensions running in Firefox 1.0.x would
be to drop the header restrictions for privileged scripts.  That would be a
pretty simple solution to implement.  Thoughts?
Comment 12 Jesse Ruderman 2005-09-15 11:37:31 PDT
That sounds like the right thing to do, at least on the Firefox 1.0.x branch.
Comment 13 Darin Fisher 2005-09-15 11:38:38 PDT
I presume this means checking for UniversalXPConnect.  Or, is there some other
capability that I should check for?  (I want to make sure that signed scripts
get to play as well.)
Comment 14 Benjamin Smedberg [:bsmedberg] 2005-09-15 11:50:21 PDT
I suggest UniversalBrowserWrite, assuming that chrome scripts actually have that
privilege by default.
Comment 15 Darin Fisher 2005-09-15 14:16:53 PDT
Created attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

Yeah, this seems to do the trick.
Comment 16 Benjamin Smedberg [:bsmedberg] 2005-09-15 14:18:16 PDT
Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

I can't review this before vacation.
Comment 17 Daniel Veditz [:dveditz] 2005-09-15 15:01:23 PDT
Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

I'm ok with this as-is, r=dveditz

nit, if we care enough:

>+  if (!privileged) {

If this were instead "if (privileged) return NS_OK;" that makes the patch
smaller, since then the rest of this is just moved, not indented. I'm fine
either way.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-15 15:10:11 PDT
Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

sr=jst
Comment 19 Darin Fisher 2005-09-15 15:21:03 PDT
> If this were instead "if (privileged) return NS_OK;" that makes the patch
> smaller, since then the rest of this is just moved, not indented. I'm fine
> either way.

dveditz: actually, take another look... there's code below the "if
(!privileged)" section.
Comment 20 Darin Fisher 2005-09-15 16:04:39 PDT
fixed1.7.12, fixed-aviary1.0.7
Comment 21 Darin Fisher 2005-09-15 16:05:45 PDT
marking FIXED

I'm going to keep this patch out of the trunk and 1.8 branch until or unless we
hear about similar problems there.
Comment 22 Wladimir Palant 2005-09-16 05:14:11 PDT
Verified, Xpoint sidebar works fine in Firefox 1.0.7 build 2005091518.
Comment 23 Mike Schroepfer 2005-09-19 14:33:55 PDT
Updating 1.7.12 blocking to match 1.0.7 blocking
Comment 24 Daniel Veditz [:dveditz] 2005-09-28 19:38:53 PDT
This was never fixed on the trunk/1.8 branch
Comment 25 Darin Fisher 2005-09-28 20:48:46 PDT
(In reply to comment #24)
> This was never fixed on the trunk/1.8 branch

Because consumers of XMLHttpRequest on the trunk and 1.8 branch do not need to
set the Content-Length header.  The patch was only needed because of the way
XMLHttpRequest works on the 1.0 branch.  That said, given bug 310048, it
probably makes sense to land this patch on the trunk and 1.8 branch so that
extensions may set the "Upgrade" header and other restricted headers.
Comment 26 Christian :Biesinger (don't email me, ping me on IRC) 2005-09-29 07:06:39 PDT
+      if (header.Equals(kInvalidHeaders[i],
+                        nsCaseInsensitiveCStringComparator())) {

LowerCaseEqualsASCII on trunk/1.8?
Comment 27 Darin Fisher 2005-09-29 12:29:19 PDT
fixed-on-trunk, fixed1.8 w/ LowerCaseEqualsASCII :)
Comment 28 Wladimir Palant 2006-11-29 08:02:03 PST
Created attachment 246933 [details]
mochitest compatible testcase

Testcase to check setting problematic headers in both privileged and unprivileged mode. Note that it has to request UniversalBrowserWrite privileges (UniversalXPConnect won't work) meaning that it depends on bug 362230.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2006-11-29 18:07:20 PST
Comment on attachment 246933 [details]
mochitest compatible testcase

>  ok(value != "test" + i, "Setting " + headers[i] + " header in unprivileged context");

I'd prefer isnot() here instead of ok().  That would make it clear to the tester what failed.

>  ok(value == "test" + i, "Setting " + headers[i] + " header in privileged context");

And here I'd prefer is().

But in general, please ask whoever wrote the patch for test reviews -- I don't really have the bandwidth to review tests across the board...
Comment 30 Wladimir Palant 2006-11-30 02:00:02 PST
Created attachment 247053 [details]
mochitest testcase with bz's comments addressed
Comment 31 Robert Sayre 2006-12-09 10:02:45 PST
Checking in test_bug308484.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug308484.html,v  <--  test_bug308484.html
initial revision: 1.1
done

Note You need to log in before you can comment on or make changes to this bug.