-
Notifications
You must be signed in to change notification settings - Fork 14
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
Move all fetch() requests to background scripts #426
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote this function so that if the URL param is not included, it fails. Everything else has a defined "default".
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.
Unexpected/error responses from the API will probably get even more opaque, but I think we're already not really handling them, so as far as I'm concerned that's not really a regression :)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 25255d1
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.js
script, which only runs on the/accounts/profile/*
pages.Links:
Testing: