623836 - Misc header improvements

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Some of Sync's code expects it:

  // Note: Header names should be all lower case, there's no explicit
  // check for duplicates due to case!

and some of it misbehaves:

      if (key == 'Authorization')
        this._log.trace("HTTP Header " + key + ": ***** (suppressed)");

Comment 1

[Richard Newman [:rnewman]]

Morphing this bug to be a little more general...

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 0: remove unused multi-value support in Resource.setHeader.

Description says it all.

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523193 [details] [diff] [review]
Part 0: remove unused multi-value support in Resource.setHeader.

Yes. Yes! YES!

Comment 4

[Richard Newman [:rnewman]]

Pushed Part 0:

https://hg.mozilla.org/services/services-central/rev/6b5bee7bbb01

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 1: correct an inconsistency. v1

Resource's header machinery counts on callers who modify headers directly (not through setHeader) to do it with lowercase keys. The authenticators don't do this, which allows a situation such as

  res.setHeader("Authorization", "Basic foobar");
  res.authenticator = new BasicAuthenticator(...);
  res.headers ...    // Causes onRequest to be called.

res.headers now contains both "authorization" (downcased from the setHeader) and "Authorization" (from the authenticator). The latter will be printed specially by _createRequest, but which one actually applies in the request depends on the order of object traversal!

This hasn't been a problem because we don't add that header ourselves, but it makes the code confusing to have different parts of the same file totally ignoring comments elsewhere within it!

This patch adds a test to verify that the two methods correctly collide, and fixes the code.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 523637 [details] [diff] [review]
Part 1: correct an inconsistency. v1

>+  _("Test that the BasicAuthenticator doesn't screw up header case.");
>+  let res1 = new Resource("http://localhost:8080/foo");
>+  res1.setHeader("Authorization", "Basic foobar");
>+  res1.authenticator = new NoOpAuthenticator();
>+  do_check_eq(res1._headers["authorization"], "Basic foobar");
>+  do_check_eq(res1.headers["authorization"], "Basic foobar");
>+  let id = new Identity("secret", "guest", "guest");
>+  res1.authenticator = new BasicAuthenticator(id);
>+
>+  // In other words... it correctly overwrites our downcased version
>+  // when accessed through .headers.
>+  do_check_eq(res1._headers["authorization"], "Basic foobar");
>+  do_check_eq(res1.headers["authorization"], "Basic Z3Vlc3Q6Z3Vlc3Q=");
>+  do_check_eq(res1._headers["authorization"], "Basic Z3Vlc3Q6Z3Vlc3Q=");
>+  do_check_true(!res1._headers["Authorization"]);
>+  do_check_true(!res1.headers["Authorization"]);
>+

Please add those tests to test_async_resource.js too! r=me with that.

Comment 7

[Richard Newman [:rnewman]]

Pushed:

https://hg.mozilla.org/services/services-central/rev/7fa18567d4aa

Comment 8

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/6b5bee7bbb01
http://hg.mozilla.org/mozilla-central/rev/7fa18567d4aa