Move all fetch() requests to background scripts #426
Conversation
src/js/background/background.js
Outdated
| response = await fetchRequestFromBackground( | ||
| m.url, | ||
| m.fetchMethod, | ||
| m.body, | ||
| m.opts |
There was a problem hiding this comment.
Question: I didn't run into any issues but is there any danger passing null/undefined values here? (Most of the time when this message is called, fetchMethod, body and opts are blank.
There was a problem hiding this comment.
I wouldn't think so, but your asking the question makes me nervous :P What danger were you thinking about?
There was a problem hiding this comment.
Rewrote this function so that if the URL param is not included, it fails. Everything else has a defined "default".
| const cookieString = | ||
| typeof document.cookie === "string" ? document.cookie : ""; | ||
|
|
||
| const cookieStringArray = cookieString | ||
| .split(";") | ||
| .map((individualCookieString) => individualCookieString.split("=")) | ||
| .map(([cookieKey, cookieValue]) => [ | ||
| cookieKey.trim(), | ||
| cookieValue.trim(), | ||
| ]); | ||
|
|
||
| const [_csrfCookieKey, csrfCookieValue] = cookieStringArray.find( | ||
| ([cookieKey, _cookieValue]) => cookieKey === "csrftoken" | ||
| ); | ||
|
|
||
| await browser.storage.local.set({ csrfCookieValue: csrfCookieValue }); | ||
|
|
||
|
|
There was a problem hiding this comment.
I don't think you need a CSRF cookie if you're not making the request from the Relay website?
| const cookieString = | |
| typeof document.cookie === "string" ? document.cookie : ""; | |
| const cookieStringArray = cookieString | |
| .split(";") | |
| .map((individualCookieString) => individualCookieString.split("=")) | |
| .map(([cookieKey, cookieValue]) => [ | |
| cookieKey.trim(), | |
| cookieValue.trim(), | |
| ]); | |
| const [_csrfCookieKey, csrfCookieValue] = cookieStringArray.find( | |
| ([cookieKey, _cookieValue]) => cookieKey === "csrftoken" | |
| ); | |
| await browser.storage.local.set({ csrfCookieValue: csrfCookieValue }); |
There was a problem hiding this comment.
I dug into the git-blame on the cookie string origins. It was me. 🪞👨
I think the initial reasoning was to verify if a user had the server-storage pref enabled? I'll remove this and confirm this doesn't break anything.
There was a problem hiding this comment.
Correction: I wasn't reading the full use-case of it. The CSRF token is passed in the Headers object for the follow API calls:
- patchMaskInfo
- getServerStoragePref
- postReportWebcompatIssue
- getAliasesFromServer
- makeRelayAddress
I did remove the CSRF header setter for the fetchRequestFromBackground() function.
There was a problem hiding this comment.
Ah OK, I wasn't aware that there was already code in background.js that read csrfCookieValue from the extension storage. AFAIK that's not needed, but it's outside of the scope of this PR then anyway.
src/js/background/background.js
Outdated
| @@ -25,6 +25,32 @@ browser.runtime.onInstalled.addListener(async (details) => { | |||
| } | |||
| }); | |||
|
|
|||
|
|
|||
| async function fetchRequestFromBackground(url, fetchMethod = "GET", body = null, opts=null) { | |||
There was a problem hiding this comment.
Wouldn't it make sense to either move fetchMethod onto opts, or alternatively, to just accept the entire init parameter (i.e. the second parameter to fetch)?
There was a problem hiding this comment.
Refactored! It's now one larger object with options either default-defined or null'd.
src/js/background/background.js
Outdated
| response = await fetchRequestFromBackground( | ||
| m.url, | ||
| m.fetchMethod, | ||
| m.body, | ||
| m.opts |
There was a problem hiding this comment.
I wouldn't think so, but your asking the question makes me nervous :P What danger were you thinking about?
This is part of a pre-migration to Manifest v3.
4a1c299
to
2ffa9f4
Compare
…one object, with the URL param being the only required param. Also added JSDoc for more readability of the function
…ackground message pass method
| const cookieString = | ||
| typeof document.cookie === "string" ? document.cookie : ""; | ||
|
|
||
| const cookieStringArray = cookieString | ||
| .split(";") | ||
| .map((individualCookieString) => individualCookieString.split("=")) | ||
| .map(([cookieKey, cookieValue]) => [ | ||
| cookieKey.trim(), | ||
| cookieValue.trim(), | ||
| ]); | ||
|
|
||
| const [_csrfCookieKey, csrfCookieValue] = cookieStringArray.find( | ||
| ([cookieKey, _cookieValue]) => cookieKey === "csrftoken" | ||
| ); | ||
|
|
||
| await browser.storage.local.set({ csrfCookieValue: csrfCookieValue }); | ||
|
|
||
|
|
There was a problem hiding this comment.
Ah OK, I wasn't aware that there was already code in background.js that read csrfCookieValue from the extension storage. AFAIK that's not needed, but it's outside of the scope of this PR then anyway.
Summary:
This is part of a pre-migration to Manifest v3. The only content script where
fetch()requests were happening was in theget_profile_data.jsscript, which only runs on the/accounts/profile/*pages.Links:
Testing: