Open Bug 1421725 Opened 2 years ago Updated 6 days ago

finalize how changing headers should work

Categories

(WebExtensions :: Request Handling, enhancement, P3)

58 Branch
enhancement

Tracking

(Not tracked)

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

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
See Also: → 1377689
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.
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.
Priority: -- → P3
Assignee: nobody → mixedpuppy
Product: Toolkit → WebExtensions
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.
See Also: → 1485526

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

Flags: needinfo?(kmaglione+bmo)

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.

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

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.

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.

(In reply to tartpvule from comment #11)

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

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

Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.