Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Reporter
	Description • 12 years ago

+++ 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.

	Patrick McManus [:mcmanus]
	Comment 2 • 11 years ago

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.

	jgotts
	Comment 3 • 11 years ago

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.

	Jason Duell
	Comment 4 • 11 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 11 years ago

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

	Anne (:annevk)
	Comment 6 • 11 years ago

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.

	Hallvord R. M. Steen [:hallvors]
	Comment 7 • 11 years ago

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.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Reporter
	Comment 8 • 11 years ago

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.

	(no longer active) Assignee
	Comment 9 • 9 years ago

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.

	Dragana Damjanovic [:dragana]
	Comment 10 • 9 years ago

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.

	(no longer active) Assignee
	Comment 11 • 9 years ago

(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.

	Patrick McManus [:mcmanus]
	Comment 15 • 9 years ago

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

	(no longer active) Assignee
	Comment 16 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31150a004cca

	Josh Matthews [:jdm]
	Comment 17 • 9 years ago

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.

	Josh Matthews [:jdm]
	Comment 18 • 9 years ago

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.

	(no longer active) Assignee
	Comment 19 • 9 years ago

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.

	Josh Matthews [:jdm]
	Comment 20 • 9 years ago

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.

	(no longer active) Assignee
	Comment 21 • 9 years ago

(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.

	(no longer active) Assignee
	Comment 22 • 9 years ago

ping?

	Dragana Damjanovic [:dragana]
	Comment 23 • 9 years ago

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"

	(no longer active) Assignee
	Comment 24 • 9 years ago

(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!

	Pulsebot
	Comment 25 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a798c03d76c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3555d4b1ab1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a2a9972b4c7

	(no longer active) Assignee
	Comment 26 • 9 years ago

Sorry, I forgot to address Josh's review comments.  Did that here: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3adfd2ca52

	Nigel Babu [:nigelb]
	Comment 27 • 9 years ago

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

	Pulsebot
	Comment 28 • 9 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f2051c912c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5defda5bf2
https://hg.mozilla.org/integration/mozilla-inbound/rev/73250801ff8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9cb31373ec2

	Hallvord R. M. Steen [:hallvors]
	Comment 29 • 9 years ago

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.

	(no longer active) Assignee
	Comment 30 • 9 years ago

Turns out that JS is an untyped language, so |bool headerFound = false;| doesn't work.  :-)

	Pulsebot
	Comment 31 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/35502cb5d5b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a32b845ba8
https://hg.mozilla.org/integration/mozilla-inbound/rev/55d57fcbc791

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 32 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/35502cb5d5b1
https://hg.mozilla.org/mozilla-central/rev/a9a32b845ba8
https://hg.mozilla.org/mozilla-central/rev/55d57fcbc791

	Sebastian Zartner [:sebo]
	Comment 33 • 8 years ago

Described at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel.

Sebastian