-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
fix: mv2 firefox csp header #27770
base: develop
Are you sure you want to change the base?
fix: mv2 firefox csp header #27770
Changes from all commits
f8014e2
51e7033
c6bd1bd
1905247
cd551fd
a35b17e
d1a84c2
885c320
ba383e4
c048d41
dc0d053
95b3ad8
ab2896d
9a7cd35
d1e0bce
267a35c
52053ab
8f68b2a
9baaa86
fe9188e
f32f598
2e53360
f72aa1d
a939b23
fe6f667
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ 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 migrations from './migrations'; | ||
import Migrator from './lib/migrator'; | ||
import ExtensionPlatform from './platforms/extension'; | ||
|
@@ -333,6 +334,29 @@ 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() { | ||
browser.webRequest.onHeadersReceived.addListener( | ||
({ responseHeaders }) => { | ||
for (const header of responseHeaders) { | ||
if (header.name.toLowerCase() === 'content-security-policy') { | ||
header.value = addNonceToCsp( | ||
header.value, | ||
btoa(browser.runtime.getURL('/')), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: you could precompute this once outside the listener callback and reuse the value; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: a comment about how |
||
); | ||
} | ||
} | ||
return { responseHeaders }; | ||
}, | ||
{ types: ['main_frame', 'sub_frame'], urls: ['http://*/*', 'https://*/*'] }, | ||
['blocking', 'responseHeaders'], | ||
); | ||
} | ||
|
||
// These are set after initialization | ||
let connectRemote; | ||
let connectExternalExtension; | ||
|
@@ -479,6 +503,12 @@ 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 | ||
const platformName = getPlatform(); | ||
if (platformName === PLATFORM_FIREFOX) { | ||
overrideContentSecurityPolicyHeader(); | ||
} | ||
} | ||
await sendReadyMessageToTabs(); | ||
log.info('MetaMask initialization complete.'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(browser.runtime.getURL('/'));d.documentElement.appendChild(s).remove();}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might need to be |
||
writeFileSync(filePath, js, 'utf8'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,6 +142,7 @@ export class SelfInjectPlugin { | |
`\`\\n//# sourceURL=\${${this.options.sourceUrlExpression(file)}};\``, | ||
); | ||
newSource.add(`;`); | ||
newSource.add(`s.nonce=btoa(browser.runtime.getURL('/'));`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the So this line here would become something like: newSource.add(`s.nonce=${this.options.nonceExpression(file)}`); |
||
// add and immediately remove the script to avoid modifying the DOM. | ||
newSource.add(`d.documentElement.appendChild(s).remove()`); | ||
newSource.add(`}`); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
import { addNonceToCsp } from './add-nonce-to-csp'; | ||
|
||
describe('addNonceToCsp', () => { | ||
it('empty string', () => { | ||
const input = ''; | ||
const expected = ''; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('one empty directive', () => { | ||
const input = 'script-src'; | ||
const expected = `script-src 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('one directive, one value', () => { | ||
const input = 'script-src default.example'; | ||
const expected = `script-src default.example 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('one directive, two values', () => { | ||
const input = "script-src 'self' default.example"; | ||
const expected = `script-src 'self' default.example 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('multiple directives', () => { | ||
const input = | ||
"default-src 'self'; script-src 'unsafe-eval' scripts.example; object-src; style-src styles.example"; | ||
const expected = `default-src 'self'; script-src 'unsafe-eval' scripts.example 'nonce-test'; object-src; style-src styles.example`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('no applicable directive', () => { | ||
const input = 'img-src https://example.com'; | ||
const expected = `img-src https://example.com`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('non-ASCII directives', () => { | ||
const input = 'script-src default.example;\u0080;style-src style.example'; | ||
const expected = `script-src default.example 'nonce-test';\u0080;style-src style.example`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('uppercase directive names', () => { | ||
const input = 'SCRIPT-SRC DEFAULT.EXAMPLE'; | ||
const expected = `SCRIPT-SRC DEFAULT.EXAMPLE 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('duplicate directive names', () => { | ||
const input = | ||
'default-src default.example; script-src script.example; script-src script.example'; | ||
const expected = `default-src default.example; script-src script.example 'nonce-test'; script-src script.example`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('nonce value contains script-src', () => { | ||
const input = | ||
"default-src 'self' 'nonce-script-src'; script-src 'self' https://example.com"; | ||
const expected = `default-src 'self' 'nonce-script-src'; script-src 'self' https://example.com 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('url value contains script-src', () => { | ||
const input = | ||
"default-src 'self' https://script-src.com; script-src 'self' https://example.com"; | ||
const expected = `default-src 'self' https://script-src.com; script-src 'self' https://example.com 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('fallback to default-src', () => { | ||
const input = `default-src 'none'`; | ||
const expected = `default-src 'none' 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
|
||
it('keep ascii whitespace characters', () => { | ||
const input = ' script-src default.example '; | ||
const expected = ` script-src default.example 'nonce-test'`; | ||
const output = addNonceToCsp(input, 'test'); | ||
expect(output).toBe(expected); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// ASCII whitespace is U+0009 TAB, U+000A LF, U+000C FF, U+000D CR, or U+0020 SPACE. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow. I love how simple this ended up being in the end! Quite the journey you went on to get here! Great work! |
||
// See <https://infra.spec.whatwg.org/#ascii-whitespace>. | ||
const ASCII_WHITESPACE_CHARS = ['\t', '\n', '\f', '\r', ' '].join(''); | ||
|
||
const matchDirective = (directive: string) => | ||
/* eslint-disable require-unicode-regexp */ | ||
new RegExp( | ||
`^([${ASCII_WHITESPACE_CHARS}]*${directive}[${ASCII_WHITESPACE_CHARS}]*)`, // Match the directive and surrounding ASCII whitespace | ||
'is', // Case-insensitive, including newlines | ||
); | ||
const matchScript = matchDirective('script-src'); | ||
const matchDefault = matchDirective('default-src'); | ||
|
||
/** | ||
* Adds a nonce to a Content Security Policy (CSP) string. | ||
* | ||
* @param text - The Content Security Policy (CSP) string to add the nonce to. | ||
* @param nonce - The nonce to add to the Content Security Policy (CSP) string. | ||
* @returns The updated Content Security Policy (CSP) string. | ||
*/ | ||
export const addNonceToCsp = (text: string, nonce: string) => { | ||
const formattedNonce = ` 'nonce-${nonce}'`; | ||
const directives = text.split(';'); | ||
const scriptIndex = directives.findIndex((directive) => | ||
matchScript.test(directive), | ||
); | ||
if (scriptIndex >= 0) { | ||
directives[scriptIndex] += formattedNonce; | ||
} else { | ||
const defaultIndex = directives.findIndex((directive) => | ||
matchDefault.test(directive), | ||
); | ||
if (defaultIndex >= 0) { | ||
directives[defaultIndex] += formattedNonce; | ||
} | ||
} | ||
return directives.join(';'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to check some additional properties before overriding the CSP headers, as there is no reason to modify the headers if we aren't going to end up injecting inpage.js.
For example, we only every try injecting the inpage provider when our function
shouldInjectProvider
returnstrue
. We can't useshouldInjectProvider
as is here in the background script, becauseshouldInjectProvider
uses the page'sdocument
andwindow
in its checks, but we could use parts of it (with a little refactoring).If
suffixCheck
andblockedDomainCheck
could be refactored to take aURL
as a param, instead of relying onwindow.location
as they do now, we could use these existing functions to further limit when we might modify the CSP headers.