Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inpage injection fails in Firefox under some CSP settings #3133

Closed
marcusmolchany opened this issue Jan 30, 2018 · 72 comments · Fixed by #27770
Closed

Inpage injection fails in Firefox under some CSP settings #3133

marcusmolchany opened this issue Jan 30, 2018 · 72 comments · Fixed by #27770
Assignees
Labels
area-injection Relating to how the JS interface is injected into a website. browser-firefox release-12.8.0 Issue or pull request that will be included in release 12.8.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-extension-platform type-bug

Comments

@marcusmolchany
Copy link

Hey, as far as I can tell, my content security policy is preventing MetaMask from injecting its scripts. This is only happening in Firefox. It works correctly in Chrome, Safari, Opera, and Brave. My script-src directive looks like this:

script-src 'self';

and I'm seeing this csp violation in the js console:

Content Security Policy: The page’s settings blocked the loading of a resource at self. Source: (function e(t,n,r){function s(o,u){if(!n ....

Unfortunately Firefox only shows a preview of the blocked script. I've tried sha256 hashing each of the scripts in the latest Metamask release and adding them to the CSP, but that did not work. If you have any ideas that would be great!

Browser: Firefox 58.0.1
Operating System: Mac OSX 10.13.2

@gitcoinbot
Copy link

This issue now has a funding of 0.105 ETH (96.44 USD) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $9607.34 more Funded OSS Work Available at: https://gitcoin.co/explorer

@marcusmolchany
Copy link
Author

I've added funding to this issue. I'd love for my site to work on FireFox with MetaMask without having to change my CSP. Thanks!

@evgeniuz
Copy link

I think I can fix this issue, but want to verify that my approach would be correct one.

So the problem is that CSP won't allow inline script that is used here https://github.com/MetaMask/metamask-extension/blob/master/app/scripts/contentscript.js#L30. There's manifest.json property web_accessible_resources that seems to allow injecting scripts that are specified there, but it's only applied if script is loaded with src, not with textContent.

I've tried quickly and replacing scriptTag.textContent = [script_content] with scriptTag.src = [script_url] seems to resolve issue with CSP in Firefox. If this approach is ok, I'll proceed with implementation.

@evgeniuz
Copy link

Ok, this might be trickier than I expected. I see that loading with src caused race condition in the past, going to look more closely into this.

@marcusmolchany
Copy link
Author

thank you for starting @evgeniuz! I can set up an example application that uses my current CSP if that would help.

@evgeniuz
Copy link

Sorry, but it seems that those two issues are inherently conflicting: when using scriptTag.src there seems to be no way to ensure that script will load before all other scripts and using scriptTag.textContent is disallowed by CSP.

I've noticed that you've tried to use hash to whitelist the script for inline, the file to hash is scripts/inpage.js, but the problem is that sourceURL comment is added here: https://github.com/MetaMask/metamask-extension/blob/master/app/scripts/contentscript.js#L12. Since this sourceURL depends on extension ID, it may be different for each installation, so you cannot whitelist it reliably.

Removing sourceURL from final build will allow to generate hash reliably, so you can whitelist it in CSP. And it can still be added in development build for easier debugging. But I cannot make this decision myself, need some input from Metamask team: is it ok to remove sourceURL suffix in that line so that inline script is same on every installation and can be hashed and whitelisted in CSP?

Going to unassign myself from bounty, as I cannot find another solution at the moment.

@marcusmolchany
Copy link
Author

@evgeniuz thank you very much for all the help here.

@dmihal
Copy link

dmihal commented Mar 2, 2018

@marcusmolchany would you mind setting up a sample application? I'd like to play with setting up a workaround...

@marcusmolchany
Copy link
Author

marcusmolchany commented Mar 2, 2018

Hey @dmihal here is a sample application. If you open it in chrome, you should see that web3 is injected into window.web3. If you open it using firefox, you should see that the CSP blocks web3 from being injected.

Here is the repo for the sample application. And this specifically is the CSP.

@vs77bb
Copy link

vs77bb commented Mar 6, 2018

@dmihal Would you like to give this one a go? Feel free to claim it on Gitcoin by clicking 'Start Work', if so!

@KennethAshley
Copy link

Is this still an issue? MetaMask seems to be working fine on FF for me.

@marcusmolchany
Copy link
Author

Hey @KennethAshley, this is only an issue if you have a Content Security Policy. Does MetaMask successfully inject web3 on my sample application when you use Firefox?

@skkiran-pro
Copy link

This is a known bug in firefox. https://bugzilla.mozilla.org/show_bug.cgi?id=1267027. Comment on that issue to get it fixed soon. :)

@gitcoinbot
Copy link

@dmxsf1 are you still working on this issue?

1 similar comment
@gitcoinbot
Copy link

@dmxsf1 are you still working on this issue?

@gitcoinbot
Copy link

@dmxsf1 are you still working on this issue?

@gitcoinbot
Copy link

@amitkumar991 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@filips123
Copy link
Contributor

filips123 commented Nov 26, 2018

Any update? Is this fixed in newer versions of Firefox or Metamask?

@filips123
Copy link
Contributor

Could this be fixed by Metamask or it should be reported to Firefox bugzilla?
This should be fixed because many websites don't work because of that.

@MicahZoltu
Copy link

I believe the "correct" long term solution here is for dapps to change the way they interact with browser signers to better align with the official browser recommendation for pages and extensions communicating, which is via postMessage. I'm currently working on a solution to this, but it will be a big change that will require both dapp and extension buy-in and likely will be very slow to gain adoption.

I wanted to drop a comment here just to make people aware of the potential futures, but at the moment I don't believe there is a great solution. If I am successful, my hope is to get this new mechanism implemented in MetaMask eventually (likely side-by-side with the existing injection technique) and then from there we will try to get dapps to adopt this technique over time.

@tuxayo
Copy link

tuxayo commented Nov 5, 2019

try to get dapps to adopt this technique over time.

Does it mean extending the Web3 protocol? (if that's correct to call that a protocol)

@MicahZoltu
Copy link

It means changing the way dapps communicate with "web3 enabled browsers". Rather than calling functions that were attached to window.ethereum, instead fire and listen for events.

@jurosh
Copy link

jurosh commented Apr 20, 2020

Hey guys, seems this issue is very old problem, but probably there doesn't exist even hotfix for this issue yet ?

So there is currently no way to use Metamask in Firefox ? (edit: with CSP enabled)

Going to follow also this thread https://bugzilla.mozilla.org/show_bug.cgi?id=1267027 but it also seems to be there for 4 years, so we should probably not expect this to be fixed anytime soon.

As I understand, to fix Metamask in FF, there would be need to change it's communication protocol to postMessages? Is that something what is planned?

@MicahZoltu
Copy link

I use MetaMask on Firefox and it seems to work, though I think I'm on a pretty old version at the moment. What page are you unable to use? Is it served via HTTP or HTTPS?

@jurosh
Copy link

jurosh commented Apr 20, 2020

I use MetaMask on Firefox and it seems to work, though I think I'm on a pretty old version at the moment. What page are you unable to use? Is it served via HTTP or HTTPS?

Both, I was testing on localhost:3000 (http) and also live site with https. Tested with nightly FF (v77) and stable v75.

There is error regarding blocked contentscript.js: Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”).

And there is no window.web3 neither ethereum included.

Edit: Without enabled CSP it works. But we cannot turn that off and reduce web security.

@glitch-txs
Copy link

glitch-txs commented Feb 9, 2024

I created an NPM pacakge that "fixes" this.

  1. Install
npm i metamask-csp-firefox
  1. Import it on top of your app
import 'metamask-csp-firefox'

Here is the source code (at least worked for me hehe): https://github.com/glitch-txs/metamask-csp-firefox

Considerations:
I added an ugly workaround for when the wallet is not installed, relying on MetaMask always returning the chain Id when requested, even when the wallet is locked. I'm not sure if this behavior is going to change in the future, is there other way to ping the wallet? @danfinlay @Gudahtt

@shanejonas
Copy link
Contributor

@glitch-txs check out https://github.com/MetaMask/detect-provider which is doing something similar to your lib.

There's an existing bug with Firefox where it shouldn't enforce csp for injected content scripts that still hasn't been resolved:
https://bugzilla.mozilla.org/show_bug.cgi?id=1267027

@glitch-txs
Copy link

Thanks for clarifying! So sad it's 8 years old issue in Firefox :(

@davidmurdoch
Copy link
Contributor

I briefly looked into ways of solving this from MM's side. It's a bit tricky. Here are 2 ideas:

1. Xray vision

Firefox Extensions are unique in that Firefox allows for extensions to add properties to the page's window object via Xray vision via a helper function cloneInto. But cloneInto feature uses the structuredClone algorithm, plus some extra magic to clone functions, to copy the JS object from the content_script's realm to the page's realm. The problem here is that the MM provider makes use of Proxy, Promise, and async -- and those objects cannot be cloned. There are probably more objects used by the MM Provider that are "too exotic" for clone, I just didn't get that far to find them.

2. onBeforeSendHeaders

Another approach I toyed with is altering the CSP headers to allow our script. I toyed with the existing extension (ModHeader - this is not an endorsement) to alter the CSP headers to allow any script-src (script-src * data: blob: 'unsafe-inline' 'unsafe-eval';) and it works. We'd have to add "webRequestBlocking" to our manifest.json's permissions to make this work for our extension. We'd need to make use of the onBeforeSendHeaders event to alter the at least content-security-policy and content-security-policy-report-only response headers. I'm unsure if HTML meta tag will need to be altered (but I doubt it). Ideally we'd inject inpage.js with a nonce or integrity hash, and only add that value to the CSP directive.

@glitch-txs
Copy link

It would be nice to get a fix on this without altering too much the CSP as we want to keep the safe feature for the users.
It's also a very old issue and Firefox doesn't seem to have intentions to fix it, so finding a way around it would be great for all wallets.

@dmlayton
Copy link

Ideally we'd inject inpage.js with a nonce or integrity hash, and only add that value to the CSP directive.

Yes! This is what I was surprised to find lacking when I encountered the issue. Otherwise, it's not safe, right?

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Jun 17, 2024
@shoenseiwaso
Copy link

Still an issue. Thanks!

@FelipeCabreraB
Copy link

FelipeCabreraB commented Sep 11, 2024

Hi @gauthierpetetin this has been an issue since 2018, it's there any effort being made from Metamask team side to fix this in the near future?, or should we all just tell our clients to not use Metamask on Firefox when we have some csp on our app? Just to try and understand, a very straightforward answer will be very well received and really useful, thanks!

@gauthierpetetin
Copy link
Contributor

Hi @FelipeCabreraB , we will reassess this issue in the next week and determine what our next steps are. We'll keep you informed.

@gauthierpetetin
Copy link
Contributor

Hi @FelipeCabreraB , we've got a team discussion on this specific issue today. We're still not able to get an answer. As next step we'll try to reproduce it and assess how many dapps and users are impacted by it. We'll continue to keep you updated.

@coderbizman
Copy link

Any update on this? Still seems to be an issue regarding wallets.

@gauthierpetetin
Copy link
Contributor

Hi @coderbizman , this week, we've started working on a fix for this CSP issue in Firefox, consisting in overriding CSP headers. We'll share a PR in this thread as soon as available.

@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 7, 2024
matthewwalsh0 pushed a commit that referenced this issue Nov 7, 2024
<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27770?quickstart=1)

This PR implements a workaround for a long-standing Firefox MV2 bug
where the content-security-policy header is not bypassed, triggering an
error.

The solution is simple: we check if the extension is MV2 running in
Firefox. If yes, we override the header to prevent the error from
raising.

## **Related issues**

Fixes: #3133,
MetaMask/MetaMask-planning#3342

## **Manual testing steps**

1. Opening github.com should not trigger the CSP error

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<img width="726" alt="csp-toggle-off"
src="https://github.com/user-attachments/assets/3877e37a-c205-4717-af6f-92e7f63a15a4">

<img width="1725" alt="reprod"
src="https://github.com/user-attachments/assets/c923bedb-f73f-472c-8e0c-3545876a0bc3">

### **After**

<img width="719" alt="csp-toggle-on"
src="https://github.com/user-attachments/assets/8e763391-1bac-4ff0-9d07-63436d7ee41d">

<img width="1723" alt="fixed"
src="https://github.com/user-attachments/assets/1ca7c4e7-7c0e-4e75-8f0c-586ce99e4000">

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

---------

Co-authored-by: David Murdoch <[email protected]>
@gauthierpetetin
Copy link
Contributor

The fix is planned to be released in version 12.8.0.

@glitch-txs
Copy link

Are there any downsides to overrriding this?

@gauthierpetetin
Copy link
Contributor

We believe there are no downsides, but we can't be certain. Therefore, we've added a toggle in the settings to disable CSP headers overriding if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-injection Relating to how the JS interface is injected into a website. browser-firefox release-12.8.0 Issue or pull request that will be included in release 12.8.0 Sev2-normal Normal severity; minor loss of service or inconvenience. team-extension-platform type-bug
Projects
Archived in project