Closed Bug 815299 Opened 7 years ago Closed 4 years ago

Cannot set a request header to an empty value using XHR or using nsIHttpChannel

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: briansmith, Assigned: Ehsan)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

+++ This bug was initially created as a clone of Bug #474845 +++

> So far I'm hearing that we need to return "" for headers with empty
> values (bug 669259), but I haven't seen a use case yet for actually sending
> empty headers out on the wire.  Let's open a new bug for that if we need it,
> which I doubt.

We need it to be compliant with the XHR spec: "The empty string is legal and represents the empty header value." (See link to the spec in the URL field of this bug.)

Potentially there could be application-specific headers where only the presence of the header matters, and not the value, so an empty value is OK.
Flags: needinfo?(jduell.mcbugs)
otoh empty headers are a potential smuggling vector for broken apps, etc.. to the extent we don't need them for a real use case I would rather not generate them.
Use case:

A web services based point of sale application that runs as a Firefox extension. This extension has the capability of upgrading itself in the field for security reasons or for new features. As part of its protocol, the application sends its version in all request headers to the web service. If the version doesn't match what is expected, an exception is generated and the application requests a new version of the software so that there is never a protocol mismatch which can lead to bugs.

Developers can symlink to their working directories instead of installing the extension (*), and they can configure the application to send a blank version to bypass the upgrade logic. The application positively asserts that it's bypassing the upgrade logic rather than passively failing to send an expected request header or sending some trumped up guard value as the version.

Justification:

Blank headers are allowed and correctly processed by curl on the client side and PHP on the server side, and are documented as supported by the protocol.

(*) This was broken to some degree after Firefox 3. If there is something wrong with your extension, Firefox changes to the directory pointed to by the symlink and removes everything. So don't do this:

1) cd $HOME/.mozilla/firefox/<profile directory name>/extensions
2) ln -s <directory tree I want to completely erase> bad@extension.name
3) Restart firefox

The blank header idea was precipitated by this bug, which has wiped out my development tree about a dozen times, and a few times with code I hadn't yet checked into our repo.
> they can configure the application to send a blank version to bypass the upgrade logic

IMO not compelling (why not just send some special value like "BYPASS"?)

> We need it to be compliant with the XHR spec

Much more compelling :) but doesn't necessarily make it a must-have if it's not really used in the wild or it makes security complicated.

I think the call here is sicking's (he owns XHR AFAICT). My sense is that this is probably worth fixing, but isn't urgent.
Flags: needinfo?(jduell.mcbugs) → needinfo?(jonas)
I don't own XHR :)

The fact that http says that empty headers should be supported unfortunately doesn't mean much. What ultimately matters if something breaks servers. There's lots of things that we forbid in XHR because it makes servers trip.

But we should either follow the spec, or ask the spec to be changed.

If we are worried that empty headers could be used as an attack vector against buggy servers, then that's a pretty good argument. Especially if we can point to cases in the past where this has been used.

I don't have any specific use cases though. That seems like a fine question for the spec authors.

cc'ing said spec author :)
Flags: needinfo?(jonas)
So this is about setRequestHeader()? Seems no different from http://lists.w3.org/Archives/Public/public-webapi/2006Aug/0024.html to me. HTTP does not define them as equivalent and some protocols using HTTP might make use of that fact. XMLHttpRequest is just a generic API for HTTP within some legacy and security constraints. 

If you question the use cases you should ask the HTTP WG. They (together with http://fetch.spec.whatwg.org/ these days when they fell short) decide what data gets passed to the XMLHttpRequest layer.
So, this seems to be an underlying issue that affects both setRequestHeader() and getResponseHeader() 
https://bugzilla.mozilla.org/show_bug.cgi?id=918721 is about the latter.

In principle, I think if we're going to limit what authors can do with an API we should have good reasons. Security issues are excellent reasons - however, I can't quite see what security problems an empty header might cause, and Chrome seems to have no qualms about supporting it. Did you think of request splitting (confusing proxies and intermediate servers) or web servers being confused into doing wrong stuff?

I did a little testing - 
Firefox disallows sending empty headers, disallows reading them with getResponseHeader() (returns null)
Opera disallows sending empty headers, but supports reading them just fine
Chrome supports both sending and reading.
I share the point of view that there doesn't seem to be a security issue with allowing header fields to be set to the empty value and/or with reading header fields with empty values. So, I think we should fix this bug and bug 918721...sometime.
We need to fix this bug as part of my work in bug 1199049.

Here is what can currently happen: consider a page that has a service worker running this code:

fetch(new Request('https://cross.origin/foo', {headers:{'bar': ''}}));

And imagine that its service worker does a simple passthrough:

onfetch = function(e) {
  e.respondWith(fetch(e.request));
};

Currently, before we intercept the fetch request, we look at its list of headers.  In InternalHeaders.cpp, we correctly store the empty header, so we determine that the request needs a CORS preflight.  We perform the CORS preflight before we intercept the request, which allows the server at cross.origin to make a decision on whether the eventual request going to the network is allowed.

After my changes, we effectively forget the empty bar header in <https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#433>, and then we intercept the request.  On the service worker side, we create the Request object associated with the FetchEvent by looking at the HTTP channel.  At that point, we have lost the empty header, so we create a Request object that only has simple headers, so when the service worker attempts to do a fetch() on the same request object, we conclude that a CORS preflight is not needed, and we just make the request directly from the server, which is completely broken.

Additionally, with service workers these headers getting lost in the networking layer will be observable to script, and can cause any number of issues similar to the above.

In order to not break the backwards compatibility on nsIHttpChannel::SetRequestHeader(), I'm planning to add a new method with the new semantics.
Assignee: nobody → ehsan
Blocks: 1199049
The bug 669259 has a patch that fixes this. It in Jason's review queue.

That patch have to sets of headers "original headers" and the headers as they are now, so that we do not break something.
(In reply to Dragana Damjanovic [:dragana] from comment #10)
> The bug 669259 has a patch that fixes this. It in Jason's review queue.

That bug is about response headers, but this one is about request headers.
Attachment #8653526 - Flags: review?(josh)
Attachment #8653527 - Flags: review?(josh)
Comment on attachment 8653525 [details] [diff] [review]
Part 1: Add an API for setting an empty request header on an HTTP channel

Review of attachment 8653525 [details] [diff] [review]:
-----------------------------------------------------------------

sgtm; but let's match this up with the response header review in jason's q
Attachment #8653525 - Flags: review?(mcmanus)
Attachment #8653525 - Flags: review?(jduell.mcbugs)
Attachment #8653525 - Flags: feedback+
Comment on attachment 8653526 [details] [diff] [review]
Part 2: Accept empty HTTP headers in fetch

Review of attachment 8653526 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/serviceworkers/fetch/fetch_tests.js
@@ +304,5 @@
> +  return res.text();
> +}).then(function(body) {
> +  my_ok(body == "emptyheader", "The empty header was observed in the fetch event");
> +  finish();
> +});

Add a catch that does my_ok(false) and finishes the test, or we'll get a timeout instead of a useful failure message.
Attachment #8653526 - Flags: review?(josh) → review+
Comment on attachment 8653527 [details] [diff] [review]
Part 3: Accept empty HTTP headers in XHR

Review of attachment 8653527 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/xhr_headers_server.sjs
@@ +22,5 @@
> +
> +      if (emptyHeader && emptyHeader == "nada") {
> +        setState("emptyHeader", "nada");
> +      }
> +      return;

I'm a bit concerned that we're losing the break here. That will start allowing more cases than we used to.
Attachment #8653527 - Flags: review?(josh) → review+
Comment on attachment 8653527 [details] [diff] [review]
Part 3: Accept empty HTTP headers in XHR

Review of attachment 8653527 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/workers/test/xhr_headers_server.sjs
@@ +22,5 @@
> +
> +      if (emptyHeader && emptyHeader == "nada") {
> +        setState("emptyHeader", "nada");
> +      }
> +      return;

I'm not sure what you mean by this.  A break here means getting a 501 error out of the sjs, that is not what we want here at all.  All of the successful code paths in the switch just return.
Previously, xhr_headers_server.sjs would return a 501 if a POST was received but not "options-host" header was present. It's now impossible for a POST request to yield a 501 response.
(In reply to Josh Matthews [:jdm] from comment #20)
> Previously, xhr_headers_server.sjs would return a 501 if a POST was received
> but not "options-host" header was present. It's now impossible for a POST
> request to yield a 501 response.

I see.  OK, will fix before landing.
ping?
Comment on attachment 8653525 [details] [diff] [review]
Part 1: Add an API for setting an empty request header on an HTTP channel

Review of attachment 8653525 [details] [diff] [review]:
-----------------------------------------------------------------

Jason asked me to take a look at this and if it is in conflict with patch from bug 669259. 

r+

::: netwerk/protocol/http/nsHttpHeaderArray.h
@@ +31,5 @@
>                         bool merge = false);
>  
> +    // Used by internal setters to set an empty header
> +    nsresult SetEmptyHeader(nsHttpAtom headeri);
> +

nit: did you meant "header"
Attachment #8653525 - Flags: review?(jduell.mcbugs) → review+
(In reply to Dragana Damjanovic [:dragana] from comment #23)
> ::: netwerk/protocol/http/nsHttpHeaderArray.h
> @@ +31,5 @@
> >                         bool merge = false);
> >  
> > +    // Used by internal setters to set an empty header
> > +    nsresult SetEmptyHeader(nsHttpAtom headeri);
> > +
> 
> nit: did you meant "header"

Yup, this is vim brain damage.  :-)

Thanks for the review!
Sorry, I forgot to address Josh's review comments.  Did that here: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3adfd2ca52
Ehsan, I think this broke Android M13 like this: https://treeherder.mozilla.org/logviewer.html#?job_id=13794675&repo=mozilla-inbound

495 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/test_xhr_headers.html | Server saw expected requests - got "<html> <head><title>500 Internal Server Error</title></head> <body> <h1>500 Internal Server Error</h1> <p>Something's broken in this server and needs to be fixed.</p> </body> </html>", expected "Success: expected OPTIONS request wi
Flags: needinfo?(ehsan)
The only custom header in that test seems to be this:
xhr.setRequestHeader("options-host", otherHost);

Wonder how that causes the problem.. It's not empty either.
Turns out that JS is an untyped language, so |bool headerFound = false;| doesn't work.  :-)
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.