-
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
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. |
Quality Gate failedFailed conditions |
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.
Looks great!
Can you add an E2E test that shows that this works?
The webpack e2e tests don't currently run in FireFox, and that's fine... if we ever switch to using only the webpack builds (and thus run them in FireFox), and they end up failing for whatever reason, we'll just have to fix it at that time :-)
A note for those unlucky enough to have to read this PR review way in the future: we are aware this code may fail in the unlikely scenario where a CSP header contains a valid directive, followed by invalid CSP characters (like é
), followed by the string; script-src
. e.g. Content-Security-Policy: default-src none; éééé<invalid>ééé; script-src self
. Fully parsing a CSP directive, determining which parts will be accepted by the browser, and extending the directive is just too complex to warrant solving it right now.
function overrideContentSecurityPolicyHeader() { | ||
browser.webRequest.onHeadersReceived.addListener( | ||
({ responseHeaders }) => { | ||
for (const header of responseHeaders) { |
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
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 = addNonceToCsp( | ||
header.value, | ||
btoa(browser.runtime.getURL('/')), |
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.
nit: you could precompute this once outside the listener callback and reuse the value; browser.runtime.getURL('/')
is unique per install (on FireFox).
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 comment
The reason will be displayed to describe this comment to others. Learn more.
nit: a comment about how browser.runtime.getURL('/')
is unique per install would be useful for people reading this code in the future.
@@ -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 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.
@@ -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 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)}`);
@@ -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 comment
The 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!
Description
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, https://github.com/MetaMask/MetaMask-planning/issues/3342
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist