Closed Bug 1323036 Opened 3 years ago Closed 3 years ago

Preserve request header capitalization

Categories

(WebExtensions :: Request Handling, defect, P5)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: andy+bugzilla, Assigned: zombie)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(1 file)

In the example:

https://github.com/mdn/webextensions-examples/tree/master/user-agent-rewriter

The user-agent is changed on requests to http://useragentstring.com/. This works in Firefox 50.0.2. However in nightly (currently 53.0.a1). It no longer works. If you change line 27 here:

https://github.com/mdn/webextensions-examples/blob/master/user-agent-rewriter/background.js#L27

So that:

-    if (header.name == "User-Agent") {
+    if (header.name.toLowerCase() === "user-agent") {

It still doesn't seem to work. I think headers in Chrome are not lower case, for example "User-Agent".

Possibly this is a regression and the headers weren't meant to be lower cased for extensions developers?
Header names are case insensitive. Any add-on that depends on a specific capitalization is by definition brolen.
It looks like http://useragentstring.com/ is relying on a case sensitive header. If this is all by design, I'll change the example to point to a different site.
That's probably a good idea. It might be worth normalizing capitalization just to avoid those kinds of foot-guns, though.
Pull request for updating our example sent.
Summary: Changing request header not working → Normalize request header capitalization
Priority: -- → P5
Whiteboard: triaged
http://useragentswitcher.org/ extension also relies on the following code:
  if (n.requestHeaders[t].name === "User-Agent") {

Can we at least consider making chrome.webRequest APIs follow the same capitalisation of Chrome, granted headers are not case sensitive but this will likely increase support to other extensions (I haven't checked any others though).
Duplicate of this bug: 1349151
For the record the code that controls this is in:
  toolkit/modules/addons/WebRequest.jsm

http://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#127
and
http://searchfox.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#179

Added by this: https://reviewboard.mozilla.org/r/91630/diff/1#index_header

I'm not sure on how Chrome handles this in it's code though.
I don't think we should "normalize" the capitalization here, we just need to preserve the existing case of header names that Firefox already uses.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Summary: Normalize request header capitalization → Preserve request header capitalization
I'd rather we normalize.
As discussed over irc, I'm still going with preserving the case over normalization.

1) Normalization is an open-ended problem, not all headers follow A-Regular-Capitalization (like DNT)
2) The principle of least surprise also suggest we should just pass whatever Firefox is sending.  Otherwise, we could end up in a situation where DXR says Firefox sends one thing, we pass another thing to extensions, and the server still ends up receiving the original thing.
Attachment #8855097 - Flags: review?(mixedpuppy)
Comment on attachment 8855097 [details]
Bug 1323036 - Preserve case of header names

https://reviewboard.mozilla.org/r/126996/#review129740

r+ given another test and no further non-test changes are needed.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html:340
(Diff revision 2)
> +  async function background() {
> +    // This is testing if header names preserve case,
> +    // so the case-sensitive comparison is on purpose.
> +    function ua({requestHeaders}) {
> +      for (const header of requestHeaders) {
> +        if (header.name === "User-Agent") {

Looks like it's handled right, but I'd like to see a test that blindly sets requestHeaders['user-agent']='foo' and ensure we're not setting the same header twice due to case sensitivity.
Attachment #8855097 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> Looks like it's handled right, but I'd like to see a test that blindly sets
> requestHeaders['user-agent']='foo' and ensure we're not setting the same
> header twice due to case sensitivity.

requestHeaders is an array of {name, value} objects, so it was already possible to do this, and although we do call setHeader() multiple times, only the last value passed counts.  

Asking for r? again, in case I misunderstood your comment.
Comment on attachment 8855097 [details]
Bug 1323036 - Preserve case of header names

Added requested test.
Attachment #8855097 - Flags: review+ → review?(mixedpuppy)
We need to be sure to document this, so here's my attempt at a short description.

Headers set by Firefox and other addons are passed through "as-is".  Depending on the header, it may be camel case (e.g. User-Agent) or it may be lower case.  While the specification is that headers are not case sensitive, various consumers looking at headers may expect camel case.    Even headers modified by an addon will still preserve the original case of the header name.  If and addon wants to catch either, it will need to lowercase the header name prior to any comparison (e.g. header.name.toLowerCase()).
Keywords: dev-doc-needed
Comment on attachment 8855097 [details]
Bug 1323036 - Preserve case of header names

https://reviewboard.mozilla.org/r/126996/#review130072

r+
Attachment #8855097 - Flags: review?(mixedpuppy) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c44e3665cabf
Preserve case of header names r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c44e3665cabf
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I've attempted to explain this at the end of the into section here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeSendHeaders.

Please let me know if this covers it.
Flags: needinfo?(mixedpuppy)
lgtm
Flags: needinfo?(mixedpuppy)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.