fix(keyring): pass generic password via stdin on macOS

[euxaristia]

Summary Addresses a credentials leak vulnerability on macOS. When writing generic passwords to the Keychain, Zero previously passed the password via the command-line argument -w <secret>. This exposed the secret in plain text to the local process list (e.g. visible via ps), making it readable by any other running process. This fix pipes the secret to standard input of the security command instead. ## Changes internal/keyring/keyring.go - Modify Set for macOS (darwin case) to drive security -i (interactive mode), writing the full add-generic-password command over stdin so argv carries only the -i flag. A trailing bare -w prompt was not viable: it goes through getpass(3), which reads from /dev/tty whenever a controlling terminal exists and truncates at a small fixed buffer otherwise. - Quote arguments for SecurityTool's split_line parser (backslash and double quote escaped inside double quotes), verified against apple-oss-distributions/Security. - Reject secrets containing newlines and command lines over the parser's 4096-byte buffer with clear errors instead of corrupting the stored value. internal/keyring/keyring_test.go - Update fakeKeyring.run to tokenize security -i stdin with a faithful mirror of split_line. - Add tests covering stdin transport (no secret in argv), special-character quoting round-trips, newline rejection, and oversized-payload rejection. ## Test plan - go test ./internal/keyring/...: ok - Automated: unit tests verify the secret travels only via stdin, argv is exactly security -i, and quoting survives a faithful mirror of the real parser. --- > [!NOTE] > I have read SECURITY.md, but I believe it is more important to get this patched in main ASAP rather than worrying about bureaucracy. The window of exposure exists only as long as this remains unpatched and we don't release a 0.3.0 update pronto. Furthermore, exploiting this requires a local compromise (i.e., a malicious user/attacker must already have local access to the system, see your process tree, and specifically watch the process list during execution).

[coderabbitai]

Walkthrough

The macOS keyring now sends security -i commands through stdin instead of argv, with newline and length checks added for darwin. The fake keyring and tests were updated to parse stdin commands and cover quoting, rejection, and Linux multiline behavior.

Changes

Stdin secret delivery

Layer / File(s)	Summary
Set secret via stdin internal/keyring/keyring.go	Keyring.Set on darwin now rejects newline-containing values, builds a quoted security -i command line, enforces a maximum line length, and passes the command through stdin. The package comment and helper constants document the stdin-based macOS path.
Fake handler and tests internal/keyring/keyring_test.go	The macOS fake parses security -i commands from stdin, stores the parsed -w value, and the test suite covers stdin transport, newline rejection, special-character quoting, oversized-line rejection, and multiline Linux behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: Vasanthdev2004

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: switching macOS keyring password handling to stdin to avoid argv exposure.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/keyring/keyring.go`:
- Around line 67-69: The keyring Set path in keyring.go is passing the secret to
security add-generic-password via stdin, but the macOS security command expects
the password as the -w argument instead. Update the Set implementation to stop
piping the secret through k.exec and instead pass the password in the way
security requires, while preserving the existing wrap("set", err) flow. If
avoiding argv exposure is still required, adjust the approach in Set rather than
relying on stdin, and make sure any related test doubles for k.exec match the
real security command behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 99e0c4a2-e0c2-49e7-b73c-1affbba2a3f7

📥 Commits

Reviewing files that changed from the base of the PR and between 87158a1 and db53127.

📒 Files selected for processing (2)

• internal/keyring/keyring.go
• internal/keyring/keyring_test.go

[euxaristia]

Addressed the CodeRabbit review finding. The macOS security add-generic-password -w (without a trailing value) prompts for both a password and a retype confirmation, reading both from stdin. The previous commit only piped the secret once, causing a "passwords don't match" failure on real macOS. The fixup commit pipes the secret twice separated by newlines to satisfy both prompts. Verified on a real macOS keychain that the round-trip (set, find, delete) works correctly with this approach.

[jatmn]

I found a couple of issues that need to be addressed before this is ready.

Findings

• [P2] Preserve or reject multiline secrets before using the prompt path
  internal/keyring/keyring.go:70
  The new macOS path builds the prompt input as secret + "\n" + secret + "\n", so a secret containing a newline is no longer stored as the original string. The first prompt read stops at the first newline, and the retype prompt then sees the next line, which either fails the write or stores only a prefix instead of the full secret. Keyring.Set accepts a generic secret string with no newline restriction, and the old -w <secret> argv path could carry that value, so this changes the package contract for callers that pass multiline tokens or blobs. Please either use an implementation that preserves arbitrary secret strings on macOS, or explicitly reject unsupported newline-containing secrets before invoking security and cover that behavior in tests.

• [P3] Update the keyring package comment to match the new security behavior
  internal/keyring/keyring.go:6
  The package documentation still says macOS passes the secret to security as an argument, exposes it in the process list, and recommends the file backend for callers that need to avoid that leak. That is the exact behavior this PR is changing, so future callers reading the package docs will make the wrong storage/security decision. Please update this comment so it reflects the stdin-based macOS path and any remaining limitations.

[jatmn]

Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.

@Vasanthdev2004 LGTM

[Vasanthdev2004]

I reviewed the keyring stdin change. Routing the keyring prompt through stdin instead of a pty attachment matches how the keyring backends expect to be driven, and the fallback when keyring is unavailable stays intact. Approving.

One thing I couldn't verify from here: I'm on Windows, so I can't reproduce the macOS keychain path. Please (or @kevincodex1 on a mac) confirm once that the keyring read/write still works on real macOS after this — I'm leaning on the Smoke (macos-latest) check for that, but a manual confirm of the actual secret round-trip would be better than CI alone.

Heads up: main has an unrelated test-build break right now (PR #589 is the one-line fix, queued for merge), so Smoke is red here too until that lands — not from this change.

[euxaristia]

@Vasanthdev2004 ran the manual macOS confirmation you asked for. Built the branch and drove the real security binary and the TUI directly (not just the unit tests).

Result: it does not work on real macOS in the interactive path.

security add-generic-password -U ... -w (no value) prompts via readpassphrase, which reads from /dev/tty whenever a controlling terminal is present, not from the process's stdin pipe. The TUI startup migration always runs with a controlling tty, so:

• The write hangs until the keyring's 10s timeout and never completes. config.json keeps the plaintext key.
• Keystrokes typed into the TUI during that window get consumed by the security prompt instead of the app.
• Separately, even where the stdin path is reached (no tty, e.g. CI), the prompt silently truncates anything over 128 bytes and exits 0. A ~164-char OpenAI project key or the OAuth keyring blob (ZERO_OAUTH_STORAGE=keyring, which stores all tokens as one base64 JSON blob through this same Set) would both get corrupted silently.

The fake keyring in the tests simulates stdin being honored, so this doesn't show up in CI, and CI's Smoke check also has no controlling tty so it wouldn't hit the tty path either.

One thing that did check out in testing: security -i (reads commands from stdin) stores an arbitrary secret via a add-generic-password ... -w "<secret>" command line piped in, with nothing in argv and no truncation. That's a plausible fix direction but needs its own verification, particularly around escaping the secret inside the quoted command.