Harden bash boundary against quote, env-var, glob, and whitespace bypasses

Author: alexylon

AGENTS.md

@@ -444,6 +444,17 @@ Gitignored scratchpad for helper files the user asks to be created there — typ
     - `cargo fmt --all`
     - `cargo clippy --all-targets -- -D warnings`
     - `cargo test --all`
+    - Windows cross-check (only when the change touches a `cfg(windows)` /
+      `cfg(target_os = "windows")` / `cfg(not(unix))` arm — Git-Bash shell
+      resolution in `src/tools/bash/executor.rs`, the console / code-page /
+      `CONOUT$` handling in `src/repl/tui/mod.rs`, Windows clipboard in
+      `src/clipboard.rs`, `USERPROFILE` lookup in
+      `src/tools/permissions/manager.rs`, the case-insensitive command
+      lookup in `src/tools/permissions/command_parse.rs`, and similar).
+      Requires `rustup target add x86_64-pc-windows-gnu`, which is already
+      installed on this machine.
+        - `cargo check --package sofos --target x86_64-pc-windows-gnu`
+        - `cargo clippy --package sofos --target x86_64-pc-windows-gnu --all-targets`
 - Before finishing, review the change for bugs and corner cases.
 - Use international English. Avoid regional idioms (whether American or British), clever shorthand, and compressed phrases; prefer wording that a non-native English reader can understand on the first read. This applies to chat replies, commit messages, code comments, documentation, and error messages.
 - After you finish cross-checking against the Non-Negotiable Rules and fixing the code, if needed, do another pass for bugs and regressions.

CHANGELOG.md

@@ -12,6 +12,14 @@ All notable changes to Sofos are documented in this file.
 
 - **Installing on Windows no longer requires CMake or NASM.** The HTTP and syntax-highlighting backends now use pure-Rust crypto and regex, so `cargo install sofos` succeeds on a clean `rustup` install with no extra build tools.
 
+### Security
+
+- **Forbidden bash commands stay forbidden under blanket `Bash` allow even when the model wraps them.** Adding `Bash` to the allow list used to leave `(rm -rf /)`, `'rm' -rf /`, `\rm -rf /`, and `/bin/rm -rf /` running because the lookup compared the literal token; sofos now strips subshell, quote, backslash, and directory wrappers before checking, and lower-cases the name on Windows. The same fix applies to compound shells, so `ls && (rm bar)` is denied just like `ls && rm bar`.
+- **A bare `&` is now a statement separator.** A command like `ls foo & rm bar` used to ship as one segment (auto-allowed because the head was `ls`); sofos now splits at the background-control `&` and rejects the command on the `rm` segment. The `2>&1`, `>&2`, and similar redirection operands stay glued to their preceding `>` / `<`.
+- **Bash arguments that the shell would expand at run-time are refused upfront.** Path arguments containing `$VAR`, backticks, `~user`, or unescaped glob characters (`?`, `*`, `[`, `{`) used to slip past the Read/Bash deny rules because the literal text never matched the configured globs; sofos now rejects the command with a clear message asking for the resolved literal path. Plain `~/...`, bare `~`, and absolute paths without metacharacters still work.
+- **Whitespace tricks no longer hide dangerous `git` operations.** `git\tpush`, `git$IFS\tpush`, `git${IFS}push`, and `git\\\npush` are now treated the same as `git push` by the structural matcher and the askable-command prompt, so each is blocked or confirmed exactly like its plain form.
+- **Deny rules survive path-noise variants.** A `Read(./secrets/**)` deny now matches `.//secrets/keys`, `./secrets/./keys`, and `/etc/./passwd` against `Bash(/etc/**)` — the candidate path is lexically normalised before the globset check. When the path contains `..`, only the resolved form is matched so `./secrets/../allowed.txt` is allowed (the shell would touch `./allowed.txt`).
+
 ## [0.3.0] - 2026-05-18
 
 ### Added

src/tools/bash/executor.rs

@@ -15,7 +15,9 @@ use crate::tools::bash::output::{
 };
 use crate::tools::bash::validate::command_contains_op;
 use crate::tools::permissions::{CommandPermission, PermissionManager};
-use crate::tools::utils::{MAX_TOOL_OUTPUT_TOKENS, TruncationKind, truncate_for_context};
+use crate::tools::utils::{
+    MAX_TOOL_OUTPUT_TOKENS, TruncationKind, normalize_command_whitespace, truncate_for_context,
+};
 use std::collections::HashSet;
 use std::ffi::OsString;
 use std::io::Read;
@@ -436,9 +438,12 @@ impl BashExecutor {
     fn confirm_askable_command(&self, command: &str) -> Result<()> {
         const ASKABLE_PREFIXES: &[&str] = &["git checkout"];
 
+        // Match against normalized input so tab / `$IFS` / backslash-newline
+        // between `git` and the subcommand don't dodge the prompt.
+        let matcher_input = normalize_command_whitespace(command).to_lowercase();
         let matches = ASKABLE_PREFIXES
             .iter()
-            .any(|prefix| command_contains_op(command, prefix));
+            .any(|prefix| command_contains_op(&matcher_input, prefix));
         if !matches {
             return Ok(());
         }

src/tools/bash/mod.rs

@@ -614,4 +614,53 @@ ask = []
             panic!("Expected ToolExecution error, got: {result:?}");
         }
     }
+
+    /// `git push` smuggled through tab / `$IFS` / backslash-newline must
+    /// still be caught by the dangerous-git matcher.
+    #[test]
+    fn dangerous_git_with_whitespace_obfuscation_is_rejected() {
+        let executor = BashExecutor::new(PathBuf::from("."), false, false).unwrap();
+
+        assert!(!executor.is_safe_command_structure("git\tpush origin main"));
+        assert!(!executor.is_safe_command_structure("git$IFS\tpush"));
+        assert!(!executor.is_safe_command_structure("git${IFS}push"));
+        assert!(!executor.is_safe_command_structure("git\\\npush"));
+        assert!(!executor.is_safe_command_structure("ls && git\tcommit -m x"));
+
+        // The plain form still works.
+        assert!(executor.is_safe_command_structure("git status"));
+        assert!(executor.is_safe_command_structure("git log"));
+    }
+
+    /// Path tokens with shell-meta that the shell would expand at
+    /// run-time must be refused before they reach the deny-glob check.
+    #[test]
+    fn path_token_with_shell_meta_is_rejected() {
+        let (_temp, path) = test_support::workspace();
+        let executor = BashExecutor::new(path, false, false).unwrap();
+
+        for cmd in [
+            "cat $HOME/.ssh/id_rsa",
+            "cat /e?c/passwd",
+            "cat /etc/p[a]sswd",
+            "ls /{etc,tmp}/x",
+            "cat ~root/.ssh/id_rsa",
+            "grep pattern ${HOME}/.bashrc",
+        ] {
+            let err = executor.execute(cmd);
+            assert!(
+                matches!(&err, Err(SofosError::ToolExecution(msg)) if msg.contains("can't be checked")),
+                "expected shell-meta rejection for `{cmd}`, got {err:?}"
+            );
+        }
+    }
+
+    /// `~/foo` and bare `~` are still legitimate path forms and pass
+    /// the shell-meta check (they're handled by `expand_tilde` below).
+    #[test]
+    fn tilde_home_paths_are_not_blocked_by_shell_meta_check() {
+        let executor = BashExecutor::new(PathBuf::from("."), false, false).unwrap();
+        assert!(executor.is_safe_command_structure("ls ~/Documents"));
+        assert!(executor.is_safe_command_structure("ls ~"));
+    }
 }

src/tools/bash/validate.rs

@@ -10,7 +10,8 @@ use crate::error::{Result, SofosError};
 use crate::tools::ToolName;
 use crate::tools::bash::BashExecutor;
 use crate::tools::permissions::{CommandPermission, PermissionManager};
-use crate::tools::utils::is_absolute_path;
+use crate::tools::utils::{is_absolute_path, lexically_normalize, normalize_command_whitespace};
+use std::path::PathBuf;
 
 /// Return true when a command argument looks like a parent-directory
 /// reference (`..`, `../foo`, `foo/..`, `foo/../bar`). Substring matches
@@ -107,6 +108,13 @@ pub(super) fn detect_command_substitution(command: &str) -> Option<&'static str>
 /// `{...; }` group. False positives (e.g. `ls {git,svn}` brace
 /// expansion triggering `git` detection) are acceptable — the worst
 /// outcome is the user being prompted to confirm a benign command.
+///
+/// The caller is expected to pass `command` already routed through
+/// [`normalize_command_whitespace`] so non-space whitespace (`\t`,
+/// `\r`, `\v`, `\f`, backslash-newline) and the explicit `$IFS` /
+/// `${IFS}` shell expansion appear as plain single spaces — otherwise
+/// `git\tpush`, `git$IFS\tpush`, and `git\\\npush` would evade the
+/// boundary check while still running as `git push`.
 pub(super) fn command_contains_op(command: &str, op: &str) -> bool {
     const BOUNDARIES: &[&str] = &[" ", ";", "&&", "||", "|", "`", "$(", "(", "{"];
     if command.starts_with(op) {
@@ -117,6 +125,34 @@ pub(super) fn command_contains_op(command: &str, op: &str) -> bool {
         .any(|sep| command.contains(&format!("{sep}{op}")))
 }
 
+/// Returns the kind of expansion that would change the path between
+/// our deny check and the shell touching it: `$` / `${...}`, backticks,
+/// `~user`, or glob metacharacters (`?`, `*`, `[`, `{`). Plain `~/`
+/// and bare `~` are allowed because they expand to a known prefix and
+/// are handled by the tilde-expansion helper.
+pub(super) fn path_token_shell_meta(tok: &str) -> Option<&'static str> {
+    if tok.contains('$') {
+        return Some("$ variable expansion");
+    }
+    if tok.contains('`') {
+        return Some("backtick command substitution");
+    }
+    if tok.starts_with('~') && tok != "~" && !tok.starts_with("~/") {
+        return Some("~user home expansion");
+    }
+    if tok.contains('?') || tok.contains('*') || tok.contains('[') || tok.contains('{') {
+        return Some("glob expansion");
+    }
+    None
+}
+
+/// True for tokens that look like a path the shell will resolve at
+/// run-time. Flag tokens (`--name=value`) and regex patterns fall
+/// through so the stricter shell-meta check only fires on real paths.
+fn token_looks_like_path(tok: &str) -> bool {
+    tok.contains('/') || tok.starts_with('.') || tok.starts_with('~') || is_absolute_path(tok)
+}
+
 impl BashExecutor {
     /// Check all external paths (absolute or tilde) in a command against Bash path grants.
     /// Asks the user interactively for any paths not yet covered.
@@ -150,6 +186,17 @@ impl BashExecutor {
                 cleaned
             };
 
+            // Reject path tokens whose post-expansion shape we cannot check.
+            if token_looks_like_path(path_candidate) {
+                if let Some(kind) = path_token_shell_meta(path_candidate) {
+                    return Err(SofosError::ToolExecution(format!(
+                        "Path argument '{}' uses {} which can't be checked against the permission rules before the shell expands it\n\
+                         Hint: pass the resolved literal path instead, or split this into a separate step that doesn't reference the same path.",
+                        path_candidate, kind
+                    )));
+                }
+            }
+
             // Check tilde before absolute so `~` / `~/foo` get expanded
             // first. `is_absolute_path` catches Unix (`/foo`) and
             // Windows (`C:\foo`, `\\server\share`) shapes on every
@@ -197,27 +244,42 @@ impl BashExecutor {
         path: &str,
         permission_manager: &mut PermissionManager,
     ) -> Result<()> {
-        // Canonicalize to resolve symlinks (e.g. /var/folders -> /private/var/folders on macOS)
-        let resolved = std::fs::canonicalize(path)
+        // Canonicalize when possible; also keep a lexically normalized form
+        // and the raw input so deny rules match every shape of the same path.
+        let canonical = std::fs::canonicalize(path)
             .map(|p| p.to_string_lossy().to_string())
-            .unwrap_or_else(|_| path.to_string());
-        let check_path = resolved.as_str();
-
-        // Enforce deny rules first (takes priority over allow)
-        if permission_manager.is_bash_path_denied(check_path) {
-            return Err(SofosError::ToolExecution(format!(
-                "Bash access denied for path '{}'\n\
-                 Hint: Blocked by deny rule in .sofos/config.local.toml or ~/.sofos/config.toml",
-                check_path
-            )));
+            .ok();
+        let normalized = lexically_normalize(&PathBuf::from(path))
+            .to_string_lossy()
+            .to_string();
+        let candidates: Vec<String> = match canonical {
+            Some(c) if c == normalized || c == path => vec![c],
+            Some(c) => vec![c, normalized.clone(), path.to_string()],
+            None if normalized == path => vec![path.to_string()],
+            None => vec![normalized.clone(), path.to_string()],
+        };
+        let check_path = candidates
+            .first()
+            .cloned()
+            .unwrap_or_else(|| path.to_string());
+
+        // Deny wins over allow; check every shape.
+        for cand in &candidates {
+            if permission_manager.is_bash_path_denied(cand) {
+                return Err(SofosError::ToolExecution(format!(
+                    "Bash access denied for path '{}'\n\
+                     Hint: Blocked by deny rule in .sofos/config.local.toml or ~/.sofos/config.toml",
+                    cand
+                )));
+            }
         }
 
         // Already allowed by config?
-        if permission_manager.is_bash_path_allowed(check_path) {
+        if permission_manager.is_bash_path_allowed(&check_path) {
             return Ok(());
         }
 
-        let path_obj = std::path::Path::new(check_path);
+        let path_obj = std::path::Path::new(&check_path);
 
         // Session allowed?
         if let Ok(allowed_dirs) = self.bash_path_session_allowed.lock() {
@@ -240,10 +302,10 @@ impl BashExecutor {
             }
         }
 
-        let parent = std::path::Path::new(check_path)
+        let parent = std::path::Path::new(&check_path)
             .parent()
             .and_then(|p| p.to_str())
-            .unwrap_or(check_path);
+            .unwrap_or(&check_path);
 
         // Non-interactive mode (tests, piped input): deny with a config hint
         if !self.interactive {
@@ -296,9 +358,22 @@ impl BashExecutor {
                 continue;
             }
 
-            let is_path = cleaned.contains('/')
-                || cleaned.starts_with('.')
-                || cleaned.starts_with('~')
+            let path_shaped =
+                cleaned.contains('/') || cleaned.starts_with('.') || cleaned.starts_with('~');
+
+            if path_shaped {
+                if let Some(kind) = path_token_shell_meta(cleaned) {
+                    return Err(SofosError::ToolExecution(format!(
+                        "Read argument '{}' uses {} which can't be checked against the Read rules before the shell expands it\n\
+                         Hint: pass the resolved literal path instead, or split this into a separate step that doesn't reference the same path.",
+                        cleaned, kind
+                    )));
+                }
+            }
+
+            // Path candidates: looks-like-path, or a bare token with no
+            // expansion meta (regex / ad-hoc strings fall through).
+            let is_path = path_shaped
                 || (!cleaned.contains('$')
                     && !cleaned.contains('`')
                     && !cleaned.contains('*')
@@ -366,7 +441,10 @@ impl BashExecutor {
             return false;
         }
 
-        if !self.is_safe_git_command(&command.to_lowercase()) {
+        // Normalise whitespace before matching so `git\tpush`,
+        // `git$IFS\tpush`, `git\\\npush` are seen as `git push`.
+        let matcher_input = normalize_command_whitespace(command).to_lowercase();
+        if !self.is_safe_git_command(&matcher_input) {
             return false;
         }
 
@@ -448,7 +526,7 @@ impl BashExecutor {
     }
 
     pub(super) fn get_rejection_reason(&self, command: &str) -> String {
-        let command_lower = command.to_lowercase();
+        let matcher_input = normalize_command_whitespace(command).to_lowercase();
 
         if has_path_traversal(command) {
             return format!(
@@ -467,7 +545,7 @@ impl BashExecutor {
             );
         }
 
-        if !self.is_safe_git_command(&command_lower) {
+        if !self.is_safe_git_command(&matcher_input) {
             return self.get_git_rejection_reason(command);
         }
 
@@ -507,7 +585,9 @@ impl BashExecutor {
     }
 
     pub(super) fn get_git_rejection_reason(&self, command: &str) -> String {
-        let command_lower = command.to_lowercase();
+        // Match against the same normalized input as `is_safe_git_command`
+        // so the reason picked here lines up with the rejection.
+        let command_lower = normalize_command_whitespace(command).to_lowercase();
 
         if command_lower.contains("git push") {
             return format!(