Last Comment Bug 1232849 - Support HTTP request/response headers manipulation in the WebRequest API
: Support HTTP request/response headers manipulation in the WebRequest API
Status: VERIFIED FIXED
[webRequest] triaged
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: WebExtensions: Untriaged (show other bugs)
: unspecified
: Unspecified Unspecified
P2 normal with 3 votes (vote)
: mozilla47
Assigned To: Giorgio Maone [:mao]
:
: Andy McKay [:andym]
Mentors:
: 1226691 1266813 1269311 (view as bug list)
Depends on:
Blocks: webextensions-chrome-gaps webRequest-full webext-port-noscript webext-port-blackmenu
  Show dependency treegraph
 
Reported: 2015-12-15 16:12 PST by (Unavailable until Apr 3) [:bgrins]
Modified: 2016-05-17 15:37 PDT (History)
15 users (show)
sescalante: blocking‑webextensions+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 47.3 - Mar 7
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
MozReview Request: Bug 1232849 - Better chrome compatibility + binaryValue support + serious header manipulation tests + nits (58 bytes, text/x-review-board-request)
2016-03-03 16:27 PST, Giorgio Maone [:mao]
kmaglione+bmo: review+
Details | Review

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.      
Description User image (Unavailable until Apr 3) [:bgrins] 2015-12-15 16:12:36 PST
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 User image Giorgio Maone [:mao] 2015-12-16 10:03:35 PST
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 User image Kris Maglione [:kmag] 2015-12-16 13:04:30 PST
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 User image Giorgio Maone [:mao] 2015-12-16 13:08:52 PST
(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 User image Kris Maglione [:kmag] 2015-12-16 13:19:13 PST
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 User image Bill McCloskey (:billm) 2016-01-11 16:24:05 PST
Sorry, I didn't realize I was still needinfo'ed. I agree, this sounds good.
Comment 6 User image Giorgio Maone [:mao] 2016-03-03 16:27:20 PST
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/
Comment 7 User image Giorgio Maone [:mao] 2016-03-04 08:11:04 PST
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 8 User image Giorgio Maone [:mao] 2016-03-04 09:01:37 PST
*** Bug 1226691 has been marked as a duplicate of this bug. ***
Comment 9 User image Kris Maglione [:kmag] 2016-03-04 12:29:03 PST
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 User image Giorgio Maone [:mao] 2016-03-05 17:34:40 PST
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 User image Kris Maglione [:kmag] 2016-03-05 20:22:33 PST
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 User image Giorgio Maone [:mao] 2016-03-06 00:47:04 PST
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 User image Giorgio Maone [:mao] 2016-03-06 00:57:28 PST
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 15 User image Ryan VanderMeulen [:RyanVM] 2016-03-06 12:06:25 PST
https://hg.mozilla.org/mozilla-central/rev/b972f49b8c7e
Comment 16 User image Vasilica Mihasca, QA [:vasilica_mihasca] 2016-04-01 01:00:35 PDT
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 17 User image Kris Maglione [:kmag] 2016-04-22 11:27:06 PDT
*** Bug 1266813 has been marked as a duplicate of this bug. ***
Comment 18 User image armen.michaeli 2016-05-13 10:28:59 PDT
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 User image Kris Maglione [:kmag] 2016-05-13 10:43:06 PDT
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 User image armen.michaeli 2016-05-13 11:03:47 PDT
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 21 User image :shell escalante 2016-05-16 09:46:46 PDT
*** Bug 1269311 has been marked as a duplicate of this bug. ***
Comment 22 User image Will Bamberg [:wbamberg] 2016-05-17 15:37:24 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.