From 6233baa425fcba434e8998ad30148e7179847731 Mon Sep 17 00:00:00 2001 From: Aanish Bhirud Date: Thu, 2 Jul 2026 14:32:44 +0530 Subject: [PATCH 1/2] fix codex security scan findings --- AGENTS.md | 9 +++--- Cargo.lock | 31 +++++++++++++++++++ Cargo.toml | 1 + config/default.toml | 7 ----- src/ai/client.rs | 70 ++++++++++++++++++++++++++++++++++++++++++ src/commands/do_cmd.rs | 50 ++++++++++++++++++++++++------ src/commands/go.rs | 61 ++++++++++++++++++++++++++++++------ src/lib.rs | 1 + src/main.rs | 69 +++++++++++++++++++++++++++++++++++++++-- src/terminal.rs | 27 ++++++++++++++++ 10 files changed, 292 insertions(+), 34 deletions(-) create mode 100644 src/terminal.rs diff --git a/AGENTS.md b/AGENTS.md index faf4fc6..9af52f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -42,11 +42,10 @@ Working notes for agents and contributors on QuickRunner (`qr`). `OPENAI_BASE_URL` mapping — map those yourself). `qr do` prints the AI-suggested command and only runs it after you type `y` (default is No), so it is safe to pipe `n` for a non-executing smoke test. None of this is required for build, lint, tests, or scan/go/stats. -- **Gotcha — AI fallback provider:** the default config defines an `[ai.fallback]` pointing at - `https://api.openai.com/v1`. If you set `QR_AI_BASE_URL` to a proxy/gateway (whose key is not a - real OpenAI key) and the primary call has a transient failure/timeout, `qr` falls back to - api.openai.com and fails with a confusing `401 Incorrect API key`. When using a custom endpoint, - also set `QR_AI_FALLBACK_BASE_URL` (and `QR_AI_FALLBACK_MODEL`) to the same endpoint. +- **Gotcha — AI fallback provider:** the default config does not define an `[ai.fallback]`. If you + configure one manually, `qr` refuses to retry prompt context against a different protocol/base URL + unless `QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK=true` is set. When using a proxy/gateway and you want + automatic fallback, keep the fallback endpoint on the same base URL or opt in explicitly. - **Gotcha — secrets in the Desktop terminal:** injected secret env vars are present in the agent shell but a freshly opened Desktop GUI terminal may not inherit them. To drive an AI demo from the Desktop, write the needed vars to a temp file the agent shell can produce and `source` it in the diff --git a/Cargo.lock b/Cargo.lock index 8f9902f..d5a706c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1322,6 +1322,7 @@ dependencies = [ "predicates", "proptest", "reqwest", + "rpassword", "rusqlite", "serde", "serde_json", @@ -1569,6 +1570,27 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rpassword" +version = "7.5.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2da316a15f47e3d053de9cb2c439650bd8fa4aaeb9365f2e5f27f492ff73c196" +dependencies = [ + "libc", + "rtoolbox", + "windows-sys 0.61.2", +] + +[[package]] +name = "rtoolbox" +version = "0.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "50a0e551c1e27e1731aba276dbeaeac73f53c7cd34d1bda485d02bd1e0f36844" +dependencies = [ + "libc", + "windows-sys 0.59.0", +] + [[package]] name = "rusqlite" version = "0.37.0" @@ -2418,6 +2440,15 @@ dependencies = [ "windows-targets 0.52.6", ] +[[package]] +name = "windows-sys" +version = "0.59.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e38bc4d79ed67fd075bcc251a1c39b32a1776bbe92e5bef1f0bf1f8c531853b" +dependencies = [ + "windows-targets 0.52.6", +] + [[package]] name = "windows-sys" version = "0.60.2" diff --git a/Cargo.toml b/Cargo.toml index 25344a2..9e97f0c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ dirs = "6.0.0" fuzzy-matcher = "0.3.7" indicatif = "0.18.3" reqwest = { version = "0.12.24", default-features = false, features = ["blocking", "json", "rustls-tls"] } +rpassword = "7.4.0" rusqlite = { version = "0.37.0", features = ["bundled"] } serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.145" diff --git a/config/default.toml b/config/default.toml index e8a39de..9997331 100644 --- a/config/default.toml +++ b/config/default.toml @@ -13,13 +13,6 @@ model = "" api_key = "" api_key_env = "OPENAI_API_KEY" -[ai.fallback] -protocol = "openai" -base_url = "https://api.openai.com/v1" -model = "" -api_key = "" -api_key_env = "OPENAI_API_KEY" - [stats] enabled = false db_path = "__default__" diff --git a/src/ai/client.rs b/src/ai/client.rs index e967deb..ec44e68 100644 --- a/src/ai/client.rs +++ b/src/ai/client.rs @@ -10,6 +10,7 @@ use crate::ai::providers::{AiProtocol, ProviderConfig}; /// (`max_tokens` is 1024), so 8 MiB is generous while still bounding memory /// against a malicious or buggy provider streaming an unbounded body. const MAX_RESPONSE_BYTES: u64 = 8 * 1024 * 1024; +const ALLOW_CROSS_ENDPOINT_FALLBACK_ENV: &str = "QR_AI_ALLOW_CROSS_ENDPOINT_FALLBACK"; #[derive(Debug, Clone)] pub struct AiClient { @@ -61,6 +62,11 @@ impl AiClient { Ok(response) => Ok(response), Err(primary_error) => { if let Some(fallback) = &self.fallback { + if !fallback_can_receive_prompt(&self.primary, fallback) { + return Err(anyhow!( + "Primary provider failed: {primary_error}. Refusing to send prompt to fallback provider at a different endpoint; set {ALLOW_CROSS_ENDPOINT_FALLBACK_ENV}=true to allow cross-endpoint fallback." + )); + } self.send_prompt(fallback, "fallback", system_prompt, user_message) .map_err(|fallback_error| { anyhow!( @@ -127,6 +133,25 @@ impl AiClient { } } +fn fallback_can_receive_prompt(primary: &ProviderConfig, fallback: &ProviderConfig) -> bool { + same_ai_endpoint(primary, fallback) || env_flag_enabled(ALLOW_CROSS_ENDPOINT_FALLBACK_ENV) +} + +fn same_ai_endpoint(primary: &ProviderConfig, fallback: &ProviderConfig) -> bool { + primary.protocol == fallback.protocol && endpoint_url(primary) == endpoint_url(fallback) +} + +fn env_flag_enabled(key: &str) -> bool { + env::var(key) + .map(|value| { + matches!( + value.trim().to_ascii_lowercase().as_str(), + "1" | "true" | "yes" + ) + }) + .unwrap_or(false) +} + fn resolve_api_key(provider: &ProviderConfig, keychain_role: &str) -> Result { // 1. explicit per-provider env var (highest-precedence override) if !provider.api_key_env.trim().is_empty() { @@ -430,6 +455,51 @@ mod tests { } } + #[test] + fn cross_endpoint_fallback_requires_explicit_opt_in() { + let _guard = test_env_lock().lock().unwrap(); + clear_test_env(); + let primary = spawn_server(500, r#"{"error":{"message":"primary down"}}"#); + let fallback = spawn_server( + 200, + r#"{"choices":[{"message":{"content":"cargo test"}}],"usage":{"prompt_tokens":1,"completion_tokens":1}}"#, + ); + unsafe { + std::env::set_var("QR_TEST_AI_KEY", "token"); + } + + let client = AiClient::new( + ProviderConfig { + protocol: AiProtocol::OpenAi, + base_url: primary, + model: "demo".into(), + api_key: String::new(), + api_key_env: "QR_TEST_AI_KEY".into(), + }, + Some(ProviderConfig { + protocol: AiProtocol::OpenAi, + base_url: fallback, + model: "demo".into(), + api_key: String::new(), + api_key_env: "QR_TEST_AI_KEY".into(), + }), + ); + + let error = client + .execute_prompt("You classify tasks", "run tests") + .unwrap_err(); + assert!( + error + .to_string() + .contains("Refusing to send prompt to fallback provider at a different endpoint"), + "{error:#}" + ); + + unsafe { + std::env::remove_var("QR_TEST_AI_KEY"); + } + } + #[test] fn custom_env_var_overrides_well_known_and_config_key() { let _guard = test_env_lock().lock().unwrap(); diff --git a/src/commands/do_cmd.rs b/src/commands/do_cmd.rs index 98a2999..8d1a65e 100644 --- a/src/commands/do_cmd.rs +++ b/src/commands/do_cmd.rs @@ -11,6 +11,7 @@ use crate::{ ai::client::{AiClient, AiResponse}, config::{AgentConfig, AppConfig}, project_profile::{ProjectProfile, discover_project_root, load_profile_from}, + terminal, }; const CLASSIFICATION_PROMPT: &str = r#"You are QuickRunner's router. @@ -114,13 +115,20 @@ fn load_current_profile() -> Result { } fn print_inline_preview(command: &str, reason: Option<&str>) -> Result<()> { - println!("→ {command}"); - if let Some(reason) = reason.filter(|value| !value.trim().is_empty()) { - println!(" {reason}"); + for line in inline_preview_lines(command, reason) { + println!("{line}"); } Ok(()) } +fn inline_preview_lines(command: &str, reason: Option<&str>) -> Vec { + let mut lines = vec![format!("→ {}", terminal::escape_untrusted(command))]; + if let Some(reason) = reason.filter(|value| !value.trim().is_empty()) { + lines.push(format!(" {}", terminal::escape_untrusted(reason))); + } + lines +} + /// Confirm and run an inline command. Every AI-generated command gets the same /// single confirmation: `confirm` defaults to No and the full command is shown in /// the preview first, so a command like `git status; rm -rf ~` cannot run unless @@ -177,16 +185,25 @@ fn build_delegate_suggestions( } fn print_delegate_suggestions(suggestions: &[String], reason: Option<&str>) -> Result<()> { - println!("🧠 This looks like a multi-step coding task."); - if let Some(reason) = reason.filter(|value| !value.trim().is_empty()) { - println!(" {reason}"); - } - for suggestion in suggestions { - println!("→ Suggested: {suggestion}"); + for line in delegate_suggestion_lines(suggestions, reason) { + println!("{line}"); } Ok(()) } +fn delegate_suggestion_lines(suggestions: &[String], reason: Option<&str>) -> Vec { + let mut lines = vec!["🧠 This looks like a multi-step coding task.".to_string()]; + if let Some(reason) = reason.filter(|value| !value.trim().is_empty()) { + lines.push(format!(" {}", terminal::escape_untrusted(reason))); + } + lines.extend( + suggestions + .iter() + .map(|suggestion| format!("→ Suggested: {}", terminal::escape_untrusted(suggestion))), + ); + lines +} + #[cfg(test)] mod tests { use super::*; @@ -224,4 +241,19 @@ mod tests { build_delegate_suggestions(&AgentConfig::default(), "refactor auth", Some(&profile)); assert!(suggestions[0].starts_with("codex ")); } + + #[test] + fn inline_preview_lines_escape_terminal_controls() { + let lines = inline_preview_lines("cargo test\u{1b}[2J", Some("\u{1b}[31mred")); + assert_eq!(lines[0], "→ cargo test\\u{1b}[2J"); + assert_eq!(lines[1], " \\u{1b}[31mred"); + } + + #[test] + fn delegate_suggestion_lines_escape_terminal_controls() { + let suggestions = vec!["codex exec \u{1b}[2J".to_string()]; + let lines = delegate_suggestion_lines(&suggestions, Some("\u{1b}[31magent")); + assert_eq!(lines[1], " \\u{1b}[31magent"); + assert_eq!(lines[2], "→ Suggested: codex exec \\u{1b}[2J"); + } } diff --git a/src/commands/go.rs b/src/commands/go.rs index 61f0a8b..b18abad 100644 --- a/src/commands/go.rs +++ b/src/commands/go.rs @@ -3,7 +3,9 @@ use std::io::IsTerminal; use anyhow::{Result, anyhow}; use fuzzy_matcher::{FuzzyMatcher, skim::SkimMatcherV2}; -use crate::{config::AppConfig, picker, scanner::ProjectEntry, scanner::load_or_scan_projects}; +use crate::{ + config::AppConfig, picker, scanner::ProjectEntry, scanner::load_or_scan_projects, terminal, +}; pub struct GoResult { pub path: String, @@ -25,10 +27,7 @@ pub fn execute(config: &AppConfig, query: &str) -> Result { let selected = if matches.len() == 1 { matches[0].clone() } else if std::io::stdin().is_terminal() && std::io::stderr().is_terminal() { - let labels = matches - .iter() - .map(|entry| format!("{} ({})", entry.name, entry.path)) - .collect::>(); + let labels = matches.iter().map(project_choice_label).collect::>(); let picker_start = std::time::Instant::now(); let Some(index) = picker::pick_index(&labels)? else { return Err(anyhow!("Selection cancelled")); @@ -38,11 +37,7 @@ pub fn execute(config: &AppConfig, query: &str) -> Result { } else { return Err(anyhow!( "Multiple matches for '{query}': {}", - matches - .iter() - .map(|entry| entry.name.as_str()) - .collect::>() - .join(", ") + multiple_match_names(&matches) )); }; @@ -52,6 +47,22 @@ pub fn execute(config: &AppConfig, query: &str) -> Result { }) } +fn project_choice_label(entry: &ProjectEntry) -> String { + format!( + "{} ({})", + terminal::escape_untrusted(&entry.name), + terminal::escape_untrusted(&entry.path) + ) +} + +fn multiple_match_names(entries: &[ProjectEntry]) -> String { + entries + .iter() + .map(|entry| terminal::escape_untrusted(&entry.name)) + .collect::>() + .join(", ") +} + pub fn rank_matches(entries: &[ProjectEntry], query: &str) -> Vec { let lower_query = query.to_ascii_lowercase(); let matcher = SkimMatcherV2::default(); @@ -187,4 +198,34 @@ mod tests { assert_eq!(matches.len(), 1); assert_eq!(matches[0].name, "orion-app"); } + + #[test] + fn project_choice_label_escapes_terminal_controls() { + let entry = ProjectEntry { + name: "bad\u{1b}[2J".into(), + path: "/tmp/bad\u{7}".into(), + source: "git".into(), + }; + assert_eq!( + project_choice_label(&entry), + "bad\\u{1b}[2J (/tmp/bad\\u{7})" + ); + } + + #[test] + fn multiple_match_names_escape_terminal_controls() { + let entries = vec![ + ProjectEntry { + name: "one\u{1b}[2J".into(), + path: "/tmp/one".into(), + source: "git".into(), + }, + ProjectEntry { + name: "two\u{7}".into(), + path: "/tmp/two".into(), + source: "git".into(), + }, + ]; + assert_eq!(multiple_match_names(&entries), "one\\u{1b}[2J, two\\u{7}"); + } } diff --git a/src/lib.rs b/src/lib.rs index a16a414..05ad3e7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -9,6 +9,7 @@ pub mod scanner; pub mod secret; pub mod shell; pub mod stats_db; +pub mod terminal; /// A single process-wide lock for unit tests that mutate environment variables. /// One shared lock (rather than a per-module copy) is required because env vars diff --git a/src/main.rs b/src/main.rs index 1a9df50..9c92a07 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,6 +1,6 @@ use std::{ env, - io::{self, Write}, + io::{self, IsTerminal, Write}, process::{Command, ExitCode}, time::Instant, }; @@ -18,6 +18,7 @@ use quick_runner::{ }, pricing, scanner, secret, shell, stats_db::{CommandStats, StatsDb}, + terminal, }; #[derive(Parser)] @@ -379,7 +380,7 @@ fn prompt_provider_fields(label: &str) -> Result<(AiProtocol, String, String, St let protocol = prompt_protocol(label)?; let base_url = prompt_with_default(&format!("{label} base URL"), protocol.default_base_url())?; let model = prompt_required(&format!("{label} model name"))?; - let api_key = prompt_required(&format!("{label} API key"))?; + let api_key = prompt_required_for_provider_field(&format!("{label} API key"))?; let api_key_env = prompt_with_default( &format!("{label} API key env var override"), protocol.default_api_key_env(), @@ -387,6 +388,27 @@ fn prompt_provider_fields(label: &str) -> Result<(AiProtocol, String, String, St Ok((protocol, base_url, model, api_key, api_key_env)) } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum PromptSensitivity { + Plain, + Secret, +} + +fn provider_field_prompt_sensitivity(label: &str) -> PromptSensitivity { + if label.trim_end().ends_with("API key") { + PromptSensitivity::Secret + } else { + PromptSensitivity::Plain + } +} + +fn prompt_required_for_provider_field(label: &str) -> Result { + match provider_field_prompt_sensitivity(label) { + PromptSensitivity::Plain => prompt_required(label), + PromptSensitivity::Secret => prompt_secret_required(label), + } +} + fn prompt_ai_config(label: &str) -> Result { let (protocol, base_url, model, api_key, api_key_env) = prompt_provider_fields(label)?; let api_key = maybe_store_key_in_keychain("primary", protocol, &api_key_env, api_key)?; @@ -520,6 +542,18 @@ fn prompt_required(label: &str) -> Result { } } +fn prompt_secret_required(label: &str) -> Result { + loop { + match prompt_secret(label)? { + Some(value) if !value.trim().is_empty() => return Ok(value), + None => { + anyhow::bail!("Unexpected end of input while reading '{label}' (is stdin closed?)") + } + Some(_) => println!("{label} is required."), + } + } +} + fn prompt_with_default(label: &str, default: &str) -> Result { // A blank line or EOF both fall back to the default. let value = prompt(&format!("{label} [{default}]"))?.unwrap_or_default(); @@ -559,6 +593,14 @@ fn prompt(label: &str) -> Result> { Ok(Some(line.trim().to_string())) } +fn prompt_secret(label: &str) -> Result> { + if !io::stdin().is_terminal() { + return prompt(label); + } + let value = rpassword::prompt_password(format!("{label}: "))?; + Ok(Some(value.trim().to_string())) +} + fn install_cron() -> Result<()> { let exe = env::current_exe().context("Could not resolve qr binary path")?; let cron_line = shell::cron_line(&exe); @@ -603,7 +645,7 @@ fn print_go_result(result: &GoResult, print_path: bool) -> Result<()> { if print_path { println!("{}", result.path); } else { - println!("→ cd {}", result.path); + println!("→ cd {}", terminal::escape_untrusted(&result.path)); } Ok(()) } @@ -651,3 +693,24 @@ fn command_name(command: &Commands) -> &'static str { Commands::Do { .. } => "do", } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn api_key_provider_field_uses_secret_prompting() { + assert_eq!( + provider_field_prompt_sensitivity("primary API key"), + PromptSensitivity::Secret + ); + assert_eq!( + provider_field_prompt_sensitivity("fallback API key"), + PromptSensitivity::Secret + ); + assert_eq!( + provider_field_prompt_sensitivity("primary model name"), + PromptSensitivity::Plain + ); + } +} diff --git a/src/terminal.rs b/src/terminal.rs new file mode 100644 index 0000000..e1cd073 --- /dev/null +++ b/src/terminal.rs @@ -0,0 +1,27 @@ +/// Render untrusted text visibly before writing it to an interactive terminal. +/// +/// Filesystem metadata, Git remote names, and AI responses can contain C0/C1 +/// control bytes. Escaping controls keeps those bytes from moving the cursor, +/// clearing the screen, recoloring text, or otherwise changing the preview that +/// the user is meant to inspect. +pub fn escape_untrusted(input: &str) -> String { + let mut output = String::with_capacity(input.len()); + for ch in input.chars() { + if ch.is_control() { + output.extend(ch.escape_default()); + } else { + output.push(ch); + } + } + output +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn escapes_control_chars_and_preserves_plain_text() { + assert_eq!(escape_untrusted("ok\u{1b}[2J\nnext"), "ok\\u{1b}[2J\\nnext"); + } +} From 61bec3ac7e7fade69fbab1218a6ece30131928a5 Mon Sep 17 00:00:00 2001 From: Aanish Bhirud Date: Thu, 2 Jul 2026 15:31:21 +0530 Subject: [PATCH 2/2] address coderabbit nitpicks --- src/ai/client.rs | 12 ++++++++++++ src/terminal.rs | 42 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/ai/client.rs b/src/ai/client.rs index ec44e68..0dbe1d0 100644 --- a/src/ai/client.rs +++ b/src/ai/client.rs @@ -348,6 +348,7 @@ mod tests { "CUSTOM_QR_TEST_ANTHROPIC_KEY", "OPENAI_API_KEY", "ANTHROPIC_API_KEY", + ALLOW_CROSS_ENDPOINT_FALLBACK_ENV, ] { unsafe { std::env::remove_var(key); @@ -455,6 +456,17 @@ mod tests { } } + #[test] + fn clear_test_env_removes_cross_endpoint_fallback_opt_in() { + unsafe { + std::env::set_var(ALLOW_CROSS_ENDPOINT_FALLBACK_ENV, "true"); + } + + clear_test_env(); + + assert!(std::env::var(ALLOW_CROSS_ENDPOINT_FALLBACK_ENV).is_err()); + } + #[test] fn cross_endpoint_fallback_requires_explicit_opt_in() { let _guard = test_env_lock().lock().unwrap(); diff --git a/src/terminal.rs b/src/terminal.rs index e1cd073..96b266f 100644 --- a/src/terminal.rs +++ b/src/terminal.rs @@ -1,13 +1,13 @@ /// Render untrusted text visibly before writing it to an interactive terminal. /// /// Filesystem metadata, Git remote names, and AI responses can contain C0/C1 -/// control bytes. Escaping controls keeps those bytes from moving the cursor, -/// clearing the screen, recoloring text, or otherwise changing the preview that -/// the user is meant to inspect. +/// control bytes or Unicode format controls. Escaping them keeps those bytes +/// from moving the cursor, clearing the screen, recoloring or reordering text, +/// or otherwise changing the preview that the user is meant to inspect. 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_unicode_format_control(ch) { output.extend(ch.escape_default()); } else { output.push(ch); @@ -16,6 +16,32 @@ pub fn escape_untrusted(input: &str) -> String { output } +fn is_unicode_format_control(ch: char) -> bool { + matches!( + ch, + '\u{00ad}' + | '\u{0600}'..='\u{0605}' + | '\u{061c}' + | '\u{06dd}' + | '\u{070f}' + | '\u{0890}'..='\u{0891}' + | '\u{08e2}' + | '\u{180e}' + | '\u{200b}'..='\u{200f}' + | '\u{202a}'..='\u{202e}' + | '\u{2060}'..='\u{206f}' + | '\u{feff}' + | '\u{fff9}'..='\u{fffb}' + | '\u{110bd}' + | '\u{110cd}' + | '\u{13430}'..='\u{1343f}' + | '\u{1bca0}'..='\u{1bca3}' + | '\u{1d173}'..='\u{1d17a}' + | '\u{e0001}' + | '\u{e0020}'..='\u{e007f}' + ) +} + #[cfg(test)] mod tests { use super::*; @@ -24,4 +50,12 @@ mod tests { fn escapes_control_chars_and_preserves_plain_text() { assert_eq!(escape_untrusted("ok\u{1b}[2J\nnext"), "ok\\u{1b}[2J\\nnext"); } + + #[test] + fn escapes_bidi_and_format_controls() { + assert_eq!( + escape_untrusted("safe\u{202e}txt\u{2066}"), + "safe\\u{202e}txt\\u{2066}" + ); + } }