fix: mv2 firefox csp header #27770
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. |
|
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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