Uh oh!
There was an error while loading. Please reload this page.
fix(auth): stop repeated macOS keychain password prompts - #225
Conversation
Two changes: 1. In-process credential cache: a single CLI run reads credentials from many call sites (auth, telemetry, token refresh); each uncached keyring read is a separate keychain ACL check, i.e. one password dialog per read on an untrusted binary. First read is now cached; saves keep the cache coherent. 2. macOS keychain access now shells out to /usr/bin/security instead of the native binding. Keychain item ACLs pin trust to the requesting binary's code signature, and the Bun-compiled release binary is ad-hoc signed — a new signature every release — so 'Always Allow' never persisted across versions. /usr/bin/security is Apple-signed and stable; items it creates read back silently. Existing items are re-minted (delete + add) on next save, so migration costs at most one final prompt. Interim measure until releases are Developer ID signed, at which point darwin should revert to @napi-rs/keyring to regain app-level isolation (any user process can invoke /usr/bin/security, so this trades app isolation for prompt-free UX — comparable in practice to the existing file fallback).
Greptile SummaryThe PR consolidates credential and configuration persistence behind a shared cached store and introduces a macOS
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "refactor(auth): extract shared KeyringSt..." | Re-trigger Greptile |
| function getKeyringEntry(): KeyringEntry { | ||
| // On macOS, go through /usr/bin/security (stable Apple-signed binary) | ||
| // instead of the native binding: the ad-hoc-signed CLI binary changes | ||
| // signature every release, so native keychain access prompts per version. | ||
| // See darwin-keychain.ts; revert once releases are Developer ID signed. | ||
| if (process.platform === 'darwin') { | ||
| return new DarwinSecurityEntry(SERVICE_NAME, ACCOUNT_NAME); | ||
| } | ||
| return new Entry(SERVICE_NAME, ACCOUNT_NAME); |
There was a problem hiding this comment.
🔴 Password prompts still repeat on macOS because saved settings are read through the old keychain path
Only the login credentials were switched to the stable macOS tool and given an in-process cache (getKeyringEntry() at src/lib/credential-store.ts:149-157), while the CLI's saved settings are still read through the old, per-release-signature path on every read (new Entry(...) at src/lib/config-store.ts:131-132), so macOS users keep getting repeated password dialogs in one run.
Impact: Users still see multiple keychain password prompts per command run and may deny them, breaking authentication — the symptom the change set out to remove.
Why the settings entry keeps triggering keychain ACL checks
src/lib/config-store.ts stores its blob in the same keychain service under account config (src/lib/config-store.ts:54-55) and reads it via readFromKeyring() → new Entry(SERVICE_NAME, ACCOUNT_NAME).getPassword() (src/lib/config-store.ts:134-140). Unlike credential-store.ts, it has neither the /usr/bin/security routing nor any in-process cache: getConfig() (src/lib/config-store.ts:195) performs a fresh keyring read on every call, and it is called several times per run (e.g. src/commands/login.ts:71, src/commands/env.ts:26, src/lib/run-with-core.ts, src/lib/unclaimed-env-provision.ts:59). Because the item was written by the ad-hoc-signed binary, its ACL is pinned to a signature that changes each release — exactly the mechanism described in src/lib/darwin-keychain.ts:1-16 — so each read is a separate prompt.
Prompt for agents
The macOS prompt-storm fix was applied only to src/lib/credential-store.ts (routing through DarwinSecurityEntry plus an in-process cache). src/lib/config-store.ts stores a second keychain item (service `workos-cli`, account `config`) and still reads it through `new Entry(...)` on every `getConfig()` call, with no caching. `getConfig()` is invoked multiple times per CLI run (src/commands/login.ts, src/commands/env.ts, src/lib/run-with-core.ts, src/lib/unclaimed-env-provision.ts), so on macOS each call is another keychain ACL check against the ad-hoc-signed binary and therefore another password dialog — the exact symptom this PR targets. Consider extracting the shared backend selection (darwin → DarwinSecurityEntry, otherwise @napi-rs/keyring Entry) into a helper used by both stores, and adding an equivalent in-process cache to config-store (invalidated by saveConfig/clearConfig/setInsecureConfigStorage). Note the test setup already mocks the darwin backend globally, so config-store specs should keep working.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed — and empirically reproduced: with the credential fix in place, running a differently-built binary still produced one prompt, and it traced to this config item. Fixed in 9438450: config-store now uses the same darwin backend selection plus an equivalent in-process cache (invalidated by saveConfig/clearConfig/setInsecureConfigStorage). Kept the backend selection local to each store rather than extracting a shared helper so each spec's existing per-file keyring mocks keep working; the global test-setup mock covers the darwin backend for both.
| const r = spawnSync(SECURITY, ['-i'], { | ||
| input: `add-generic-password -a "${this.account}" -s "${this.service}" -w "${b64}"\n`, | ||
| encoding: 'utf-8', | ||
| }); | ||
| if (r.status !== 0) { | ||
| throw new Error(`security add-generic-password exited ${r.status}: ${(r.stderr ?? '').trim()}`); | ||
| } |
There was a problem hiding this comment.
🔍 Write failures may go undetected if security -i does not propagate the inner command's exit status
setPassword() shells out to /usr/bin/security -i and pipes add-generic-password ... on stdin, then relies solely on the process exit status to detect failure. In interactive mode security reads and executes commands from stdin; if it exits 0 even when the inner add-generic-password fails (e.g. keychain locked, ACL denial, malformed argument), writeToKeyring() at src/lib/credential-store.ts:186-201 would report success, the file fallback would never kick in, and the credential would silently not be persisted — forcing a re-login on the next run. The success path was verified end-to-end by the author, but the failure path depends on the exit-status semantics of security -i, which is worth confirming (e.g. printf 'add-generic-password -a x -s y -w z\nadd-generic-password -a x -s y -w z\n' | security -i; echo $? — the duplicate add should fail). A robust alternative is a read-back verification after write, or passing the secret via a temp-file/-w prompt mode whose status is unambiguous.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Verified empirically with exactly the suggested experiment before shipping: a duplicate add-generic-password through security -i exits 45 (SecKeychainItemCreateFromContent: The specified item already exists), and find-generic-password not-found exits 44 — security -i does propagate the inner command's failure status. setPassword also deletes before adding, so the duplicate case can't occur in practice. Belt-and-suspenders read-back verification already exists one level up in config-store's saveConfig.
| setPassword(password: string): void { | ||
| // Delete-then-add rather than update in place: updating keeps the old | ||
| // item's ACL (pinned to a previous binary). A fresh item is owned by | ||
| // /usr/bin/security and reads back without prompting. | ||
| this.deletePassword(); | ||
| const b64 = Buffer.from(password, 'utf-8').toString('base64'); | ||
| // -i reads commands from stdin so the secret never appears in argv, | ||
| // where it would be visible to `ps`. | ||
| const r = spawnSync(SECURITY, ['-i'], { | ||
| input: `add-generic-password -a "${this.account}" -s "${this.service}" -w "${b64}"\n`, | ||
| encoding: 'utf-8', | ||
| }); | ||
| if (r.status !== 0) { | ||
| throw new Error(`security add-generic-password exited ${r.status}: ${(r.stderr ?? '').trim()}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟨 macOS credentials become readable by any user-level process
On macOS the login credentials are now stored and read through /usr/bin/security (src/lib/darwin-keychain.ts:23-68, selected in src/lib/credential-store.ts:149-157). Items created this way carry the apple-tool: partition, so any process running as the same user can silently read the access/refresh tokens by shelling out to the same tool, losing the per-application ACL isolation the native keyring binding provided. The stored value is only base64-encoded, which is encoding, not protection.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Correct, and deliberate — this is the documented trade-off in the PR description and the module header: app-level isolation is lost on macOS until release binaries are Developer ID signed (in progress; cert acquired). In practice the prior state was weaker: per-release prompts trained users to click Always Allow or deny (breaking auth), and the store already falls back to a plaintext file when the keyring errors. The base64 is for transport through security -i quoting, not protection — noted. Marked in-code for revert to @napi-rs/keyring once signing lands.
| const r = spawnSync(SECURITY, ['-i'], { | ||
| input: `add-generic-password -a "${this.account}" -s "${this.service}" -w "${b64}"\n`, | ||
| encoding: 'utf-8', | ||
| }); |
There was a problem hiding this comment.
🟨 Secret is interpolated unquoted-safe into a security tool command line read from stdin
setPassword builds an interactive security command string by interpolating the service, account and base64 secret into a double-quoted command line fed to /usr/bin/security -i (src/lib/darwin-keychain.ts:53-56). The values are currently module constants (workos-cli/credentials) and base64 output, so no metacharacters can appear, but the pattern is fragile: any future caller passing a service/account containing a quote or backslash would break the argument parsing of the interactive command line.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fair — hardened in 9438450: the constructor now validates service/account against ^[\w.-]+$ and throws otherwise, so a future caller can't break the interactive parser or smuggle metacharacters. The secret itself is always base64 so it never needs quoting.
The config keychain item (workos-cli/config) had the same two problems as credentials: native-binding reads pinned to the per-release ad-hoc signature, and no in-process cache despite getConfig() running several times per command. This was observable: with the credential fix in place, a rebuilt binary still prompted once — for the config item. Also hardens DarwinSecurityEntry: service/account are validated against a safe charset since they are interpolated into a `security -i` command line (review feedback).
… stores credential-store and config-store were ~90% duplicated (backend selection, in-process cache, file fallback, one-shot migration, warnings). Both are now thin wrappers around one KeyringStore<T> parameterized by the parts that actually differ: account/file names, validation (credentials must read as logged-out when malformed), and save read-back verification (config only). Public APIs unchanged; net -180 lines.
Problem
Running
npx workos@latest installprompts for the login keychain password ~5 times per run (Slack report). Users reasonably read this as malware behavior and deny it, breaking auth.Two compounding causes:
ensure-authup to 5×, telemetry, analytics, token refresh, credential proxy). EachEntry.getPassword()is a separate keychain ACL check = one dialog per read.cdhash:per build; prompts only began after the Node→Bun conversion because the signednodebinary carried a stableteamid:.)Fix
credential-store.ts— first read cached, saves keep it coherent (all refresh paths write throughsaveCredentialsin-process). Invalidated byclearCredentials/setInsecureStorage./usr/bin/security(darwin-keychain.ts) instead of@napi-rs/keyring. Thesecuritytool is Apple-signed and stable, so items it creates carry theapple-tool:partition and read back silently forever. Writes are delete+add (updating in place keeps the old ACL) throughsecurity -istdin so the secret never appears in argv. Legacy raw-JSON items from the native binding still read (one final prompt), and the next save re-mints them silently.Trade-off (deliberate, interim)
Any user-level process can invoke
/usr/bin/security, so app-level isolation is lost on macOS — comparable in practice to the existing plaintext file fallback. This is a stopgap until release binaries are Developer ID signed (in progress); at that point darwin should revert to the native keyring. Marked with aponytail:comment indarwin-keychain.ts.Testing
keyring-isolation.spec.ts(a realsecurityshell-out in tests would wipe developer logins)partition: apple-tool:; 5 consecutive CLI runs complete in 3.6s with zero prompts (one unanswered prompt blocks ~60s)Checklist
bun run buildpassesbun run testpassesbun run typecheckpasses