Modify CSP headers to allow hash of injected script #14233
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA
|
|
This needs testing. It's possible it will work first-time, but I find that
unlikely.
Ref: MetaMask/detect-provider#31
|
|
Hold on CI is stuck. Rerunning... |
|
@Pandapip1 Could you provide context for this change? Otherwise this PR isn't something we can accept. |
|
The context here is MetaMask/detect-provider#31. (TL;DR: Firefox applies CSP to code injected by extensions, which means that if you don't include This is very interesting code and a novel approach, and I appreciate it for that -- I can see myself referring back to this work on occasion. However, I'm not yet convinced this approach can work in all cases. CSPs can be -- and in many notable usecases, are -- set by meta tags in addition to headers. In fact, CSP meta tags can be constructed, injected, and altered after page load by scripts, which is a technique used by all-in-one cookie consent toolkits -- if you turn off consent for a certain site, they'll just update the CSP to block it. Preemptively adding a CSP when none exists will probably break pages that don't use one, and reliably detecting meta tag CSP's doesn't seem possible. (I'd also want to verify where this filter is applied during the ServiceWorker lifecycle; if the modifications are visible to the ServiceWorker itself, that could cause problems too.) The best solution to this issue that I can see at the moment would be to update MetaMask/detect-provider to use its own inpage-provider if it doesn't see one injected by the extension. |
|
I'll add code to fix that. I think the approach may still work. |
|
Well, it doesn't really matter where the meta tag is applied during the
service worker if the content script itself modifies the CSP meta tag.
I'll see why some of those chrome tests aren't working, and submit a fix
for them.
…On Tue, Mar 29, 2022 at 1:33 PM MrNerdHair ***@***.***> wrote:
The context here is MetaMask/detect-provider#31
<MetaMask/detect-provider#31>.
This is very interesting code and a novel approach, and I appreciate it
for that -- I can see myself referring back to this work on occasion.
However, I'm not yet convinced this approach can work in all cases. CSPs
can be -- and in many notable usecases, are -- set by meta tags in addition
to headers. In fact, CSP meta tags can be constructed, injected, and
altered *after page load* by scripts, which is a technique used by
all-in-one cookie consent toolkits -- if you turn off consent for a certain
site, they'll just update the CSP to block it.
Preemptively adding a CSP when none exists will probably break pages that
don't use one, and reliably detecting meta tag CSP's doesn't seem possible.
(I'd also want to verify where this filter is applied during the
ServiceWorker lifecycle; if the modifications are visible to the
ServiceWorker itself, that could cause problems too.) The best solution to
this issue that I can see at the moment would be to update
MetaMask/detect-provider to use its own inpage-provider if it doesn't see
one injected by the extension.
—
Reply to this email directly, view it on GitHub
<#14233 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AK5WMRVN5CYEBQ264E5BMC3VCM5FTANCNFSM5R33Z67A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'd also like to add that if there is no CSP meta or header present, it
isn't modified, so pages without a CSP will still work properly.
…On Wed, Mar 30, 2022 at 8:13 AM Gavin John ***@***.***> wrote:
Well, it doesn't really matter where the meta tag is applied during the
service worker if the content script itself modifies the CSP meta tag.
I'll see why some of those chrome tests aren't working, and submit a fix
for them.
On Tue, Mar 29, 2022 at 1:33 PM MrNerdHair ***@***.***>
wrote:
> The context here is MetaMask/detect-provider#31
> <MetaMask/detect-provider#31>.
>
> This is very interesting code and a novel approach, and I appreciate it
> for that -- I can see myself referring back to this work on occasion.
>
> However, I'm not yet convinced this approach can work in all cases. CSPs
> can be -- and in many notable usecases, are -- set by meta tags in addition
> to headers. In fact, CSP meta tags can be constructed, injected, and
> altered *after page load* by scripts, which is a technique used by
> all-in-one cookie consent toolkits -- if you turn off consent for a certain
> site, they'll just update the CSP to block it.
>
> Preemptively adding a CSP when none exists will probably break pages that
> don't use one, and reliably detecting meta tag CSP's doesn't seem possible.
> (I'd also want to verify where this filter is applied during the
> ServiceWorker lifecycle; if the modifications are visible to the
> ServiceWorker itself, that could cause problems too.) The best solution to
> this issue that I can see at the moment would be to update
> MetaMask/detect-provider to use its own inpage-provider if it doesn't see
> one injected by the extension.
>
> —
> Reply to this email directly, view it on GitHub
> <#14233 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AK5WMRVN5CYEBQ264E5BMC3VCM5FTANCNFSM5R33Z67A>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Okay, the checks have passed. Can I get a review @digiwand?
…On Wed, Mar 30, 2022 at 9:54 AM Gavin John ***@***.***> wrote:
I'd also like to add that if there is no CSP meta or header present, it
isn't modified, so pages without a CSP will still work properly.
On Wed, Mar 30, 2022 at 8:13 AM Gavin John ***@***.***> wrote:
> Well, it doesn't really matter where the meta tag is applied during the
> service worker if the content script itself modifies the CSP meta tag.
>
> I'll see why some of those chrome tests aren't working, and submit a fix
> for them.
>
> On Tue, Mar 29, 2022 at 1:33 PM MrNerdHair ***@***.***>
> wrote:
>
>> The context here is MetaMask/detect-provider#31
>> <MetaMask/detect-provider#31>.
>>
>> This is very interesting code and a novel approach, and I appreciate it
>> for that -- I can see myself referring back to this work on occasion.
>>
>> However, I'm not yet convinced this approach can work in all cases. CSPs
>> can be -- and in many notable usecases, are -- set by meta tags in addition
>> to headers. In fact, CSP meta tags can be constructed, injected, and
>> altered *after page load* by scripts, which is a technique used by
>> all-in-one cookie consent toolkits -- if you turn off consent for a certain
>> site, they'll just update the CSP to block it.
>>
>> Preemptively adding a CSP when none exists will probably break pages
>> that don't use one, and reliably detecting meta tag CSP's doesn't seem
>> possible. (I'd also want to verify where this filter is applied during the
>> ServiceWorker lifecycle; if the modifications are visible to the
>> ServiceWorker itself, that could cause problems too.) The best solution to
>> this issue that I can see at the moment would be to update
>> MetaMask/detect-provider to use its own inpage-provider if it doesn't see
>> one injected by the extension.
>>
>> —
>> Reply to this email directly, view it on GitHub
>> <#14233 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AK5WMRVN5CYEBQ264E5BMC3VCM5FTANCNFSM5R33Z67A>
>> .
>> You are receiving this because you were mentioned.Message ID:
>> ***@***.***>
>>
>
|
|
Hello @Pandapip1! I will need to allocate more time to review this thoroughly. [edit] As this PR is still work in progress (WIP), I would like to withhold my personal review until it is ready. I recommend setting this PR to "draft" until it is ready. |
|
Haha, found the issue: |
|
I can't reproduce the error on Circle CI, so I've just put the code that's erroring in a try/catch. If this passes CI, I'll test it. |
|
CI just passed. I'm testing to see if this works. |
|
I think this is ready for review. I actually can't currently test this, but I'm pretty confident that the code will work (and I'll test it later). If it doesn't, then I'll just make the needed changes. |
|
I don't think this will work. Note from https://w3c.github.io/webappsec-csp/#meta-element:
|
Well then... firefox doesn't follow the spec :) I've tested the meta-modifying code, and it works as expected (see #14233 (comment)) EDIT: As you yourself said,
(emphasis added) |
|
Well, this is interesting:
I might have to do a bit of tweaking to make this work. |
|
Okay, so my plan of action is to have the background page, instead of modifying the CSP header, delete it and send it to the content script, which then re-adds it. |
|
I think that this should work. Can someone test this? I've got example webpages set up at: |
|
This no longer changes the provider code (I think). Would someone mind reviewing it? This might also fix #13972. I will also test that. |
|
Why is the CLA bot failing? EDIT: It fixed itself. |
|
Good news: it injects web3 all the time |
Fixes #3133
Fixes MetaMask/detect-provider#31