fix: redact cron stderr/stdout before truncating, report stderr tail - #5574
Conversation
run_command_sandboxed's failure branch took the HEAD of stderr, so a chatty startup warning displaced the terminal error, and sliced before redacting. run_script_sandboxed's bad-output diagnostic truncated stdout before redacting, leaking the unredacted head of a credential straddling the 200-char boundary. Redact the complete stream first, then slice; the stderr report takes the tail, where a crashing process leaves its diagnosis. Closes #5547
github-actions
Bot
added
the
readiness: checking
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) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix (redact whole stream, then slice) at both named sites, boundary layouts pinned byte-exactly by tests; scope discipline vs #4402 is sound. Suggestions
[DESIGN-REVIEWED] 93d288d |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All evidence is in. The diff does exactly what the description says (two ordering fixes plus tests), so the review turns on two counted facts: the head-slice fix re-spells an existing helper, and the same root cause has unfixed siblings elsewhere. First-Principles-Verdict: CONCERNS
What this change shipsIntent: stop failed-cron reports from leaking boundary-cut credentials and from showing startup noise instead of the terminal error — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 93d288d |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBased on my analysis, I've confirmed the structural facts without needing the benchmark. Let me finalize. Structural verification of the candidate: Reading The diff moves
Command/stderr output is untrusted external content; an attacker who shapes a failing cron command's output can emit multi-MB of dense credential tokens, and redaction now processes the whole buffer superlinearly. Diff-introduced (bounded→unbounded), so a bounded-window redact is an in-diff fix. Magnitude requires adversarially-crafted, multi-MB, dense-credential command output on a failing cron — real but a specific multi-step chain, and normal operation is unaffected. That places it below the clean-crash/DoS bar for BLOCKING; advisory is the honest call. Redact before truncate is a genuine security improvement (proven by the added tests); the only downside is the unbounded redact input. FINDING. FINDING — src/kiro_crew/cron_script.py:924,728 — [OPUS-REVIEWED] 93d288d Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
github-actions
Bot
added
readiness: passed
|
Disposition of the First Principles CONCERNS (advisory, verdict pinned to Watch item (five slice-before-redact siblings outside this diff): accepted and deferred to #5582. All five sites verified real as counted ( Subtraction ( |
…ites Each site spelled the composition as _redact(x[:n]) — slice first, redact second — so a credential or exfiltration URL straddling the truncation boundary was cut before the redaction regexes saw it, and the surviving raw fragment escaped into SEL audit rows, logs, and dashboard payloads. The prefix-anchored, fixed-width credential patterns (e.g. AKIA/ASIA + 16) cannot match a fragment missing its prefix or tail. Route all five sites through security.redact_and_truncate (redact the FULL text, then slice), keeping each module's existing redaction entry-point convention: subagent.py, dashboard/handlers_project.py and the spec_builder routes gain a thin _redact_and_truncate sibling next to their _redact wrapper (the routes variant keeps _redact's fail-closed guard), and agent.py truncates the interpolated command through the helper inside the f-string so the boundary no longer sits mid-expression. No max_chars value changes (80/4000/200/64/200 preserved); secret-free inputs produce byte-identical output. One boundary-straddling regression test per site, each mutation-checked against the unfixed spelling. Refs #5582 (the two cron_script.py items are deferred behind open PR #5574)
…ites Each site spelled the composition as _redact(x[:n]) — slice first, redact second — so a credential or exfiltration URL straddling the truncation boundary was cut before the redaction regexes saw it, and the surviving raw fragment escaped into SEL audit rows, logs, and dashboard payloads. The prefix-anchored, fixed-width credential patterns (e.g. AKIA/ASIA + 16) cannot match a fragment missing its prefix or tail. Four sites now route through security.redact_and_truncate (redact the FULL text, then slice), keeping each module's redaction entry-point convention: subagent.py, dashboard/handlers_project.py and the spec_builder routes gain a thin _redact_and_truncate sibling next to their _redact wrapper (the routes variant keeps _redact's fail-closed guard). agent.py's site interpolates the command through the module's context-aware shim before slicing — redact(command)[:200] — because redact_and_truncate applies only baseline redaction and would still cut a companion-only token before the companion regexes see it (server GPT review catch). No max_chars value changes (80/4000/200/64/200 preserved); secret-free inputs produce byte-identical output. One boundary-straddling regression test per site, each mutation-checked against the unfixed spelling. Test fixtures inline the fabricated AKIA literal rather than binding a secret-named variable, which tripped CodeQL's name-based source heuristic into flagging 10 unchanged production log lines. Refs #5582 (the two cron_script.py items are deferred behind open PR #5574)
…ites Each site spelled the composition as _redact(x[:n]) — slice first, redact second — so a credential or exfiltration URL straddling the truncation boundary was cut before the redaction regexes saw it, and the surviving raw fragment escaped into SEL audit rows, logs, and dashboard payloads. The prefix-anchored, fixed-width credential patterns (e.g. AKIA/ASIA + 16) cannot match a fragment missing its prefix or tail. Four sites now route through security.redact_and_truncate (redact the FULL text, then slice), keeping each module's redaction entry-point convention: subagent.py, dashboard/handlers_project.py and the spec_builder routes gain a thin _redact_and_truncate sibling next to their _redact wrapper (the routes variant keeps _redact's fail-closed guard). agent.py's site interpolates the command through the module's context-aware shim before slicing — redact(command)[:200] — because redact_and_truncate applies only baseline redaction and would still cut a companion-only token before the companion regexes see it (server GPT review catch). No max_chars value changes (80/4000/200/64/200 preserved); secret-free inputs produce byte-identical output. One boundary-straddling regression test per site, each mutation-checked against the unfixed spelling. Test fixtures inline the fabricated AKIA literal rather than binding a secret-named variable, which tripped CodeQL's name-based source heuristic into flagging 10 unchanged production log lines. Refs #5582 (the two cron_script.py items are deferred behind open PR #5574)
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Approving as requested
github-actions
Bot
removed
the
readiness: passed
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
…ites (#5599) Each site spelled the composition as _redact(x[:n]) — slice first, redact second — so a credential or exfiltration URL straddling the truncation boundary was cut before the redaction regexes saw it, and the surviving raw fragment escaped into SEL audit rows, logs, and dashboard payloads. The prefix-anchored, fixed-width credential patterns (e.g. AKIA/ASIA + 16) cannot match a fragment missing its prefix or tail. Four sites now route through security.redact_and_truncate (redact the FULL text, then slice), keeping each module's redaction entry-point convention: subagent.py, dashboard/handlers_project.py and the spec_builder routes gain a thin _redact_and_truncate sibling next to their _redact wrapper (the routes variant keeps _redact's fail-closed guard). agent.py's site interpolates the command through the module's context-aware shim before slicing — redact(command)[:200] — because redact_and_truncate applies only baseline redaction and would still cut a companion-only token before the companion regexes see it (server GPT review catch). No max_chars value changes (80/4000/200/64/200 preserved); secret-free inputs produce byte-identical output. One boundary-straddling regression test per site, each mutation-checked against the unfixed spelling. Test fixtures inline the fabricated AKIA literal rather than binding a secret-named variable, which tripped CodeQL's name-based source heuristic into flagging 10 unchanged production log lines. Refs #5582 (the two cron_script.py items are deferred behind open PR #5574) Co-authored-by: dwu96 <dwu96@users.noreply.github.com>
Five sites sliced text to a logging budget BEFORE redacting, so a credential straddling the cut survived as a fragment no credential regex matches: subagent run summaries (task[:80]), project run descriptions (desc[:4000]), the rejected-hook SEL audit (command[:200]), and two spec-builder surfaces (name[:64], first[:200]). Each now routes through security.redact_and_truncate, which redacts the full text first; the spec-builder module keeps its own fail-closed wrapper (_redact_truncated) so the no-security-module fallback still withholds rather than serves. cron_script._stderr_tail had the harder variant: it seeked to size - limit and redacted only what it read, so a key spanning the window START lost its anchoring prefix. The read now reaches back _STDERR_TAIL_OVERLAP bytes before redacting and tail-slices after. The redact(stdout[:200]) site from kirodotdev#5574 is consolidated onto redact_and_truncate while here.
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
2 participants
Problem / Motivation
Two sibling sites in
src/kiro_crew/cron_script.pyretain the truncation patterns that the fix torun_script_sandboxed's failure branch (#4402) removed, and were deliberately left out of that PR's scope:run_command_sandboxedfailure branch appendedstderr_out[:1000]-- the HEAD of stderr, unredacted. A command cron that emits a chatty startup warning before crashing reports the warning instead of the terminal error, and a credential anywhere in that head reaches the cron result raw.run_script_sandboxed's bad-output diagnostic builtredact(stdout[:200])-- truncating BEFORE redacting. A credential straddling the 200-char boundary loses the tail the redaction pattern needs, so its unredacted head leaks into the diagnostic.Why it matters
Cron failure output flows to alert surfaces (Slack, dashboard notifications, logs). The head slice makes the report useless exactly when it is needed (the terminal error is displaced by startup noise), and both slice-before-redact orderings defeat credential redaction at the truncation boundary --
redact()'s patterns (e.g. AKIA-prefixed access-key ids) cannot match a string the slice already cut in half.What changed (motivation → approach → change)
Symptom, root cause, then the change, mirroring the ordering #4402 established:
run_command_sandboxedfailure branch:stderr_out[:1000]becomesredact(stderr_out.rstrip())[-1000:]. Redact the complete stderr first (so no boundary can strand a secret), then take the TAIL -- a process that dies hard leaves its diagnosis last, so the tail carries the traceback, not the startup warning.run_script_sandboxedbad-output diagnostic:redact(stdout[:200])becomesredact(stdout)[:200]. Same 200-char window, but redaction now sees the whole stream before the slice.Deliberately NOT touched: the nonzero-exit/empty-stdout branch a few lines above the diagnostic (
stderr[:500]) -- that hunk is exactly the diff of the still-open #4402 and is fixed there; changing it here would collide with that PR. This PR covers only the two sites #5547 names.Tests
Four new tests, each mutation-verified red against the pre-fix code:
TestRunCommandSandboxed::test_nonzero_exit_reports_stderr_tail_not_head-- a >1000-char leading warning must not displace the terminal error.TestRunCommandSandboxed::test_nonzero_exit_redacts_stderr_before_truncating-- an AWS-key-shaped credential laid so the 1000-char tail window starts 2 chars into it: slice-then-redact leaks the 16-char tail, redact-then-slice does not. Positive assertions pin that the redaction marker and surrounding context survive (no vacuous pass).TestRunCommandSandboxed::test_nonzero_exit_short_stderr_stays_whole-- a short stderr is reported whole (pins therstripbehavior).TestRunScriptSandboxedErrorPaths::test_bad_json_output_redacts_before_truncating-- a credential laid so the 200-char head window ends 10 chars into it: slice-then-redact leaks the raw head, redact-then-slice shows the marker's head instead.Local gates: black gate, subprocess-encoding gate, isort, flake8,
mypy src/kiro_crew(1088 files clean),test/test_cron_script.py105 passed / 1 skipped. Full suite run; the only failures are this host's pre-existing real-home-layout floor failures, reproduced identically on a pristinemainworktree (zero cron-related).Manual verification
N/A -- unit coverage sufficient: both changes are pure string-ordering fixes on already-tested branches, and the tests pin the exact byte layouts of the leak.
Closes #5547