Repository navigation
fix(runtime): bound canvas sanitizer output growth - #6913
Conversation
Lead the fragment filename with the PR number so it sorts correctly, unwrap the hard-wrapped body to one sentence per line, and add the (#6913) reference so the release flow doesn't also emit a generated line for this PR.
houko
left a comment
There was a problem hiding this comment.
Automated daily review pass. No code changes needed — leaving two minor, non-blocking notes rather than editing:
-
Core fix looks correct.
append_sanitized_htmlchecksoutput.len() + fragment.len() > max_bytesbefore everypush_str, which correctly catches the case this PR targets (entity/tag escaping expanding a within-cap input past the cap during sanitization, previously only checked once at the very end). The two new tests (bounded_sanitized_append_rejects_before_growing_output,sanitizer_enforces_output_cap_during_escaping) exercise the helper directly and the escaping-growth path end-to-end. Changelog fragment (changelog.d/fixed/6913-canvas-sanitized-output-cap.md) is correctly formatted and placed underfixed/, consistent with prior similarly-shaped memory-growth bounding fixes in this repo (e.g. the #6446 attachment-fetch streaming cap), sofixed/rather thansecurity/looks right here too. -
Non-blocking style nit: the second commit's message body (
docs(changelog): fix fragment format for #6913) is hard-wrapped at ~72 cols across 4 lines rather than one-sentence-per-line per the repo's prose rule (CLAUDE.md: "Commit message bodies" — subject-line truncation is a git display convention, not license to wrap the body). Not fixing via rebase since the branch is already pushed and out of caution around rewriting pushed history. -
Out-of-scope observation, not a finding against this diff:
tool_canvas_present'stitle = escape_html(raw_title)path (unchanged by this PR) still only gets bounded indirectly, via the post-constructionfull_html.len() > max_bytescheck after formatting the whole document string. That's pre-existing behavior this PR doesn't touch, so not flagging as a defect here — just worth a maintainer's eyes on whether an unusually largetitleargument is already bounded upstream (e.g. tool-call argument size limits) independent of the canvas HTML cap this PR fixes.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated daily review pass.
Checked both commits' author identity (fa88e42... by Evan Hu suzukaze.haduki@gmail.com, df57fac... by houko suzukaze.haduki@gmail.com) — no Claude/Anthropic attribution in either author identity or commit message, so no blocking finding there this time.
Minor, non-blocking note: commit df57facd290f902d0b341944dd44b9d50ebb1a51's message body is hard-wrapped at a fixed column mid-sentence ("Lead the fragment filename with the PR number so it sorts correctly,\nunwrap the hard-wrapped body to one sentence per line, ..."), which is itself what CLAUDE.md's prose-wrapping rule asks commit bodies to avoid (one sentence per line, break only at sentence boundaries). Not asking for a history rewrite over this alone — flagging for awareness only.
Changelog fragment (changelog.d/fixed/6913-canvas-sanitized-output-cap.md) is correctly placed, correctly formatted (no leading - , one sentence, ends (#6913) (@houko)), and matches the fix. No action needed there.
One inline finding on the actual fix, see below.
Generated by Claude Code
|
|
||
| #[test] | ||
| fn sanitizer_enforces_output_cap_during_escaping() { | ||
| for html in ["<", ">", "&"] { |
There was a problem hiding this comment.
The bound-enforcement logic itself looks correct: append_sanitized_html checks the cap before every push, so the amplification cases (& to &, < to <, > to >) now fail as soon as the next fragment would cross max_bytes, instead of after the whole (potentially several-times-oversized) buffer was built and compared once at the end. That matches the resource-exhaustion fix this PR claims.
One coverage gap: both new tests (bounded_sanitized_append_rejects_before_growing_output and this one) only exercise single-byte/small inputs where max_bytes equals the input length. There's no test with a large adversarial payload (e.g. tens of thousands of & characters against a small max_bytes) asserting that the function fails fast and that memory growth actually stays bounded — the property this PR exists to guarantee. A happy-path small-input assertion that sanitize_canvas_html("&", 1) errors doesn't distinguish "bounded before growth" from "bounded after growth" the way a large-input regression test would (e.g. assert the error's reported next_len stays within max_bytes + fragment.len() rather than anywhere near 5 * input.len(), or use a large enough input that the old post-hoc check would have been measurably slower/larger). Recommend adding one large-input case before merge.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated daily review pass: DoS-bound fix looks correct; one non-blocking nit on a commit message's prose wrapping. See inline comment.
Generated by Claude Code
There was a problem hiding this comment.
Nit, not blocking: the docs(changelog): fix fragment format for #6913 commit (df57fac) has a hard-wrapped message body, breaking mid-sentence at ~72 columns rather than at sentence boundaries.
CLAUDE.md's prose-wrapping rule explicitly covers commit message bodies (only the subject line gets the git-display-truncation exception), so this commit's own message doesn't follow the convention it's fixing the changelog fragment to comply with.
Not fixing this myself since it would need a history rewrite (amend/rebase) and force-push, which requires explicit maintainer OK — flagging for awareness only.
Everything else looks solid: the sanitizer growth cap review checked out.
append_sanitized_html is the only mutation point for result now — every push/push_str call site in sanitize_canvas_html_with_tags (tag open/close, </>/& escaping, entity passthrough, and the raw-text run) was converted, so the bound is enforced on every code path that grows output, not just the common case.
The check happens before push_str, so on rejection the buffer is left untouched (verified by the new bounded_sanitized_append_rejects_before_growing_output test) and there's no unbounded interim allocation before the cap kicks in — the original bug (escaping &/</> can 4-5x a small input) is closed.
There's no truncation logic anywhere (the function errors out wholesale rather than cutting the buffer), so there's no multi-byte UTF-8 boundary risk, and small/valid outputs are unaffected since the checked appends are byte-for-byte identical to the old unconditional pushes when they succeed.
Also confirmed no AI/Claude attribution on either commit on this branch (fa88e42 — Evan Hu, df57fac — houko; both real human identities).
Generated by Claude Code
Changes
<,>, and bare&expansion pathsTests
cargo test -p librefang-runtime tool_runner::canvas::tests --lib(9 passed)cargo clippy -p librefang-runtime --lib --no-deps -- -D warningscargo check --workspace --libcargo fmt --all -- --checkgit diff --checkOut of scope