fix(security): strip PM_CLI_BRIDGE_PASSWORD from child process env - #20
Merged
Merged
Conversation
mail watch --exec built its child environment with append(os.Environ(), ...), so the Bridge password supplied via PM_CLI_BRIDGE_PASSWORD was inherited by the user-supplied command and by everything that command shelled out to. Any third-party triage script could read the mail credential. The env-var credential path and this spawn site were each fine on their own; the exposure only exists once both are present, which is first true in 0.2.6. It is therefore introduced by the unreleased version rather than pre-existing, and is fixed before the release goes out. Adds config.ScrubSecrets, which removes pm-cli credential variables from an environment slice, and routes the exec site through it. The secret list lives next to the variable it names, so a future credential variable is covered by adding one entry. Matching on the exact variable name rather than a prefix keeps unrelated names such as PM_CLI_BRIDGE_PASSWORD_BACKUP intact; bare entries with no "=" are dropped, since that is the safe direction. Tested with a real child process: the scrubbed environment exposes nothing, while the unscrubbed one reproduces the leak. Also documents the environment-variable credential path in SECURITY.md, which described only the keyring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Blocks the v0.2.6 release.
The leak
MailWatchCmd.executeCommandbuilt its child environment asappend(os.Environ(), ...). Since #14 addedPM_CLI_BRIDGE_PASSWORD, that variable was inherited by every commandmail watch --execruns, and by anything those commands shell out to. A third-party triage script gets the mail password.Reproduced against a real child process before the fix:
Worth being precise about provenance: the env-var credential path (#14) and this spawn site were each fine in isolation. The exposure exists only once both are present, which is first true in 0.2.6 — so this is introduced by the unreleased version, not a pre-existing issue, and is fixed before the release ships rather than disclosed as a caveat.
The fix
Adds
config.ScrubSecrets(env []string) []string, which removes pm-cli credential variables from an environment slice, and routes the singleexec.Commandsite through it. The secret list lives beside the constant it names, so a future credential variable is one entry.Two deliberate details:
PM_CLI_BRIDGE_PASSWORD_BACKUPis unrelated and survives.=are dropped. Go permits them, and dropping is the safe direction.The input slice is not mutated.
Tests
Eight cases in
internal/config/scrub_secrets_test.go, including two that spawn a realshchild:TestScrubbedEnvironNotVisibleToChild— the child reads an empty value through the scrubbed environment.TestUnscrubbedEnvironWouldLeak— pins the regression by confirmingos.Environ()really does propagate the credential, so the scrubbing above stays load-bearing.Also
SECURITY.mddocumented only the keyring and never mentionedPM_CLI_BRIDGE_PASSWORD— a credential mechanism absent from the security policy. Adds a subsection covering the tradeoff honestly: readable by same-user processes, visible in/proc/<pid>/environ, and now excluded from spawned children.gofmt,go vet,go test ./...all clean.🤖 Generated with Claude Code