fix(security): batch 3 — issues #498-#499 (#503)

## Summary

- **#498 (TM-DOS-046)**: Add `validate_path()` to all `MountableFs`
write methods — path depth/character validation no longer bypassed for
mounted filesystems
- **#498 (TM-DOS-049)**: Add explicit depth parameter to
`collect_dirs_recursive`, capped by filesystem `max_path_depth`
- **#498 (TM-DOS-050)**: Propagate caller-configured parser limits
through `parse_word_string` via new `parse_word_string_with_limits()`
method
- **#499 (TM-PY-028)**: `BashTool.reset()` now preserves `username`,
`hostname`, `max_commands`, and `max_loop_iterations` — matching
`PyBash.reset()` behavior

## Test plan

- [x] All 21 security audit regression tests pass (4 new tests for batch
3)
- [x] Full Rust test suite passes (`cargo test --all-features` — 1515+
tests)
- [x] `cargo fmt --check` clean
- [x] `cargo clippy --all-targets --all-features -- -D warnings` clean
- [x] Python lint clean (`ruff check` + `ruff format --check`)
- [x] Python test added for `BashTool.reset()` config preservation

Closes #498, closes #499

https://claude.ai/code/session_01TTiLUJVtmMNAo1NC9aQTn1

Co-authored-by: Claude <noreply@anthropic.com>

Author: chaliy

crates/bashkit-python/src/lib.rs

@@ -467,13 +467,32 @@ impl BashTool {
     }
 
     /// Releases GIL before blocking on tokio to prevent deadlock.
+    /// THREAT[TM-PY-028]: Rebuild with same config to preserve security limits.
     fn reset(&self, py: Python<'_>) -> PyResult<()> {
         let inner = self.inner.clone();
+        let username = self.username.clone();
+        let hostname = self.hostname.clone();
+        let max_commands = self.max_commands;
+        let max_loop_iterations = self.max_loop_iterations;
 
         py.detach(|| {
             self.rt.block_on(async move {
                 let mut bash = inner.lock().await;
-                let builder = Bash::builder();
+                let mut builder = Bash::builder();
+                if let Some(ref u) = username {
+                    builder = builder.username(u);
+                }
+                if let Some(ref h) = hostname {
+                    builder = builder.hostname(h);
+                }
+                let mut limits = ExecutionLimits::new();
+                if let Some(mc) = max_commands {
+                    limits = limits.max_commands(usize::try_from(mc).unwrap_or(usize::MAX));
+                }
+                if let Some(mli) = max_loop_iterations {
+                    limits = limits.max_loop_iterations(usize::try_from(mli).unwrap_or(usize::MAX));
+                }
+                builder = builder.limits(limits);
                 *bash = builder.build();
                 Ok(())
             })

crates/bashkit-python/tests/test_bashkit.py

@@ -906,6 +906,27 @@ def test_bashtool_rapid_reset_no_resource_exhaustion():
     assert r.stdout.strip() == "ok"
 
 
+# TM-PY-028: BashTool.reset() must preserve security config
+def test_bashtool_reset_preserves_config():
+    tool = BashTool(
+        username="secuser",
+        hostname="sechost",
+        max_commands=5,
+    )
+    # Verify config before reset
+    r = tool.execute_sync("whoami")
+    assert r.stdout.strip() == "secuser"
+
+    tool.reset()
+
+    # Config must survive reset
+    r = tool.execute_sync("whoami")
+    assert r.stdout.strip() == "secuser", "BashTool username lost after reset"
+
+    r = tool.execute_sync("hostname")
+    assert r.stdout.strip() == "sechost", "BashTool hostname lost after reset"
+
+
 def test_scripted_tool_rapid_sync_calls_no_resource_exhaustion():
     """Rapid execute_sync calls on ScriptedTool reuse a single runtime."""
     tool = ScriptedTool("api")

crates/bashkit/src/fs/mountable.rs

@@ -16,6 +16,7 @@ use std::sync::{Arc, RwLock};
 use super::limits::{FsLimits, FsUsage};
 use super::traits::{DirEntry, FileSystem, FileType, Metadata};
 use crate::error::Result;
+use std::io::ErrorKind;
 
 /// Filesystem with Unix-style mount points.
 ///
@@ -295,6 +296,15 @@ impl MountableFs {
         result
     }
 
+    /// THREAT[TM-DOS-046]: Validate path using root filesystem limits before delegation.
+    fn validate_path(&self, path: &Path) -> Result<()> {
+        self.root
+            .limits()
+            .validate_path(path)
+            .map_err(|e| IoError::new(ErrorKind::InvalidInput, e.to_string()))?;
+        Ok(())
+    }
+
     /// Resolve a path to the appropriate filesystem and relative path.
     ///
     /// Returns (filesystem, path_within_mount).
@@ -353,21 +363,26 @@ impl FileSystem for MountableFs {
     }
 
     async fn write_file(&self, path: &Path, content: &[u8]) -> Result<()> {
+        // THREAT[TM-DOS-046]: Validate path before delegation
+        self.validate_path(path)?;
         let (fs, resolved) = self.resolve(path);
         fs.write_file(&resolved, content).await
     }
 
     async fn append_file(&self, path: &Path, content: &[u8]) -> Result<()> {
+        self.validate_path(path)?;
         let (fs, resolved) = self.resolve(path);
         fs.append_file(&resolved, content).await
     }
 
     async fn mkdir(&self, path: &Path, recursive: bool) -> Result<()> {
+        self.validate_path(path)?;
         let (fs, resolved) = self.resolve(path);
         fs.mkdir(&resolved, recursive).await
     }
 
     async fn remove(&self, path: &Path, recursive: bool) -> Result<()> {
+        self.validate_path(path)?;
         let (fs, resolved) = self.resolve(path);
         fs.remove(&resolved, recursive).await
     }
@@ -425,6 +440,8 @@ impl FileSystem for MountableFs {
     }
 
     async fn rename(&self, from: &Path, to: &Path) -> Result<()> {
+        self.validate_path(from)?;
+        self.validate_path(to)?;
         let (from_fs, from_resolved) = self.resolve(from);
         let (to_fs, to_resolved) = self.resolve(to);
 
@@ -442,6 +459,8 @@ impl FileSystem for MountableFs {
     }
 
     async fn copy(&self, from: &Path, to: &Path) -> Result<()> {
+        self.validate_path(from)?;
+        self.validate_path(to)?;
         let (from_fs, from_resolved) = self.resolve(from);
         let (to_fs, to_resolved) = self.resolve(to);
 
@@ -455,6 +474,7 @@ impl FileSystem for MountableFs {
     }
 
     async fn symlink(&self, target: &Path, link: &Path) -> Result<()> {
+        self.validate_path(link)?;
         let (fs, resolved) = self.resolve(link);
         fs.symlink(target, &resolved).await
     }
@@ -465,6 +485,7 @@ impl FileSystem for MountableFs {
     }
 
     async fn chmod(&self, path: &Path, mode: u32) -> Result<()> {
+        self.validate_path(path)?;
         let (fs, resolved) = self.resolve(path);
         fs.chmod(&resolved, mode).await
     }

crates/bashkit/src/interpreter/mod.rs

@@ -6576,7 +6576,12 @@ impl Interpreter {
         if operand.is_empty() {
             return String::new();
         }
-        let word = Parser::parse_word_string(operand);
+        // THREAT[TM-DOS-050]: Propagate caller-configured limits to word parsing
+        let word = Parser::parse_word_string_with_limits(
+            operand,
+            self.limits.max_ast_depth,
+            self.limits.max_parser_operations,
+        );
         let mut result = String::new();
         for part in &word.parts {
             match part {
@@ -8380,7 +8385,10 @@ impl Interpreter {
 
         // Collect all directories recursively (including the base)
         let mut all_dirs = vec![base_dir.clone()];
-        self.collect_dirs_recursive(&base_dir, &mut all_dirs).await;
+        // THREAT[TM-DOS-049]: Cap recursion depth using filesystem path depth limit
+        let max_depth = self.fs.limits().max_path_depth;
+        self.collect_dirs_recursive(&base_dir, &mut all_dirs, max_depth)
+            .await;
 
         let mut matches = Vec::new();
 
@@ -8419,18 +8427,24 @@ impl Interpreter {
     }
 
     /// Recursively collect all subdirectories starting from dir.
+    /// THREAT[TM-DOS-049]: `max_depth` caps recursion to prevent stack exhaustion.
     fn collect_dirs_recursive<'a>(
         &'a self,
         dir: &'a Path,
         result: &'a mut Vec<PathBuf>,
+        max_depth: usize,
     ) -> std::pin::Pin<Box<dyn std::future::Future<Output = ()> + Send + 'a>> {
         Box::pin(async move {
+            if max_depth == 0 {
+                return;
+            }
             if let Ok(entries) = self.fs.read_dir(dir).await {
                 for entry in entries {
                     if entry.metadata.file_type.is_dir() {
                         let subdir = dir.join(&entry.name);
                         result.push(subdir.clone());
-                        self.collect_dirs_recursive(&subdir, result).await;
+                        self.collect_dirs_recursive(&subdir, result, max_depth - 1)
+                            .await;
                     }
                 }
             }

crates/bashkit/src/parser/mod.rs

@@ -111,6 +111,13 @@ impl<'a> Parser<'a> {
         parser.parse_word(input.to_string())
     }
 
+    /// THREAT[TM-DOS-050]: Parse a word string with caller-configured limits.
+    /// Prevents bypass of parser limits in parameter expansion contexts.
+    pub fn parse_word_string_with_limits(input: &str, max_depth: usize, max_fuel: usize) -> Word {
+        let parser = Parser::with_limits(input, max_depth, max_fuel);
+        parser.parse_word(input.to_string())
+    }
+
     /// Create a parse error with the current position.
     fn error(&self, message: impl Into<String>) -> Error {
         Error::parse_at(

crates/bashkit/tests/security_audit_pocs.rs

@@ -518,3 +518,103 @@ mod lexer_stack_overflow {
         // NOTE: depth=50 causes SIGABRT (TM-DOS-044). Not tested here.
     }
 }
+
+// =============================================================================
+// 9. MOUNTABLE FS MISSING validate_path (TM-DOS-046)
+//
+// Root cause: MountableFs delegates all operations without calling
+// validate_path() first, bypassing path depth/character validation.
+// Files: fs/mountable.rs:348-491
+// =============================================================================
+
+mod mountable_fs_validate_path {
+    use super::*;
+    use bashkit::{FileSystem, InMemoryFs, MountableFs};
+    use std::path::Path;
+
+    /// TM-DOS-046: MountableFs must reject paths with control characters.
+    #[tokio::test]
+    async fn security_audit_mountable_rejects_control_chars() {
+        let root = Arc::new(InMemoryFs::new());
+        let mountable = MountableFs::new(root);
+
+        let bad_path = Path::new("/tmp/file\x01name");
+        let result = mountable.write_file(bad_path, b"payload").await;
+        assert!(
+            result.is_err(),
+            "MountableFs must reject paths with control characters"
+        );
+    }
+
+    /// TM-DOS-046: MountableFs must validate paths on symlink creation.
+    #[tokio::test]
+    async fn security_audit_mountable_validates_symlink_path() {
+        let root = Arc::new(InMemoryFs::new());
+        let mountable = MountableFs::new(root);
+
+        let bad_link = Path::new("/tmp/link\x02name");
+        let result = mountable.symlink(Path::new("/target"), bad_link).await;
+        assert!(result.is_err(), "MountableFs must validate symlink paths");
+    }
+}
+
+// =============================================================================
+// 10. collect_dirs_recursive DEPTH LIMIT (TM-DOS-049)
+//
+// Root cause: No explicit depth limit on directory recursion.
+// Files: interpreter/mod.rs:8352
+// =============================================================================
+
+mod collect_dirs_depth_limit {
+    use super::*;
+
+    /// TM-DOS-049: collect_dirs_recursive has an explicit depth cap.
+    /// Verify ** glob completes without stack overflow on a simple tree.
+    #[tokio::test]
+    async fn security_audit_glob_star_star_respects_depth() {
+        let limits = ExecutionLimits::new()
+            .max_commands(200)
+            .timeout(Duration::from_secs(10));
+        let mut bash = Bash::builder().limits(limits).build();
+
+        // Create a shallow directory tree
+        let result = bash
+            .exec("mkdir -p /tmp/globtest/sub && touch /tmp/globtest/sub/file.txt && echo ok")
+            .await
+            .unwrap();
+        assert_eq!(result.stdout.trim(), "ok");
+
+        // ** glob must complete without stack overflow (the fix adds depth limit)
+        let result = bash.exec("echo /tmp/globtest/**").await;
+        assert!(
+            result.is_ok(),
+            "** glob must complete without stack overflow"
+        );
+    }
+}
+
+// =============================================================================
+// 11. parse_word_string USES DEFAULT LIMITS (TM-DOS-050)
+//
+// Root cause: parse_word_string() creates parser with default limits,
+// ignoring caller-configured tighter limits.
+// Files: parser/mod.rs:109
+// =============================================================================
+
+mod parse_word_string_limits {
+    use super::*;
+
+    /// TM-DOS-050: Parameter expansion word parsing should respect configured limits.
+    /// With a tight AST depth limit, deeply nested ${...} should not bypass it.
+    #[tokio::test]
+    async fn security_audit_word_parse_uses_configured_limits() {
+        let limits = ExecutionLimits::new()
+            .max_ast_depth(5)
+            .timeout(Duration::from_secs(5));
+        let mut bash = Bash::builder().limits(limits).build();
+
+        // Simple parameter expansion should work
+        let result = bash.exec("x=hello; echo ${x:-world}").await.unwrap();
+        assert_eq!(result.stdout.trim(), "hello");
+    }
+}