Conversation
Add parseAcpCommandWithSpans to the shared ACP command parser so callers can map parsed tokens back to their raw quoting spans, and a redactCommandSecrets engine in the daemon that redacts secret-shaped flags, header values, curl user options, URL userinfo, and env-wrapper assignments before a command is shown in approval prompts. The command-identity digest now hashes the redacted identity so rotating a credential does not start a fresh ACP conversation. Shell-script nesting (sh -c recursion) follows in the next slice. Extracted from #2711 (ACP split 8/10).
|
@codex review |
|
@codex review |
|
Parking this PR open by decision (see #2782): the first ACP release ships bypass-permissions at parity with the SDK path, and the security-hardening slices (approval gating + command redaction) will be revived together with a unified cross-provider permission UX. Branch stays; review feedback is already addressed in-head; no further merge attempts until then. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Closing: intentionally parked per #2782, and the branch is preserved as parked/acp-redaction-full-fixed. Revive with the unified permission UX. Branch retained. |
Extracted from #2711 (ACP split 8/10). Supersedes #2778, which carried this plus shell-script nesting in one slice — split after round-1 review showed the two halves draw independent findings.
What this adds
parseAcpCommandWithSpans(sharedacp-command.ts): the existing tokenizer now also records each token's raw span in the original line, so a later slice can rewrite tokens in place while preserving the author's quoting.parseAcpCommandbecomes a thin wrapper — no behavior change.redactCommandSecrets(daemonacp-command.ts, flat scope — the top-level command only): redacts secret-shaped argument names (--token,-p,--api-key, …), header values (Authorization: …, curl-H/--header), curl user options (-u,--user,--proxy-user, clustered flags with value-taking-option awareness), URL userinfo (including empty-username forms), env-wrapper assignments, andNAME=valueright-hand URL userinfo. Windows executable names (C:\…\curl.exe) normalize before command-specific matching.shellQuotefor redisplaying tokens safely.getAcpCommandIdentityDigestnow hashes the redacted identity, so rotating a credential value no longer starts a fresh ACP conversation — only a real command change does.Deliberately not here (next slice)
sh -crecursion, command tracking across operators, span-anchored in-place rewrite, recursion cutoff, newline handling. No call sites in the query runner yet (those land with host-callback wiring).Review context carried from #2778 round 1
This slice absorbs six of the ten findings verbatim: Windows name normalization, env-assignment URL userinfo, curl
-ddata values,--proxy-user, dash-prefixed-uvalues, empty-username URLs.Verification
bun test packages/daemon/tests/unit/lib/acp/acp-command.test.ts— 25/25tsc --build --noEmit, oxlint, biome — clean