fix: redact model-authored tool titles on all deny surfaces (#2274) - #2389
Conversation
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThis is a security-hardening refactor: it consolidates title redaction into Let me verify the refactor preserves behavior and doesn't drop any audit/reject step. Checking the key concerns:
No guard removed without replacement; no new reachable leak on a changed line; no AUTOSDE rule violation (the redaction hardening strengthens the No findings. [OPUS-REVIEWED] e8d4d46 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound chokepoint design, but the description and the shipped code disagree on the trust-reads path — a claimed Watch
[DESIGN-REVIEWED] e8d4d46 |
|
Addressed the GPT 5.6 blocking finding in d58378e: removed the Also applied the Design review suggestion: the four remaining inline two-call redaction sites ( |
57a9dbc to
d58378e
Compare
event.title prefers the model's own description field, so it is LLM-controlled display text. The PreToolUse hook-block deny path already redacts it, but nine other deny rows interpolated the raw title into blocked transcript rows (broadcast to the dashboard and persisted to the ConversationLog) and fourteen SEL audit calls passed tool_name=event.title unredacted, so a credential or exfiltration URL the model planted in the title reached both surfaces verbatim. Redact at the chokepoint instead of per site: a new _redact_display_text helper carries the redaction idiom, and two shared deny helpers (_reject_invalid_tool, _reject_hook_error) render each deny shape in exactly one place so a permission path added later cannot reintroduce the leak by omission. The six audit-only sites reuse the redacted variable already in scope or call the helper directly. Both redactors return clean input unchanged, so titles without credential/exfil content are byte-identical in rows and audit records. Closes #2274
d58378e to
e8d4d46
Compare
…ev#2274) (kirodotdev#2389) event.title prefers the model's own description field, so it is LLM-controlled display text. The PreToolUse hook-block deny path already redacts it, but nine other deny rows interpolated the raw title into blocked transcript rows (broadcast to the dashboard and persisted to the ConversationLog) and fourteen SEL audit calls passed tool_name=event.title unredacted, so a credential or exfiltration URL the model planted in the title reached both surfaces verbatim. Redact at the chokepoint instead of per site: a new _redact_display_text helper carries the redaction idiom, and two shared deny helpers (_reject_invalid_tool, _reject_hook_error) render each deny shape in exactly one place so a permission path added later cannot reintroduce the leak by omission. The six audit-only sites reuse the redacted variable already in scope or call the helper directly. Both redactors return clean input unchanged, so titles without credential/exfil content are byte-identical in rows and audit records. Closes kirodotdev#2274
Summary
event.titleis model-authored display text (_select_tool_titleprefers the backend-supplieddescription, including for Bash tools). The PreToolUse hook-block deny path already redacts it (_redacted_hook_block/_reject_hook_blocked), but every other deny path published the raw title — verified on main @ 8b502bc, 23 leaking surfaces insrc/kiro_crew/dashboard/chat_runner.py:event.titleinto🚫 … (invalid: …)/🚫 … (hook error)rows that are broadcast to the dashboard AND persisted to the ConversationLog (auto-approve, context-builder continuation, trust-reads, trust/YOLO, interactive-approval paths).tool_name=event.titleunredacted — 8 under the rows above, 6 on paths whose row was already redacted (normal invocation, host-gate deny, trusted-pattern, trust-reads, batch rejection, interactive approve).A credential or exfiltration URL the model plants in a tool title reached both surfaces verbatim.
Fix
Redact at the chokepoint, mirroring the existing hook-block pattern — one redaction site per deny shape, not nine inline copies:
_redact_display_text()helper carries the redaction idiom (redact_exfiltration_urls+redact_credentials, idempotent, byte-identical for clean titles);_redacted_hook_block()is rewritten on top of it (behavior-identical)._reject_invalid_tool()and_reject_hook_error()render each deny shape in exactly one place (reject → redacted row → redacted audit), so a permission path added later cannot reintroduce the leak by omission. All 9 inline sites replaced; per-site control flow (continue/break) and per-sitemetadata={"reason": "interactive"}preserved.audit=Falseto keep this change redaction-only._deny_title,_safe,_title,_approved_title) or call_redact_display_text(event.title).errorfield is also redacted: hooks are fired with the tool name and parsed input, so an exception that wraps its inputs can carry model-authored text.{e}needs no redaction:_validate_tool_nameraises only fixed messages that never echo the offending name.docs/system-specs/modules/security.mddocuments the chokepoint contract.Tests
test/test_dashboard_approval.py::TestDenyRowTitleRedaction— 10 new tests, red-checked (all 10 fail on unfixed code with the raw credential leaking, pass with the fix):tool_name, and[REDACTED: credential]appears in the blocked row. Both invalid-name and hook-error triggers covered where the path has both; the trust-reads test also pins the no-audit behavior.🚫 {event.title}andtool_name=event.titlecount zero in the module source, and each deny shape is rendered exactly once (its helper).Verified locally: isort/flake8/mypy clean;
test_dashboard_approval.py60/60; full suite 39,496 passed (74 pre-existing host-environment failures — sandbox backend unavailable — identical on unmodified main); docs-lint and brand gate clean.Out of scope (deliberate)
logger.warning(...)call sites (e.g.AUTO-REJECTED tool=%r, Slack-delivery failure, approval timeout) still write the raw title to the server log. Server-log redaction is a separate surface class from the issue's two (ConversationLog rows + SEL audit) and is intentionally not touched here; flagged by pre-push review and worth a follow-up issue.Risk notes
Audit consumers that key on exact
tool_namevalues will see the redacted form only for titles containing a credential or suspicious URL — identical to the change already made on the hook-block path. No behavior change for clean titles (both redactors return clean input unchanged).Closes #2274