fixes execute allow commands

dingfeli · dingfeli · commit 4c97b9c990b4 · 2025-08-14T18:24:57.000-07:00
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -8,7 +8,7 @@ authors = ["Amazon Q CLI Team (q-cli@amazon.com)", "Chay Nabors (nabochay@amazon
 edition = "2024"
 homepage = "https://aws.amazon.com/q/"
 publish = false
-version = "1.13.3"
+version = "1.14.0"
 license = "MIT OR Apache-2.0"
 
 [workspace.dependencies]
diff --git a/crates/chat-cli/src/api_client/delay_interceptor.rs b/crates/chat-cli/src/api_client/delay_interceptor.rs
@@ -0,0 +1,92 @@
+use std::time::{
+    Duration,
+    Instant,
+};
+
+use aws_smithy_runtime_api::box_error::BoxError;
+use aws_smithy_runtime_api::client::interceptors::Intercept;
+use aws_smithy_runtime_api::client::interceptors::context::BeforeTransmitInterceptorContextRef;
+use aws_smithy_runtime_api::client::retries::RequestAttempts;
+use aws_smithy_runtime_api::client::runtime_components::RuntimeComponents;
+use aws_smithy_types::config_bag::{
+    ConfigBag,
+    Storable,
+    StoreReplace,
+};
+use crossterm::style::Color;
+use crossterm::{
+    execute,
+    style,
+};
+
+#[derive(Debug, Clone)]
+pub struct DelayTrackingInterceptor {
+    minor_delay_threshold: Duration,
+    major_delay_threshold: Duration,
+}
+
+impl DelayTrackingInterceptor {
+    pub fn new() -> Self {
+        Self {
+            minor_delay_threshold: Duration::from_secs(2),
+            major_delay_threshold: Duration::from_secs(5),
+        }
+    }
+
+    fn print_warning(message: String) {
+        let mut stderr = std::io::stderr();
+        let _ = execute!(
+            stderr,
+            style::SetForegroundColor(Color::Yellow),
+            style::Print("\nWARNING: "),
+            style::SetForegroundColor(Color::Reset),
+            style::Print(message),
+            style::Print("\n")
+        );
+    }
+}
+
+impl Intercept for DelayTrackingInterceptor {
+    fn name(&self) -> &'static str {
+        "DelayTrackingInterceptor"
+    }
+
+    fn read_before_transmit(
+        &self,
+        _: &BeforeTransmitInterceptorContextRef<'_>,
+        _: &RuntimeComponents,
+        cfg: &mut ConfigBag,
+    ) -> Result<(), BoxError> {
+        let attempt_number = cfg.load::<RequestAttempts>().map_or(1, |attempts| attempts.attempts());
+
+        let now = Instant::now();
+
+        if let Some(last_attempt_time) = cfg.load::<LastAttemptTime>() {
+            let delay = now.duration_since(last_attempt_time.0);
+
+            if delay >= self.major_delay_threshold {
+                Self::print_warning(format!(
+                    "Auto Retry #{} delayed by {:.1}s. Service is under heavy load - consider switching models.",
+                    attempt_number,
+                    delay.as_secs_f64()
+                ));
+            } else if delay >= self.minor_delay_threshold {
+                Self::print_warning(format!(
+                    "Auto Retry #{} delayed by {:.1}s due to transient issues.",
+                    attempt_number,
+                    delay.as_secs_f64()
+                ));
+            }
+        }
+
+        cfg.interceptor_state().store_put(LastAttemptTime(Instant::now()));
+        Ok(())
+    }
+}
+
+#[derive(Debug, Clone)]
+struct LastAttemptTime(Instant);
+
+impl Storable for LastAttemptTime {
+    type Storer = StoreReplace<Self>;
+}
diff --git a/crates/chat-cli/src/api_client/mod.rs b/crates/chat-cli/src/api_client/mod.rs
@@ -1,5 +1,6 @@
 mod credentials;
 pub mod customization;
+mod delay_interceptor;
 mod endpoints;
 mod error;
 pub mod model;
@@ -41,6 +42,7 @@ use tracing::{
 };
 
 use crate::api_client::credentials::CredentialsChain;
+use crate::api_client::delay_interceptor::DelayTrackingInterceptor;
 use crate::api_client::model::{
     ChatResponseStream,
     ConversationState,
@@ -163,6 +165,7 @@ impl ApiClient {
                     .http_client(crate::aws_common::http_client::client())
                     .interceptor(OptOutInterceptor::new(database))
                     .interceptor(UserAgentOverrideInterceptor::new())
+                    .interceptor(DelayTrackingInterceptor::new())
                     .app_name(app_name())
                     .endpoint_url(endpoint.url())
                     .retry_classifier(retry_classifier::QCliRetryClassifier::new())
@@ -176,6 +179,7 @@ impl ApiClient {
                         .http_client(crate::aws_common::http_client::client())
                         .interceptor(OptOutInterceptor::new(database))
                         .interceptor(UserAgentOverrideInterceptor::new())
+                        .interceptor(DelayTrackingInterceptor::new())
                         .bearer_token_resolver(BearerResolver)
                         .app_name(app_name())
                         .endpoint_url(endpoint.url())
diff --git a/crates/chat-cli/src/cli/chat/cli/model.rs b/crates/chat-cli/src/cli/chat/cli/model.rs
@@ -170,7 +170,7 @@ pub async fn get_available_models(os: &Os) -> Result<(Vec<ModelInfo>, ModelInfo)
         Err(e) => {
             tracing::error!("Failed to fetch models from API: {}, using fallback list", e);
 
-            let models = get_fallback_models();
+            let models = get_fallback_models(region);
             let default_model = models[0].clone();
 
             Ok((models, default_model))
@@ -188,17 +188,49 @@ fn default_context_window() -> usize {
     200_000
 }
 
-fn get_fallback_models() -> Vec<ModelInfo> {
-    vec![
-        ModelInfo {
-            model_name: Some("claude-3.7-sonnet".to_string()),
-            model_id: "claude-3.7-sonnet".to_string(),
-            context_window_tokens: 200_000,
-        },
-        ModelInfo {
-            model_name: Some("claude-4-sonnet".to_string()),
-            model_id: "claude-4-sonnet".to_string(),
-            context_window_tokens: 200_000,
-        },
-    ]
+fn get_fallback_models(region: &str) -> Vec<ModelInfo> {
+    match region {
+        "eu-central-1" => vec![
+            ModelInfo {
+                model_name: Some("claude-sonnet-4".to_string()),
+                model_id: "claude-sonnet-4".to_string(),
+                context_window_tokens: 200_000,
+            },
+            ModelInfo {
+                model_name: Some("claude-3.5-sonnet-v1".to_string()),
+                model_id: "claude-3.5-sonnet-v1".to_string(),
+                context_window_tokens: 200_000,
+            },
+        ],
+        _ => vec![
+            ModelInfo {
+                model_name: Some("claude-sonnet-4".to_string()),
+                model_id: "claude-sonnet-4".to_string(),
+                context_window_tokens: 200_000,
+            },
+            ModelInfo {
+                model_name: Some("claude-3.7-sonnet".to_string()),
+                model_id: "claude-3.7-sonnet".to_string(),
+                context_window_tokens: 200_000,
+            },
+        ],
+    }
+}
+
+pub fn normalize_model_name(name: &str) -> &str {
+    match name {
+        "claude-4-sonnet" => "claude-sonnet-4",
+        // can add more mapping for backward compatibility
+        _ => name,
+    }
+}
+
+pub fn find_model<'a>(models: &'a [ModelInfo], name: &str) -> Option<&'a ModelInfo> {
+    let normalized = normalize_model_name(name);
+    models.iter().find(|m| {
+        m.model_name
+            .as_deref()
+            .is_some_and(|n| n.eq_ignore_ascii_case(normalized))
+            || m.model_id.eq_ignore_ascii_case(normalized)
+    })
 }
diff --git a/crates/chat-cli/src/cli/chat/mod.rs b/crates/chat-cli/src/cli/chat/mod.rs
@@ -136,6 +136,7 @@ use crate::auth::AuthError;
 use crate::auth::builder_id::is_idc_user;
 use crate::cli::agent::Agents;
 use crate::cli::chat::cli::SlashCommand;
+use crate::cli::chat::cli::model::find_model;
 use crate::cli::chat::cli::prompts::{
     GetPromptError,
     PromptsSubcommand,
@@ -318,13 +319,7 @@ impl ChatArgs {
         // Otherwise, CLI will use a default model when starting chat
         let (models, default_model_opt) = get_available_models(os).await?;
         let model_id: Option<String> = if let Some(requested) = self.model.as_ref() {
-            let requested_lower = requested.to_lowercase();
-            if let Some(m) = models.iter().find(|m| {
-                m.model_name
-                    .as_deref()
-                    .is_some_and(|n| n.eq_ignore_ascii_case(&requested_lower))
-                    || m.model_id.eq_ignore_ascii_case(&requested_lower)
-            }) {
+            if let Some(m) = find_model(&models, requested) {
                 Some(m.model_id.clone())
             } else {
                 let available = models
@@ -335,14 +330,9 @@ impl ChatArgs {
                 bail!("Model '{}' does not exist. Available models: {}", requested, available);
             }
         } else if let Some(saved) = os.database.settings.get_string(Setting::ChatDefaultModel) {
-            if let Some(m) = models.iter().find(|m| {
-                m.model_name.as_deref().is_some_and(|n| n.eq_ignore_ascii_case(&saved))
-                    || m.model_id.eq_ignore_ascii_case(&saved)
-            }) {
-                Some(m.model_id.clone())
-            } else {
-                Some(default_model_opt.model_id.clone())
-            }
+            find_model(&models, &saved)
+                .map(|m| m.model_id.clone())
+                .or(Some(default_model_opt.model_id.clone()))
         } else {
             Some(default_model_opt.model_id.clone())
         };
diff --git a/crates/chat-cli/src/cli/chat/tools/execute/mod.rs b/crates/chat-cli/src/cli/chat/tools/execute/mod.rs
@@ -48,6 +48,11 @@ pub struct ExecuteCommand {
 
 impl ExecuteCommand {
     pub fn requires_acceptance(&self, allowed_commands: Option<&Vec<String>>, allow_read_only: bool) -> bool {
+        // Always require acceptance for multi-line commands.
+        if self.command.contains("\n") || self.command.contains("\r") {
+            return true;
+        }
+
         let default_arr = vec![];
         let allowed_commands = allowed_commands.unwrap_or(&default_arr);
 
@@ -64,7 +69,7 @@ impl ExecuteCommand {
         let Some(args) = shlex::split(&self.command) else {
             return true;
         };
-        const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";"];
+        const DANGEROUS_PATTERNS: &[&str] = &["<(", "$(", "`", ">", "&&", "||", "&", ";", "${", "\n", "\r", "IFS"];
 
         if args
             .iter()
@@ -106,6 +111,7 @@ impl ExecuteCommand {
                             arg.contains("-exec") // includes -execdir
                                 || arg.contains("-delete")
                                 || arg.contains("-ok") // includes -okdir
+                                || arg.contains("-fprint") // includes -fprint0 and -fprintf
                         }) =>
                 {
                     return true;
@@ -114,7 +120,11 @@ impl ExecuteCommand {
                     // Special casing for `grep`. -P flag for perl regexp has RCE issues, apparently
                     // should not be supported within grep but is flagged as a possibility since this is perl
                     // regexp.
-                    if cmd == "grep" && cmd_args.iter().any(|arg| arg.contains("-P")) {
+                    if cmd == "grep"
+                        && cmd_args
+                            .iter()
+                            .any(|arg| arg.contains("-P") || arg.contains("--perl-regexp"))
+                    {
                         return true;
                     }
                     let is_cmd_read_only = READONLY_COMMANDS.contains(&cmd.as_str());
@@ -226,7 +236,9 @@ impl ExecuteCommand {
                     return PermissionEvalResult::Deny(denied_match_set);
                 }
 
-                if !is_in_allowlist || self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
+                if is_in_allowlist {
+                    PermissionEvalResult::Allow
+                } else if self.requires_acceptance(Some(&allowed_commands), allow_read_only) {
                     PermissionEvalResult::Ask
                 } else {
                     PermissionEvalResult::Allow
@@ -293,6 +305,14 @@ mod tests {
             ("cat <<< 'some string here' > myimportantfile", true),
             ("echo '\n#!/usr/bin/env bash\necho hello\n' > myscript.sh", true),
             ("cat <<EOF > myimportantfile\nhello world\nEOF", true),
+            // newline checks
+            ("which ls\ntouch asdf", true),
+            ("which ls\rtouch asdf", true),
+            // $IFS check
+            (
+                r#"IFS=';'; for cmd in "which ls;touch asdf"; do eval "$cmd"; done"#,
+                true,
+            ),
             // Safe piped commands
             ("find . -name '*.rs' | grep main", false),
             ("ls -la | grep .git", false),
@@ -310,8 +330,12 @@ mod tests {
                 true,
             ),
             ("find important-dir/ -name '*.txt'", false),
+            (r#"find / -fprintf "/path/to/file" <data-to-write> -quit"#, true),
+            (r"find . -${t}exec touch asdf \{\} +", true),
+            (r"find . -${t:=exec} touch asdf2 \{\} +", true),
             // `grep` command arguments
             ("echo 'test data' | grep -P '(?{system(\"date\")})'", true),
+            ("echo 'test data' | grep --perl-regexp '(?{system(\"date\")})'", true),
         ];
         for (cmd, expected) in cmds {
             let tool = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
@@ -412,6 +436,7 @@ mod tests {
                 map.insert(
                     ToolSettingTarget(tool_name.to_string()),
                     serde_json::json!({
+                        "allowedCommands": ["allow_wild_card .*", "allow_exact"],
                         "deniedCommands": ["git .*"]
                     }),
                 );
@@ -429,13 +454,34 @@ mod tests {
         assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
 
         let tool_two = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
-            "command": "echo hello",
+            "command": "this_is_not_a_read_only_command",
         }))
         .unwrap();
 
         let res = tool_two.eval_perm(&agent);
         assert!(matches!(res, PermissionEvalResult::Ask));
 
+        let tool_allow_wild_card = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_wild_card some_arg",
+        }))
+        .unwrap();
+        let res = tool_allow_wild_card.eval_perm(&agent);
+        assert!(matches!(res, PermissionEvalResult::Allow));
+
+        let tool_allow_exact_should_ask = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_exact some_arg",
+        }))
+        .unwrap();
+        let res = tool_allow_exact_should_ask.eval_perm(&agent);
+        assert!(matches!(res, PermissionEvalResult::Ask));
+
+        let tool_allow_exact_should_allow = serde_json::from_value::<ExecuteCommand>(serde_json::json!({
+            "command": "allow_exact",
+        }))
+        .unwrap();
+        let res = tool_allow_exact_should_allow.eval_perm(&agent);
+        assert!(matches!(res, PermissionEvalResult::Allow));
+
         agent.allowed_tools.insert(tool_name.to_string());
 
         let res = tool_two.eval_perm(&agent);
@@ -444,9 +490,6 @@ mod tests {
         // Denied list should remain denied
         let res = tool_one.eval_perm(&agent);
         assert!(matches!(res, PermissionEvalResult::Deny(ref rules) if rules.contains(&"\\Agit .*\\z".to_string())));
-
-        let res = tool_two.eval_perm(&agent);
-        assert!(matches!(res, PermissionEvalResult::Allow));
     }
 
     #[tokio::test]
