1421725 - finalize how changing headers should work

Status: RESOLVED DUPLICATE of bug 1462989

(WebExtensions :: Request Handling, enhancement, P3)

Description

[Shane Caraveo (:mixedpuppy)]

While reviewing bug 1377689 I see that we're incorrectly documenting how header changes work in webrequest.  There's also a distinct difference between Chrome and Firefox.

Chrome says[1] only one extension can change the headers.

MDN[2] currently says each subsequent listener will see prior changes and can make more changes, which is not what we do at this point, each overwrites the prior now.  

We should fix this, the question is what is the best fix.

- Just fix the documentation, and note the chrome incompatibility.  That incompatibility would be; when changing headers:
   - first extension listener fired on chrome wins (I think)
   - last extension on firefox wins
- Fix firefox to match what MDN currently says.

I prefer the latter, but it may not be possible.  Firefox used to work this way, but changes to support OOP changed how this works.  

The reason I prefer it is that it makes sense to me that more than one extension should be able to add cookies, or differing headers.  If they change/touch the same header/cookie they would of course overwrite prior changes.

[1] https://developer.chrome.com/extensions/webRequest#type-HttpHeaders
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onHeadersReceived

Comment 1

[Kris Maglione [:kmag]]

The old behavior is no longer possible. It would introduce too much lag, especially with asynchronous listeners.

I don't think there's anything more to do here other than fix the docs. There are no guarantees as to the order listeners will be fired in (and they'll generally fire in parallel when there are multiple extension processes), so there will always be a dice roll when there are conflicts. And I expect our conflict handling to change over time, given that there are no guarantees and therefore no expectations to break.

Comment 2

[Kris Maglione [:kmag]]

Also, frankly, adding cookies is not really a good use case for this. Extensions can already do that without risk of conflict using the cookies API.

Comment 3

[kkapsner]

A good use case is to add CSP headers (Example WebExtensions that do that: uBlock Origin, NoScript, CanvasBlocker).

What about adding an additional field to the header definition like "forceMerge" which then can be used in applyChanges (https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#101) to force the merge flag:

    for (let {name, value, binaryValue, forceMerge} of headers) {
      if (binaryValue) {
        value = String.fromCharCode(...binaryValue);
      }

      let lowerCaseName = name.toLowerCase();
      let original = origHeaders.get(lowerCaseName);

      if (!original || value !== original.value) {
        let shouldMerge = forceMerge || headersAlreadySet.has(lowerCaseName);
        this.setHeader(name, value, shouldMerge);
      }

      headersAlreadySet.add(lowerCaseName);
    }

No performance penalty here and the extension would have the option to respect other extension changes. It would mean that all extension would have to work together but it's better than the current situation.

Comment 4

[Steph]

I am not sure that the importance of fixing this is well assessed. Just in case, I'm adding a real world case and needinfoing you, Kris, since that seems to be your domain.

 

Setup steps:

1/ Create a new Firefox profile
2/ Install uMatrix and make it block JavaScript on all sites (with rule "* * script block")
3/ Install uBlock Origin and turn on font blocking in the parameters
4/ Visit any website where inline JavaScript does something visible, like this one

 

Expected result: Inline JavaScript is blocked. In the example page, that means text will be missing a word (at position _) and say "Cookies are _ in this browser"

Actual result: Inline JS is executed. In the page I linked to, that means text will display "Cookies are enabled in this browser". If you don't get this result, try disabling and re-enabling uBO, then reload the page. The last add-on to be enabled appears to win the HTTP headers modifier game.

 

5/ Turn off font blocking in uBO. Chances are that no filter triggered by this website modifies CSP headers, meaning uMatrix's inline JS blocking will not be discarded
6/ Reload page and see that inline JS is indeed blocked.

 

7/ Consider creating a CSP filter in uBO that applies to the test page. It should win over uMatrix and let inline JS be executed against the user's will. (Not tested, but likely.) There are quite a few such filters in popular content blocking lists.

 

I hope you can see how JavaScript executing on websites where users think they are safe is a rather critical issue exposing them to privacy and security risks they explicitly want to be protected against. The software is not working as expected, and users discover it the hard way: It has been talked about on Reddit, Github, Ghacks and so on, by different users whose privacy or security extensions failed to work as advertised but in ways that were hard to notice, meaning they had been unknowingly exposed ever since Firefox 57.

In light of this, would you mind bumping the priority of this bug ? Maybe the suggestion from comment #3 solves issues ?

For the record, this should be related to this comment and the following few.

Thanks

Comment 5

[Steph]

For point 7, I confirmed that it does make uMatrix fail. You can try with uBO filter $csp=img-src 'none' *,domain=whatarecookies.com

It's quite unpredictable which add-on feature is going to make another critical add-on feature fail. It means there is a high incentive not to install any add-on but the bare minimum.

Comment 6

[tartpvule]

Please raise the priority of this bug, uBlock and uMatrix failing unpredictably is quite a security risk.

If returning to the old behavior of running all listeners sequentially is bad performance-wise, is it possible to only run "blocking" listeners sequentially while leaving other observe-only listeners with the current behavior? This would make the listeners that can modify things aware of each other's changes, while still running the other listeners in parallel.
Might this be acceptable?

Thank you

Comment 7

[Keith Hizal]

This bug breaks many extensions without the user knowing it. With the way things are right now, one cannot do things like use uBlock Origin while also enabling HTTPS Everywhere's EASE functionality.

https://github.com/ghacksuserjs/ghacks-user.js/issues/664 offers more examples.

Comment 8

[bugzee]

nothing of relevance i can add here, but i felt compelled to comment and urge that this be addressed ASAP - this is indeed a very serious security and privacy issue

Comment 9

[joesph66]

Yep. Time to fix it.

Comment 10

[kriss.free]

+1 to fix that bug. Serious enough for all and especially TOR users.

Comment 11

[tartpvule]

Attachment: Run and apply changes from blocking listeners sequentially

I have created an experimental, not yet extensively tested patch.

1. Lifts runChannelListener and applyChanges from HttpObserverManager in WebRequest.jsm into a new class for easier context management.
2. For blocking listeners, apply changes sequentially. Allows blocking listeners to see prior modifications, like what MDN says.
   First time contributing a patch here, not quite sure what to do, comments appreciated.

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to tartpvule from comment #11)

Created attachment 9074965 [details] [diff] [review]

2. For blocking listeners, apply changes sequentially. Allows blocking listeners to see prior modifications, like what MDN says.
   First time contributing a patch here, not quite sure what to do, comments appreciated.

We used to do that, it was removed for performance reasons (see comment 1). We wouldn't re-introduce this. We should rather explore ideas on how to deal with the conflict issue.

Comment 13

[Keith Hizal]

Any update on this? This has been open for a while and is of great importance to many users, including myself.

I'm going to copy and paste what I posted on a related bug report, just to clarify why this is important and how it can break things.

I'd say most users have no idea what CSP is, and would not know why their extensions are failing.

The important thing here is that to the end user, there is zero indication that only one CSP extension is allowed. Extension developers shouldn't have to put a big fat warning that that indicates every feature that uses CSP. The end user shouldn't have to search online to find out why their extensions don't work as intended. And that's if they realize the extension doesn't work in the first place.

For an example use case, I have three extensions that modify CSP, uBlock Origin, HTTPS Everywhere, and CanvasBlocker. If I want uBlock Origin at its full potential, I can't use EASE from HTTPS Everywhere, nor can I have the "Block data URL page" option checked in CanvasBlocker, which is hidden under the "Expert Settings" options, and is enabled by default. And it was only until I tried enabling EASE that I realized that things weren't working right. There was zero indication that uBlock Origin and CanvasBlocker conflicted with each other.

Some WebExtensions will conflict with each other. Having two tab hiding extensions is obviously going to cause a conflict. But uBo, HTTPS Everywhere, and CanvasBlocker serve three completely different purposes. It's illogical for them to have any sort of conflict. At the end of the day, the way things are right now is completely user hostile.

Either all extensions should be able to modify CSP at the same time, or none of them should be able to modify CSP at all.

Comment 14

[jscher2000]

When I experimented with the two extensions referenced in comment 4, I got different results depending on which was disabled/enabled most recently. (Disabling and re-enabling uMatrix resolved the issue of scripts not being blocked when fonts are blocked in uBlock Origin.)

Since it's not clear how to explain which extension wins, I updated the obsolete text in MDN in a more general way:

"Note that it is possible for extensions to conflict here. If two extensions listen to onHeadersReceived for the same request, and return responseHeaders attempting to set the same header (for example, Content-Security-Policy), only one of the changes will be successful."

Diff: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/onHeadersReceived$compare?locale=en-US&to=1574022&from=1475458

Meanwhile, y'all can sort out what should happen in these cases.

Comment 15

[tartpvule]

Just my two cents here, the only thing that makes sense in the long run is the previous behavior: make no guarantee about the order blocking listeners are run in, but let later listeners see prior modifications in sequence.
Aiming for a robust merging of header modifications in browser code is just asking for a hell of edge cases and nothing less than parsers for all current and future headers will be necessary. (If the level of functionality is to be preserved and not outright crippled)

As an example, imagine there is a site with a CSP header with media-src 'none'.
Extension A tries to add cat videos from www.cats.com, and so media-src www.cats.com
Extension B tries to add dog videos from www.dogs.com, and so media-src www.dogs.com
The browser must parse the values and do some magic to merge them or let one of the extension fail.
(More practical examples would be extensions trying to wrestle with the script-src directive)
What about new headers in the future? Feature-Policy? Must the browser be coded to understand them all?

From my daily usages, sequentially running and applying changes for blocking listeners has no noticable impact on performance at all, as far more time is spent waiting for the network/disk I/O and running unnecessarily big script files from sites like a certain spying social media anyway.

P.S. It should go without saying that the website author's wishes should always be less important than extension authors' wishes. Please don't go the way of Chromium and allow websites to do things without giving users (and extension authors) control over them.

Thank you