[codex] fix codex security scan findings#49
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ai/client.rs (1)
344-356: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTest doesn't guard against ambient
QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK.
clear_test_envonly clears API-key vars; it never unsetsQR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK. If that variable happens to be set in the shell/CI environment running the suite,fallback_can_receive_promptwould returntrue, the fallback server (which returns 200) would succeed, and.unwrap_err()at line 490 would panic instead of asserting the refusal message.🧪 Proposed fix to make the test hermetic
fn clear_test_env() { for key in [ "QR_TEST_AI_KEY", "CUSTOM_QR_TEST_AI_KEY", "CUSTOM_QR_TEST_ANTHROPIC_KEY", "OPENAI_API_KEY", "ANTHROPIC_API_KEY", + "QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK", ] { unsafe { std::env::remove_var(key); } } }Also applies to: 458-501
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ai/client.rs` around lines 344 - 356, The test setup in clear_test_env is not hermetic because it leaves QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK intact, which can change fallback_can_receive_prompt and make the fallback assertion in fallback_can_receive_prompt / the unwrap_err path fail unpredictably. Update clear_test_env to also remove QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK so the tests controlling cross-endpoint fallback behavior are isolated from ambient shell or CI environment state.
🧹 Nitpick comments (1)
src/terminal.rs (1)
7-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider also escaping Unicode bidi/format control characters, not just C0/C1.
ch.is_control()only matches Unicode general category Cc (C0/C1 controls). It does not match category Cf format characters such as RLO/LRO/PDF (U+202A–U+202E) or the isolate controls (U+2066–U+2069). These can reorder/hide the visual rendering of a string in many terminals without any ANSI escape sequence — a Trojan-Source-style spoof — which defeats the stated goal of this function for the exact untrusted sources it's applied to (AI-generated command previews, git/filesystem-derived project names).Since this utility is specifically meant to keep the preview "the user is meant to inspect" trustworthy, consider extending the filter to also escape Cf-category characters (or use a small allow-list of confirmed-safe scalar values instead of only excluding controls).
💡 Possible extension
pub fn escape_untrusted(input: &str) -> String { let mut output = String::with_capacity(input.len()); for ch in input.chars() { - if ch.is_control() { + if ch.is_control() || is_bidi_or_format_override(ch) { output.extend(ch.escape_default()); } else { output.push(ch); } } output } + +fn is_bidi_or_format_override(ch: char) -> bool { + matches!(ch, + '\u{200B}'..='\u{200F}' | '\u{202A}'..='\u{202E}' | '\u{2066}'..='\u{2069}' | '\u{FEFF}' + ) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/terminal.rs` around lines 7 - 17, escape_untrusted currently only filters ch.is_control(), so it misses Unicode format/bidi controls that can visually reorder terminal text. Update escape_untrusted in terminal.rs to also escape or reject Cf characters such as RLO/LRO/PDF and the isolate controls, using the existing escape_default path or a small safe allow-list. Keep the behavior centered on the escape_untrusted function so the preview remains trustworthy for untrusted command and project-name input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/ai/client.rs`:
- Around line 344-356: The test setup in clear_test_env is not hermetic because
it leaves QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK intact, which can change
fallback_can_receive_prompt and make the fallback assertion in
fallback_can_receive_prompt / the unwrap_err path fail unpredictably. Update
clear_test_env to also remove QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK so the tests
controlling cross-endpoint fallback behavior are isolated from ambient shell or
CI environment state.
---
Nitpick comments:
In `@src/terminal.rs`:
- Around line 7-17: escape_untrusted currently only filters ch.is_control(), so
it misses Unicode format/bidi controls that can visually reorder terminal text.
Update escape_untrusted in terminal.rs to also escape or reject Cf characters
such as RLO/LRO/PDF and the isolate controls, using the existing escape_default
path or a small safe allow-list. Keep the behavior centered on the
escape_untrusted function so the preview remains trustworthy for untrusted
command and project-name input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbe7fa18-daa2-4264-a46f-db69da335a11
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
AGENTS.mdCargo.tomlconfig/default.tomlsrc/ai/client.rssrc/commands/do_cmd.rssrc/commands/go.rssrc/lib.rssrc/main.rssrc/terminal.rs
💤 Files with no reviewable changes (1)
- config/default.toml
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Review NotesIncremental review since Previous Review Summary (commit 6233baa)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6233baa)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Review NotesReviewed the security-hardening changes: new Reviewed by glm-5.2-short · Input: 12.2K · Output: 3.1K · Cached: 181.4K |
1 participant
Summary
History scrub
.meowcode.jsonfrom history..meowcode.jsonpath/object hits.refs/pull/*updates during mirror push; those refs are not writable via Git push and may require GitHub-side cleanup if they still expose old objects.Validation
cargo testcargo fmt --all -- --checkcargo clippy --all-targets --locked -- -D warningscargo build --locked --verbosegit diff --checkSummary by CodeRabbit
New Features
Bug Fixes