support merging content-security-policy headers provided by multiple extensions
Categories
(WebExtensions :: Request Handling, enhancement, P3)
Tracking
(firefox77 fixed)
| Tracking | Status | |
|---|---|---|
| firefox77 | --- | fixed |
People
(Reporter: sscarl24, Assigned: t-mozbugs)
References
Details
(Keywords: dev-doc-complete)
Attachments
(6 files, 1 obsolete file)
|
1.17 KB,
patch
|
Details | Diff | Splinter Review | |
|
1.27 KB,
patch
|
Details | Diff | Splinter Review | |
|
1.17 KB,
patch
|
Details | Diff | Splinter Review | |
|
6.23 KB,
patch
|
Details | Diff | Splinter Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
|
Reporter | |
Comment 1•7 years ago
|
||
|
|
Reporter | |
Comment 2•7 years ago
|
||
|
||
Updated•7 years ago
|
|
||
Updated•7 years ago
|
|
||
Updated•7 years ago
|
|
||
Updated•6 years ago
|
|
|
Reporter | |
Comment 3•6 years ago
|
||
|
|
||
Comment 4•6 years ago
|
||
|
|
Reporter | |
Comment 5•6 years ago
|
||
|
|
||
Comment 6•6 years ago
|
||
|
|
Reporter | |
Comment 7•6 years ago
|
||
|
||
Comment 8•6 years ago
|
||
This bug is really relevant for security as it allows to execute malicious javascript if it is disabled.
|
|
||
Comment 9•6 years ago
|
||
(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?
|
Assignee | |
Comment 10•6 years ago
|
||
(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.
|
|
Reporter | |
Comment 11•6 years ago
|
||
Unless the priority level gets bumped to P1, this bug is not getting fixed anytime soon.
|
||
Comment 12•6 years ago
|
||
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.
|
|
||
Updated•6 years ago
|
|
|
Reporter | |
Comment 13•5 years ago
|
||
@Kris Why is it tagged now as enhancement ? It's a bug, it's not something I requested to enhance Firefox.
|
|
||
Comment 14•5 years ago
|
||
It's not a bug. It works as documented and expected. Changing the expected behavior in cases like this is an enhancement.
|
|
Reporter | |
Comment 15•5 years ago
|
||
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 ?
|
|
||
Comment 16•5 years ago
|
||
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.
|
|
Reporter | |
Comment 17•5 years ago
|
||
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.
| Comment hidden (advocacy) |
|
||
Comment 19•5 years ago
|
||
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.
|
||
Comment 20•5 years ago
|
||
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?
|
||
Comment 21•5 years ago
|
||
(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.
|
||
Comment 22•5 years ago
|
||
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).
|
||
Comment 23•5 years ago
|
||
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?)
|
||
Comment 24•5 years ago
|
||
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.
|
||
Comment 25•5 years ago
|
||
Could you please reopen this for comments? Comment activity and dupes are a useful measure in detecting issues devs feel are important to address.
|
|
Reporter | |
Comment 27•5 years ago
|
||
Why set priority to P3 ?
|
|
Reporter | |
Comment 28•5 years ago
|
||
Also what addional info do you need ?
|
|
||
Comment 29•5 years ago
|
||
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 hidden (advocacy) |
| Comment hidden (advocacy) |
| Comment hidden (advocacy) |
|
|
||
Comment 33•5 years ago
|
||
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.
|
|
Assignee | |
Comment 34•5 years ago
|
||
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.
|
||
Updated•5 years ago
|
|
||
Comment 35•5 years ago
|
||
|
|
||
Comment 36•5 years ago
|
||
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).
|
|
||
Comment 37•5 years ago
|
||
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.
|
|
Assignee | |
Comment 38•5 years ago
|
||
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?
|
|
||
Comment 39•5 years ago
|
||
(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 doneThat 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.
|
|
Assignee | |
Comment 40•5 years ago
|
||
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.
|
|
Assignee | |
Comment 41•5 years ago
|
||
|
|
Assignee | |
Comment 42•5 years ago
|
||
|
|
||
Comment 43•5 years ago
|
||
(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?
|
|
||
Comment 44•5 years ago
|
||
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).
|
||
Comment 45•5 years ago
|
||
Chromium's conflict resolution logic for response headers is as follows (similar for request headers):
- Dispatch
webRequest.onHeadersReceivedto all interested extensions. - Wait until every event handler has returned.
- 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).
- 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:
- Iterate over the return values (in the order of extension installation: from newest to oldest) and change the header as follows (Chromium source:
MergeOnHeadersReceivedResponses).- 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 thedeclarativeNetRequestAPI,
fire thewebRequest.onActionIgnoredevent (this was introduced in crbug.com/111700),
and skip the next steps (i.e. do not apply the header modifications). - Apply header modifications:
- Remove header lines if needed.
- Append new headers (by appending the header line to the raw headers and parsing again).
- If a removed header has been removed before (in step 3ii at a previous iteration, usually a different extension but potentially the same extension!),
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 toimg-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 .
|
|
||
Comment 46•5 years ago
|
||
(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.
|
|
||
Comment 47•5 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #45)
- 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 thedeclarativeNetRequestAPI,
fire thewebRequest.onActionIgnoredevent (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';.
|
|
||
Comment 48•5 years ago
|
||
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.
|
|
Reporter | |
Comment 49•5 years ago
|
||
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 ?
|
|
||
Comment 50•5 years ago
|
||
(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.
|
|
Reporter | |
Comment 51•5 years ago
|
||
What if you choose to just merge them regardless of the intention ?
|
||
Comment 52•5 years ago
|
||
(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);
}
|
|
Assignee | |
Comment 53•5 years ago
|
||
Can we please move this forward? are any of the patches I proposed acceptable (if not, what do you want changed)?
|
|
||
Comment 54•5 years ago
•
|
||
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:
And the singleton headers where appropriate:
There is also this bit here that handles how a header is merged:
I'm not convinced about the allowdelete patch at all, but haven't fully thought that through either.
|
|
Assignee | |
Comment 55•5 years ago
|
||
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 "".
|
|
||
Comment 56•5 years ago
|
||
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.
|
|
Reporter | |
Comment 57•5 years ago
|
||
I'd like to limit to CSP headers
Does this issue currently also affect Feature-Policy Headers in the same way ?
|
|
Assignee | |
Comment 58•5 years ago
|
||
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.
|
|
||
Comment 59•5 years ago
|
||
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?
|
|
Assignee | |
Comment 60•5 years ago
|
||
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.
|
|
Assignee | |
Comment 61•5 years ago
|
||
Depends on D63554
|
|
Assignee | |
Comment 62•5 years ago
|
||
(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.
|
||
Updated•5 years ago
|
|
|
||
Comment 63•5 years ago
|
||
Tobais, can you write up a small blurb describing the change in behavior for mdn docs?
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 64•5 years ago
|
||
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.
|
|
||
Comment 65•5 years ago
|
||
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.
|
|
Assignee | |
Comment 66•5 years ago
|
||
(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.
|
|
Assignee | |
Comment 67•5 years ago
|
||
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.
|
|
||
Comment 68•5 years ago
|
||
Is there a specific question?
|
|
Assignee | |
Comment 69•5 years ago
|
||
yes: if the above text is suitable for the MDN page.
|
|
||
Comment 70•5 years ago
•
|
||
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.
|
|
Assignee | |
Comment 71•5 years ago
|
||
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?
|
|
||
Comment 72•5 years ago
|
||
Docs will get updated by someone sometime after the bug is closed. Will look at pushing to try tomorrow.
|
|
Reporter | |
Comment 73•5 years ago
|
||
Have the patches landed in Nightly yet ?
|
|
||
Comment 74•5 years ago
|
||
(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.
|
|
Assignee | |
Comment 75•5 years ago
|
||
(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?
|
|
||
Comment 76•5 years ago
|
||
@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.
|
||
Comment 77•5 years ago
|
||
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.
|
|
Assignee | |
Comment 78•5 years ago
|
||
re: Comment 77
I hope this is best handled in your hands.
|
|
||
Comment 79•5 years ago
|
||
I went to land and noticed the eslint errors, can you fix those up then I'll land the patches.
|
|
Assignee | |
Comment 80•5 years ago
|
||
eslint errors fixed. Once again, thanks for your time!
|
||
Comment 81•5 years ago
|
||
|
||
Comment 82•5 years ago
|
||
Backed out 2 changesets (Bug 1462989) for causing xpcshell failures at components/extensions/test/xpcshell/test_ext_webRequest_cached.js
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=297625984&repo=autoland&lineNumber=6554
[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)
|
|
||
Comment 83•5 years ago
|
||
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 84•5 years ago
|
||
Changed the test to the new merged behaviour and checked all tests in toolkit/components/extensions/test/xpcshell.
|
|
Reporter | |
Comment 85•5 years ago
|
||
Have the patches landed in Nightly ?
|
|
Assignee | |
Comment 86•5 years ago
|
||
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.
|
|
||
Comment 87•5 years ago
|
||
(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.
|
|
Assignee | |
Comment 88•5 years ago
|
||
Thanks for the info, Rob. I assumed I had to wait for another review. Tag is added.
|
|
||
Comment 89•5 years ago
|
||
|
||
Comment 90•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/cea45f637b49
https://hg.mozilla.org/mozilla-central/rev/ee0f0e5aea63
|
||
Comment 91•5 years ago
|
||
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))
|
||
Comment 92•5 years ago
|
||
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?
|
|
||
Comment 93•5 years ago
•
|
||
(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).
|
|
Assignee | |
Comment 94•5 years ago
|
||
sorry, don't know about that. cleaning my ni?.
|
|
||
Comment 95•5 years ago
|
||
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?
|
|
||
Comment 96•5 years ago
|
||
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
|
|
||
Comment 97•5 years ago
|
||
Thanks, updated and suggested
|
|
||
Updated•5 years ago
|
|
|
||
Updated•4 years ago
|
|
||
Comment 98•4 years ago
|
||
(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
Description
•