308484 - Extensions can't set Content-Length header for XMLHttpRequest

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Wladimir Palant]

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]

See https://bugzilla.mozilla.org/show_bug.cgi?id=302263#c3

Comment 2

[Darin Fisher]

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]

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]

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 [:bzbarsky]]

Note that on trunk and 1.8 branch this only works because of bug 307769 at the
moment...

Comment 6

[Jesse Ruderman]

> 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]

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]

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]

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]

OK, I'm working on a fix for the 1.7 and aviary-1.0.1 branches.

Comment 11

[Darin Fisher]

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]

That sounds like the right thing to do, at least on the Firefox 1.0.x branch.

Comment 13

[Darin Fisher]

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]

I suggest UniversalBrowserWrite, assuming that chrome scripts actually have that
privilege by default.

Comment 15

[Darin Fisher]

Attachment: v1 patch: for 1.7 / 1.0.1 branches

Yeah, this seems to do the trick.

Comment 16

[Benjamin Smedberg]

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]]

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)]

Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

sr=jst

Comment 19

[Darin Fisher]

> 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]

fixed1.7.12, fixed-aviary1.0.7

Comment 21

[Darin Fisher]

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]

Verified, Xpoint sidebar works fine in Firefox 1.0.7 build 2005091518.

Comment 23

[Mike Schroepfer]

Updating 1.7.12 blocking to match 1.0.7 blocking

Comment 24

[Daniel Veditz [:dveditz]]

This was never fixed on the trunk/1.8 branch

Comment 25

[Darin Fisher]

(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)]

+      if (header.Equals(kInvalidHeaders[i],
+                        nsCaseInsensitiveCStringComparator())) {

LowerCaseEqualsASCII on trunk/1.8?

Comment 27

[Darin Fisher]

fixed-on-trunk, fixed1.8 w/ LowerCaseEqualsASCII :)

Comment 28

[Wladimir Palant]

Attachment: 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 [:bzbarsky]]

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]

Attachment: mochitest testcase with bz's comments addressed

Comment 31

[Robert Sayre]

Checking in test_bug308484.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug308484.html,v  <--  test_bug308484.html
initial revision: 1.1
done