Skip to content

fix: prevent macOS keychain writes from exposing API keys in process list#245

Open
bukinoshita wants to merge 3 commits intomainfrom
fix/macos-keychain-secret-exposure-3905
Open

fix: prevent macOS keychain writes from exposing API keys in process list#245
bukinoshita wants to merge 3 commits intomainfrom
fix/macos-keychain-secret-exposure-3905

Conversation

@bukinoshita
Copy link
Copy Markdown
Member

@bukinoshita bukinoshita commented Apr 9, 2026

Summary

Resolves BU-628: macOS keychain writes expose API keys in process list.

The MacOSBackend.set() method was passing the API key as a command-line argument to /usr/bin/security add-generic-password -w <secret>, making it visible to any process listing tool (ps, endpoint monitoring, etc.) during the write.

Summary by cubic

Secure macOS Keychain writes so API keys never appear in process args, and stop auto-migrating credentials during reads. Fixes BU-628.

  • Bug Fixes

    • macOS backend now writes via a JXA script piped to /usr/bin/osascript, calling SecItemAdd/SecItemUpdate with secrets sent over stdin.
    • Removed auto-migration from resolveApiKeyAsync; reads never write credentials. Added tests for stdin-only handling and no auto-migration.
  • Refactors

    • Reverted arrow functions to function declarations in the macOS backend for consistency.

Written for commit 5241187. Summary will update on new commits.

…list

- Replace /usr/bin/security add-generic-password (which passes secret as
  CLI arg visible in ps) with JXA script piped via stdin to osascript,
  using the Security framework (SecItemAdd/SecItemUpdate) directly.
  The secret never appears in process arguments.

- Remove auto-migration from resolveApiKeyAsync: read-only commands
  (whoami, doctor, etc.) no longer trigger credential writes. Migration
  now only happens through explicit write paths (login, storeApiKeyAsync).

- Add macOS backend unit tests verifying secret is passed via stdin.
- Add config-async test verifying no auto-migration on read paths.

Co-authored-by: Bu Kinoshita <bukinoshita@users.noreply.github.com>
…ckend

Co-authored-by: Bu Kinoshita <bukinoshita@users.noreply.github.com>
@bukinoshita
Copy link
Copy Markdown
Member Author

@cubic-dev-ai review this PR?

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai bot commented Apr 9, 2026

@cubic-dev-ai review this PR?

@bukinoshita I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

@bukinoshita bukinoshita marked this pull request as ready for review April 10, 2026 16:17
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 4 files

Copy link
Copy Markdown
Contributor

@felipefreitag felipefreitag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't working for me:

◇  Enter your Resend API key:
│  ▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪▪
  ✔ API key is valid (full access)
│
◇  Save API key to which profile?
│  qa-test-pr245 (overwrite)
Error: Failed to store credential in macOS Keychain: execution error: Error: Error: Keychain error: -50 (-2700)
 ELIFECYCLE  Command failed with exit code 1.

Likely problem:

  1. $.kSec* constants don't work as NSDictionary keys in JXA. They're opaque Ref objects that all hash to the same key, so the dictionary ends up with 1 entry instead of 4. SecItemAdd returns -50 (errSecParam). Fix: use the raw string values ($("class"), $("genp"), $("svce"), $("acct"), $("v_Data")) instead of $.kSecClass, $.kSecClassGenericPassword, etc.

I investigated a bit and got past that error, but then any attempt to read the key keps prompting for keychain access and failing.

  1. ACL mismatch between set() and get(). Items written by osascript via SecItemAdd get an ACL that only trusts /usr/bin/osascript. But get() still uses /usr/bin/security find-generic-password -w, which is a different binary — so macOS shows a keychain authorization prompt, the 5s timeout fires, and get() fails with exit code null. Either get() needs to also use JXA/SecItemCopyMatching, or the set() script needs to add /usr/bin/security as a trusted application in the ACL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants