1232849 - Support HTTP request/response headers manipulation in the WebRequest API

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Brian Grinstead [:bgrins]]

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

Comment 1

[Giorgio Maone [:ma1]]

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.

Comment 2

[Kris Maglione [:kmag]]

It sounds good to me. I think we need to make sure to clear headers that were omitted from the new list too, though.

Comment 3

[Giorgio Maone [:ma1]]

(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.

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I didn't realize I was still needinfo'ed. I agree, this sounds good.

Comment 6

[Giorgio Maone [:ma1]]

Attachment: 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/

Comment 7

[Giorgio Maone [:ma1]]

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/

Comment 9

[Kris Maglione [:kmag]]

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.

Comment 10

[Giorgio Maone [:ma1]]

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 11

[Kris Maglione [:kmag]]

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"?

Comment 12

[Giorgio Maone [:ma1]]

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/

Comment 13

[Giorgio Maone [:ma1]]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b972f49b8c7e

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b972f49b8c7e

Comment 16

[Vasilica Tamas, QA [:vtamas]]

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?

Comment 18

[armen.michaeli]

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

[Kris Maglione [:kmag]]

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

[armen.michaeli]

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)

Comment 22

[Will Bamberg [:wbamberg]]

(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.