fix: scope the deny scan to command fields and keep redacted paths distinct

[bolichen97]

Problem / Motivation

Two security-boundary defects on the permission funnel and the dashboard egress path, batched because both are the same shape: a check that is right about the mechanism but wrong about the field it reads.

• The always-enforced tool_input scan handed every string in a non-shell tool's payload to the shell-command predicates. A document write whose body merely quoted a denied command (rm -rf /, git push origin main) or named a credential path was refused, and any benign body over the command size ceiling was refused for its length.
• The project tree and git-status listings redacted each path with the whole-string redactor, which collapses every matched token to one fixed tag. Two genuinely different paths whose only differing segment is credential-shaped redacted to the same string, and the de-duplication that guards the dashboard tree against its "Duplicate path" crash then silently dropped one of them.

Why it matters

The first defect blocks legitimate agent work (writing docs, tests or scripts that mention a command) with a security refusal, which trains users to distrust the deny list. The second makes the dashboard tree and git panel lie by omission: a file exists in the repository and is missing from the listing, with no indicator that anything was dropped.

What changed (motivation → approach → change)

• Field-scoped deny scan for non-shell tools . For a tool whose provenance the client established from the tool_call frame (resolved non-shell classification, params from the cache, resolved tool identity), the scan now skips strings sitting directly under a document-body key and keeps every other string: a command word, every path spelling, a URL, any unlisted key. A shell tool keeps the full scan over every string. A frame missing any provenance flag is treated as an unknown tool and keeps the full scan (fail closed). A walk that hits its work cap is denied as unverifiable. The security floor and the deny list are untouched: the same rules run over fewer fields for a narrower set of tools.
• Path-aware egress redaction. A segment-wise redactor wraps the whole-string redactor: each path segment is redacted on its own, and every redacted segment carries a short opaque label derived from a random key the gateway generates once per process and keeps only in memory (HMAC over the segment). Two different secrets in the same position therefore stay two entries, the label reveals no byte of the secret and cannot be brute-forced offline without the key, it does not depend on listing order, and it is stable across the tree and git-status responses of one process so the dashboard can join them. The whole-string result is kept as the floor whenever the segment-wise result would leave anything sensitive behind, so redaction never gets weaker.

Behavior changes

• diff_signals.py flagged no structural ! signals.
• Built-in document-writing tools with trusted provenance (the kiro-cli file tool under its fs_write / write names): strings directly under content, fileText/file_text, text, newStr/oldStr, new_str/old_str, newText/oldText are no longer run through the shell-command deny predicates. The claude-agent-acp backend's tools are not on the list because their frames carry no _meta.kiro.toolName, so they keep the full scan. Every other string still is. Shell tools, MCP tools, other built-ins and unclassified frames are unchanged.
• A non-shell tool whose params walk exceeds the node cap is now denied with Blocked: tool arguments too large to security-scan (deny-by-default) instead of being scanned partially.
• Redacted paths in the project tree and git-status listings now carry a ~ plus 12-character label (for example [REDACTED_AWS_KEY]~3f9a1c0b7d2e); the label is stable within one gateway process and changes on restart.
• The security module spec (docs/system-specs/modules/security.md) documents both mechanisms.

Tests

New:

• test/test_llm_helpers_non_shell_gate.py (24 tests): body keys skipped only when all three provenance flags hold; shell tools and every partial-provenance combination keep the full scan; a command word, a path and a URL in a non-body field are still denied; a mapping under a body key is still walked; a truncated walk is denied; the security floor is untouched.
• test/test_redact_path_segments.py: distinct credential-shaped segments stay distinct; identical originals map to identical outputs; no byte of the token and no unkeyed hash prefix of it survives; the label is stable across calls and listings, depends on the process key, and is independent of input order; the whole-string fallback and fixed-point properties hold.

Updated:

• test/test_project_tree.py (+2): redaction-collision paths stay distinct; a true collision is still de-duplicated.
• test/test_project_git_status_log.py (+2): redaction-collision files stay distinct; a true collision is still de-duplicated.

Commands run locally (exit codes checked, output redirected to files):

• .venv/bin/python -m pytest -q test/test_llm_helpers_non_shell_gate.py test/test_redact_path_segments.py test/test_project_tree.py test/test_project_git_status_log.py test/test_llm_helpers_edit_gate.py: 109 passed.
• Full profile gate list from prepare-pr/profiles/kirocrew.json: see the gate table in the PR's first comment thread / PREP notes; every backend and meta gate green, frontend build and tsc -b green, remaining frontend gates skipped because the diff does not touch website/.

Manual verification

N/A: unit coverage exercises both handlers through their aiohttp test clients and the permission funnel through its event fixtures; there is no UI change to inspect.

Related Issues

no linked issue: closeout batch from an internal deployment, no GitHub issue tracks it.

Pattern harvest

Rule candidate: review-prompt
Pattern: "a security check applied to the wrong field shape: a shell-command predicate over a document body, or a whole-string redactor over a structured path whose consumer de-duplicates". Both fixes keep the mechanism and change only which strings reach it; a reviewer prompt asking "what does this field actually carry, and what consumes the output" would have caught both at authoring time.

Not in scope

• No change to assert_security_floor, the deny catalog, or MAX_SCANNABLE_COMMAND_CHARS.
• No change to the edit gate (_edit_target_denial) beyond the spec sentence that now describes non-edit kinds elsewhere.
• No redaction change on any surface other than the two dashboard listings; other callers of redact keep the whole-string form.
• Windows path separators: the segment-wise redactor splits on / only, because the listings it serves emit POSIX-relative paths.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[bolichen97]

Intent: Stop two egress-side false positives without weakening the deny path: a non-shell tool with client-established provenance is no longer refused for a document body that merely quotes a command shape or a credential path, and project-tree redaction keeps distinct paths distinct so the tree never silently drops a file.
Not a goal: Changing the deny rules themselves, the security floor, shell-tool scanning, unknown or untrusted-provenance tool handling, or how much any single string is redacted.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of bbf90df894269733f45750290df666846b714983 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Both fixes are root-cause corrections with fail-closed provenance, floor-preserving redaction, and the frontend join semantics actually verified — the right shape, spec updated in-commit.

The scan scoping keys on client-derived provenance plus a positive, producer-named tool list rather than the agent-influenced kind, mirrors the existing edit-gate pattern (deny-on-truncation, MCP never exempt, unknown frames keep the full scan), and the deliberate exclusion of _meta.kiro.toolName-less backends is covered for their file edits by the existing diff-block edit gate. The segment-labelled redaction was checked against the real consumer: PierreWorkspaceTreeImpl.tsx reconstructs absolute paths from repoRoot + status path and re-relativizes against the redacted tree root, and the new whole-string prefix redaction plus the cross-response label-stability tests pin exactly the equality that join needs. The rewritten collision tests preserve the old pins' actual invariant (no duplicate entries reach @pierre/trees) while fixing the silent-drop defect they tolerated.

Suggestions

• The five-property rationale for the HMAC label lives only in a docstring; a one-line pointer from docs/system-specs/modules/session.md-style consumer docs isn't needed, but consider noting in security.md that a second consumer of the label must not persist it across restarts.

[DESIGN-REVIEWED] bbf90df

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed bbf90df894269733f45750290df666846b714983 via the fork AI-review pipeline; updated in place on each push.

Review details

The candidate list contains no candidates — the discovery pass reported "No candidates." I've independently examined the changed code for grounded defects (Step 2): the field-scoped scan's provenance gating (is_document_writing_tool + four client-derived flags, all fail-closed), the command_shaped_strings node cap (fails closed via truncated), the write-target path field still reaching is_sensitive_path, the redact_path_segments two-floor construction (segment-join must equal the whole-string result; labelled candidate must be a redactor fixed point), the popped _prefix (no raw subdir leak), and the straddle-token fallback to whole-path redaction. Each potential hole is closed in the diff itself and pinned by the accompanying tests. Nothing reaches the 80+ bar.

No findings.

[OPUS-REVIEWED] bbf90df

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of bbf90df894269733f45750290df666846b714983 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks are done. I have what I need for the verdict: the defects both have checkable provenance in the base tree (issue #8053 is cited in base llm_helpers.py:922; the collapse-then-drop behavior is documented in base files.py comments and the old tests), the provenance fields all pre-exist in acp/types.py, the frontend join is real, and the only weak spot is that some DOCUMENT_BODY_KEYS entries have no producer that can reach the exemption.

First-Principles-Verdict: CONCERNS

Two of the ten DOCUMENT_BODY_KEYS (oldText, newText) have no producer that can reach the exemption — by the PR's own rule they don't belong on the list.

Not justified as shipped

• 7 — zero producers (partial): grep oldText|newText over src/ = 13 hits, every one a content-block read (cb.get("oldText") in _dispatch.py, client.py, chat_runner.py); none in rawInput. The exemption only reads rawInput of fs_write/write, so these keys are exactly the "exemption waiting for a producer nobody vetted" the docstring forbids. The file_text/old_str/new_str trio rests on the unverified claim that kiro-cli "can present a file edit through" the text-editor shape (fails inert if wrong, so not availability).
• 5 — undeclared: the git-status repo-subdirectory prefix mechanism (_prefix, _redact_status_path, straddle fallback) appears nowhere in the description; it is derived from the frontend re-anchoring at PierreWorkspaceTreeImpl.tsx:248-262.
• 9 — rides along: comment rewrites in three untouched-by-the-fix functions plus the baseline drop 6656→6651; sanctioned by the baseline's own prune rule, but not this fix.

What this change ships

Inventory (9 items) — 6 justified

Intent: stop the deny scan refusing benign document bodies and stop the dashboard silently dropping redaction-colliding files — a FIX (both defects checkable in base: issue #8053 at llm_helpers.py:922; the drop pinned by the old collision tests).

1. A built-in file-tool write whose body quotes a denied command or credential path is now approved — justified
2. A non-shell tool with params too deep to walk is now denied as unverifiable instead of part-scanned — justified
3. Colliding redacted paths both stay in the tree and git panel, each carrying ~ + 12-hex label — justified
4. Labels are stable within one gateway process and change on restart — justified
5. Git-status paths under a repo-subdirectory get a whole-string-redacted, unlabelled prefix — undeclared
6. New security-facade export redact_path_segments — justified (2 consumers: the two listings)
7. DOCUMENT_BODY_KEYS/DOCUMENT_WRITING_TOOLS exemption tables — zero consumers for 2 of 10 keys
8. Security spec rewritten in the same commit — justified
9. Unrelated comment rewrites plus baseline count drop — rides along

Watch

• Unreachable exemption keys in a security scan are latent surface: oldText/newText counted 0 rawInput producers; the snake_case trio is claimed, not shown. Clears when: a rawInput producer for each key is named on kiro-cli, or the unproduced keys are deleted.

Subtractions

• Drop oldText/newText from DOCUMENT_BODY_KEYS (platform/tool_paths.py) — 0 reachable producers; re-add each snake_case key only with a demonstrated kiro-cli rawInput frame.
• Delete test_the_listing_helper_is_gone (test/test_redact_path_segments.py) — it pins the absence of redact_paths_distinct, which never existed in base (grep: 0 hits outside this test).

[FIRST-PRINCIPLES-REVIEWED] bbf90df

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed bbf90df894269733f45750290df666846b714983 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] bbf90df

[bolichen97]

self-added: yes
mechanism: per-segment discriminator appended by redact_path_segments; length raised from 6 to 16 hex digits

• fixed span=50b2215ffa8d src/kiro_crew/security/redaction.py 24-bit discriminators have practical collisions (c3b6568)

_PATH_SEGMENT_DISCRIMINATOR_CHARS is now 16 hex digits (64 bits of SHA-256 over the original segment). An accidental collision between two segments of one tree is about 2^-64 per pair, and a chosen collision needs a second preimage on SHA-256, so two distinct credential-shaped names cannot be made to redact to the same listing entry in practice.
The discriminator still reveals no byte of the token: it is a hash prefix, not a slice of the segment, and the whole-string fallback that keeps the redaction floor is unchanged.
This ruling covers the class of "discriminator too short to keep distinct inputs distinct" for this helper. Tests: test/test_redact_path_segments.py derive the expected suffix from the constant and pass at the new length; the listing de-dup tests in test_project_tree.py and test_project_git_status_log.py are unchanged and green.

[bolichen97]

self-added: yes
mechanism: DOCUMENT_WRITING_TOOLS allowlist plus is_document_writing_tool predicate gating the body-key exemption

• fixed span=5404319ff12a src/kiro_crew/platform/tool_paths.py Generic body keys bypass command denial for executable MCP tools (b18d250)

The body-key exemption now applies only to a BUILT-IN document writer: the resolved tool name must be in DOCUMENT_WRITING_TOOLS (kiro-cli fs_write, the claude-agent-acp Write/Edit family, the dashboard's own write/edit/create/insert verbs) and the resolved MCP server name must be empty. An MCP tool, whatever it names its fields and even when it reuses a built-in's name, keeps the full scan over every string.
This ruling covers the class "a field-shape exemption reachable by a tool the client cannot prove is inert": the exemption is keyed on tool identity the client established, not on the payload's field names, and any name outside the list falls back to the full scan.
Tests: test/test_llm_helpers_non_shell_gate.py now asserts a trusted MCP tool body is refused, an MCP tool named fs_write is refused, a built-in that is not a writer (use_aws) is refused, and every listed writer skips the body.

[bolichen97]

self-added: yes
mechanism: redact_paths_distinct assigns per-response ordinals (~1, ~2) to redacted paths that collide; the secret-derived hash suffix is removed

• fixed span=50b2215ffa8d src/kiro_crew/security/redaction.py Secret-derived suffix enables credential recovery (6afc405)

A retrospective ran this round (the span recurred and the round count reached three). It ruled the secret-derived discriminator is not required by the intent, which only needs distinct inputs to stay distinct within one listing, so the mechanism is REPLACED, not repaired: no hash, no prefix, nothing derived from the original segment is emitted any more.
redact_path_segments now returns the plain segment-wise redaction; redact_paths_distinct(paths) runs over the whole listing at the two call sites and appends an opaque per-response ordinal only to redacted strings that two or more different originals collapse to. The ordinal is listing-order, carries no information about the secret, and is not stable across responses by design. The whole-string fallback and the fixed-point property are unchanged.
This ruling retires the class "discriminator derived from the redacted secret" for this helper entirely: there is no derivation left to shorten or lengthen. Tests in test/test_redact_path_segments.py assert the sha256 prefix of the segment is absent from the output and that distinct inputs stay distinct.
The same retrospective removed two riders that no finding required: the speculative allowlist rows with no producer (create, insert, edit, strReplace, body keys text, body) and the cdk.out prune of the project tree.

[bolichen97]

Rebased onto current main to clear a merge conflict; the only conflicting file was comment-history-baseline.json, regenerated by the gate's own --write-baseline. No code change; touched tests, mypy and the static gates are green locally. Reviewer dispositions for the prior head carry over unchanged in content.

[bolichen97]

self-added: yes
mechanism: the joined prefix/below result is re-scanned whole and falls back to whole-path redaction when anything sensitive remains

• fixed span=0bf67dcbaf49 src/kiro_crew/dashboard/handlers/files.py Prefix splitting bypasses whole-path credential redaction (6c42082)

_redact_status_path now applies the same floor redact_path_segments applies to its own assembly: after joining the whole-redacted prefix with the segment-labelled remainder it runs redact(joined); when the result is not a fixed point (a token straddling the joining slash, a shape that only matches in context) it returns redact(path) on the whole original path instead. The dashboard-join property is unchanged for every path the redactor leaves alone. Test test_a_token_straddling_the_prefix_join_falls_back_to_whole_path uses a redactor that matches only the joined shape and asserts the whole-path result.
This push also rebases onto current main (comment-history-baseline.json regenerated).

[bolichen97]

• fixed span=9daefa2de6b5 src/kiro_crew/platform/tool_paths.py "Write" is unreachable for Claude because its frames lack _meta.kiro.toolName, so document bodies remain fully scanned and refused (1b795da)

Confirmed: the client caches a tool name only from _meta.kiro.toolName (acp._dispatch._kiro_tool_name), which the claude-agent-acp backend does not emit, so its Write / Edit / MultiEdit / NotebookEdit rows could match nothing. Those four names are removed from DOCUMENT_WRITING_TOOLS, and the body keys only they produce (old_string, new_string, new_source) are removed from DOCUMENT_BODY_KEYS; the comments, docs/system-specs/modules/security.md and the PR description now say the Claude backend keeps the full scan and why. The unit test that used the Claude edit shape uses the kiro-cli oldStr/newStr shape instead. No behaviour change for any reachable tool.

[bolichen97]

• fixed span=c2e5c92f85c1 docs/system-specs/modules/security.md "Write/Edit/MultiEdit/NotebookEdit" contradicts DOCUMENT_WRITING_TOOLS, which contains only fs_write and write (ccb53b0)

The remaining Claude-family sentence in the field-scoped-scan bullet is replaced by the statement that the claude-agent-acp backend's tools are not listed because their frames carry no _meta.kiro.toolName, and the bullet's lead-in now says only a built-in document writer has its body keys skipped. Docs-only; the push also rebases onto current main (a comment-wording conflict in security/redaction.py resolved to main's text, baseline regenerated).

[bolichen97]

self-added: yes
mechanism: segment-wise redaction is accepted only when its unlabelled join equals the whole-string result, before labels are added

• fixed span=50b2215ffa8d src/kiro_crew/security/redaction.py Fixed-point check leaks cross-segment credential tails (bbf90df)

redact_path_segments now redacts every segment first and compares the unlabelled join with the whole-string result byte for byte; any difference (a key=value whose value carries a / is matched across the separator by the whole pass but only up to it by the segment pass, leaving a tail that is not a match on its own) returns the whole-string result. Labels are added only after that check passes, and the labelled result must still be a fixed point of the redactor. Test test_a_token_whose_value_spans_a_separator_falls_back_to_the_whole_pass uses a token=[A-Za-z0-9/]+ redactor and asserts the whole-pass result with no byte of the value and no label.