Closed Bug 1462989 Opened 3 years ago Closed 7 months ago

support merging content-security-policy headers provided by multiple extensions

Categories

(WebExtensions :: Request Handling, enhancement, P3)

enhancement

Tracking

(firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: sscarl24, Assigned: t-mozbugs)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3435.0 Safari/537.36

Steps to reproduce:

Have two extensions which apply CSP policies for websites installed. For ex. uBlock Origin and uMatrix.

1) Add a CSP filter such as ||youtube.com^$csp=img-src 'none' in uBlock Origin under My Filters tab and save it.

2) In uMatrix, open the dashboard, browse to My Rules and add "no-workers: * true" and save it which will apply a CSP filter such as worker-src 'none' when browsed to any website.

3) Visit https://www.youtube.com/ and notice none of the images are blocked even though a CSP policy for img-src was set by uBlock Origin was set, but rather Firefox only applies uMatrix's worker-src CSP.

PS - This ONLY occurs in Firefox, while with Chromium, both CSP policies from both the extensions are applied.


It might be related to this - https://bugzilla.mozilla.org/show_bug.cgi?id=1377689



Actual results:

Images not blocked despite img-src CSP policy being set by uBlock Origin which can been seen from the console screenshot here - 

https://i.gyazo.com/862e806400876750681e71fda82c7eb7.png


Expected results:

Images should have been blocked because a CSP policy for images is being set by uBlock Origin along with uMatrix's worker-src CSP policy.
I can reproduce this on Firefox v61/62(Nightly.)
PS - This happens ONLY when there is no original CSP header in response.
Component: Untriaged → WebExtensions: Request Handling
Product: Firefox → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: If two CSP policies from two extensions are set, Firefox only applies one → Consider merging new mergable headers that didn't exist in the original request
Priority: -- → P2
Product: Toolkit → WebExtensions
Is this still being worked on ?
It is still in the queue, yes. But it's not under active development right now.
Which queue ?
It is not a literal queue; P2 means we will work on it after the critical things we're doing now (P1) are done.
New STR as old one is no longer relevant --



Add a CSP filter such as ||w3schools.com^$csp=default-src 'self' 'unsafe-inline' 'unsafe-eval' in uBlock Origin under My Filters tab and save it.

Install uMatrix from https://github.com/gorhill/uMatrix/releases/tag/1.3.17b0 by clicking on the file name uMatrix.firefox.signed.xpi. In uMatrix, open the dashboard, browse to My Rules and add "no-workers: * true" and save it which will apply a CSP filter such as worker-src 'none' when browsed to any website.

Visit https://www.w3schools.com/html/tryit.asp?filename=tryhtml5_webworker and open the uMatrix logger(via opening moz-extension://41cfb02c-c075-468a-b815-858fdbc2a15c/logger-ui.html in a new tab), now go back to the w3schools tab and click Start worker, the count will begin and check the logger tab we opened in a new tab before, the request appears but is not blocked, even though it should have been because of no-workers : * true rule which applies the worker-csp to block all workers.

This bug is really relevant for security as it allows to execute malicious javascript if it is disabled.

(In reply to Klaus Ethgen from comment #8)

This bug is really relevant for security as it allows to execute malicious javascript if it is disabled.

Sorry, I don't understand what you mean. Can you clarify?

(In reply to Kris Maglione [:kmag] from comment #9)

What Klaus means is that when two add-ons 'fight' for CSP policies, the less strict might win, and this can cause third party Javascript to be downloaded and executed.

This is a real problem I face regularly; µMatrix is overridden by some other add-on. This causes some Javascript to load, until µMatrix is dis- and then reenabled, putting it back on top of the priority list.

Unless the priority level gets bumped to P1, this bug is not getting fixed anytime soon.

This bug outright breaks extensions. I have several extensions that modify csp, but only one of them can go through, and I can't add any extension that requires it. The worst part about this is that the average user might not notice anything wrong, as this bug can be, for the lack of a better term, silent unless the extension is completely broken, like HTTPS Everywhere's EASE/Any addon that injects the upgrade mixed content header.

Type: defect → enhancement

@Kris Why is it tagged now as enhancement ? It's a bug, it's not something I requested to enhance Firefox.

It's not a bug. It works as documented and expected. Changing the expected behavior in cases like this is an enhancement.

So was breakage of extensions also expected ? Because this only happens in Firefox and not in Chromium and why was this considered expected in the first place if ultimately it breaks extensions ?

The same thing happens in Chromium, at least, if they still follow their documented behavior. If multiple extensions modify the same header, there is a conflict, and no matter how we try to resolve that conflict, it will upset someone. Chrome's documented behavior is "If more than one extension attempts to modify the request, the most recently installed extension wins and all others are ignored" though its actual behavior may differ.

The same thing happens in Chromium

AFAIK, the headers are merged to avoid this issue happening there. uBO inserts a CSP in Chromium the same way it does in Firefox, but yet headers are merged in Chromium and not in Firefox, this is why I call it a bug as the merging of headers is something I expected on the contrary.

Not sure what info you need from me here. We don't recommend installing additional extensions or messing with extensions settings (as that could have weird side-effects endangering your privacy) + EASE mode is off in HTTPS Everywhere.

Flags: needinfo?(gk)

Chrome's documented behavior is "If more than one extension attempts to modify the request, the most recently installed extension wins and all others are ignored"

That is a generic sentence. You ignored the more specific preceding sentence -- which actually explains why it works with Chromium:

Only one extension is allowed to redirect a request or modify a header at a time.

"A header".

NoScript, uBO, uMatrix, AdGuard, etc. are not modifying "a[n existing] header", they are pushing a new one, so there should be no such limitation with regard to mergeable headers (i.e CSP headers), which Firefox legacy perfectly supported. The hint that this is the key sentence to pay attention to is that merging CSP headers work fine in Chromium.

Of course it makes sense that the same existing, non-mergeable header can't be modified by two extensions, one would overwrite the other's change, but here extensions are injecting a new header (which happens to be a mergeable one), not modifying an existing one.

Also:

It works as documented and expected.

It does not even work as documented. The documentation says:

If two extensions listen to onHeadersReceived for the same request, then the second listener will see modifications made by the first listener

uBO can't see modifications made by other listeners. This would be better than the current situation, at least this would allow me to use an already existing code path in uBO to deal with platforms which do not support merging of headers -- but even the documentation is wrong, uBO is unable to see the CSP header injected by another extension.

Tails OS ships the Tor browser with both uBlock Origin and NoScript. There are over 300 CSP-injecting rules in uBO with default filter lists which may silently fail if there happens to be another extension injecting a CSP header, and that is not counting the JS/font switches. NoScript uses some trickery to ensure its CSP headers are injected, and the dismissal of the seriousness of the issue here suggest that all extensions which want their important CSP-tightening to be honored should do the same. This will risk users ending up with a broken browser but at least they will become aware of the incompatibility of some of their installed extensions and force them to make a choice.

The webRequest API has already been modified to accommodate for actually needs in the actual world, no "but Chromium documentation" was invoked as an obstacle to fix/enhance the API (ex: browser.webRequest.filterResponseData). Why not do the same for this critical shortcoming and provide an API to explicitly merge new (mergeable) headers given a request id?

(In reply to Kris Maglione [:kmag] from comment #16)

If multiple extensions modify the same header, there is a conflict, and no matter how we try to resolve that conflict, it will upset someone.

This cannot be "merging headers" problem, because merging already works!
There is no issue when response contains their own, original CSP header.
For example, here on bugzilla:

  • add ||bugzilla.mozilla.org^$csp=img-src 'none' to uBO Dashboard -> My filters
  • block scripts in µMartix (mark "script" cell red in popup) (µMarix uses CSP to block inline scripts)
  • open console and reload page

Result: img-src and script-src are in effect.

Problem is - shady websites, where we will benefit from this type of blocking most, don't use CSP.

Supporting multiple extensions to inject mergeable headers (with no need to see each others' injection, therefore coherently to current specs) and be accounted for by the underlying HTTP header merging algorithms, rather than all dropped except for the last one, wouldn't cause any conflict or performance issue I could think of, and wouldn't require any special new API IMHO.
CSP in particular can only be tightened, never relaxed, by subsequent headers, therefore I can't see any security issue either.
And this could allow me to remove those awful hacks allowing NoScript to be always the last to inject (and breaking everyone else).

If the ToR browser can unexpectedly execute Javascript, as Gorhill explained, and Giorgio's suggestion has no disadvantages except the implementation work required, that sounds like a good solution.

Would it then still be possible for extensions to remove/ignore CSP headers set by the page itself? (I believe the philosophy behind Firefox is that the browser should not let other parties (website owners) restrict what users can do without the possibility of an override of sorts?)

Hey folks, we're getting a lot of advocacy chatter on this bug, so I'm going to close comments on it for the time being. If you're following this bug, you'll receive notifications if it moves into active development.

Restrict Comments: true

Could you please reopen this for comments? Comment activity and dupes are a useful measure in detecting issues devs feel are important to address.

Flags: needinfo?(cneiman)
Priority: P2 → P3

Re-opening comments.

Flags: needinfo?(cneiman)
Restrict Comments: false

Why set priority to P3 ?

Also what addional info do you need ?

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.

Let's keep this discussion on point, shall we? I agree that this is important, but our complaining in such a way won't help.

Attached patch Bug1421725-mc.patch (obsolete) — Splinter Review

Less complaining, more doing!

I've ported tartvpule's out of tree patch for 68ESR to m-c, and I'd be happy to address any feedback that's coming in. I'm fairly certain this patch requires some changes before its accepted, but I don't know who should look at this next.

Attachment #9112605 - Flags: review?(mixedpuppy)
Comment on attachment 9112605 [details] [diff] [review]
Bug1421725-mc.patch

A brief look at this (on holiday), it seems that this is making the event calls into extensions happen synchronously.  I don't mean the actual call itself, but that it will call one extension at a time, waiting for the response prior to calling the next extension.  Any solution using that approach cant be accepted.
Attachment #9112605 - Flags: review?(mixedpuppy) → review-

A potential path forward is to change how we apply headers[1]. We currently remove any headers not in the response from the extension, then add any headers returned. CSP is allowed to have multiple headers[2], and the most restrictive settings are taken, so we simply shouldn't remove CSP headers. The logic would need to change to do that.

The issue with the above (never remove CSP headers) is, I'm unsure if this would break some important use case. From a certain position (of which I lean to) we shouldn't remove CSP ever. If it were to break something in extensions by doing so, we'd have to evaluate whether that is acceptable or not. I'm also unsure what Chrome does here, whether it would remove CSP headers in this case.

ni?robwu since he's quite up on how/why Chrome does things.

I realize this approach does not solve extension B being able to see what extension A has done, but that is unlikely to be achievable without making the calls synchronous, which wont happen (comment 35).

[1] https://searchfox.org/mozilla-central/rev/d69bff9bdb2dcf1788eec6232b21eb3eec342799/toolkit/components/extensions/webrequest/WebRequest.jsm#108

[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy#Multiple_content_security_policies

Flags: needinfo?(rob)

Essentially, I think the fix is to do something like changing this line[1] to be:

let newHeaders = new Set(["content-security-policy", ...headers.map(({ name }) => name.toLowerCase())]);

That should allow any extension to add a CSP header without removing those added by other extensions.

[1] https://searchfox.org/mozilla-central/rev/d69bff9bdb2dcf1788eec6232b21eb3eec342799/toolkit/components/extensions/webrequest/WebRequest.jsm#115

Thanks for looking at this!

waiting for the response prior to calling the next extension. Any solution using that approach cant be accepted.

The reason being performance? I don't believe it should have a massive impact (being linear to the number of extensions using webrequestblocking, which (imo) is a valid trade-off to those extensions just silently failing, considering most users probably don't have any such extensions installed). But OK, let's try a different approach.

simply shouldn't remove CSP headers. The logic would need to change to do that.
[...]
I realize this approach does not solve extension B being able to see what extension A has done

That would solve the main use case (blocking content for privacy and/or security) just fine. I guess that this might break extensions that want to relax CSP (I don't know if any such exist or what purpose those could serve).

Essentially, I think the fix is to do something like changing this line[1] to be:

that change alone does not fix it, but I assume you gave that as an example for the general direction to take.

I think I'll have a go at implementing the "never removing headers" idea; any pointers you can give on this?

(In reply to tobias from comment #38)

Thanks for looking at this!

waiting for the response prior to calling the next extension. Any solution using that approach cant be accepted.

The reason being performance? I don't believe it should have a massive impact

We used to do exactly this...and moved away from it for the impact it had.

simply shouldn't remove CSP headers. The logic would need to change to do that.
[...]
I realize this approach does not solve extension B being able to see what extension A has done

That would solve the main use case (blocking content for privacy and/or security) just fine. I guess that this might break extensions that want to relax CSP (I don't know if any such exist or what purpose those could serve).

Without a highly valuable use case, I don't care, I'd rather see only an ability to move to a more restrictive csp. Someone may disagree with some valid reason I'm not thinking of.

Essentially, I think the fix is to do something like changing this line[1] to be:

that change alone does not fix it, but I assume you gave that as an example for the general direction to take.

I think I'll have a go at implementing the "never removing headers" idea; any pointers you can give on this?

It should essentially do it, I haven't fully looked at the underlying code[1] to figure out if there is a reason it wouldn't.

[1] https://searchfox.org/mozilla-central/rev/d69bff9bdb2dcf1788eec6232b21eb3eec342799/netwerk/protocol/http/nsHttpHeaderArray.cpp#39

So I've prepared tree patches: one that just unconditionally merges (mergable) headers, another one that treats the CSP header as a special case, and a third that only merges headers if the new one is not the empty string.
That third case allows an extension to set a header to "" to delete it (that is and was unreliable when multiple extensions are installed, though).

I think the third patch, Bug1462989-allowdelete.patch, (with the "" exception) is the best.

This change would require the statement about extension conflicts on MDN2 to be changed to something like (replacing 2nd sentence):

If two extensions listen to onHeadersReceived for the same request, and return
responseHeaders attempting to set the same header (for example, Set-Cookie),
they will be merged, unless it is a singleton header (like e.g. Content-Type) or the new value is the empty string then only one of those changes will be successful.

or, for the special case patch, in between the 2nd and 3rd sentences:

An exception are multiple Content-Security-Policy headers, which will be
merged (so it is possible for multiple extensions to tighten CSP, but not to
loosen it).

Reading chrome's documentation on WebRequest1, it says:

Only one extension is allowed to redirect a request or modify a header at a
time. If more than one extension attempts to modify the request, the most
recently installed extension wins and all others are ignored. An extension is
not notified if its instruction to modify or redirect has been ignored.

That is what consistent to the current behaviour of Firefox. However, this is not what Chromium actually does: testing with uBlock Origin blocking Javascript and HTTPS everywhere in EASE-mode, both modifications are applied.

Attachment #9112605 - Attachment is obsolete: true

(In reply to Shane Caraveo (:mixedpuppy) from comment #36)

I realize this approach does not solve extension B being able to see what extension A has done, but that is unlikely to be achievable without making the calls synchronous, which wont happen (comment 35).

I personally have no use case for this -- and this has never been supported in Chromium either, so no extension ported from Chromium is going to expect such behavior.

I always found Chromium's way of dealing with header modification/addition/removal to be sub-optimal. Is this something that should be opportunistically improved while fixing the issue here?

How about extending the current API to allow for adding/modifying/removing headers? Similar to how filterResponseData extended the webRequest API, I feel this could be improved by adding a addHeader(), modifyHeader(), removeHeader() API methods. Wouldn't this approach also remove guess work from the extensions framework by having extensions tell exactly what is to be done with a specific header?

Just a note about CSP header removal: I understand removing existing CSP headers can be seen as controversial (and I'd prefer a world where we can do without it), but there's currently a quite compelling use case: since those headers get often cached (even when they're added by extensions, which may be useful in edge cases such as Session Restore, if it was reliable mechanism - which is not), when user interacts with the extension UI to change settings for a certain page resulting in a CSP relaxation and reloads the page (which is much better be done from cache, not just for performance but as an attempt to preserve form fields and stuff like that) we need to remove the cached header (NoScript currently uses the report-to CSP field to recognize "its own" CSPs) and replace it with a more permissive one or none.
Ideally there would be an API allowing us to recognize "our own" CSP headers and delete just those, but I can see how complex this would get.

That (and DNT) are the main reasons pushing me to manage CSP injection entirely DOM side from content scripts, the main blocker for that being a reliable and not hackish way to synchronously configure content scripts, not just by URL matching (like in the current contentScripts.register() API) but by tab and frame hierarchy as well (the same API could be recycled with extra options).

Chromium's conflict resolution logic for response headers is as follows (similar for request headers):

  1. Dispatch webRequest.onHeadersReceived to all interested extensions.
  2. Wait until every event handler has returned.
    1. For every event handler result, compare the original headers with the returned headers and identify the removed headers and the new headers. Modifying a header value = remove + add. (Chromium source: CalculateOnHeadersReceivedDelta).
  3. Iterate over the return values (in the order of extension installation: from newest to oldest) and change the header as follows (Chromium source: MergeOnHeadersReceivedResponses).
    1. If a removed header has been removed before (in step 3ii at a previous iteration, usually a different extension but potentially the same extension!),
      or if an added header was removed by the declarativeNetRequest API,
      fire the webRequest.onActionIgnored event (this was introduced in crbug.com/111700),
      and skip the next steps (i.e. do not apply the header modifications).
    2. Apply header modifications:
      • Remove header lines if needed.
      • Append new headers (by appending the header line to the raw headers and parsing again).

This bug is about the scenario of at least 2 extensions that "modify" response headers by adding a CSP.
When the original parsed response headers already contains a CSP, then only one extension can modify the CSP header (due to step 3i).
If the original parsed response headers did not contain a CSP, the raw headers become:

[original parsed response headers here]
Content-Security-Policy: policy-from-ext1
Content-Security-Policy: policy-from-ext2

After parsing the headers, the repeated header lines are concatenated (and every directive in the CSP is applied insofar possible):

[original parsed response headers here]
Content-Security-Policy: policy-from-ext1, policy-from-ext2

Chromium's algorithm works in some cases, but is not the ideal solution when the goal is to meaningfully merge headers, as there are obvious disadvantages:

  • Only works if the original response did not contain the (CSP) header.
  • Only works in very specific cases, when directives do not overlap.
    E.g. img-src example.com; + img-src example.net; ends up being equivalent to img-src 'none' because there is no other way to satisfy both directives.

When more than one extension modifies the CSP header, it is not possible for the browser to guess what the extension author(s) intended. Asking to follow Chromium's header merging algorithm is not the solution for the described use cases. Alternative approaches could be explored in bug 1376949 .

Flags: needinfo?(rob)
See Also: → 1388486

(In reply to Giorgio Maone [:mao] from comment #44)

Just a note about CSP header removal:

I think that rather than handling this in headers/webRequest/etc., we may want to consider a more specific api. For this particular bug, I'd like to figure out how to best address the ability to add the headers, so perhaps more effort in this direction can go into bug 1376949 as Rob mentioned.

(In reply to Rob Wu [:robwu] from comment #45)

1. If a removed header has been removed before (in step 3ii at a previous iteration, usually a different extension but potentially the same extension!),
  or if an added header was removed by the `declarativeNetRequest` API,
  fire the [`webRequest.onActionIgnored` event](https://developer.chrome.com/extensions/webRequest#type-IgnoredActionType) (this was introduced in [crbug.com/111700](https://bugs.chromium.org/p/chromium/issues/detail?id=111700)),
  and skip the next steps (i.e. do not apply the header modifications).

Doe that mean all header modifications are dropped?

  • Only works if the original response did not contain the (CSP) header.

If we just append the CSP headers added by extensions we should be fine with it working regardless whether the original headers have CSP.

When more than one extension modifies the CSP header, it is not possible for the browser to guess what the extension author(s) intended.

Per spec we're allowed to have multiple CSP headers, so I assume that the underlying CSP handling deals with this somehow. I'm fine with simply allowing the multiple headers and let platform do what it does (making this potentially firefox specific in its behavior).

ni?ckerschb for input on supporting multiple CSP headers, perhaps he knows some details of the internal handling that can guide us a bit on this.

Flags: needinfo?(rob)
Flags: needinfo?(ckerschb)

(In reply to Rob Wu [:robwu] from comment #45)

  1. If a removed header has been removed before (in step 3ii at a previous iteration, usually a different extension but potentially the same extension!),
    or if an added header was removed by the declarativeNetRequest API,
    fire the webRequest.onActionIgnored event (this was introduced in crbug.com/111700),
    and skip the next steps (i.e. do not apply the header modifications).

Doe that mean all header modifications are dropped?

All header modifications from that event listener are dropped.
A different listener (from the same extension, or a different extension) can continue to modify the headers when their return value does not conflict.

  • Only works if the original response did not contain the (CSP) header.

If we just append the CSP headers added by extensions we should be fine with it working regardless whether the original headers have CSP.

We can indeed choose to do that. But we will run into irreconcilable conflicts when extensions modify the headers according to different rules. This kind of conflict is a worse version of second problem that I mentioned before and is quoted below.

When more than one extension modifies the CSP header, it is not possible for the browser to guess what the extension author(s) intended.

Per spec we're allowed to have multiple CSP headers, so I assume that the underlying CSP handling deals with this somehow. I'm fine with simply allowing the multiple headers and let platform do what it does (making this potentially firefox specific in its behavior).

Multiple CSP policies are handled by being enforced simultaneously. Any directive without overlapping values will block everything. I've given an example before, img-src example.com; + img-src example.net = img-src 'none';.

Flags: needinfo?(rob)

Any directive without overlapping values will block everything.

I'm kind of ok with that, we have to do something in conflict cases, and going strict seems to be the right approach.

I guess my question is still, do we allow removal of CSP at all? If so, we probably would need to implement onActionIgnored with some similar mechanism as chrome. If not, I think just ignoring removal of CSP and allowing multiple CSP headers to be added is fine.

A followup with a CSP api, at least in terms of logging notifications, might be something to consider.

img-src example.com; + img-src example.net = img-src 'none';

is img-src example.com; + img-src example.net = img-src example.com example.net; not possible ?

(In reply to uBlock-user from comment #49)

img-src example.com; + img-src example.net = img-src 'none';

is img-src example.com; + img-src example.net = img-src example.com example.net; not possible ?

If it's two headers from two different extensions...we have no way to know the intention. Merge? overwrite? We could choose to reject the second extensions attempt to set the header and implement onActionIgnored.

What if you choose to just merge them regardless of the intention ?

(In reply to Shane Caraveo (:mixedpuppy) from comment #46)

ni?ckerschb for input on supporting multiple CSP headers, perhaps he knows some details of the internal handling that can guide us a bit on this.

Yes, multiple CSP headers are allowed and our CSP implementation can deal with it just fine. Simply do something like.

nsCOMPtr<nsIContentSecurityPolicy> csp = new nsCSPContext();
csp->SetRequestContextWithDocument(aDocument);
for (header in Headers) {
csp->AppendPolicy(header);
}

Flags: needinfo?(ckerschb)

Can we please move this forward? are any of the patches I proposed acceptable (if not, what do you want changed)?

Flags: needinfo?(mixedpuppy)

I seem to recall a list of "mergeable" headers somewhere in the platform code that does the merging (edit: see singleton code listed below). We could consider using that same list in the webrequest code to determine what is mergeable ahead of time, doing something similar to attachment 9112692 [details] [diff] [review] but checking against the list.

Specifically, we'd need to take into account this logic:

https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/netwerk/protocol/http/nsHttpHeaderArray.cpp#68-81

And the singleton headers where appropriate:

https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/netwerk/protocol/http/nsHttpHeaderArray.h#210

There is also this bit here that handles how a header is merged:

https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/netwerk/protocol/http/nsHttpHeaderArray.h#244-253

I'm not convinced about the allowdelete patch at all, but haven't fully thought that through either.

Flags: needinfo?(mixedpuppy)

We could consider using that [list of "mergeable" headers] in the webrequest
code to determine what is mergeable ahead of time, doing something similar
to attachment 9112692 [details] [diff] [review] but checking against the list.

This is what attachment 9112693 [details] [diff] [review] (merge-all) does, if I understand the C++ code correctly:

When we call setHeader in HeaderChanger.applyChanges, this calls ResponseHeaderChanger.setHeader, which calls setResponseHeader, which is defined in nsIHttpChannel.idl and implemented in HttpBaseChannel::SetResponseHeader, which calls nsHttpResponseHead::SetHeader, which calls SetHeader_locked, which finally calls nsHttpHeaderArray::SetHeader -- the first code snippet you've linked to in Comment #54.

So when we set shouldMerge (which is passed unmodified through all those functions as merge) to true, we run into the logic you want, and also automatically check the list of singleton headers (code snippet 2). Only then we call MergeHeader, which is the third snippet you've linked.

Additionally to this, the allowdelete patch then also takes into account this block right above the 'logic we need to take into account' that removes headers iff merge is false and the header's value is "".

Flags: needinfo?(mixedpuppy)

From a conservative approach, I think I'd like to limit to CSP headers just to deal with this particular problem. That is a situation where we can comfortably merge from multiple extensions had have the backend work as expected, or at least break in a very strict manor. Write some tests for the csp specific patch.

Flags: needinfo?(mixedpuppy)

I'd like to limit to CSP headers

Does this issue currently also affect Feature-Policy Headers in the same way ?

I've written a mochitest that can be applied for any of the three patches. It is designed to be extendable, so any combination of CSP headers from up to two extensions can be experimented with. In its current form it passes es-lint, but probably still violates Mozilla's style guide; I'll happily clean it up with some guidance.

As a start, I implemented four sub-tests: the first two are the known-good cases where only 1 extension tries to modify CSP; the other two are the cases where two extensions attempt to change CSP (once with a server-side CSP, once without).

I've tested it with Bug1462989-csp-special-case.patch: without, subtest number 4 (2 extensions, no site-CSP) fails consistently, with it applied, it works perfectly.

Flags: needinfo?(mixedpuppy)

Lets focus on the csp header for now, and add a followup bug for other headers.

Are you able to push these into phabricator?

There are some style issues that I would have thought eslint would fix.

The tests look good, can you add a test where the CSPs overlap, as discussed in comment 47 through comment 50?

Flags: needinfo?(mixedpuppy)

Headers that are not present in the original request but are added by two or
more competing extensions are not merged, but only one of the changes is
applied. Since this causes issues with privacy and security enhancing
extensions trying to tighten up CSP, this introduces a special case to
explicitly always merge the CSP header.

(In reply to Shane Caraveo (:mixedpuppy) from comment #59)

Are you able to push these into phabricator?

I hope that worked?
https://phabricator.services.mozilla.com/D63556 (tests)
https://phabricator.services.mozilla.com/D63554 (merge csp)

There are some style issues that I would have thought eslint would fix.

As expected. Thanks for having a look!

The tests look good, can you add a test where the CSPs overlap, as discussed in comment 47 through comment 50?

Done.

Flags: needinfo?(mixedpuppy)
Assignee: nobody → t-mozbugs
Status: NEW → ASSIGNED

Tobais, can you write up a small blurb describing the change in behavior for mdn docs?

Flags: needinfo?(mixedpuppy) → needinfo?(t-mozbugs)
Keywords: dev-doc-needed
Summary: Consider merging new mergable headers that didn't exist in the original request → support merging content-security-policy headers provided by multiple extensions

I presume you need a text for this MDN document? In the paragraph beginning Note that it is possible for..., I would suggest changing the example from Content-Security-Policy to something else, e.g. Set-Cookie, and adding the following text below the paragraph:

The `Content-Security-Policy` header is treated specially, as the policy can only be tightened by an extension, not relaxed. This has the advantage that conflicting extensions will not undo each others changes in this special case only.
Flags: needinfo?(t-mozbugs)

The proposed documentation in comment 64 is either wrong, or the implementation is about to head towards an incorrect direction.

Extensions can relax the CSP header (by removing the one that the web server had sent).
When multiple extensions attempt to modify the header, then they are combined, by applying all specified policies.

(In reply to Rob Wu [:robwu] from comment #65)

Extensions can relax the CSP header (by removing the one that the web server had sent).

This is true (just verified with a hackcked up version of the test). So my docs changes in comment 64 are indeed false.

I'm not sure we need to call anything out then specially, do we? maybe a warning that adding headers not present in the original request might fail (unless it's CSP). New wording, replacing the second sentence from the before mentioned paragraph:

If two extensions listen to onHeadersReceived for the same request, and return responseHeaders attempting to set the same header (for example, `Set-Cookie`) not present in the original response, the changes will not be merged, but only one of them will be successful.

Yet another version (Apologies for the multiple messages), pointing out the CSP special case (also splitting the paragraph in two for readability):

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,
`Set-Cookie`) not present in the original response, only one of the
changes will be successful. The `Content-Security-Policy` header is
treated specially in this regard and does not suffer from this limitation; its
values are always combined, by applying all specified policies.

If you want to see the headers that are actually processed by the system,
without the risk that another extension will subsequently alter them, use 
[`onResponseStarted`](XXX), although you can't modify headers on this event.

(In reply to Rob Wu [:robwu] from comment #65)

Extensions can relax the CSP header (by removing the one that the web server had sent).

That still is not guaranteed to work, though: when the extension deleting the header is executed first, and a second extension re-adds it, the header of the second one will be transmitted.

Flags: needinfo?(mixedpuppy)

Is there a specific question?

yes: if the above text is suitable for the MDN page.

I would move the CSP comment onto its own paragraph. Then I would add something about conflicts as Rob described. It could be something like (feel free to word smith):

The `Content-Security-Policy` header is treated specially in this regard 
and does not suffer from this limitation; its values are always combined, 
by applying all specified policies.  However, if two extensions set a CSP 
value that conflicts, the CSP service may make the restriction more strict 
to resolve the conflict.  For example, one extension sets 
img-src: example.com, and another extension sets img-src: example.org.  
The result will be img-src: none.  Merged modifications will always lean 
towards being more restrictive, though an extension may remove the 
original CSP header.
Flags: needinfo?(mixedpuppy)

That above text is fine with me (one nit: img-src: none -> img-src: 'none').

Who is going to commit this to MDN? I don't think I have permission/authority to do this. What are the next steps in regards to this Bug?

Docs will get updated by someone sometime after the bug is closed. Will look at pushing to try tomorrow.

Have the patches landed in Nightly yet ?

(In reply to uBlock-user from comment #73)

Have the patches landed in Nightly yet ?

Not yet. There will be an automated comment when the patches are merged, and the bug will automatically be closed once released to Nightly.

(In reply to Shane Caraveo (:mixedpuppy) from comment #72)

Will look at pushing to try tomorrow.

Have you found time to submit this to try?

Flags: needinfo?(mixedpuppy)

@tobias, sorry about that, I have been distracted with a couple other items that need to land. I've a couple changes I'm going to request after discussing this more with rob.

Flags: needinfo?(mixedpuppy)

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:t-mozbugs, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(t-mozbugs)

re: Comment 77

I hope this is best handled in your hands.

Flags: needinfo?(t-mozbugs) → needinfo?(mixedpuppy)

I went to land and noticed the eslint errors, can you fix those up then I'll land the patches.

Flags: needinfo?(mixedpuppy) → needinfo?(t-mozbugs)

eslint errors fixed. Once again, thanks for your time!

Flags: needinfo?(t-mozbugs) → needinfo?(mixedpuppy)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f81cfd0d4f
Force merging of Content-Security-Policy header r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/1487aaf8ccd2
Add tests for merging Content-Security-Policy headers r=mixedpuppy

Backed out 2 changesets (Bug 1462989) for causing xpcshell failures at components/extensions/test/xpcshell/test_ext_webRequest_cached.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=1487aaf8ccd249cacd645957cd74afce2a3e785e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297625984&repo=autoland&lineNumber=6554

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=c224327f2bddd5f1a00b2c8edc6b098188884202

[task 2020-04-14T23:49:09.297Z] 23:49:09     INFO -  TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_cached.js
[task 2020-04-14T23:49:11.546Z] 23:49:11  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_cached.js | xpcshell return code: 1
[task 2020-04-14T23:49:11.546Z] 23:49:11     INFO -  TEST-INFO took 2252ms
[task 2020-04-14T23:49:11.547Z] 23:49:11     INFO -  >>>>>>>
[task 2020-04-14T23:49:11.547Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 88
[task 2020-04-14T23:49:11.548Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 88
[task 2020-04-14T23:49:11.548Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 88
[task 2020-04-14T23:49:11.549Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 88
[task 2020-04-14T23:49:11.549Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Failed to get directory to cache.: file /builds/worker/checkouts/gecko/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 88
[task 2020-04-14T23:49:11.550Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/checkouts/gecko/toolkit/crashreporter/nsExceptionHandler.cpp, line 2895
[task 2020-04-14T23:49:11.550Z] 23:49:11     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2020-04-14T23:49:11.550Z] 23:49:11     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2020-04-14T23:49:11.551Z] 23:49:11     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2020-04-14T23:49:11.551Z] 23:49:11     INFO -  running event loop
[task 2020-04-14T23:49:11.551Z] 23:49:11     INFO -  PID 15840 | [15840, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, kKnownEsrVersion) failed with result 0x80004002 (NS_NOINTERFACE): file /builds/worker/checkouts/gecko/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 673
[task 2020-04-14T23:49:11.552Z] 23:49:11     INFO -  xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_webRequest_cached.js | Starting check_remote
[task 2020-04-14T23:49:11.552Z] 23:49:11     INFO -  (xpcshell/head.js) | test check_remote pending (2)
Flags: needinfo?(t-mozbugs)
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d4d0b5092d8
Backed out 2 changesets for causing xpcshell failures at components/extensions/test/xpcshell/test_ext_webRequest_cached.js
Attachment #9127953 - Attachment description: Bug 1462989 - Add tests for merging Content-Security-Policy headers → Bug 1462989 - Add tests for merging Content-Security-Policy headers and fix existing ones

Changed the test to the new merged behaviour and checked all tests in toolkit/components/extensions/test/xpcshell.

Flags: needinfo?(t-mozbugs)

Have the patches landed in Nightly ?

not yet; they were backed out after a test failure. I fixed that test in the phabricator patch, so hopefully they'll stick when they are pushed again (hopefully) soon.

(In reply to tobias from comment #86)

not yet; they were backed out after a test failure. I fixed that test in the phabricator patch, so hopefully they'll stick when they are pushed again (hopefully) soon.

The patch will not automatically be landed again after a backout.

If the patch has been reviewed, edit the revision and add the "Check-in needed" tag to reland it. In this case the changes are very minor, so consider the previous r+ to still be valid.

Flags: needinfo?(mixedpuppy)

Thanks for the info, Rob. I assumed I had to wait for another review. Tag is added.

Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cea45f637b49
Force merging of Content-Security-Policy header r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/ee0f0e5aea63
Add tests for merging Content-Security-Policy headers and fix existing ones r=mixedpuppy
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Does this solve either:

bug 1421725 (finalize how changing headers should work)
or
bug 1267027 ([meta] Page CSP should not apply to content inserted by content scripts (V2 issue))

I have now added details to the release notes and updated webRequest.onHeadersReceived with the suggested details, although modified slightly.

In addition, I noted in webRequest.onHeadersReceived "From Firefox 52 onwards, instead of returning BlockingResponse, the listener can return a Promise that is resolved with a BlockingResponse. This enables the listener to process the request asynchronously.' However, I couldn't see any way of switching the listener between returning a promise standard call back. Given that Firefox 52 was released some time ago, should we change the documentation to note that webRequest.onHeadersReceived returns a promise?

Flags: needinfo?(t-mozbugs)
Flags: needinfo?(mixedpuppy)

(In reply to Richard Bloor from comment #92)

However, I couldn't see any way of switching the listener between returning a promise standard call back. Given that Firefox 52 was released some time ago, should we change the documentation to note that webRequest.onHeadersReceived returns a promise?

The listener is a user-defined function. Returning a BlockingResponse is valid (and supported by other browsers too), whereas Firefox also supports a Promise. I would keep the existing return type (BlockingResponse), and also note that the a Promise is supported as a return value (in Firefox only).

sorry, don't know about that. cleaning my ni?.

Flags: needinfo?(t-mozbugs)

I have modified the section to read:

To modify the headers, pass "blocking" in extraInfoSpec. Then, in your event listener, return an object with a property named responseHeaders whose value is the set of response headers to use. You can choose to return BlockingResponse or a Promise that is resolved with a BlockingResponse. Using a Promise enables the listener to process the request asynchronously. The browser then behaves as if the server had sent the modified headers.

OK?

Flags: needinfo?(rob)

I would remove The browser then behaves as if the server had sent the modified headers.; the listener can be used for other things, such as canceling the request or redirecting it.

I would even remove the whole paragraph, and add a brief note that in Firefox the return value can be a promise that resolves to a BlockingResponse at https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/onHeadersReceived#Parameters

Flags: needinfo?(rob)

Thanks, updated and suggested

Flags: needinfo?(mixedpuppy)

(In reply to Mike Kaply [:mkaply] from comment #91)

Does this solve either:

bug 1421725 (finalize how changing headers should work)
or
bug 1267027 ([meta] Page CSP should not apply to content inserted by content scripts (V2 issue))

It should at least solve bug 1477696

Duplicate of this bug: 1421725
Regressions: 1635781
You need to log in before you can comment on or make changes to this bug.