SUB-185: bounded redacted tool-result capture - #1
Conversation
Pavel-Tk
left a comment
There was a problem hiding this comment.
Adversarial credential-safety review — REQUEST CHANGES
I found 4 CRITICAL blockers. Branch checks pass (131 passed, ruff, candidate verifier, compileall), but the exploit suite is 10 failed, 7 passed.
CRITICAL 1: scanning resets at tool-result structure boundaries
_tool_result_text() inserts "\n" between inner text blocks, while _parse_record() scans each top-level block independently. I split one configured synthetic credential after super-secret-:
- across two top-level
tool_resultblocks; and - across two inner text blocks in one
tool_result.
Both raw halves were retained. A receiver can reassemble the credential. The redaction stream must cover the original structural boundaries (or conservatively quarantine adjacent pieces); joining first with an inserted character and scanning envelopes independently does not meet the split-block invariant.
CRITICAL 2: new tool identity fields are sliced before credential scanning
At transcript.py:249-250,267, tool_use.id, tool_use.name, and tool_result.tool_use_id are sliced to 512 characters before normalization/redaction. With a valid 600-character configured ANTHROPIC_API_KEY placed in the new id/name fields, the event retained the raw 512-character prefix in tool_call_id, tool_name, and source_identity; the block was not quarantined. Scan the complete block, including metadata, before any bound.
CRITICAL 3: pairing is not exact, one-to-one, or lineage-scoped
At transcript.py:284-314:
- an orphan clears
tool_name/source_identitybut retainstool_call_id; the current schema-v2 server contract treats that fallback as attributable identity; - one call plus two results with the same id attributed both results as
Read; - a root call plus a different sidechain's result using the same id attributed the sidechain result as
TrustedTool.
A trustworthy pair must be exactly one call and one result in the same lineage/structural context. Every orphan/ambiguous path must remove all server-attributable fallback identity, not just tool_name and source_identity.
CRITICAL 4: process-local spool locking can consume the boundary reserve and erase counters
Each hook constructs a new DurableSpool, but admission and stats use only a per-instance threading.Lock. A deterministic two-instance race let both ordinary writes observe the same available slot. The spool reached 3/3 ordinary entries and the next session_end was refused with spool capacity unavailable.
The same stale-instance pattern erased a persisted loss: instance A wrote dropped=1; instance B then recorded a duplicate from its stale counter snapshot and rewrote the file to dropped=0, duplicates=1. Restart preserved the incorrect zero.
Also, for the supported max_bytes=300000, a 220KB ordinary item was admitted but only 75KB was reserved, so a legal 80KB pre_compress event (below the 262,144 event cap) was refused. Admission + write and counter read-modify-write need a root-scoped interprocess transaction. Every accepted spool configuration must reserve one maximum legal boundary event.
Verified passing cases
The former 65,536 boundary, tool-use input secret, UTF-8 adjacency, orphan-result secret quarantine, blank-content digest, sidechain kill-switch, normal single-instance counter restart, post-redaction 262,144 cap, and exact new schema-v2 payload acceptance by the current server all passed.
Full reviewer log: /tmp/review185_log.md on the review host.
Pavel-Tk
left a comment
There was a problem hiding this comment.
Security re-check — commit 908d197
OVERALL VERDICT: PASS. I could not reproduce any of the four previously reported credential/capture safety defects. I used fresh, out-of-tree probes rather than the committed regression tests.
- Cross-block and 512-boundary credential detection — PASS. A new synthetic configured credential was split across two adjacent text blocks; both blocks were fully quarantined (
content="",retained_bytes=0,redaction_codes=["credential_detected"]). It was also split (a) across the 512-character cutoff inside a tool id and (b) exactly between a 512-character tool id and its adjacent tool name. Both whole tool blocks were quarantined and all tool identity fields were cleared. - Full tool id/name scanning before bounds — PASS. Separate fresh cases placed the complete credential after offsets 577 in
idand 603 inname. Both were detected before bounding; each block was emptied andtool_call_id,tool_name, andsource_identitywere null. - Unattributable tool results — PASS. Fresh cases produced: unmatched result →
orphaned_tool_result; two calls sharing one id →ambiguous_tool_call_id; root call with sidechain result →cross_stream_tool_result. In all three results,tool_call_id,tool_name, andsource_identitywere null. - Real cross-process spool race/accounting — PASS. Two independently initialized OS worker processes raced against the same spool. At the reserved slot, a
pre_compressboundary was accepted while the ordinary event was refused; stored ids contained both seeds plus the boundary, withdropped=1. In a second simultaneous two-process refusal race, both refusals were preserved atomically asdropped=2. Accounting balanced exactly: attempted = stored + dropped in both phases; no unrecorded loss.
Full plugin suite: 148 passed in 6.54s.
No merge was performed.
Pavel-Tk
left a comment
There was a problem hiding this comment.
Follow-up review of 908d197 — REQUEST CHANGES (2 CRITICAL remain)
I re-ran the original regression set and added fresh boundary/process variants.
Resolved and verified:
- the original same-record top-level and nested block splits;
- secrets spanning the former 65,536 content bound and the new 512-character tool-field bound, including UTF-8;
- tool input/orphan quarantine and content-free digest;
- orphan, duplicate-result, and ordinary cross-sidechain attribution cases;
- session/scope redaction and 262,144-byte event cap;
- small-spool full boundary reservation;
- a real two-OS-process admission race and six-process counter RMW/restart race.
The full branch suite passes: 148 passed. The committed adversarial regression file passes: 17 passed. My fresh suite is 8 passed, 2 failed, exposing these remaining blockers:
CRITICAL 1: joint scanning still resets at each JSONL record
The joint scan at transcript.py:271-274 covers only one record's message.content. I split configured fresh-secret-0123456789 between two adjacent tool_result records in the same session/sidechain. Both raw halves remained in the two emitted messages. The receiver can reconstruct the exact configured credential.
The security invariant was specifically that no path leaves partial credential fragments across tool-result boundaries. Moving the boundary from two blocks in one record to two blocks in adjacent records must not restore the leak. The scan/overlap must span the transcript's adjacent visible block stream, not reset per JSONL record; a detection must quarantine every contributing block.
CRITICAL 2: provenance lineage is computed from 512-character wire-bounded values
_coordinates() bounds agent_id, then _lineage_key() at transcript.py:354-363 uses that bounded value for trust pairing. I used two distinct concurrent sidechain agent IDs:
"a" * 512 + "-one"
"a" * 512 + "-two"
A tool_call(id="shared", name="TrustedTool") in the first and a forged tool_result(tool_use_id="shared") in the second collided after bounding. The forged result retained tool_call_id="shared", tool_name="TrustedTool", and source_identity="TrustedTool" with no reason code.
Use a non-emitted, content-free digest of the complete redacted/raw lineage coordinate for pairing, or another collision-resistant full-coordinate key. Do not make provenance decisions from display-bounded fields.
Counts
- Full plugin suite: 148 passed, 0 failed
- Committed review regressions: 17 passed, 0 failed
- Fresh variants: 8 passed, 2 failed
- Remaining CRITICAL findings: 2
Artifacts: /tmp/review185_reverify_fresh.py, /tmp/review185_reverify_fresh_output_final.txt, /tmp/review185_reverify_full_suite.txt.
Summary
Tests
uv run ruff check .uv run pytest -q— 131 passeduv run python scripts/verify_public_plugin_candidate.py --root .— no findingsuv run python -m compileall -q src scriptsNo release, tag, or
SHA256SUMSchange is included. Release work follows review.Linear: SUB-185