Repository navigation
fix(session): keep a tool call's arguments masked when masking leaves no JSON to parse - #1482
Conversation
|
No findings in the specified diff. The fallback preserves the masked arguments instead of restoring secrets, and the regression test covers numeric-secret masking that makes the JSON unparsable. The PR description documents sufficient testing: type checking, lint, 168 targeted tests, and a representative before/after real-CLI |
|
No findings in the specified diff. The earlier review’s no-findings conclusion still holds. The fallback retains masked arguments rather than restoring secrets, and the regression test covers numeric masking that makes the JSON invalid. The PR description documents sufficient testing, including a representative before/after real-CLI Review was read-only; no PR code was run, built, or installed. |
@hz89Dev 9d30c88 declares |
9d30c88 to
1e863ff
Compare
|
No findings in the specified diff. The earlier reviews’ no-findings conclusion still holds. The fallback preserves masked arguments instead of restoring secrets, and the regression test covers numeric-secret masking that makes the JSON invalid. The PR description documents sufficient testing, including a representative before/after real-CLI Review was read-only; no PR code was run, built, or installed. |
… no JSON to parse A placeholder that replaces a number, such as a declared secret 4938, leaves the masked arguments unparsable, and scrubbing then kept the original arguments, a token beside it included. It now keeps the masked text.
1e863ff to
3ad59c8
Compare
|
No findings in the specified diff. The earlier reviews’ no-findings conclusion still holds. The fallback preserves masked arguments instead of restoring secrets, and the regression test covers numeric-secret masking that makes the JSON invalid. The PR description documents sufficient testing, including a representative before/after real-CLI Review was read-only; no PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving at 3ad59c8, which is aae5fa7 rebased onto main with the sessions commit dropped (range-diff identical).
Built CLI, main against this PR (git-kind team declaring WAREHOUSE_PIN=4938): after CI=true teamai score --drain and teamai session push --session … --source claude-code, the ghp_… token next to the pin appears twice in the team repo's history on main and not at all here.
- A numeric secret at the top level, nested in an object or in an array now comes out as
{"redacted": "…"}with the masked text; a secret inside a string keeps the arguments' structure, as before; non-secret numbers (12345, 4939, 49380) are unchanged. session pull,list,searchandresumeinto Claude Code and Codex all handle{redacted}arguments. No dashboard code reads archived tool-call arguments.scrub.ts:74is the only place that parses JSON after masking, so no other spot has this fallback.npx tsc --noEmitandnpm run lintexit 0; the 6 related unit files (168 tests) pass, also with a symlinked TMPDIR; the new test fails on main because the original token comes back. CI is green at this head, on a merge with main after #1483.
[P3 nit] The comment at scrub.ts:76 says a placeholder in "a key" breaks parsing; keys are always quoted, so only a bare number or literal triggers the fallback.
Two related gaps we found while checking, both on main already, not caused by this PR:
- A declared secret containing
",\or a control character is never masked in tool-call arguments: masking runs on the JSON text, where those characters are escaped, so the secret no longer matches (pa"ss1234came through unmasked). redactJson(workflow records, the JSON envelope) does not mask a number equal to a declared secret:{"pin":4938}is kept.
@hz89Dev could you take those two in a follow-up PR?
Summary
A scrubbed session can carry a token unmasked in a tool call. When masking replaces a number in the call's arguments, such as a declared secret
WAREHOUSE_PIN=4938in{ "pin": 4938, "token": "ghp_…" }, the masked arguments are no longer JSON. Scrubbing then kept the original arguments, so the token next to the number reached the archive. It now keeps the masked text as{ "redacted": "<masked arguments>" }.This reaches every scrubbed archive:
score --drain, the background pass,session push --session,workflow publish --sessions, andsession push/session migrate --pushwith--scrub. #1478 made declared secrets, which can be 4 characters, part of that masking, so a numeric value is now a likely trigger. An environment secret with a numeric value could already trigger it.Type of Change
Test Plan
npx tsc --noEmit,npm run lint: clean.npx vitest runonscrub-session,session-title,score-drain,session-push,session-cmd,agent-session-find: 168 passed. The newscrub-sessiontest fails without the fix, with the token back in the arguments.WAREHOUSE_PINinenv/secrets.yaml. A captured unattended Claude Code session has atool_usewith input{"pin":4938,"token":"ghp_A1b2…"}, andCI=true WAREHOUSE_PIN=4938 teamai score --drainarchives it.Built from
main(993984d):Built from this branch:
Not run here, left to CI: the gitlab and github providers, Windows, and agents other than Claude Code.
Related Issues
For #1385. Follow-up to #1478, found while reviewing it.
Notes for Reviewers
"password": "…"), so the masked text is kept whole.