fix(wrappers): guard against undefined Browser.cookies in getChatGptAccessToken#965
Open
octo-patch wants to merge 1 commit intoChatGPTBox-dev:masterfrom
Open
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>
Contributor
📝 WalkthroughWalkthroughModified Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 |
Contributor
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
|
Contributor
Code Review by Qodo
1. Missing session-cookie fallback
|
![gemini-code-assist[bot]](https://cdn.statically.io/img/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.
Comment on lines
+18
to
+25
| 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://cdn.statically.io/img/avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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://cdn.statically.io/img/avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+18
to
28
| 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, |
Contributor
There was a problem hiding this comment.
1. Missing session-cookie fallback 🐞 Bug ≡ Correctness
When Browser.cookies is unavailable, getChatGptAccessToken still sends an always-present (often empty) Cookie header and does not set credentials: 'include', so the /api/auth/session request may not carry any session cookies and will keep returning no accessToken (throwing UNAUTHORIZED). This makes the new guard avoid the crash but still breaks token retrieval in the target environments.
Agent Prompt
### Issue description
`getChatGptAccessToken()` now avoids a crash when `Browser.cookies` is missing, but the fallback still makes a `fetch('https://chatgpt.com/api/auth/session')` request that may not include any session cookies (it always sets `headers.Cookie`, often to `''`, and does not set `credentials: 'include'`). This can cause the endpoint to return no `accessToken`, resulting in persistent `UNAUTHORIZED` errors in the very environments this PR targets.
### Issue Context
A more robust pattern already exists in `chatgpt-web.mjs`: always use `credentials: 'include'`, and only set a manual `Cookie` header when you actually have a non-empty cookie string.
### Fix Focus Areas
- src/services/wrappers.mjs[18-30]
### Suggested change
- Build `headers` so `Cookie` is only included when `cookie` is non-empty (e.g. `...(cookie && { Cookie: cookie })`).
- Add `credentials: 'include'` to the `fetch` options so environments without `Browser.cookies` still have a chance to send existing session cookies automatically.
- (Optional but recommended) Add/extend a unit test to cover the `Browser.cookies` undefined case and assert that the request either omits `Cookie` when empty and/or sets `credentials: 'include'`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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