refactor: consolidate output hygiene into output.rs#527
Conversation
Introduce src/output.rs that consolidates: - sanitize_for_terminal (upgraded: now also strips bidi overrides, zero-width chars, directional isolates, line/paragraph separators) - reject_dangerous_chars (renamed from reject_control_chars) - is_dangerous_unicode predicate - colorize + stderr_supports_color (NO_COLOR + TTY detection) - status/warn/info stderr helpers (auto-sanitize) Migrate existing callers via re-exports from error.rs and validate.rs. Fix watch.rs: - Sanitize raw API error body in eprintln (was high-risk injection vector) - Replace 3 inline ANSI escape codes with colorize() for NO_COLOR support Fix triage.rs: - Sanitize user --query string in no_messages_msg output
🦋 Changeset detectedLatest commit: db5713d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's terminal output handling by consolidating all sanitization, coloring, and stderr messaging into a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that consolidates terminal output-related functionality into a new output.rs module. The changes significantly improve code organization and security by introducing robust sanitization for terminal output, which helps prevent control sequence injection. The added Unicode character filtering is a valuable enhancement. The migration to use the new colorize function correctly respects the NO_COLOR environment variable. I've found one area for improvement in the new reject_dangerous_chars function to make it even more robust.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
+ Coverage 68.10% 68.92% +0.82%
==========================================
Files 40 42 +2
Lines 17954 18762 +808
==========================================
+ Hits 12227 12932 +705
- Misses 5727 5830 +103 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Update all 10 caller files to import sanitize_for_terminal directly from crate::output instead of going through crate::error re-exports. Remove pub(crate) re-exports from error.rs and validate.rs.
|
/gemini review |
Address PR review: the manual (c as u32) < 0x20 check missed C1 control characters (U+0080-U+009F), including CSI (U+009B) which can inject terminal escape sequences. Using char::is_control() covers both C0 and C1 ranges. Add test for CSI rejection.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that consolidates terminal output hygiene into a new output.rs module. The changes significantly improve security by introducing sanitization for user-provided query strings and API error responses, preventing potential terminal injection attacks. The move from raw ANSI escape codes to a centralized colorize function that respects NO_COLOR is also a welcome improvement for correctness and maintainability.
I've found one potential security issue in the new colorize function where the ansi_color parameter is not validated, which could lead to an escape sequence injection if used with untrusted input in the future. My review includes a suggestion to harden this function.
There was a problem hiding this comment.
Code Review
This is an excellent refactoring that significantly improves code organization and security by consolidating terminal output sanitization into a new output.rs module. The changes are well-executed, with the new module being comprehensive and well-tested. The security enhancements, such as sanitizing user input and API responses before printing and replacing hardcoded ANSI codes, are crucial improvements. The migration path is also well-designed, ensuring backward compatibility. I have reviewed the changes and found no issues.
Defense-in-depth: only emit ANSI escape codes when ansi_color contains exclusively ASCII digits. Falls back to plain text if an invalid color code is passed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a solid refactoring that consolidates terminal output hygiene into a new output.rs module, enhancing both code organization and security. The upgrades to sanitization to cover dangerous Unicode characters and the replacement of hardcoded ANSI codes to respect NO_COLOR environments are excellent improvements. I've identified one high-severity issue in the new is_dangerous_unicode function where its implementation is largely redundant with char::is_control(), leading to inefficiency and misleading documentation. My suggestion simplifies the function to improve clarity and performance. Overall, these are very positive changes.
Summary
Introduce
src/output.rsthat consolidates terminal sanitization, coloring, and stderr messaging into a single module.Changes
New
output.rsmodule:sanitize_for_terminal— upgraded to also strip dangerous Unicode (bidi overrides, zero-width chars, directional isolates, line/paragraph separators)reject_dangerous_chars— renamed fromreject_control_chars, moved fromvalidate.rsis_dangerous_unicode— shared predicatecolorize+stderr_supports_color— moved fromerror.rs(respectsNO_COLOR+ TTY detection)status/warn/info— auto-sanitizing stderr helpers for future useSecurity fixes:
watch.rs:103—triage.rs— user--querystring now sanitized before printingwatch.rs— 3 inline\x1b[...]ANSI codes replaced withcolorize()to respectNO_COLORMigration:
error.rsre-exportssanitize_for_terminal+colorizefromoutput.rs(no caller changes needed)validate.rsre-exportsreject_control_charsas alias forreject_dangerous_chars(backwards compatible)Test results
cargo clippy -- -D warnings✅