Harden rendering and preview resource handling - #4
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
ANSI highlighting currently can drop trailing SGR sequences buffered at end-of-string, which can let styling leak into later rendered UI content.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens Yuru’s TUI rendering and preview subsystems against terminal control-sequence injection and unbounded resource consumption, while adding key-generation budgets for CJK backends and expanding regression coverage.
Changes:
- Add terminal-safe rendering utilities and optional ANSI (SGR-only) support for candidate display rendering, with bounded highlighting work.
- Introduce bounded preview process execution/output capture plus cancellation/join semantics for preview workers and image-encode workers.
- Enforce key-generation budgets in Japanese/Chinese/Korean backends and document preview security limits.
File summaries
| File | Description |
|---|---|
| README.md | Documents preview output/time limits and SVG href restrictions. |
| docs/internals.md | Expands internal docs with new preview bounds, SVG resource restrictions, and worker lifecycle behavior. |
| crates/yuru/src/main.rs | Passes ANSI enablement into TUI options. |
| crates/yuru-zh/src/pinyin.rs | Adds budget-enforced pinyin key generation and tests. |
| crates/yuru-zh/src/lib.rs | Wires KeyBudget max-bytes into pinyin key generation. |
| crates/yuru-tui/src/tests/render.rs | Adds regression tests for terminal control neutralization and SGR-only ANSI mode. |
| crates/yuru-tui/src/tests/preview.rs | Adds tests for bounded builtin/shell preview output and worker cancellation. |
| crates/yuru-tui/src/tests/image.rs | Adds SVG resource-loading regression test and worker cancellation/join coverage. |
| crates/yuru-tui/src/tests/highlight.rs | Adds a test ensuring highlighting work is bounded to visible prefix. |
| crates/yuru-tui/src/run.rs | Propagates ANSI mode through render context in interactive modes. |
| crates/yuru-tui/src/render/results.rs | Sanitizes pointer/marker, uses ANSI-aware highlighting, and counts visible width correctly with SGR. |
| crates/yuru-tui/src/render/mod.rs | Adjusts exports to keep tests compiling while hiding non-test APIs. |
| crates/yuru-tui/src/render/layout.rs | Adds terminal sanitization helpers (safe text/prefix, SGR parsing, visible-width extraction). |
| crates/yuru-tui/src/render/highlight.rs | Adds ANSI-aware highlighting pipeline with bounded work and safer handling of control sequences. |
| crates/yuru-tui/src/preview/process.rs | New bounded process runner with output caps, timeouts, cancellation, and process-group teardown. |
| crates/yuru-tui/src/preview/mod.rs | Introduces PreviewCancellation and reorganizes exports for tests/feature gates. |
| crates/yuru-tui/src/preview/image.rs | Adds bounded file reads and decoder limits; prevents SVG external/local href loading; adds cancellable encoding worker. |
| crates/yuru-tui/src/preview/command.rs | Routes shell/builtin previews through bounded/cancellable execution and adds truncation/timeout messaging. |
| crates/yuru-tui/src/preview/cache.rs | Adds cancellation + join for preview workers and image workers; ensures cleanup via Drop. |
| crates/yuru-tui/src/preview/builtin.rs | Removes unbounded reads/cat fallback; caps directory entries and text reads to 1 MiB with truncation notice. |
| crates/yuru-tui/src/api.rs | Extends TuiOptions with ansi flag (default false). |
| crates/yuru-tui/Cargo.toml | Adds unix-only libc dependency for process-group kill. |
| crates/yuru-ko/src/lib.rs | Enforces key-generation byte budget before expansion. |
| crates/yuru-ja/src/reading.rs | Adds budget-enforced reading expansion to prevent pre-budget cloning/allocations, plus regression test. |
| crates/yuru-ja/src/lib.rs | Wires KeyBudget max-bytes into reading expansion. |
| crates/yuru-core/src/config.rs | Extends KeyBudget to include max-total-bytes and exposes it via SearchConfig::key_budget(). |
| Cargo.lock | Records new libc dependency for yuru-tui on unix. |
Review details
- Files reviewed: 26/27 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Greptile SummaryThis PR hardens rendering and preview handling against oversized or untrusted input.
Confidence Score: 4/5The PR is not yet safe to merge because Korean key generation can still exceed the configured cumulative source-map budget. Although the reply reports that cumulative text and source-map budgeting was fixed, the current remaining-budget calculation subtracts only prior key text, while each generated representation allocates another source map; the regression test likewise checks source maps only individually, leaving the previously reported aggregate allocation path reachable. Files Needing Attention: crates/yuru-ko/src/hangul.rs
|
| Filename | Overview |
|---|---|
| crates/yuru-ko/src/hangul.rs | Adds incremental Korean key-generation limits, but aggregate source-map allocations remain outside the cumulative budget. |
| crates/yuru-tui/src/render/highlight.rs | Bounds inspected text, terms, and pattern lengths while correctly skipping over-limit terms rather than matching truncated prefixes. |
| crates/yuru-tui/src/preview/process.rs | Adds bounded subprocess output, timeout and cancellation handling, and Unix process-group termination. |
| crates/yuru-tui/src/preview/image.rs | Adds bounded image reads and decoding, dimension limits, and disabled SVG href resolution. |
| crates/yuru-tui/src/preview/cache.rs | Adds cancellable, joined preview and image worker lifetimes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Input[Untrusted candidate or preview input] --> Render[Sanitized bounded rendering]
Input --> Preview[Bounded preview worker]
Preview --> Process[Timed and output-limited process]
Preview --> File[Bounded text and image reads]
File --> Decode[Dimension and allocation-limited decoding]
Input --> Keys[Budgeted CJK key generation]
Reviews (3): Last reviewed commit: "Fix security audit and merge integration" | Re-trigger Greptile
Summary
Security coverage
Addresses all 12 findings from the codex-security scan: terminal injection in candidates, previews, and headers; superlinear highlighting; unbounded image, command, and text preview inputs; SVG local resource loading; stale worker/process lifetimes; and pre-budget CJK allocations.
Performance
Normal-input behavior remains asymptotically unchanged. Rendering sanitization and highlighting operate only on the visible prefix, and preview bookkeeping is constant-sized. Adversarial inputs now stop at fixed limits instead of scaling without bounds. Local Criterion runs were too host-variable for trustworthy percentage claims, including a large variance in the unchanged control benchmark, so no precise delta is asserted.
Validation