476302 - Allow httpd.js return multiple headers with same name

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1

There is need to have tests for Proxy-Authenticate with multiple authentication methods. Common way how proxies or web servers tell a user agent to choose from more then one auth method is to send multiple Proxy- or WWW-Authenticate headers each with different auth method and its data. Necko is only then capable to process all offered methods of authentication.

The patch allows this, using an array as a value argument to setHeader. But maybe a better way is to have a new method for this?

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 359945 [details] [diff] [review]
v1

(In reply to comment #0)
> The patch allows this, using an array as a value argument to setHeader. But
> maybe a better way is to have a new method for this?

I assume you've never looked at how Necko handles this internally?

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpHeaderArray.cpp#48

Please modify header storage such that each header name is associated with an array of header lines.  For any header but the three Necko special-cases, the array consists of a single element with the syntax used for storage now.  However, if one of those three headers is being set, and if the value being set is to be merged into existing headers, just push it on the end of the array of values.  (Do, however, make sure to normalize the value first -- your patch here doesn't, which introduces the potential for HTTP injection attacks and other interesting things.)

getHeader presents a more interesting case; for simplicity I think we want to keep returning a string as the server IDL does, but we do want to differentiate the two types of header.  No header value can contain a CRLF after we've processed it, so how about if we just return valuesArray.join("\r\n")?  Users attempting to retrieve Set-Cookie or the other two will have to be careful with the returned string, but I guess there's not much choice about that.  We could also add a getSpecialHeader method that returned an enumerator over values, perhaps, but I'd rather put the burden of handling the three special headers differently on the user and not on the server.

Make sure to update the IDL documentation for getHeader when you do this, and to be safe bump the uuid as well.  I don't think setHeader will need any modifications, at the moment.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v2

This patch makes headerUtils class internally merge header values as _headers["name"][0] = "val1,val2,val3" for common headers and as _headers["name"] = ["val1","val2","val3"] for those 3 specific.

getHeader returns the same value as the current implementation for common headers and, as you suggest, values separated by '\n' for those 3 specific, as nsHttpHeaderArray does. It simply does _headers["name"].join('\n').

I added new getHeaderValues method into headerUtils that returns values as unjoined array (directly). It is than used in ServerHandler._end(). There is no need to carry this method out through nsIHttpRequestMetadata interface.

Sorry for the first patch, was made a bit in a rush.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 360150 [details] [diff] [review]
v2

>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js

>-      preamble += fieldName + ": " + head.getHeader(fieldName) + "\r\n";
>+      let values = head.getHeaderValues(fieldName);
>+      for (index in values)
>+        preamble += fieldName + ": " + values[index] + "\r\n";

I despise gratuitous use of let; use var.  :-P


>   setHeader: function(fieldName, fieldValue, merge)
>   {
>     var name = headerUtils.normalizeFieldName(fieldName);
>     var value = headerUtils.normalizeFieldValue(fieldValue);
> 
>     if (merge && name in this._headers)
>-      this._headers[name] = this._headers[name] + "," + value;
>-    else
>-      this._headers[name] = value;
>+      if (name == "www-authenticate" ||
>+          name == "proxy-authenticate" ||
>+          name == "set-cookie") {

Please put a comment next to this pointing the reader to the corresponding set of headers in Necko.

>+        this._headers[name].push(value);
>+      }
>+      else {
>+        this._headers[name][0] = this._headers[name][0] + "," + value;

Make this a += statement, please.

>+      }
>+    else
>+      this._headers[name] = [value];

If the condition or either the then-statement or else-statement covers multiple lines, both then and else parts must be braced.  Also, opening and closing braces go on their own lines.


>   /**
>    * Returns the value for the header specified by this.
>    *
>    * @throws NS_ERROR_INVALID_ARG
>    *   if fieldName does not constitute a valid header field name
>    * @throws NS_ERROR_NOT_AVAILABLE
>    *   if the given header does not exist in this
>    * @returns string
>    *   the field value for the given header, possibly with non-semantic changes
>    *   (i.e., leading/trailing whitespace stripped, whitespace runs replaced
>    *   with spaces, etc.) at the option of the implementation
>    */
>   getHeader: function(fieldName)
>+  {
>+    return this.getHeaderValues(fieldName).join("\n");

Join with "\r\n", please; it seems like a more correct joiner given that line endings in HTTP are CRLF.  Also note this in the @returns section, please.


>+  /**
>+   * Returns values as an Array object.  Only WWW-Authenticate, 
>+   * Proxy-Authenticate, Set-Cookie headers are stored as array. We need this
>+   * method to be able to send these headers as separate 'Name: value CRLF'
>+   * lines in the response.
>+   */
>+  getHeaderValues: function(fieldName)

Please follow the style used with getHeader and throughout the file: need @throws for if fieldName is malformed or if the header isn't set, and need to more clearly describe when a multiple-element array is returned and when it isn't.  I think the following would suffice for the latter; feel free to suggest changes if you think any are necessary:

  @returns [string]
    an array of all the header values in this for the given
    header name.  Header values will generally be collapsed
    into a single header by joining all header values together
    with commas, but certain headers (Proxy-Authenticate,
    WWW-Authenticate, and Set-Cookie) violate the HTTP spec
    and cannot be collapsed in this manner.  For these headers
    only, the returned array may contain multiple elements if
    that header has been added more than once.


>diff --git a/netwerk/test/httpserver/nsIHttpServer.idl b/netwerk/test/httpserver/nsIHttpServer.idl

>    * @returns
>    *   the field value for the given header; note that this value may be
>    *   normalized (e.g., leading/trailing whitespace removed from the value [or
>+   *   from the values which make this up, if the header is a comma- or '\n'-
>+   *   separated list of values] whitespace runs compressed to single spaces,

Missing comma after the ].

>+   *   etc.), the result is '\n'-separated list for 3 following header names:
>+   *        WWW-Authenticate
>+   *        Proxy-Authenticate
>+   *        Set-Cookie 
>+   *   otherwise is comma-separated

"The result is a string containing the individual values of the header, usually separated with a comma.  The headers WWW-Authenticate, Proxy-Authenticate, and Set-Cookie violate the HTTP specification, however, and for these headers only the separator string is '\n'." ?


>diff --git a/netwerk/test/httpserver/test/data/sjs/headerArray.sjs b/netwerk/test/httpserver/test/data/sjs/headerArray.sjs

You don't need an SJS for this, just put it directly in the .js test file and use registerPathHandler for the particular path.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> (From update of attachment 360150 [details] [diff] [review])
> Join with "\r\n", please; it seems like a more correct joiner given that line
> endings in HTTP are CRLF.  Also note this in the @returns section, please.
> 

I am not sure I agree. Necko joins with \n in this case. It is not split to put this as output to request or responseand be in any way conforming the http conventions. It is split with \n to allow parsing the response headers separatly in a code. See http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpHeaderArray.cpp#74 and http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#3090.

If you really really want crlf then I'll do it that way, but please give me an argument.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Please see comment 4 why I don't fix the crlf objection yet.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 361642 [details] [diff] [review]
v2.1

Fine, doesn't really matter that much if it's '\n' or '\r\n'.


>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js

>+    // Following three headers are kept as array elements because
>+    // violate HTTP spec and are sent as separate headers.
>+    // See http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/netwerk/protocol/http/src/nsHttpHeaderArray.cpp#l77

This has some sentence fragments and grammatical errors.  How about:

The following three headers are stored as arrays because their real-world syntax prevents joining individual headers into a single header using ",".  See also <http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/netwerk/protocol/http/src/nsHttpHeaderArray.cpp#l77>.


>     if (merge && name in this._headers)
>-      this._headers[name] = this._headers[name] + "," + value;
>-    else
>-      this._headers[name] = value;
>+    {
>+      if (name == "www-authenticate" ||
>+          name == "proxy-authenticate" ||

Make all three of these === to avoid ever having to think about types when reading the code; no semantic difference, but I prefer strict-equality checks to loose-equality, in general.


>+          name == "set-cookie") {
>+      else {

Brace on next line, please.


>+        this._headers[name][0] += "," + value;

Please add

NS_ASSERT(this._headers[name].length === 1,
          "how'd a non-special header have multiple values?")

since we rely on that here.


>   /**
>    * Returns the value for the header specified by this.
>    *
>    * @throws NS_ERROR_INVALID_ARG
>    *   if fieldName does not constitute a valid header field name
>    * @throws NS_ERROR_NOT_AVAILABLE
>    *   if the given header does not exist in this
>    * @returns string
>    *   the field value for the given header, possibly with non-semantic changes
>    *   (i.e., leading/trailing whitespace stripped, whitespace runs replaced
>    *   with spaces, etc.) at the option of the implementation
>    */
>   getHeader: function(fieldName)

Add "; multiple instances of the header will be combined with a comma, except for the three headers noted in the description of getHeaderValues" to the end of the @returns section.


>+  /**
>+   * Returns the value for the header specified by fieldName
>+   * as unflatted array.

Just "as an array" here, please (and for future reference, "unflattened").


>diff --git a/netwerk/test/httpserver/test/test_header_array.js b/netwerk/test/httpserver/test/test_header_array.js

>+// test special headers are sent as an array of hreaders with the same name

"test that special" and "array of headers"


>+var srv;

Make this a local variable in run_test.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v2.1 + r comments

Comment 8

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/mozilla-central/rev/d9068a1a0648

Comment 9

[Jeff Walden [:Waldo]]

I didn't notice it when reviewing, but |for (index in values)| is a no-no because it fails if Array.prototype has any new methods on it, and perhaps more importantly |index| isn't defined so it's treated as global.  I fixed both of these in this followup push:

http://hg.mozilla.org/mozilla-central/rev/80284f994cc0

I don't see a reason not to get this on branch as well; assuming that happens the followup fix needs to make it in at the same time.

Comment 10

[Honza Bambas (:mayhemer)]

I'll create a merged patch and land in it on 1.9.1.

Comment 11

[Honza Bambas (:mayhemer)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0a4ff08f8bf3

Comment 12

[Jeff Walden [:Waldo]]

Erm, that's missing comment 9, isn't it?  A diff between the two files in the different repos says it is, at least.

Comment 13

[Honza Bambas (:mayhemer)]

I know, I forget to add it there. However I have a patch ready, I will land it with next bunch of patches if you are not against.

Comment 14

[Jeff Walden [:Waldo]]

Okay, works for me -- just want to keep things synced so that bug 396226 can land both places -- because I know people will want to use it everywhere as soon as it exists, and what it would allow is going to be difficult for nightly testers to exercise in all the ways a good test would.

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to comment #14)
> Okay, works for me -- just want to keep things synced so that bug 396226 can
> land both places -- because I know people will want to use it everywhere as
> soon as it exists, and what it would allow is going to be difficult for nightly
> testers to exercise in all the ways a good test would.

Rather do it now: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9673649decc5