Repository navigation
fix(auth): prevent infinite auth loop from file contention, headless keyring, and supervisor state drops (#28341) - #29448
Conversation
…keyring, and supervisor state drops - Implement atomic writes and retry loops for settings.json and oauth_creds.json - Automatically fallback to FileKeychain in WSL and headless environments - Preserve selectedType state across supervisor relaunches (exit code 199) - Prevent premature oauth credentials file deletion in AuthDialog Fixes b/561555286 (google-gemini#28341)
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 stability issues causing infinite authentication loops on Windows, WSL, and headless environments. By implementing atomic file I/O, robust retry mechanisms for file reads, and a more reliable state persistence strategy during supervisor relaunches, the changes ensure that authentication settings remain consistent and accessible even under concurrent access or environment-specific constraints. 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
|
|
📊 PR Size: size/L
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for overriding the selected authentication type across application relaunches, implements atomic file operations for settings and credentials to prevent corruption, and adds a bypass for the native keychain in WSL and headless Linux environments. Feedback on these changes highlights a missing check for headless environments in isHeadlessLinuxOrWsl, a potential race condition when sending IPC messages before process exit, and a blocking busy-wait loop in the file read retry logic that should be optimized using Atomics.wait.
…s keyring, IPC drain, and sleep loop - Add headless Linux display check (!DISPLAY && !WAYLAND_DISPLAY) to isHeadlessLinuxOrWsl - Await process.send completion callback before relaunch cleanup to prevent IPC drops - Replace CPU busy-wait loop in readFileWithRetry with Atomics.wait
|
/Gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for overriding the selected authentication type across application relaunches via the GEMINI_CLI_AUTH_OVERRIDE environment variable, implements atomic file writing and retries for settings and credentials to prevent corruption, and adds detection for WSL/headless Linux environments to bypass native keychain usage and avoid libsecret lockups. Feedback on the changes suggests mutating process.env['GEMINI_CLI_AUTH_OVERRIDE'] directly in relaunchAppInChildProcess to ensure the override persists across subsequent iterations of the relaunch loop, as newEnv is recreated on each iteration.
…ss runner - Set process.env.GEMINI_CLI_AUTH_OVERRIDE directly upon receiving auth-selected-type IPC message in relaunchAppInChildProcess and index.ts - Clean up cacheCredentials conflict resolution in oauth2.ts - Add unit test for auth override persistence in relaunch.test.ts
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness improvements to the Gemini CLI, including support for overriding the authentication type via the GEMINI_CLI_AUTH_OVERRIDE environment variable during relaunch transitions, atomic file writes and read retries for settings and credential caching to prevent file corruption, and bypassing the native keychain in WSL or headless Linux environments to avoid libsecret lockups. The review feedback highlights two critical issues: a potential race condition where the process exits before the auth-selected-type IPC message is fully flushed, and an issue where the authentication override is ignored if a stale configuration already exists on disk.
…visor auth override - Add 500ms fallback timeout in relaunchApp when process.send experiences channel backpressure - Unconditionally apply GEMINI_CLI_AUTH_OVERRIDE from supervisor over stale disk settings - Add unit tests for process.send backpressure timeout and unconditional auth override
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for overriding the selected authentication type via the GEMINI_CLI_AUTH_OVERRIDE environment variable during application relaunches, improves file write robustness with atomic operations and retries, and adds a fallback to file-based keychains in WSL and headless Linux environments to prevent libsecret lockups. The review feedback highlights a critical prototype pollution vulnerability in the settings merging logic (applyKeyDiff) when handling untrusted workspace configurations, and recommends refactoring the synchronous file-retry helper to use asynchronous operations to avoid blocking the event loop.
…ENOENT read retries - Guard applyKeyDiff and preserveCommentsOnPropertyDeletion against dangerous prototype keys (__proto__, constructor, prototype) - Immediately rethrow ENOENT in readFileWithRetry without redundant backoff retries - Add unit tests for prototype pollution resistance and ENOENT handling in commentJson.test.ts
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the CLI's robustness and security by supporting authentication overrides via the GEMINI_CLI_AUTH_OVERRIDE environment variable during relaunch, implementing atomic file writes and read-with-retry logic in commentJson.ts, preventing prototype pollution, and bypassing the native keychain in WSL or headless Linux environments. The review feedback recommends extending the read-with-retry mechanism to oauth_creds.json in the core package to mitigate potential file contention issues on Windows/WSL.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness, security, and compatibility improvements. It adds support for propagating authentication overrides (GEMINI_CLI_AUTH_OVERRIDE) across application relaunches via IPC messages and environment variables. To prevent file corruption and handle transient file locks, it implements atomic file writes and read retries for settings and OAuth credentials. It also adds prototype pollution protection when updating JSON settings and automatically bypasses the native keychain in WSL or headless Linux environments to avoid libsecret lockups. Comprehensive unit tests have been added to validate these changes. No review comments were provided, so I have no additional feedback to offer.
Note: Security Review did not run due to the size of the PR.
…ory filesystem errors - Format packages/cli/src/utils/commentJson.test.ts to satisfy Prettier - Explicitly import vitest test primitives in processUtils.test.ts - Handle EACCES alongside ENOENT in eval-inventory.ts for restricted path stat checks - Safeguard empty file handling in commentJson.ts atomic updates - Keep userOriginalSettings synchronized with supervisor auth override in settings.ts
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for environment variable overrides (GEMINI_CLI_AUTH_OVERRIDE) during application relaunch, bypasses the native keychain in WSL or headless Linux environments to prevent lockups, and implements safer file operations (atomic writes and retries) to avoid corruption and handle transient errors. It also adds prototype pollution protection when updating JSON settings. The review feedback highlights that the synchronous busy-wait loop used for retrying file reads in commentJson.ts blocks the Node.js event loop and wastes CPU, suggesting the use of Atomics.wait on a SharedArrayBuffer for a more efficient synchronous sleep.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness, security, and compatibility improvements. Key changes include supporting authentication type overrides across supervisor relaunches via IPC and environment variables, implementing atomic file writes and read retries for settings and OAuth credentials to prevent corruption, protecting against prototype pollution in JSON settings updates, and bypassing native keychains in WSL/headless Linux environments to avoid libsecret lockups. I have no additional feedback to provide as no review comments were submitted.
Note: Security Review did not run due to the size of the PR.
…2E lock contention - Utilize native fs.rmSync maxRetries and retryDelay options for atomic directory deletion - Remove disallowed main-thread Atomics.wait and eliminate long busy-wait loops in test-rig cleanup
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness and security improvements across the codebase. Key changes include supporting an authentication type override via the GEMINI_CLI_AUTH_OVERRIDE environment variable (propagated across application relaunches via IPC), protecting against prototype pollution in JSON settings by ignoring dangerous keys, implementing atomic writes and read retries for settings and OAuth credentials, and automatically bypassing the native keychain in WSL or headless Linux environments to prevent lockups. I have no feedback to provide as there are no review comments to address.
Note: Security Review did not run due to the size of the PR.
Findings1. 🔴 Critical: CI Test Failures in
|
…d auth override - Refactor keychainService to bypass only in WSL, relying on isKeychainFunctional timeout probe for headless Linux - Clean up test environment stubs with vi.unstubAllEnvs() in keychainService.test.ts - Remove synchronous busy-wait CPU spinning in commentJson.ts and test-rig.ts - Consume and delete GEMINI_CLI_AUTH_OVERRIDE once read in settings.ts and clear after spawn in relaunch supervisor - Use numeric maxRetries parameter in readOAuthCredsWithRetry
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness and security improvements across the CLI and core packages. Key changes include handling authentication overrides across application relaunches, preventing prototype pollution in JSON settings updates, bypassing native keychains in WSL environments to avoid lockups, and implementing atomic file writes and retries for settings and OAuth credentials. Additionally, test cleanup was simplified using native retry options. The review feedback suggests refactoring the synchronous file read retry logic in commentJson.ts to use asynchronous operations (fs.promises.readFile) to prevent blocking the event loop.
Note: Security Review did not run due to the size of the PR.
- Read settings files directly via fs.readFileSync to avoid synchronous retry overhead and event loop blocking
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness, security, and environment-compatibility improvements. Key updates include implementing atomic file writes and retry mechanisms for settings and OAuth credentials to prevent corruption, protecting against prototype pollution in JSON settings parsing, and bypassing the native keychain in WSL environments to avoid libsecret lockups. Additionally, it adds support for passing authentication overrides across application relaunches via IPC, simplifies test directory cleanup using native fs.rmSync retries, and handles EACCES errors in the evaluation inventory script. I have no feedback to provide as there are no review comments to assess.
Note: Security Review did not run due to the size of the PR.
Summary
Fixes an infinite authentication loop affecting users on Windows, WSL, and headless environments (#28341). This change resolves file contention collisions with companion tools (like the Gemini Code Assist VS Code extension), provides automatic fallback to encrypted file storage when
libsecretis headless or locked in WSL, and guaranteessecurity.auth.selectedTypeis preserved across supervisor relaunch transitions (exit code 199).Details
oauth_creds.json(cacheCredentialsinoauth2.ts) andsettings.json(updateSettingsFilePreservingFormatincommentJson.ts) now write atomically via sibling temp files (.${filename}.${pid}.${timestamp}.tmp) followed by atomic rename operations.readFileWithRetryto handle transient file lock conditions (e.g.EBUSY/EPERM/ partial reads on Windows NTFS) during concurrent access by external editors and extensions.WSL_DISTRO_NAME/WSL_INTEROP) and headless environments without active GUI prompters (!DISPLAY && !WAYLAND_DISPLAY), cleanly bypassinglibsecret/@github/keytardeadlocks and safely falling back toFileKeychain(~/.gemini/token-storage.json).KeychainService.isKeychainFunctionalprobe against unhandled rejections during test cycles.exit code 199):relaunchApp({ overrideAuthType })now emits anauth-selected-typeIPC event to the supervisor parent before exit, which injectsGEMINI_CLI_AUTH_OVERRIDEinto the spawned child process. This preventssecurity.auth.selectedTypefrom reverting toundefinedif disk synchronization is mid-flight.AuthDialogno longer unconditionally executesclearCachedCredentialFile()upon choosing Google Login, only clearing credentials when switching away from a differing configured authentication type.relaunchAppcalls inside timers are properly caught and handled.Related Issues
Fixes #28341
How to Validate
Ran full unit and integration test suites:
npm test -w @google/gemini-cli-core -- src/services/keychainService.test.ts src/code_assist/oauth2.test.ts src/code_assist/oauth-credential-storage.test.ts(All 65 tests passed)npm test -w @google/gemini-cli -- src/ui/auth/AuthDialog.test.tsx src/utils/processUtils.test.ts src/config/settings.test.ts src/utils/commentJson.test.ts(All 154 tests passed)npm run typecheck&npm run lint(Passed with 0 errors/warnings)Pre-Merge Checklist