Skip to content

Commit

Permalink
fix: mv2 firefox csp header (#27770)
Browse files Browse the repository at this point in the history
<!--
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 <187813+davidmurdoch@users.noreply.github.com>
  • Loading branch information
itsyoboieltr and davidmurdoch authored Nov 7, 2024
1 parent 2484e86 commit 54c563e
Show file tree
Hide file tree
Showing 37 changed files with 515 additions and 60 deletions.
6 changes: 6 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 41 additions & 0 deletions app/scripts/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ import {
// TODO: Remove restricted import
// eslint-disable-next-line import/no-restricted-paths
import { getCurrentChainId } from '../../ui/selectors';
import { addNonceToCsp } from '../../shared/modules/add-nonce-to-csp';
import { checkURLForProviderInjection } from '../../shared/modules/provider-injection';
import migrations from './migrations';
import Migrator from './lib/migrator';
import ExtensionPlatform from './platforms/extension';
Expand Down Expand Up @@ -333,6 +335,40 @@ function maybeDetectPhishing(theController) {
);
}

/**
* Overrides the Content-Security-Policy (CSP) header by adding a nonce to the `script-src` directive.
* This is a workaround for [Bug #1446231](https://bugzilla.mozilla.org/show_bug.cgi?id=1446231),
* which involves overriding the page CSP for inline script nodes injected by extension content scripts.
*/
function overrideContentSecurityPolicyHeader() {
// The extension url is unique per install on Firefox, so we can safely add it as a nonce to the CSP header
const nonce = btoa(browser.runtime.getURL('/'));
browser.webRequest.onHeadersReceived.addListener(
({ responseHeaders, url }) => {
// Check whether inpage.js is going to be injected into the page or not.
// There is no reason to modify the headers if we are not injecting inpage.js.
const isInjected = checkURLForProviderInjection(new URL(url));

// Check if the user has enabled the overrideContentSecurityPolicyHeader preference
const isEnabled =
controller.preferencesController.state
.overrideContentSecurityPolicyHeader;

if (isInjected && isEnabled) {
for (const header of responseHeaders) {
if (header.name.toLowerCase() === 'content-security-policy') {
header.value = addNonceToCsp(header.value, nonce);
}
}
}

return { responseHeaders };
},
{ types: ['main_frame', 'sub_frame'], urls: ['http://*/*', 'https://*/*'] },
['blocking', 'responseHeaders'],
);
}

// These are set after initialization
let connectRemote;
let connectExternalExtension;
Expand Down Expand Up @@ -479,6 +515,11 @@ async function initialize() {

if (!isManifestV3) {
await loadPhishingWarningPage();
// Workaround for Bug #1446231 to override page CSP for inline script nodes injected by extension content scripts
// https://bugzilla.mozilla.org/show_bug.cgi?id=1446231
if (getPlatform() === PLATFORM_FIREFOX) {
overrideContentSecurityPolicyHeader();
}
}
await sendReadyMessageToTabs();
log.info('MetaMask initialization complete.');
Expand Down
1 change: 1 addition & 0 deletions app/scripts/constants/sentry-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ export const SENTRY_BACKGROUND_STATE = {
advancedGasFee: true,
currentLocale: true,
dismissSeedBackUpReminder: true,
overrideContentSecurityPolicyHeader: true,
featureFlags: true,
forgottenPassword: true,
identities: false,
Expand Down
17 changes: 17 additions & 0 deletions app/scripts/controllers/preferences-controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,23 @@ describe('preferences controller', () => {
});
});

describe('overrideContentSecurityPolicyHeader', () => {
it('defaults overrideContentSecurityPolicyHeader to true', () => {
const { controller } = setupController({});
expect(
controller.state.overrideContentSecurityPolicyHeader,
).toStrictEqual(true);
});

it('set overrideContentSecurityPolicyHeader to false', () => {
const { controller } = setupController({});
controller.setOverrideContentSecurityPolicyHeader(false);
expect(
controller.state.overrideContentSecurityPolicyHeader,
).toStrictEqual(false);
});
});

describe('snapsAddSnapAccountModalDismissed', () => {
it('defaults snapsAddSnapAccountModalDismissed to false', () => {
const { controller } = setupController({});
Expand Down
20 changes: 20 additions & 0 deletions app/scripts/controllers/preferences-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ export type PreferencesControllerState = Omit<
useNonceField: boolean;
usePhishDetect: boolean;
dismissSeedBackUpReminder: boolean;
overrideContentSecurityPolicyHeader: boolean;
useMultiAccountBalanceChecker: boolean;
useSafeChainsListValidation: boolean;
use4ByteResolution: boolean;
Expand Down Expand Up @@ -175,6 +176,7 @@ export const getDefaultPreferencesControllerState =
useNonceField: false,
usePhishDetect: true,
dismissSeedBackUpReminder: false,
overrideContentSecurityPolicyHeader: true,
useMultiAccountBalanceChecker: true,
useSafeChainsListValidation: true,
// set to true means the dynamic list from the API is being used
Expand Down Expand Up @@ -306,6 +308,10 @@ const controllerMetadata = {
persist: true,
anonymous: true,
},
overrideContentSecurityPolicyHeader: {
persist: true,
anonymous: true,
},
useMultiAccountBalanceChecker: {
persist: true,
anonymous: true,
Expand Down Expand Up @@ -1009,6 +1015,20 @@ export class PreferencesController extends BaseController<
});
}

/**
* A setter for the user preference to override the Content-Security-Policy header
*
* @param overrideContentSecurityPolicyHeader - User preference for overriding the Content-Security-Policy header.
*/
setOverrideContentSecurityPolicyHeader(
overrideContentSecurityPolicyHeader: boolean,
): void {
this.update((state) => {
state.overrideContentSecurityPolicyHeader =
overrideContentSecurityPolicyHeader;
});
}

/**
* A setter for the incomingTransactions in preference to be updated
*
Expand Down
1 change: 1 addition & 0 deletions app/scripts/lib/backup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ const jsonData = JSON.stringify({
useNonceField: false,
usePhishDetect: true,
dismissSeedBackUpReminder: false,
overrideContentSecurityPolicyHeader: true,
useTokenDetection: false,
useCollectibleDetection: false,
openSeaEnabled: false,
Expand Down
4 changes: 4 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -3487,6 +3487,10 @@ export default class MetamaskController extends EventEmitter {
preferencesController.setDismissSeedBackUpReminder.bind(
preferencesController,
),
setOverrideContentSecurityPolicyHeader:
preferencesController.setOverrideContentSecurityPolicyHeader.bind(
preferencesController,
),
setAdvancedGasFee: preferencesController.setAdvancedGasFee.bind(
preferencesController,
),
Expand Down
2 changes: 1 addition & 1 deletion development/build/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ function getBuildName({
function makeSelfInjecting(filePath) {
const fileContents = readFileSync(filePath, 'utf8');
const textContent = JSON.stringify(fileContents);
const js = `{let d=document,s=d.createElement('script');s.textContent=${textContent};d.documentElement.appendChild(s).remove();}`;
const js = `{let d=document,s=d.createElement('script');s.textContent=${textContent};s.nonce=btoa((globalThis.browser||chrome).runtime.getURL('/'));d.documentElement.appendChild(s).remove();}`;
writeFileSync(filePath, js, 'utf8');
}

Expand Down
13 changes: 10 additions & 3 deletions development/create-static-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,25 @@ const path = require('path');

const serveHandler = require('serve-handler');

const createStaticServer = (rootDirectory) => {
/**
* Creates an HTTP server that serves static files from a directory using serve-handler.
* If a request URL starts with `/node_modules/`, it rewrites the URL and serves files from the `node_modules` directory.
*
* @param { NonNullable<Parameters<typeof import("serve-handler")>[2]> } options - Configuration options for serve-handler. Documentation can be found here: https://github.com/vercel/serve-handler
* @returns {http.Server} An instance of an HTTP server configured with the specified options.
*/
const createStaticServer = (options) => {
return http.createServer((request, response) => {
if (request.url.startsWith('/node_modules/')) {
request.url = request.url.substr(14);
request.url = request.url.slice(14);
return serveHandler(request, response, {
directoryListing: false,
public: path.resolve('./node_modules'),
});
}
return serveHandler(request, response, {
directoryListing: false,
public: rootDirectory,
...options,
});
});
};
Expand Down
2 changes: 1 addition & 1 deletion development/static-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const onRequest = (request, response) => {
};

const startServer = ({ port, rootDirectory }) => {
const server = createStaticServer(rootDirectory);
const server = createStaticServer({ public: rootDirectory });

server.on('request', onRequest);

Expand Down
4 changes: 2 additions & 2 deletions development/webpack/test/plugins.SelfInjectPlugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('SelfInjectPlugin', () => {
// reference the `sourceMappingURL`
assert.strictEqual(
newSource,
`{let d=document,s=d.createElement('script');s.textContent="${source}\\n//# sourceMappingURL=${filename}.map"+\`\\n//# sourceURL=\${(globalThis.browser||chrome).runtime.getURL("${filename}")};\`;d.documentElement.appendChild(s).remove()}`,
`{let d=document,s=d.createElement('script');s.textContent="${source}\\n//# sourceMappingURL=${filename}.map"+\`\\n//# sourceURL=\${(globalThis.browser||chrome).runtime.getURL("${filename}")};\`;s.nonce=btoa((globalThis.browser||chrome).runtime.getURL("/"));d.documentElement.appendChild(s).remove()}`,
);
} else {
// the new source should NOT reference the new sourcemap, since it's
Expand All @@ -66,7 +66,7 @@ describe('SelfInjectPlugin', () => {
// console.
assert.strictEqual(
newSource,
`{let d=document,s=d.createElement('script');s.textContent="console.log(3);"+\`\\n//# sourceURL=\${(globalThis.browser||chrome).runtime.getURL("${filename}")};\`;d.documentElement.appendChild(s).remove()}`,
`{let d=document,s=d.createElement('script');s.textContent="console.log(3);"+\`\\n//# sourceURL=\${(globalThis.browser||chrome).runtime.getURL("${filename}")};\`;s.nonce=btoa((globalThis.browser||chrome).runtime.getURL("/"));d.documentElement.appendChild(s).remove()}`,
);
}

Expand Down
21 changes: 19 additions & 2 deletions development/webpack/utils/plugins/SelfInjectPlugin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,31 @@ import type { SelfInjectPluginOptions, Source, Compiler } from './types';

export { type SelfInjectPluginOptions } from './types';

/**
* Generates a runtime URL expression for a given path.
*
* This function constructs a URL string using the `runtime.getURL` method
* from either the `globalThis.browser` or `chrome` object, depending on
* which one is available in the global scope.
*
* @param path - The path of the runtime URL.
* @returns The constructed runtime URL string.
*/
const getRuntimeURLExpression = (path: string) =>
`(globalThis.browser||chrome).runtime.getURL(${JSON.stringify(path)})`;

/**
* Default options for the SelfInjectPlugin.
*/
const defaultOptions = {
// The default `sourceUrlExpression` is configured for browser extensions.
// It generates the absolute url of the given file as an extension url.
// e.g., `chrome-extension://<extension-id>/scripts/inpage.js`
sourceUrlExpression: (filename: string) =>
`(globalThis.browser||chrome).runtime.getURL(${JSON.stringify(filename)})`,
sourceUrlExpression: getRuntimeURLExpression,
// The default `nonceExpression` is configured for browser extensions.
// It generates the absolute url of a path as an extension url in base64.
// e.g., `Y2hyb21lLWV4dGVuc2lvbjovLzxleHRlbnNpb24taWQ+Lw==`
nonceExpression: (path: string) => `btoa(${getRuntimeURLExpression(path)})`,
} satisfies SelfInjectPluginOptions;

/**
Expand Down Expand Up @@ -142,6 +158,7 @@ export class SelfInjectPlugin {
`\`\\n//# sourceURL=\${${this.options.sourceUrlExpression(file)}};\``,
);
newSource.add(`;`);
newSource.add(`s.nonce=${this.options.nonceExpression('/')};`);
// add and immediately remove the script to avoid modifying the DOM.
newSource.add(`d.documentElement.appendChild(s).remove()`);
newSource.add(`}`);
Expand Down
15 changes: 13 additions & 2 deletions development/webpack/utils/plugins/SelfInjectPlugin/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export type SelfInjectPluginOptions = {
* will be injected into matched file to provide a sourceURL for the self
* injected script.
*
* Defaults to `(filename: string) => (globalThis.browser||globalThis.chrome).runtime.getURL("${filename}")`
* Defaults to `(filename: string) => (globalThis.browser||chrome).runtime.getURL("${filename}")`
*
* @example Custom
* ```js
Expand All @@ -39,11 +39,22 @@ export type SelfInjectPluginOptions = {
*
* ```js
* {
* sourceUrlExpression: (filename) => `(globalThis.browser||globalThis.chrome).runtime.getURL("${filename}")`
* sourceUrlExpression: (filename) => `(globalThis.browser||chrome).runtime.getURL("${filename}")`
* }
* ```
* @param filename - the chunk's relative filename as it will exist in the output directory
* @returns
*/
sourceUrlExpression?: (filename: string) => string;
/**
* A function that returns a JavaScript expression escaped as a string which
* will be injected into matched file to set a nonce for the self
* injected script.
*
* Defaults to `(path: string) => btoa((globalThis.browser||chrome).runtime.getURL("${path}"))`
*
* @param path - the path to be encoded as a nonce
* @returns
*/
nonceExpression?: (path: string) => string;
};
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,7 @@
"@types/redux-mock-store": "1.0.6",
"@types/remote-redux-devtools": "^0.5.5",
"@types/selenium-webdriver": "^4.1.19",
"@types/serve-handler": "^6.1.4",
"@types/sinon": "^10.0.13",
"@types/sprintf-js": "^1",
"@types/w3c-web-hid": "^1.0.3",
Expand Down
Loading

0 comments on commit 54c563e

Please sign in to comment.