feat(oauth): store OAuth credentials in the OS keychain by default#748
Open
Brooooooklyn wants to merge 16 commits into
Open
feat(oauth): store OAuth credentials in the OS keychain by default#748Brooooooklyn wants to merge 16 commits into
Brooooooklyn wants to merge 16 commits into
Conversation
|
Brooooooklyn
force-pushed
the
feat/keyring-credential-storage
branch
from
d66e71c to
2af84ff
Compare
![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: 2af84ff91d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Brooooooklyn
force-pushed
the
feat/keyring-credential-storage
branch
from
244b574 to
08ecb15
Compare
Add KeyringTokenStorage (backed by @napi-rs/keyring) and a resolveTokenStorage factory that selects the OS keychain when usable and falls back to the existing plaintext FileTokenStorage otherwise. The factory guards two distinct failure modes: native binary not loadable (require throws at import) and binary loads but has no live OS backend (operations throw at call time, caught by a set/get/delete capability probe under an isolated sentinel service). KIMI_DISABLE_KEYRING=1 forces the file backend. When the keychain is selected but a token is still only on disk (older file-only build), load() migrates it into the keychain then deletes the plaintext file. remove()/list() reconcile against the legacy store so pre-migration plaintext can never linger or go missing. The class depends on an injectable KeyringApi interface, keeping it fully unit-testable without touching the real OS keychain. Adds a `test` script to the package (was missing) so the suite is runnable via pnpm filter.
Three correctness fixes to KeyringTokenStorage/resolveTokenStorage: 1. Migration now re-reads the legacy token immediately before deleting the plaintext file. A concurrent file-backend writer could previously slip a newer token onto disk between our copy-in and our remove; we now let the latest observed value win so the keychain stays authoritative and the newer token is never dropped. 2. remove() now always clears the plaintext file even when the native keyring delete throws (permissions, lock state, ambiguous entries). The keyring error is re-thrown after legacy cleanup rather than leaving cleartext behind. A missing credential is still a no-op. 3. The capability probe now derives a unique account per attempt (probe-<pid>-<rand>) and cleans it up in a finally, so concurrent probes can no longer clobber each other's sentinel and force a spurious file fallback on a healthy keychain. Adds four hermetic tests covering each fix; existing 12 keyring tests intact.
Wire resolveTokenStorage onto the KimiOAuthToolkit default storage path so
the keychain backend (added in Task 1) is actually reachable at runtime,
and export the new symbols from the package entry.
- toolkit: default storage is now resolveTokenStorage(credentialsDir)
instead of new FileTokenStorage(...); explicit options.storage injection
is untouched.
- index: export KeyringTokenStorage, resolveTokenStorage, and the
KeyringApi / KeyringEntry types.
- apps/kimi-code and packages/node-sdk both bundle the oauth package, so
declare @napi-rs/keyring as a runtime dependency in each; otherwise the
bundled createRequire("@napi-rs/keyring") fails to resolve in a consumer
install and credentials silently fall back to plaintext.
- vitest: force KIMI_DISABLE_KEYRING=1 for node-sdk and cli test projects
so credential tests stay hermetic and never touch the developer's real
OS keychain (the source alias resolves the native binary).
- test: add a hermetic integration test proving the keyring store is
reachable end-to-end through KimiOAuthToolkit (status/read/refresh/logout
all hit a fake keychain, nothing lands in plaintext on disk), plus a
default-path assertion that the factory is used when no storage injected.
Note: the native/SEA build packaging of @napi-rs/keyring is intentionally
out of scope (a separate later task).
Mirror the clipboard precedent so the native single-executable build keeps
@napi-rs/keyring external and collects its host JS plus the per-target .node
binary into the SEA asset blob:
- tsdown.native.config.ts: add @napi-rs/keyring to optionalNativeDependencies
(neverBundle, kept external like cpu-features).
- native-deps.mjs: add keyringSubpackageByTarget map and keyring-host
(js-only) + keyring-target (native-files) descriptors.
- check-bundle.mjs: allow the external @napi-rs/keyring runtime require.
The assets are now embedded and resolvable via the native-asset runtime
(loadNativePackage). NOTE: the oauth loadNativeKeyring loader still uses a
plain createRequire(import.meta.url)('@napi-rs/keyring') that is not routed
through the native-asset runtime, so on machines without node_modules the SEA
binary falls back to file storage. Wiring the loader to the runtime is a
follow-up outside this task's scope.
…e hook
The native module hook only intercepted `koffi`, so the oauth code's
`createRequire(...)('@napi-rs/keyring')` resolved against on-disk
node_modules (absent in an end-user SEA install) and threw, silently
falling back to plaintext file storage. Generalize the hook to a small
NATIVE_ASSET_PACKAGES set covering `koffi` and `@napi-rs/keyring`, and
add keyring to the native asset smoke test.
…espace, safe migration delete)
The keyring backend now namespaces the keychain service per credentials
directory (keyringServiceForCredentialsDir), matching the file backend's
per-directory isolation, and migration uses a lock-free compare-and-delete
loop that only unlinks a plaintext file whose bytes still match the value
made keychain-authoritative.
Align the tests with the namespaced service and the converge-loop semantics:
- toolkit-keyring-integration.test.ts: storage is built via
resolveTokenStorage(dir, ...), so derive the service from the dir via
keyringServiceForCredentialsDir instead of the bare KEYRING_SERVICE
constant (tmp dirs hash to kimi-code-<hash>). keychainTokenNames now
takes the service explicitly. (status / refresh / logout assertions.)
- keyring-storage.test.ts: the "selects KeyringTokenStorage when probe
succeeds" test inspects the resolveTokenStorage-built service, so query
keyringServiceForCredentialsDir(dir). Direct new KeyringTokenStorage({...})
tests keep KEYRING_SERVICE (the default service).
- keyring-storage.test.ts: rework the compare-and-delete race test so its
fake legacy store returns a DIFFERENT token on every read (persistent
churn). The file then never stabilises within the bounded converge budget,
so the loop exhausts its budget WITHOUT deleting the file (removeCalls === 0)
and the keychain holds the newest observed value. The previous harness
returned a fixed newer token forever, which the loop legitimately
re-migrates and then deletes once keychain == file (no data loss), so its
"NOT deleted" expectation contradicted the intended converge behavior.
No production logic changed for the race: at the moment legacy.remove() runs,
the keychain already holds the exact bytes on disk, so deletion is safe.
…al fallback) KeyringTokenStorage.load() early-returned the keychain value on any keychain hit, consulting the legacy file only on a miss. A per-run backend flip-flop (keychain locked / headless / KIMI_DISABLE_KEYRING=1 / probe fail) could leave an OLDER token in the keychain while a fallback run wrote a NEWER token to the plaintext file, so a later keychain-usable run silently ignored the user's valid newer token — and could overwrite the keychain with a revoked tombstone built from the stale value. load() now reconciles against the legacy file on the HIT path too, with a conservative rule: adopt the file token (and make the keychain authoritative, then compare-and-delete the cleartext) ONLY when BOTH sides are valid and the file is strictly newer by expiresAt. A deliberate revoked tombstone is never un-revoked from stale plaintext; a redundant byte-identical plaintext copy is pruned, otherwise the file is left in place. Lock-free, exactly like FileTokenStorage; the keychain-MISS migration loop is untouched.
…ie test Correct the 'byte-identical' wording in keyring-storage.ts to 'equal to the keychain value after canonical re-serialization' (the cleanup compares serialize(fileToken) === raw, so reordered keys / extra fields would not match and are conservatively left). Document that a file left by the conservative keychain-wins branch persists until the next explicit remove()/logout, and note that removeIfBytesMatch's re-read is the compare-and-delete guard, not redundant I/O. Rename the duplicate-cleanup test to match the wording, and add a regression test pinning the strict > (not >=) adoption: equal expiresAt with differing bytes leaves the keychain authoritative and the file intact.
…ncile The keychain HIT reconcile guard used `expiresAt` as a write-order proxy to decide whether to adopt a newer plaintext token. But `expiresAt` is an EXPIRATION time (mint second + `expiresIn`), so a server returning a shorter `expires_in` on a later refresh can mint a NEWER token B whose `expiresAt` is LESS than an OLDER, longer-lived token A. The guard then kept returning stale A and never adopted B, resurrecting the split-state failure (a 401 on A's rotated-out refresh token tombstones the keychain over the valid, ignored B). Compare issuance order instead: `issuedAt = expiresAt - expiresIn` recovers the mint second (the `expiresIn` cancels out), so the comparison is robust to a variable `expires_in` and needs NO new wire field. Strict `>` keeps the keychain authoritative on a same-second tie (1s granularity; practically unreachable). Everything else (both-valid precondition, compare-and-delete adoption, duplicate cleanup, lock-free design, MISS migration converge loop) is unchanged.
…YRING opt-out OAuth credentials are now stored in the OS keychain by default, with any pre-existing plaintext credential file migrated into the keychain and then removed. Document this behavior near the provider `oauth` field and add a `KIMI_DISABLE_KEYRING` row to the runtime-switches table, in both the en and zh locales.
The FakeKeyring Map-key separator was written as a literal NUL byte in the
template literals, which made git/GitHub classify the two test files as binary
("Binary file not shown" — diff unreviewable). Replace each literal NUL with
the \x00 escape sequence: it evaluates to the same U+0000 separator at runtime
(keys stay NUL-separated, tests unchanged) while keeping the source plain UTF-8
text so the diff renders.
…ng returns false, never throws)
Brooooooklyn
force-pushed
the
feat/keyring-credential-storage
branch
from
08ecb15 to
73a3c96
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a3c96f7d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…allback resurrection)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a790e84cef
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } finally { | ||
| // Always remove our own sentinel, even if the round-trip threw mid-way. | ||
| try { | ||
| entry.deleteCredential(); |
There was a problem hiding this comment.
In environments where the backend can set/read the sentinel but deletion is denied or returns false (the real keyring binding uses false for failed deletes), the probe still returns success because the delete result in finally is ignored. That selects the keyring backend and can migrate plaintext credentials into a store that cannot reliably remove OAuth tokens later, so logout either fails or leaves credentials/probe entries behind instead of falling back to file storage. Treat a failed probe delete as an unusable keyring before migration.
Useful? React with 👍 / 👎.
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.
Summary
@napi-rs/keyring, by default. Fall back to the legacy plaintextFileTokenStorageonly when the keychain is unavailable —KIMI_DISABLE_KEYRING=1, the native binary can't be required, or a runtime capability probe fails.issuedAt = expiresAt − expiresIn(the lifetime cancels, so it is robust to a variable serverexpires_in). A stale plaintext file can never un-revoke a keychain tombstone.@napi-rs/keyringas a native asset, route itsrequirethrough the module hook, add it to the smoke test; update the NixfetchPnpmDepshash.KIMI_DISABLE_KEYRING=1opt-out.The
OAuthManagerrefresh lock, the MCP OAuth store,FileTokenStorage, and the wire format are unchanged.oauth.storagestays byte-identical tomain(vestigial, auto-injected); the file opt-out is theKIMI_DISABLE_KEYRINGenv var.