Skip to content

fix: redact caller argv before it reaches the agent session history - #61

Merged
menitasa merged 2 commits into
jitpass:mainfrom
ctrl-alt-automate:fix-agent-history-argv-redaction
Aug 16, 2026
Merged

fix: redact caller argv before it reaches the agent session history#61
menitasa merged 2 commits into
jitpass:mainfrom
ctrl-alt-automate:fix-agent-history-argv-redaction

Conversation

@ctrl-alt-automate

Copy link
Copy Markdown
Contributor

Summary

Fixes the HIGH finding of GHSA-5f6j-w3xx-jx8g: the agent session history recorded the caller's full argv unredacted, so a secret carried on a command line (jit vault set <path> <value>, or a token in a jit run -- tool --token=… tail) was persisted in plaintext to agent-history.jsonl and rendered back by jit audit / jit agent history — while the application audit log promises it "records that a command RAN, not the secret it may have carried" and cli/auditrecord.go already masks these exact positions in jit's own os.Args.

Three layers, one shared judgement:

  • auditlog.RedactCommandLine (new): the credential-shaped RedactText pass plus an unconditional mask of a jit vault set line's value positionals — the entropy test alone would let a weak value ("hunter2") through, the same reason sanitizeInvocationArgs masks that position for jit's own args. Positional counting is reliable here because every flag that command can see is a boolean; a future value-taking flag would over-mask, never leak. Placed in auditlog because both producers need the identical judgement and it is the leaf they already share (the same reasoning RedactToken's export comment records).
  • caller.command() routes argv through it, so every By — unlock, use aggregate, serve-error, grant end — is masked at the single point it is minted, and no future recording site can forget to.
  • historyLog.load / readNewEvents scrub By on read, so a legacy line written before this fix stops leaking the moment it is displayed (seeded ring, jit agent history, jit audit text + JSON) rather than the year it rotates out of the 2MB file.

The path of a jit vault set stays legible (it is the one part an investigation needs and is not a secret), including the value-from-prompt/--stdin form where masking the last positional would have redacted the path itself.

Test plan

  • New tests: TestRedactCommandLine* (auditlog — grammar, flags, -- terminator, multi-word values, path-only forms, non-jit lines), TestUnlockEventNeverRecordsACredentialFromTheCallersArgv, TestUnlockEventMasksAVaultSetValueTooWeakForTheEntropyTest, TestUnlockEventKeepsVaultSetPathWhenValueCameFromPrompt, TestRecordServeErrorRedactsTheRejectedPeersArgv (agent), TestHistoryLogLoadScrubsLegacyPlaintextBy (cli)
  • go build ./... && go test -race -timeout 2m ./... — full suite green
  • gofmt -l ./cmd ./internal (clean), go vet ./..., go mod verify && go mod tidy (no drift)
  • staticcheck ./... @v0.7.0, govulncheck ./... @v1.6.0, gosec -exclude-generated ./... @v2.28.0 — all clean
  • Maintainer: consider a one-shot trim/rewrite of existing agent-history.jsonl files if you want poisoned legacy lines gone from disk (this PR scrubs them on every read, but the bytes remain in the file until rotation)

Reported privately first per SECURITY.md; the advisory (GHSA-5f6j-w3xx-jx8g) has the full analysis, including two smaller findings this PR deliberately does not touch.

The agent stamped every unlock/use/serve-error/grant event's By with the
caller's full argv exactly as the kernel reported it, and the durable
half of that history (agent-history.jsonl) appended it verbatim. A
secret on a caller's command line — jit vault set <path> <value>, or a
token in a jit run -- tool --token=… tail — was therefore persisted in
plaintext and rendered back by jit audit and jit agent history, while
the application audit log's own doctrine promises it records that a
command RAN, not the secret it may have carried (internal/auditlog), and
cli/auditrecord.go already masks these exact positions in jit's own
os.Args. Reported as GHSA-5f6j-w3xx-jx8g.

Three layers, one judgement:

- auditlog.RedactCommandLine is the shared implementation: the
  credential-shaped RedactText pass plus an unconditional mask of a
  jit vault set line's value positionals, which the entropy test alone
  would let through when the value is weak (hunter2). It lives in
  auditlog because the agent (recording) and the CLI (re-reading) need
  the identical judgement and auditlog is the leaf they already share.
- caller.command() routes argv through it, so every By is masked at the
  single point it is minted — no recording site can forget.
- historyLog.load and readNewEvents scrub By on read, so a legacy line
  written before this fix stops leaking the moment it is displayed
  rather than the year it rotates out of the 2MB file.

Signed-off-by: ctrl-alt-automate <31536997+ctrl-alt-automate@users.noreply.github.com>
@menitasa

menitasa commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Thanks for the report and the fix. The mechanism (redact at the one point argv becomes a By, plus a read-time scrub for legacy lines) is exactly right, and we've kept it. A deep review of the branch confirmed the direction but found the redactor itself both under- and over-masked, so I've pushed a follow-up commit on top of yours rather than round-tripping through review comments:

  • RedactCommandLine now judges each field with redactArg (the KEY=VALUE-splitting, path-exempting judgement Redact already applies to jit's own args) instead of a whole-line RedactText pass. That fixes the two big ones: --token=ghp_... / KEY=VALUE glued secrets were still leaking (the prefix test never saw past the key, and short vendor tokens are under the entropy floor), and ordinary versioned paths, including the vault path your grammar pass deliberately kept, were being entropy-masked out of By, permanently.
  • The vault-set grammar is now ONE exported implementation (auditlog.MaskVaultSetValues) shared with sanitizeInvocationArgs, which previously masked only the last positional; it also catches a dash-prefixed value ("-hunter2") that positionalIndexes waved through as a flag. A drift test walks the live cobra command so the flag list can't rot.
  • The same doctrine now covers the sibling surfaces the chokepoint missed: use-aggregation keys on the raw command (redaction could merge two different callers into one misattributed event), mount-run service-log lines, GrantStatus.Command, and a client-side scrub for a still-running pre-fix agent serving raw ring events over the status RPC.
  • Your unchecked test-plan item is done: scrubLegacy() rewrites agent-history.jsonl once at service startup, so legacy plaintext leaves the disk rather than waiting out rotation; already-clean lines are kept byte-for-byte.

All of your tests still pass unchanged. Merging once CI is green.

…gv surfaces

Review of the argv-redaction fix confirmed ten findings; this addresses
all of them on top of it.

The redactor itself both under- and over-masked. RedactText was designed
for free-form error text: it never splits KEY=VALUE, so the motivating
example itself (`jit run -- tool --token=ghp_...`) still leaked any
glued or short recognized-format credential, and its entropy pass has no
path exemption, so ordinary versioned paths (/opt/homebrew/Cellar/...,
an MCP server's nvm path, even the long vault path the grammar pass had
deliberately kept) were wiped from By, permanently at record time and
retroactively across all legacy history on display. RedactCommandLine
now judges each field with redactArg, the KEY=VALUE-splitting,
path-exempting judgement jit's own recorded args already get, with a
RedactText sweep only for path-free fields a punctuation-glued token
could hide in.

The vault-set grammar is now ONE implementation, exported as
auditlog.MaskVaultSetValues and shared by RedactCommandLine and
sanitizeInvocationArgs, whose parse-error fallback previously masked
only the LAST positional and disagreed with the new grammar on the
mis-pasted-extra-argument case its own comment warns about. It also
masks a dash-prefixed value ("-hunter2") that positionalIndexes used to
wave through as a flag: only vault set's real boolean flags stay
legible, and TestVaultSetGrammarMatchesCommand walks the live cobra
command so the flag list cannot drift.

Redaction reached only one of the places argv becomes recorded output;
the same doctrine now covers the siblings:

- use-aggregation keys on the RAW command (caller.rawCommand, in-memory
  only): two callers whose argvs redact to the same string no longer
  merge into one KindUse event stamped with the first caller's pid.
- mount-run attachments redact at capture, so the durable service log's
  "serving real content to pid N's process tree (...)" lines and
  MountGrantStatus.Command stop carrying raw argv.
- grant creation redacts target.Command() before it reaches
  GrantStatus.Command and every `jit grant list` / `jit status` surface.
- the agent CLIENT scrubs By on Status/History responses, closing the
  window where a still-running pre-fix agent (foreground `jit agent run`
  has no stale-binary restart) serves raw ring events to an upgraded CLI.

And the legacy story now does what its comment claimed: scrubLegacy
rewrites agent-history.jsonl once at service startup, so a pre-fix
line's plaintext leaves the DISK instead of surviving until rotation;
already-clean lines are kept byte-for-byte so a newer binary's fields
survive. load() scrubs after the cap instead of redacting ~10k lines to
keep 200 on every service start.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011YsDoTdd24LnbBtHwkvwrB
@menitasa
menitasa force-pushed the fix-agent-history-argv-redaction branch from ea2398d to 2c7f19d Compare August 16, 2026 12:04

@menitasa menitasa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the full diff (deep multi-agent review, 10 findings) and fixed all findings in the follow-up commit 2c7f19d. Mechanism is right, tests are thorough, DCO present.

@menitasa menitasa closed this Aug 16, 2026
@menitasa menitasa reopened this Aug 16, 2026
@menitasa menitasa closed this Aug 16, 2026
@menitasa menitasa reopened this Aug 16, 2026
@menitasa
menitasa merged commit b39fe9b into jitpass:main Aug 16, 2026
8 of 14 checks passed
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.

2 participants