fix(core): preserve refresh_token in file-based cacheCredentials and … - #27463
fix(core): preserve refresh_token in file-based cacheCredentials and …#27463ProthamD wants to merge 4 commits into
Conversation
…make deleteCredentials idempotent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical issues in the OAuth credential management flow. It ensures that file-based storage correctly merges new access tokens with existing refresh tokens rather than overwriting them, and improves the robustness of the keychain deletion process by making it idempotent. These changes stabilize the re-authentication cycle and prevent errors when clearing sessions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces robust credential management handling. Specifically, it preserves the existing refresh_token in cacheCredentials when a token rotation event from Google omits it, and it treats missing credentials as a successful no-op in deleteCredentials within KeychainTokenStorage to ensure idempotency. Corresponding unit tests have been added and updated to verify these behaviors. I have no feedback to provide as there are no active review comments.
|
📊 PR Size: size/M
|
|
hello @scidomino , if you have some time , could you please give this PR a review? |
|
I no longer work on GCLI. |
|
Ohh okey |
Fixes #21691
Situation:
Upstream PR #26924 resolved the
refresh_tokenoverwrite issue for users withGEMINI_FORCE_ENCRYPTED_FILE_STORAGE=true. However, issue #21691 still persisted for users relying on the default file-based storage (cacheCredentialsinoauth2.ts), causingrefresh_tokenloss during Google access token rotation. Additionally, thedeleteCredentialsmethod threw an error if the credential didn't exist, leading to a cascading failure loop blocking re-authentication after session expiration.Task:
Fix the file-based credentials cache to merge the new access token with the existing refresh token, and refactor the keychain token deletion to be idempotent.
Action:
cacheCredentialsinoauth2.tsto read and parse the existingoauth_creds.jsonfile. It now defensively merges the existingrefresh_token(after verifying it's a non-empty string) with the new payload to prevent overwriting. Caught errors are correctly narrowed toENOENTto prevent swallowing valid I/O exceptions.deleteCredentialsinKeychainTokenStorageto silently no-op when the credential to be deleted is missing, ensuring idempotent behaviour during session clear loops.oauth2.test.tsandkeychain-token-storage.test.tsto verify these behaviors.Result:
Users on file-based storage will no longer lose their
refresh_tokenduring normal rotation cycles. The re-authentication flow is now stable sinceclearCredentialshandles missing sessions gracefully without crashing. 53/53 tests pass.