Harden subprocess output handling (LOW-1, INFO-1, LOW-2) - #11
Merged
Conversation
…uences Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the obsolete #![allow(dead_code, unused_imports)] from output.rs now that all symbols are wired up in server.rs and command.rs. Add a paragraph to the Security Model section of the README describing the defensive output-capture and ANSI-filtering behaviour. Also apply cargo fmt which collapses a stray double-blank-line in command.rs and updates one test helper in output.rs to use repeat_n (clippy::manual_repeat_n). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three security-audit findings in how child-process output is captured and echoed, and DRYs the previously-duplicated capture code in
server.rs/command.rsinto a single sharedsrc/core/output.rs.LineAccumulatorforce-flushes a line once it reachesMAX_LINE_BYTES(64 KiB), bounding memory when a child emits a long run of bytes with no newline.from_utf8_lossy) only at newline boundaries, so multibyte characters split across 8 KB reads are no longer mangled.AnsiTeeFilter(a streaming state machine) preserves SGR colour/style (ESC[…m) but strips OSC/DCS/APC/PM/SOS string payloads (window-title, clipboard), cursor-movement, screen/line-clear CSI, overlong CSI, and stray C0 control bytes. The TUI render path was already safe (ratatui renders into cells).Captured log lines additionally have control characters (except tab) stripped. The README Security Model section documents the new behavior.
Threat model
Config files and the commands they spawn are trusted (documented); the residual risk mitigated here is untrusted data flowing through a trusted server's output (e.g. logged HTTP request data). The 8-bit C1 introducers (0x9b/0x9d) are intentionally passed through — they're indistinguishable from UTF-8 continuation bytes in a byte-wise filter — which only matters on legacy Latin-1 terminals; this is documented in a code comment.
Test Plan
cargo test --bins— 52/52 passing (deterministic; includes newcore::outputunit tests for line capping, UTF-8 boundary decoding, control stripping, and the full ANSI filter matrix)cargo fmt -- --checkcleancargo clippy --all-targets -- -D warningscleanpreserves_final_command_stdout_and_stderrintegration test passes (output forwarding unchanged)tests/cli.rshas pre-existing flaky failures (process-group-kill timing races, "Failed to stop process …") that exist onmainand are unrelated to this change🤖 Generated with Claude Code