The default bug view has changed. See this FAQ.

Support HTTP request/response headers manipulation in the WebRequest API

VERIFIED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: bgrins, Assigned: mao)

Tracking

(Blocks: 4 bugs, {dev-doc-complete})

unspecified
mozilla47
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox47 fixed, firefox48 verified)

Details

(Whiteboard: [webRequest] triaged)

User Story

The ability of inserting, deleting and modifying HTTP request and response headers in WebExtensions' WebRequest implementation was drafted, but actually incomplete, broken and untested. Here we're fixing it and adding tests.

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Blake helped convert a web extension meant for testing CSP: https://github.com/yandex/csp-tester to: https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/csp-tester-master.xpi

STR:
Install the xpi file above (or build it somehow from the source)
Open https://github.com/
Open the "CSP" popup from the toolbar, then:
* enter https://github.com/ as the URL pattern
* enter "none" in the "img-src" field
* tick the 'active' box
* press "save"
Reload the page

Expected:
Images don't load

Actual:
I see this in browser console: NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIHttpChannel.setResponseHeader] at resource://gre/modules/WebRequest.jsm:323

I have no idea if this is something weird the extension is doing or what.

I've traced it down far enough to see that the header that's attempted to be set is 'Content-Type' here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#323
(Assignee)

Comment 1

a year ago
The root cause is that Necko just doesn't allow to change the response content-type header (and a bunch of other headers as well):
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1623

On the other hand, this extension doesn't *intentionally* try to change it, but rather follows the common pattern to modify request and response headers -- suggested in https://developer.chrome.com/extensions/webRequest#examples -- i.e. modifying in place the headers array received by the callback and returning it. This makes sense because the returned headers are meant to replace the original list as a whole, hence even if you want to modify just one header you must return them all.

Therefore this issue is actually our fault, because we pass the callback *all* the headers our visitor grabs from the channel, including those which are not meant to be modified (and rightly so, because they might be worth to be examined, instead).

Furthermore, the Chrome API is not supposed to throw even if it doesn't manage or isn't allowed to set a certain header.

All that considered, I propose to fix this bug by wrapping our set(Response|Request)Header() calls inside a try {} catch() {} block: we might log the failures for debugging purposes, but never throw.

Additionally as an optimization, we should avoid to erase and write back those headers which have been left unchanged in the array (this alone would have worked around the problem at hand).

Bill, Kris, do you agree? If you do I'm gonna grab this and fix it.
Assignee: nobody → g.maone
Status: NEW → ASSIGNED
Flags: needinfo?(wmccloskey)
Flags: needinfo?(kmaglione+bmo)
Summary: Error when setting Content-Type header in CSP addon - Component returned failure code NS_ERROR_ILLEGAL_VALUE [nsIHttpChannel.setResponseHeader] → Setting (response|request)Headers in webRequest should never throw
Whiteboard: [webRequest]
It sounds good to me. I think we need to make sure to clear headers that were omitted from the new list too, though.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 3

a year ago
(In reply to Kris Maglione [:kmag] from comment #2)
> It sounds good to me. I think we need to make sure to clear headers that
> were omitted from the new list too, though.

We already do, as far as I can see (that's where we throw in this bug). However I'm gonna double check and ensure this is tested as well.
We do now, yes, but if you're going to try to avoid setting unchanged headers, we need to make sure to still clear the ones that are omitted.
Sorry, I didn't realize I was still needinfo'ed. I agree, this sounds good.
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

a year ago
Iteration: --- → 47.1 - Feb 8

Updated

a year ago
Flags: blocking-webextensions?
Priority: -- → P2
Whiteboard: [webRequest] → [webRequest] triaged

Updated

a year ago
Flags: blocking-webextensions? → blocking-webextensions+
(Assignee)

Updated

a year ago
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
(Assignee)

Updated

a year ago
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7

Updated

a year ago
Blocks: 1253374
(Assignee)

Comment 6

a year ago
Created attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

Review commit: https://reviewboard.mozilla.org/r/38027/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38027/
(Assignee)

Updated

a year ago
Attachment #8726478 - Flags: review?(wmccloskey)
(Assignee)

Updated

a year ago
Attachment #8726478 - Attachment description: MozReview Request: Bug 1232849 - Wrap headers manipulations in try...catch blocks and perform them in tests. → MozReview Request: Bug 1232849 - Wrap headers manipulations in try...catch blocks and test them, eslint-happy
(Assignee)

Comment 7

a year ago
Comment on attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38027/diff/1-2/
(Assignee)

Updated

a year ago
Duplicate of this bug: 1226691
(Assignee)

Updated

a year ago
Attachment #8726478 - Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Attachment #8726478 - Flags: review?(kmaglione+bmo)
Comment on attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

https://reviewboard.mozilla.org/r/38027/#review34749

This looks good, aside from a few nits (and an issue or two for follow-ups), but I think we need a few more tests before it lands.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:145
(Diff revision 2)
> +    if (!headers) return;

Please move the `return` to its own line, and always use curly brackets (ESLint won't accept this as-is).

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:146
(Diff revision 2)
> +    headers.push({name: "X-Fake-Header", value: "some value"});

Can we make sure that changing and removing headers also works?

Also, I think we need to check that the headers are actually changed. It doesn't look like we do that anywhere, at the moment. Maybe just add a .sjs script to respond with its recieved headers as a JSON object?

::: toolkit/modules/addons/WebRequest.jsm:353
(Diff revision 2)
> +    for (let {name, value} of headers) {

Shouldn't we also handle `binaryValue`?

Also, what if the extension doesn't return an array, or one of the entries isn't the expected type of object?

I guess those are problems for a different bug, though.

::: toolkit/modules/addons/WebRequest.jsm:429
(Diff revision 2)
> -          requestHeaders = this.getHeaders(channel, "visitRequestHeaders");
> +          data.requestHeaders = this.getHeaders(channel, "visitRequestHeaders")

Please rewrite this to avoid the side-effect in the argument. It shouldn't be any less readable as:

        data.requestHeaders = this.getHeaders(channel, "visitRequestHeaders");
        requestHeaderNames = data.requestHeaders.map(h => h.name);

The same goes for response headers.

::: toolkit/modules/addons/WebRequest.jsm:460
(Diff revision 2)
> -        // Start by clearing everything.
> +        this.replaceHeaders(result.requestHeaders, requestHeaderNames,

Please either align the last argument with the opening `(` or move the first argument to the next line.

::: toolkit/modules/addons/WebRequest.jsm:461
(Diff revision 2)
> -        for (let {name} of requestHeaders) {
> +          (name, value) => channel.setRequestHeader(name, value, ""));

The third argument to `setRequestHeader` and `setResponseHeader` should be a boolean.
(Assignee)

Updated

a year ago
Attachment #8726478 - Attachment description: MozReview Request: Bug 1232849 - Wrap headers manipulations in try...catch blocks and test them, eslint-happy → MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests
Attachment #8726478 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

a year ago
Comment on attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38027/diff/2-3/
Comment on attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

https://reviewboard.mozilla.org/r/38027/#review34947

This looks great. Thanks!

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:185
(Diff revisions 2 - 3)
> +        header.binaryValue = Array.map(added[name], c => c.charCodeAt(0));

`Array.map` is deprecated. Please use `Array.from`.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:200
(Diff revisions 2 - 3)
> +    headers.forEach((h, j) => {

`headers = headers.filter(...` might be better. `forEach` has undefined behavior when the array is modified during execution.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:221
(Diff revisions 2 - 3)
> +      browser.test.assertTrue(headers.find(h => h.name === name && h.value === added[name]), `header ${name} correctly injected in ${phase}Headers`);

I think `headers.some` makes more sense here.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:337
(Diff revisions 2 - 3)
> +    if (!(kind in completedUrls)) completedUrls[kind] = new Set();

`if` statements always need braces and newlines for their bodies (ESLint will reject this).

::: toolkit/modules/addons/WebRequest.jsm:350
(Diff revisions 2 - 3)
> -        // We can safely be quiet here.
> +        // Let's collect phisiological failures in order

Do you mean "physiological"?
Attachment #8726478 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 12

a year ago
Comment on attachment 8726478 [details]
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38027/diff/3-4/
Attachment #8726478 - Attachment description: MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests → MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits
(Assignee)

Comment 13

a year ago
https://reviewboard.mozilla.org/r/38027/#review34947

> `headers = headers.filter(...` might be better. `forEach` has undefined behavior when the array is modified during execution.

A traditional for loop is probably the best, because filter() wouldn't change the array in place, making the caller unhappy.

Comment 14

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b972f49b8c7e
(Assignee)

Updated

a year ago
User Story: (updated)
Summary: Setting (response|request)Headers in webRequest should never throw → Support HTTP request/response headers manipulation in the WebRequest API

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b972f49b8c7e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Keywords: dev-doc-needed
I was able to reproduce this issue on Firefox 46.0a1 (2015-12-15) under Windows 10 64-bit.
Verified fixed using https://addons.allizom.org/en-US/firefox/addon/testfor1232849/ on Firefox 48.0a1 (2016-03-29) under Windows 10 64-bit, Mac OS X 10.11 and Ubuntu 12.04 32-bit. The Browser Console error does no longer appear while reloading the page.

I was unable to test this bug on Firefox 47.0a2 (2016-03-29) because the webextension seems to be corrupted under this Firefox version even with the xpinstall.signatures.dev-root pref set to true. Does anyone know why?
Status: RESOLVED → VERIFIED
status-firefox48: --- → verified

Updated

11 months ago
Duplicate of this bug: 1266813

Comment 18

11 months ago
I am on [vanilla] Firefox 46.0.1 on Windows 10 x86_64, and I am getting the following error message in my Browser Console when debugging my Web Extension code:

NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIHttpChannel.setResponseHeader]

I am dealing specifically with the `browser.webRequest.onHeadersReceived` event emitter, attempting to filter the array of response headers and return the modified (filtered) array, in effect filtering certain host (response) headers from making their way further along the processing chain. It's part of an addon that applies rules on "incoming" cookies. Anyway, here is example of code prompting the error message above:

    browser.webRequest.onHeadersReceived.addListener(function(details) {
	for(var i = 0; i < details.responseHeaders.length; i++) {
	    if(details.responseHeaders[i].name == "Set-Cookie") {
	        details.responseHeaders.splice(i, i);
            } 
        }
	return details;
    }, { "urls": [ "<all_urls>" ] }, [ "blocking", "responseHeaders" ]);

I tried other variants, hoping that certain strategies like modifying the array in-place or returning same [modified] object, are what causes the failure. I tried `Array.prototype.filter`, applying a more functional stance for filtering an array, and I also tried `return { "responseHeaders": details.responseHeaders }` and `return { "responseHeaders": filtered_headers_copy() }`, but all give me the same error above.

Comment 19

11 months ago
This fix didn't land until Firefox 47. If you're still having problems in the latest beta or developer edition, please let us know.

Comment 20

11 months ago
I can now confirm with Firefox Developer Edition 48.0a2 (2016-05-13) that the aforemtioned is no longer an issue. Response headers are also observed modified, as intended. Would be great if the bug was referenced from the MDN pages documenting webRequest.onHeadersReceived (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebRequest/onHeadersReceived)

Updated

11 months ago
Duplicate of this bug: 1269311
(In reply to armen.michaeli from comment #20)
> I can now confirm with Firefox Developer Edition 48.0a2 (2016-05-13) that
> the aforemtioned is no longer an issue. Response headers are also observed
> modified, as intended. Would be great if the bug was referenced from the MDN
> pages documenting webRequest.onHeadersReceived
> (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebRequest/
> onHeadersReceived)

I've noted this in the compat table for onHeadersReceived:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onHeadersReceived#Browser_compatibility

I think, with that, this bug is dev-doc-complete, but please let me know if you want anything else.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.