Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 582cf2e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
|
❌ Nix build failed Hash mismatch in
Please update |
|
@codex review |
7Sageer
changed the title
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f810f0c460
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const previousService = | ||
| legacyService ?? | ||
| (service === KEYRING_MCP_OAUTH_SERVICE ? undefined : KEYRING_MCP_OAUTH_SERVICE); |
There was a problem hiding this comment.
When KIMI_CODE_HOME selects a nonstandard credentials directory, this assigns the active default-profile service (kimi-code-mcp) as previousService. If both profiles use the same MCP key, a read in the custom profile copies the default profile's grant and deletes the original; writes and removals also delete from that namespace. This leaks credentials across profiles and can log the default profile out, so migration must use an explicitly versioned legacy namespace rather than the namespace still owned by the standard profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This looks like a false positive: the only production call site (McpOAuthStoreAdapter in oauthStore.ts) always passes undefined as legacyService, so previousService is never the default-profile kimi-code-mcp namespace and no cross-profile migration happens. The explicitly-versioned legacy namespace (kimi-code-mcp-legacy-v1) only appears in tests, which is exactly the pattern suggested here.
| const raw = read.value; | ||
| if (raw !== null) return this.reconcileOnHitUnlocked(name, raw, access); | ||
|
|
||
| const first = await access.load(); | ||
| if (first === undefined) return undefined; |
There was a problem hiding this comment.
If the keychain becomes locked or inaccessible after the initial probe, the native binding can return null for getPassword() on both a failed read and an absent credential, as the KeyringEntry contract above documents. With no plaintext fallback, this path returns undefined, so status/login incorrectly treats a keychain-only user as logged out and may start a login that overwrites the existing credential. Confirm absence through findAccounts() or surface OAuthStorageUnavailableError instead of treating every null as a miss.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 5ac4370. On a null read, load() now disambiguates via findAccounts(): a listed account (or an unreachable listing) means the read failed, so it falls back to the plaintext file or throws OAuthStorageUnavailableError instead of reporting logged-out, and never migrates over the surviving keychain entry. Added three tests covering the strict/coexist/degraded paths.
da5e92f to
f81afed
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f81afed53d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| async load(name: string): Promise<TokenInfo | undefined> { | ||
| assertValidTokenName(name); | ||
| return this.withTokenLock(name, async (access) => access.load()); |
There was a problem hiding this comment.
When the credentials directory is readable but not writable—for example, a read-only home mount with a 0444 token file—load() now enters withTokenLock(), which creates or opens a lock target for writing before reading the JSON. The previous implementation could load such a credential, but this path now rejects and prevents authentication despite the token being readable. Keep a read-only load path, or handle failure to create the lock without discarding an otherwise usable credential.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 582cf2e. withFileLock now falls back to running unlocked when lock acquisition fails with EROFS/EACCES/EPERM (e.g. a read-only credentials mount), so load() works against a readable-but-unwritable token file again; writes still fail at their own write step as before. Added a regression test with a 0555 credentials dir.
| } | ||
| if (value === undefined || degraded) return value; | ||
| try { | ||
| api.createEntry(service, key).setPassword(JSON.stringify(value)); |
There was a problem hiding this comment.
When another process is using the document fallback—such as an older build or a process where the keyring probe failed—an already-running proactive refresh can write a grant back after this process has reset the MCP credentials. A subsequent read sees the keyring miss, accepts that fallback value here, and writes it back into the keyring, undoing the explicit logout. The per-key lock does not protect writes made directly through the fallback store, so the removal needs a durable tombstone/removal generation that is checked before migrating fallback data.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 582cf2e. remove() now writes a durable tombstone document (<key>.removed with a digest of the fallback value) into the fallback store, mirroring the token store's removal-marker semantics. On a keyring miss, a fallback value whose digest still matches the tombstone is treated as removed (no migration back into the keyring); a value that changed since the removal counts as a fresh login and is adopted with the marker cleared. write() clears the marker and list() hides marked keys. Note the residual race you describe (a peer's proactive refresh minting a new grant after the reset) is shared with the token store by design: a still-logged-in peer re-publishing a newer valid credential is indistinguishable from a fresh login without server-side revocation — the tombstone guarantees the same-bytes case, which is the parity the token store provides. Added three tests.
f81afed to
5ac4370
Compare
|
@codex review |
5ac4370 to
0ea6def
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac43708f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!degraded) { | ||
| try { | ||
| let sourceService = service; | ||
| let raw = api.createEntry(service, key).getPassword(); |
There was a problem hiding this comment.
When credentials_store = "keyring" has pruned the document fallback and the OS vault becomes locked after the startup probe, the native binding can return null here for both a missing entry and a failed read. The provider-store fix does not cover this separate MCP wrapper: because no exception marks degraded, the code falls through to an empty fallback and returns undefined, so the process-wide MCP auth surface reports a surviving grant as logged out. Confirm absence with findAccounts() or surface an unavailable-storage error before accepting the fallback miss.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
| const credentialsDir = options.credentialsDir ?? join(this.homeDir, 'credentials'); | ||
| this.storage = options.storage ?? new FileTokenStorage(credentialsDir); | ||
| this.storage = | ||
| options.storage ?? resolveTokenStorage(credentialsDir, { configPath: options.configPath }); |
There was a problem hiding this comment.
When a v2 host supplies BootstrapInput.configPath outside <home>/config.toml, this selection reads the inferred home config unless every caller forwards configPath; OAuthToolkitService in packages/agent-core-v2/src/app/auth/authService.ts still constructs the toolkit with only homeDir and identity. Consequently managed-provider OAuth ignores the active config's credentials_store setting—for example, explicit keyring mode silently behaves as auto and retains plaintext—while the MCP store uses the real bootstrap config path. Pass bootstrap.configPath into that toolkit construction.
Useful? React with 👍 / 👎.
Store OAuth credentials (managed provider logins and MCP server grants) in the OS keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service) whenever it is usable, with the plaintext file store as the automatic fallback. The credentials_store key in config.toml picks the mode: auto (default; keychain preferred, plaintext kept in sync as a compatibility bridge for file-only peers), keyring (authoritative, plaintext pruned after migration), or file (plaintext only). KIMI_DISABLE_KEYRING=1 overrides the config and forces the file store.
0ea6def to
582cf2e
Compare
2 participants
Related Issue
N/A — no tracking issue; the motivation is described below.
Problem
OAuth credentials (managed provider logins and MCP server grants) are stored as plaintext JSON files under
~/.kimi-code/credentials. File permissions (0600) are their only protection — anything running as the user can read long-lived refresh tokens, and secrets like these belong in the OS credential vault.What changed
Stores OAuth credentials in the OS keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service) whenever it is usable, on by default — no opt-in required:
packages/oauthstays pure TypeScript and gains aKeyringTokenStoragethat stores each token in the OS keychain using the exact same wire payload as the file store. Thecredentials_storekey inconfig.tomlpicks the mode:auto(default — keychain preferred, plaintext files kept in sync as a compatibility bridge so older builds and SDK hosts without a registered backend keep working against the same home),keyring(keychain authoritative, plaintext pruned after migration), orfile(plaintext only).KIMI_DISABLE_KEYRING=1overrides the config and forces the file store — the kill switch for hosts where keychain calls hang or prompt instead of throwing (containers, headless SSH), which the throw-based probe/degradation cannot catch. The env override is shared by every keyring consumer (provider tokens and MCP grants alike).apps/kimi-codeloads the@napi-rs/keyringnative binding at bootstrap and registers it as the backend, wired into the SEA native-asset pipeline and bundle checks; a load failure only warns and keeps the file store.packages/agent-core-v2gives the MCP OAuth grant store the same keychain-backed treatment, selected when a backend is registered and the probe passes.Note on the default mode:
autois a rollout bridge — the plaintext file is intentionally kept in sync, so the security benefit of the keychain only becomes effective once the bridge is removed (or viacredentials_store = "keyring"). The value today is completing the migration path without breaking file-only peers (desktop app, older builds, SDK hosts).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.