Closed Bug 623836 Opened 9 years ago Closed 9 years ago

Misc header improvements

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: rnewman, Assigned: rnewman)

Details

(Whiteboard: [qa-])

Attachments

(2 files)

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)");
Morphing this bug to be a little more general...
Summary: Downcase HTTP header names → Misc header improvements
Description says it all.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #523193 - Flags: review?(philipp)
Whiteboard: [has patch][needs review philikon]
Comment on attachment 523193 [details] [diff] [review]
Part 0: remove unused multi-value support in Resource.setHeader.

Yes. Yes! YES!
Attachment #523193 - Flags: review?(philipp) → review+
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.
Attachment #523637 - Flags: review?(philipp)
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.
Attachment #523637 - Flags: review?(philipp) → review+
Whiteboard: [has patch][needs review philikon] → [has patch]
Pushed:

https://hg.mozilla.org/services/services-central/rev/7fa18567d4aa
Whiteboard: [has patch] → [fixed in services]
Whiteboard: [fixed in services] → [fixed in services][qa-]
https://hg.mozilla.org/mozilla-central/rev/6b5bee7bbb01
http://hg.mozilla.org/mozilla-central/rev/7fa18567d4aa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → mozilla5
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.