474845 - An empty HTTP header and an absent HTTP header aren't semantically equivalent

Status: RESOLVED DUPLICATE of bug 669259

(Core :: Networking: HTTP, defect)

Description

[Jeff Walden [:Waldo]]

nsHttpHeaderArray doesn't meaningfully store headers which are present but have an empty value, either when set from code in Firefox or in response to incoming responses.  Instead, it overloads this value to say "remove this header" (assuming the newly-set value isn't being merged into existing header values).  I can find nothing in the HTTP RFC, nor in the next-gen HTTP drafts, to support this action.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This makes us fail the following XHR test:

http://w3c-test.org/webapps/XMLHttpRequest/tests/submissions/Opera/getresponseheader-cookies-and-more.htm

Comment 2

[Jason Duell]

So we split nsHttpHeaderArray::SetHeader into two functions in the past year, with a new version called SetHeaderFromNet() that has different semantics.  An empty header for SetHeaderFromNet does not remove an existing header (and for certain security-sensitive headers cause the entire request to be marked CORRUPT).  But we still mostly ignore empty headers (except for those sensitive headers).

What's the exact semantic you need here?  I'll be adding support for keeping track of all HTTP headers including blanks ones in bug 669259.  If you tell me what you need we can roll it into that fix probably.

If some XML code is explicitly calling SetHeader("foo", "") then yes, we are still treating that as a request to throw away any existing values.  That's (sadly) the behavior specified in the IDL and I was wary of changing it (but might be convinced otherwise with a decent code audit and/or telemetry to make sure addons aren't using it that way).

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #2)
> What's the exact semantic you need here?  I'll be adding support for keeping
> track of all HTTP headers including blanks ones in bug 669259.  If you tell
> me what you need we can roll it into that fix probably.

When we parse a header with an empty value from the network, we should preserve the empty value instead of un-setting the value.

> If some XML code is explicitly calling SetHeader("foo", "") then yes, we are
> still treating that as a request to throw away any existing values.  That's
> (sadly) the behavior specified in the IDL and I was wary of changing it (but
> might be convinced otherwise with a decent code audit and/or telemetry to
> make sure addons aren't using it that way).

We should remove SetHeader and add a new method with a different name and/or different prototype that allows the setting of a blank value. ClearHeader can be used to remove a header.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #3)
> We should remove SetHeader and add a new method with a different name and/or
> different prototype that allows the setting of a blank value. ClearHeader
> can be used to remove a header.

And, do something similar in nsIHttpResponse:

Probably we should make a new getResponseHeaderSafe method with a prototype like this:

   typedef uint32_t MultipleValueRule;
   const MultipleValueRule TAKE_FIRST_WHEN_MULTIPLE = 1;
   const MultipleValueRule FAIL_WHEN_MULTIPLE = 2;
   const MultipleValueRule FOLD_MULTIPLE = 3;
   ACString getResponseHeaderSafe(in ACString header, MultipleValueRule rule);

   /* Use getResponseHeaderSafe instead of this. */
   [deprecated] ACString getResponseHeader(in ACString header);

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Sorry, the multi-valued header comment isn't really for this bug.

Comment 6

[Jason Duell]

> We should remove SetHeader and add a new method with a different name 
> and/or different prototype that allows the setting of a blank value.

Removing SetHeader would break all addons that use it to clear header values.  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.

I agree we'll need to have some way to probe for 2nd and following header values as part of bug 669259.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from bug 474845 comment 6)
> Removing SetHeader would break all addons that use it to clear header
> values.

To be clear, I was talking about nsHttpRequestHead::SetHeader and nsHttpResponseHead::SetHeader. I think you must be talking about nsIHttp*::setHeader. I agree we cannot remove the SetHeader methods exposed to addons, but we can add a replacement and add a console warning that is printed when the old version is used.

(In reply to Jason Duell (:jduell) from comment #6)
> 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

I filed bug 815299.