fix(core): handle missing credentials gracefully in deleteCredentials#21949
fix(core): handle missing credentials gracefully in deleteCredentials#21949mvanhorn wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 enhances the robustness of credential deletion mechanisms by preventing Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 modifies deleteCredentials in FileTokenStorage and KeychainTokenStorage to handle non-existent credentials gracefully by not throwing an error, making the operation idempotent. This change is intended to prevent cascading errors in authentication flows. The tests have been updated accordingly to reflect the new behavior. While the change is correct for deleteCredentials, a related method in KeychainTokenStorage remains inconsistent, which could lead to similar issues in other code paths.
| async deleteCredentials(serverName: string): Promise<void> { | ||
| const sanitizedName = this.sanitizeServerName(serverName); | ||
| const deleted = await this.keychainService.deletePassword(sanitizedName); | ||
|
|
||
| if (!deleted) { | ||
| throw new Error(`No credentials found for ${serverName}`); | ||
| } | ||
| await this.keychainService.deletePassword(sanitizedName); | ||
| } |
There was a problem hiding this comment.
This change correctly makes deleteCredentials idempotent. For consistency, I recommend applying the same logic to the deleteSecret method in this file. Currently, deleteSecret (lines 166-173) throws an error if the secret is not found. This inconsistency can lead to bugs similar to the one this PR is fixing, but in code paths that use secrets instead of credentials.
gemini-cli
bot
added
the
priority/p1
Wrap credential deletion in try/catch to prevent cascading errors when credentials don't exist (e.g., double logout, auth-switch). Fixes google-gemini#21768
mvanhorn
force-pushed
the
osc/21768-fix-delete-credentials
branch
from
3ecd2eb to
8636b05
Compare
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
1 participant
Summary
KeychainTokenStorage.deleteCredentials()andFileTokenStorage.deleteCredentials()throw errors when the credential doesn't exist, causing cascading "Failed to clear OAuth credentials" errors during re-auth flows.Changes
packages/core/src/mcp/token-storage/keychain-token-storage.ts:if (!deleted) throwcheck - now silently succeeds when entry doesn't existpackages/core/src/mcp/token-storage/file-token-storage.ts:Tests updated:
keychain-token-storage.test.ts: Added test for deleting non-existent credentialsfile-token-storage.test.ts: Changed "should throw" test to "should not throw"Scenarios fixed
/auth logouttwice)Fixes #21768
This contribution was developed with AI assistance (Claude Code).