Extensions can't set Content-Length header for XMLHttpRequest

RESOLVED FIXED

Status

()

Core
XML
--
major
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Wladimir Palant, Assigned: Darin Fisher)

Tracking

(4 keywords)

Trunk
fixed-aviary1.0.7, fixed1.7.12, fixed1.8, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b5 +
blocking1.9a1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Flags: blocking-aviary1.0.7?
See https://bugzilla.mozilla.org/show_bug.cgi?id=302263#c3
(Assignee)

Comment 2

12 years ago
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.
(Assignee)

Comment 3

12 years ago
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!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 4

12 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Version: Trunk → 1.7 Branch
Flags: blocking1.7.12?
Note that on trunk and 1.8 branch this only works because of bug 307769 at the
moment...
Blocks: 307769

Comment 6

12 years ago
> 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

12 years ago
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.
(Reporter)

Comment 8

12 years ago
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.
(Reporter)

Comment 9

12 years ago
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.
(Assignee)

Comment 10

12 years ago
OK, I'm working on a fix for the 1.7 and aviary-1.0.1 branches.
Assignee: xml → darin
Status: REOPENED → NEW
(Assignee)

Comment 11

12 years ago
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

12 years ago
That sounds like the right thing to do, at least on the Firefox 1.0.x branch.
(Assignee)

Comment 13

12 years ago
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.)
I suggest UniversalBrowserWrite, assuming that chrome scripts actually have that
privilege by default.
(Assignee)

Comment 15

12 years ago
Created attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

Yeah, this seems to do the trick.
Attachment #196241 - Flags: superreview?(jst)
Attachment #196241 - Flags: review?(benjamin)
Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

I can't review this before vacation.
Attachment #196241 - Flags: review?(benjamin) → review?(doronr)
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.
Attachment #196241 - Flags: review?(doronr)
Attachment #196241 - Flags: review+
Attachment #196241 - Flags: approval1.7.12?
Attachment #196241 - Flags: approval-aviary1.0.7?

Updated

12 years ago
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
Comment on attachment 196241 [details] [diff] [review]
v1 patch: for 1.7 / 1.0.1 branches

sr=jst
Attachment #196241 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 19

12 years ago
> 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.
Attachment #196241 - Flags: approval1.7.12?
Attachment #196241 - Flags: approval1.7.12+
Attachment #196241 - Flags: approval-aviary1.0.7?
Attachment #196241 - Flags: approval-aviary1.0.7+
(Assignee)

Comment 20

12 years ago
fixed1.7.12, fixed-aviary1.0.7
Keywords: fixed-aviary1.0.7, fixed1.7.12
(Assignee)

Comment 21

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Summary: Content-Length header can't be set for XMLHttpRequest → Extensions can't set Content-Length header for XMLHttpRequest
(Reporter)

Comment 22

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

Comment 23

12 years ago
Updating 1.7.12 blocking to match 1.0.7 blocking
Flags: blocking1.7.12? → blocking1.7.12+
This was never fixed on the trunk/1.8 branch
Status: RESOLVED → REOPENED
Flags: blocking1.9a1+
Flags: blocking1.8b5+
Resolution: FIXED → ---
(Assignee)

Comment 25

12 years ago
(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.
(Assignee)

Updated

12 years ago
Attachment #196241 - Flags: approval1.8b5?
+      if (header.Equals(kInvalidHeaders[i],
+                        nsCaseInsensitiveCStringComparator())) {

LowerCaseEqualsASCII on trunk/1.8?
Blocks: 310048

Updated

12 years ago
Attachment #196241 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 27

12 years ago
fixed-on-trunk, fixed1.8 w/ LowerCaseEqualsASCII :)
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Version: 1.7 Branch → Trunk
(Reporter)

Comment 28

11 years ago
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.
Attachment #246933 - Flags: review?(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...
Attachment #246933 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 30

11 years ago
Created attachment 247053 [details]
mochitest testcase with bz's comments addressed
Attachment #246933 - Attachment is obsolete: true
Attachment #247053 - Flags: review+

Comment 31

11 years ago
Checking in test_bug308484.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug308484.html,v  <--  test_bug308484.html
initial revision: 1.1
done
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.