fix(wrappers): guard against undefined Browser.cookies in getChatGptAccessToken#965
Conversation
…ccessToken In some browser contexts (e.g. Safari, or environments where the cookies permission is unavailable), Browser.cookies can be undefined, causing: "can't access property 'getAll', a.cookies is undefined" chatgpt-web.mjs already uses this guard pattern. Apply the same defensive check before calling Browser.cookies.getAll in getChatGptAccessToken, allowing the session endpoint to be reached without cookies when the API is unavailable. Fixes ChatGPTBox-dev#912 Co-Authored-By: Octopus <liyuan851277048@icloud.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughgetChatGptAccessToken now checks for Browser.cookies.getAll before reading cookies; if unavailable (or Browser.cookies is undefined) it omits the ChangesChatGPT Cookie Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoGuard against undefined Browser.cookies in getChatGptAccessToken
WalkthroughsDescription• Guard against undefined Browser.cookies in getChatGptAccessToken • Prevents runtime error in Safari and restricted permission contexts • Matches existing defensive pattern used in chatgpt-web.mjs • Session endpoint still accessible via credentials when cookies unavailable Diagramflowchart LR
A["getChatGptAccessToken called"] --> B{"Browser.cookies exists?"}
B -->|Yes| C["Get cookies from Browser.cookies.getAll"]
B -->|No| D["Use empty cookie string"]
C --> E["Fetch session endpoint"]
D --> E
E --> F["Return accessToken"]
File Changes1. src/services/wrappers.mjs
|
Code Review by Qodo
1.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a safety check for the Browser.cookies API in the getChatGptAccessToken function to prevent potential errors when the API is unavailable. The review feedback suggests refactoring the implementation to use optional chaining and a ternary operator for better conciseness and to avoid variable shadowing within the map callback.
| let cookie = '' | ||
| if (Browser.cookies && Browser.cookies.getAll) { | ||
| cookie = (await Browser.cookies.getAll({ url: 'https://chatgpt.com/' })) | ||
| .map((cookie) => { | ||
| return `${cookie.name}=${cookie.value}` | ||
| }) | ||
| .join('; ') | ||
| } |
There was a problem hiding this comment.
The variable cookie is shadowed by the parameter in the .map() callback, which is a poor practice that can lead to confusion and potential bugs. Additionally, the logic can be simplified by using a const declaration with a ternary operator and optional chaining, making the code more concise and idiomatic while avoiding the let declaration.
const cookie = (Browser.cookies?.getAll
? await Browser.cookies.getAll({ url: 'https://chatgpt.com/' })
: [])
.map((c) => `${c.name}=${c.value}`)
.join('; ')![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/services/wrappers.mjs (1)
18-25: Guard correctly fixes the runtime error; minor polish and a heads-up on sibling functions.The added check resolves issue
#912and matches the pattern used inchatgpt-web.mjs. A couple of small notes:
- The map callback parameter
cookie(line 21) shadows the outerlet cookie(line 18). Renaming toc(or similar) avoids the shadowing and keeps the inner reference unambiguous.- Optional chaining would express the same intent more concisely:
if (Browser.cookies?.getAll).- Heads-up (out of scope for this PR):
getBingAccessToken(line 44),getBardCookies(line 48), andgetClaudeSessionKey(line 54) still callBrowser.cookies.get(...)unconditionally and will throw the samea.cookies is undefinederror in browser contexts whereBrowser.cookiesis unavailable. Consider applying the same guard there in a follow-up.♻️ Optional refactor for the changed block
- let cookie = '' - if (Browser.cookies && Browser.cookies.getAll) { - cookie = (await Browser.cookies.getAll({ url: 'https://chatgpt.com/' })) - .map((cookie) => { - return `${cookie.name}=${cookie.value}` - }) - .join('; ') - } + let cookie = '' + if (Browser.cookies?.getAll) { + cookie = (await Browser.cookies.getAll({ url: 'https://chatgpt.com/' })) + .map((c) => `${c.name}=${c.value}`) + .join('; ') + }Want me to open a follow-up issue to apply the same guard to
getBingAccessToken,getBardCookies, andgetClaudeSessionKey?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/wrappers.mjs` around lines 18 - 25, The current block declares let cookie then shadows it in the map callback and uses a verbose guard; rename the map parameter (e.g., c) to avoid shadowing, replace the guard with optional chaining for clarity (check Browser.cookies?.getAll), and ensure the result assignment still builds the cookie string from Browser.cookies?.getAll({ url: 'https://chatgpt.com/' }) mapping each entry; also note the sibling functions getBingAccessToken, getBardCookies, and getClaudeSessionKey call Browser.cookies.get(...) without a guard—plan a follow-up to add similar optional chaining guards there to prevent undefined access errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/wrappers.mjs`:
- Around line 18-25: The current block declares let cookie then shadows it in
the map callback and uses a verbose guard; rename the map parameter (e.g., c) to
avoid shadowing, replace the guard with optional chaining for clarity (check
Browser.cookies?.getAll), and ensure the result assignment still builds the
cookie string from Browser.cookies?.getAll({ url: 'https://chatgpt.com/' })
mapping each entry; also note the sibling functions getBingAccessToken,
getBardCookies, and getClaudeSessionKey call Browser.cookies.get(...) without a
guard—plan a follow-up to add similar optional chaining guards there to prevent
undefined access errors.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Guards getChatGptAccessToken() against environments where the WebExtension cookies API is unavailable, preventing runtime crashes when attempting to read ChatGPT cookies.
Changes:
- Add a defensive check for
Browser.cookies?.getAllbefore reading cookies. - Default the
Cookieheader value to an empty string when cookies cannot be read.
Comments suppressed due to low confidence (1)
src/services/wrappers.mjs:30
- When
Browser.cookiesis unavailable, this fetch currently runs withoutcredentials: 'include', so the browser’s existing session cookies likely won’t be sent and the session endpoint will always returnUNAUTHORIZEDeven if the user is logged in. Consider addingcredentials: 'include'and only setting theCookieheader whencookieis non-empty (aligning with the pattern insrc/services/apis/chatgpt-web.mjs).
const resp = await fetch('https://chatgpt.com/api/auth/session', {
headers: {
Cookie: cookie,
},
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map((cookie) => { | ||
| return `${cookie.name}=${cookie.value}` |
| if (Browser.cookies && Browser.cookies.getAll) { | ||
| cookie = (await Browser.cookies.getAll({ url: 'https://chatgpt.com/' })) | ||
| .map((cookie) => { | ||
| return `${cookie.name}=${cookie.value}` | ||
| }) | ||
| .join('; ') | ||
| } | ||
| const resp = await fetch('https://chatgpt.com/api/auth/session', { |
|
Please also add a unit test for the Browser.cookies unavailable path, since that is the core branch introduced by this PR. |
…ccessToken Adds two unit tests for the new defensive branch introduced in 5e3329e (issue ChatGPTBox-dev#912): - getAll missing on Browser.cookies (e.g. restricted Firefox contexts) - Browser.cookies entirely undefined Both cases assert that getChatGptAccessToken does not throw before the fetch and that the request goes out with an empty Cookie header. The polyfill state is restored via t.after so other tests are unaffected.
|
Thanks @PeterDaveHello! Pushed commit
Both tests use |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/services/wrappers.mjs:29
getChatGptAccessTokenalways sets aCookieheader (even when cookies are unavailable andcookieis''). This diverges from the existing pattern insrc/services/apis/chatgpt-web.mjswhere theCookieheader is conditionally included only when a non-empty cookie string is available. Consider omitting theCookieheader entirely whenBrowser.cookies/getAllis unavailable (e.g., buildheaderswith a conditional spread) to avoid sending an explicit empty Cookie header and to keep behavior consistent across the codebase.
let cookie = ''
if (Browser.cookies && Browser.cookies.getAll) {
cookie = (await Browser.cookies.getAll({ url: 'https://chatgpt.com/' }))
.map((cookie) => {
return `${cookie.name}=${cookie.value}`
})
.join('; ')
}
const resp = await fetch('https://chatgpt.com/api/auth/session', {
headers: {
Cookie: cookie,
},
| test('getChatGptAccessToken sends empty Cookie header when Browser.cookies.getAll is unavailable', async (t) => { | ||
| t.mock.method(console, 'debug', () => {}) | ||
| setStorage({ tokenSavedOn: Date.now() }) | ||
|
|
||
| // Simulate environments (e.g. some Firefox / restricted contexts) where | ||
| // chrome.cookies.getAll is not exposed. Override on the polyfill object | ||
| // and restore afterwards so other tests are unaffected. | ||
| const originalGetAll = Browser.cookies.getAll | ||
| Object.defineProperty(Browser.cookies, 'getAll', { | ||
| value: undefined, | ||
| writable: true, | ||
| configurable: true, | ||
| }) | ||
| t.after(() => { | ||
| Object.defineProperty(Browser.cookies, 'getAll', { | ||
| value: originalGetAll, | ||
| writable: true, | ||
| configurable: true, | ||
| }) | ||
| }) | ||
|
|
||
| let observedCookieHeader | ||
| t.mock.method(globalThis, 'fetch', async (url, init) => { | ||
| assert.equal(url, 'https://chatgpt.com/api/auth/session') | ||
| observedCookieHeader = init.headers.Cookie | ||
| return { | ||
| status: 200, | ||
| json: async () => ({ accessToken: 'token-without-cookies' }), | ||
| } | ||
| }) | ||
|
|
||
| const token = await getChatGptAccessToken() | ||
| assert.equal(token, 'token-without-cookies') | ||
| assert.equal(observedCookieHeader, '') | ||
| }) |
|
Thanks for adding the regression tests for the I think this still needs one small follow-up before merging. Right now Could you please align this with the existing After that and a green CI run, this should be good to merge. |
- set credentials: 'include' so the browser attaches existing chatgpt.com session cookies in environments where Browser.cookies.getAll cannot build the Cookie header manually - only emit the Cookie header when the assembled cookie string is non-empty, matching the pattern already used in src/services/apis/chatgpt-web.mjs - rename the .map((cookie) => ...) callback parameter so it does not shadow the outer cookie string - update the regression tests to assert the absent Cookie header and presence of credentials: 'include'
|
Thanks @PeterDaveHello! Pushed
|
Fixes #912
Problem
In some browser contexts (e.g. Safari, or environments where the
cookiespermission is unavailable),Browser.cookiescan beundefined. The currentgetChatGptAccessTokenimplementation callsBrowser.cookies.getAll(...)without checking ifBrowser.cookiesexists, causing:Solution
Apply the same defensive guard pattern that already exists in
chatgpt-web.mjs(line 255):In
wrappers.mjs, wrap theBrowser.cookies.getAllcall in the same guard, defaultingcookieto an empty string when cookies are unavailable. The session endpoint athttps://chatgpt.com/api/auth/sessionis still fetched; it will return anaccessTokenif the browser already has a valid session established viacredentials: includeor other means.Testing
tests/unit/services/wrappers-register.test.mjsstill pass with this change.Summary by CodeRabbit