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.

Sign up for GitHub

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

Jump to bottom

fix: mv2 firefox csp header #27770

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from
+170 −3
Open

fix: mv2 firefox csp header #27770

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f8014e2
fix: mv2 firefox csp header
itsyoboieltr Oct 10, 2024
51e7033
Merge branch 'develop' into fix-firefox-csp
itsyoboieltr Oct 11, 2024
c6bd1bd
feat: nonce
itsyoboieltr Oct 14, 2024
1905247
fix: nonce
itsyoboieltr Oct 15, 2024
cd551fd
fix: random url
itsyoboieltr Oct 15, 2024
a35b17e
Merge branch 'develop' into fix-firefox-csp
itsyoboieltr Oct 15, 2024
d1a84c2
fix: webpack tests
itsyoboieltr Oct 15, 2024
885c320
fix: regex
itsyoboieltr Oct 15, 2024
ba383e4
fix: regex whitespace
itsyoboieltr Oct 15, 2024
c048d41
feat: addNonceToCsp with tests
itsyoboieltr Oct 15, 2024
dc0d053
Merge branch 'develop' into fix-firefox-csp
itsyoboieltr Oct 17, 2024
95b3ad8
fix: limit types
itsyoboieltr Oct 17, 2024
ab2896d
fix: getPlatform
itsyoboieltr Oct 17, 2024
9a7cd35
feat: CSP
itsyoboieltr Oct 17, 2024
d1e0bce
fix: new RegExp
itsyoboieltr Oct 18, 2024
267a35c
feat: comments
itsyoboieltr Oct 18, 2024
52053ab
feat: array of directives
itsyoboieltr Oct 18, 2024
8f68b2a
fix: lint
itsyoboieltr Oct 21, 2024
9baaa86
feat: respect whitespace
itsyoboieltr Oct 21, 2024
fe9188e
feat: make it simpler
itsyoboieltr Oct 22, 2024
f32f598
fix: filename
itsyoboieltr Oct 22, 2024
2e53360
Merge branch 'develop' into fix-firefox-csp
itsyoboieltr Oct 22, 2024
f72aa1d
fix: casing
itsyoboieltr Oct 22, 2024
a939b23
feat: tests
itsyoboieltr Oct 22, 2024
fe6f667
Merge branch 'develop' into fix-firefox-csp
itsyoboieltr Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions app/scripts/background.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,27 @@ function maybeDetectPhishing(theController) {
);
}

/**
* Overrides the Content-Security-Policy Header, acting as a workaround for an MV2 Firefox Bug.
itsyoboieltr marked this conversation as resolved.
Show resolved Hide resolved
*/
function overrideContentSecurityPolicyHeader() {
browser.webRequest.onHeadersReceived.addListener(
({ responseHeaders }) => {
for (const header of responseHeaders) {
Copy link
Contributor

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 returns true. We can't use shouldInjectProvider as is here in the background script, because shouldInjectProvider uses the page's document and window in its checks, but we could use parts of it (with a little refactoring).

If suffixCheck and blockedDomainCheck could be refactored to take a URL as a param, instead of relying on window.location as they do now, we could use these existing functions to further limit when we might modify the CSP headers.

if (header.name.toLowerCase() === 'content-security-policy') {
header.value = header.value.replace(
/(^|;\s*)script-src([^;]*)/u,
itsyoboieltr marked this conversation as resolved.
Show resolved Hide resolved
(match) => `${match} 'nonce-${btoa(browser.runtime.getURL('/'))}'`,
);
}
}
return { responseHeaders };
},
{ urls: ['http://*/*', 'https://*/*'] },
['blocking', 'responseHeaders'],
itsyoboieltr marked this conversation as resolved.
Show resolved Hide resolved
);
}

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

if (!isManifestV3) {
await loadPhishingWarningPage();
const { name } = await browser.runtime.getBrowserInfo();
itsyoboieltr marked this conversation as resolved.
Show resolved Hide resolved
if (name === PLATFORM_FIREFOX) {
overrideContentSecurityPolicyHeader();
}
}
await sendReadyMessageToTabs();
log.info('MetaMask initialization complete.');
Expand Down
2 changes: 1 addition & 1 deletion development/build/utils.js
View file
Open in desktop
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(browser.runtime.getURL('/'));d.documentElement.appendChild(s).remove();}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to be (globalThis.browser||chrome).runtime.getURL('/'), as chrome doesn't user the global browser property.

writeFileSync(filePath, js, 'utf8');
}

Expand Down
4 changes: 2 additions & 2 deletions development/webpack/test/plugins.SelfInjectPlugin.test.ts
View file
Open in desktop
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(browser.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(browser.runtime.getURL('/'));d.documentElement.appendChild(s).remove()}`,
);
}

Expand Down
1 change: 1 addition & 0 deletions development/webpack/utils/plugins/SelfInjectPlugin/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ export class SelfInjectPlugin {
`\`\\n//# sourceURL=\${${this.options.sourceUrlExpression(file)}};\``,
);
newSource.add(`;`);
newSource.add(`s.nonce=btoa(browser.runtime.getURL('/'));`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the SelfInjectPlugin's options should be extended to accept a nonceExpression option, similar to its existing sourceURLExpression option, that defaults to the string "(globalThis.browser||chrome).runtime.getURL('/')".

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(`}`);
Expand Down