fix(indirect): preserve nested subscripts with a complete-reference boundary

crates/bashkit/docs/threat-model.md

@@ -82,7 +82,7 @@ through configurable limits.
 | History files (TM-DOS-131) | `history -a` loops, huge `$HISTSIZE`, giant `$HISTFILE` | History list bounded by `max_history_entries`/`max_history_bytes`; `$HISTFILE` reads refused past `max_input_bytes`; VFS only | MITIGATED |
 | Completion specs and bindings (TM-DOS-132) | `complete`/`bind` in a loop | 1024 specs (64 KiB each) and 1024 binding changes (4 KiB each) per interpreter; never shared between instances | MITIGATED |
 | Prompt recursion (TM-DOS-133) | `x='${x@P}'; echo "${x@P}"`, mutual cycles or substitution-mediated cycles | Cumulative prompt depth capped at `min(max_ast_depth, 8)`; shared work, aggregate input, cancellation and deadline checks on each prompt; descendant shells inherit depth | MITIGATED |
-| Indirect operand recursion (TM-DOS-134) | `r='a[0]:-${!r}]'` re-entering `${!...}` through an unset-element default | Bracket targets truncate at the first `]` (bash parity: `a[0]` resolves, rest ignored), so trailing operands never reach the indirect re-parse | MITIGATED |
+| Indirect operand recursion (TM-DOS-135) | `r='a[0]:-${!r}]'` re-entering `${!...}` through an unset-element default | A bracket target is refused unless the first complete `[...]` reference consumes the whole value (bash 5.x parity; nested subscripts resolve whole) | MITIGATED |
 
 **Stack Overflow / Recursion:**
 

crates/bashkit/src/interpreter/expansion.rs

@@ -2637,6 +2637,40 @@ impl Interpreter {
         })
     }
 
+    // End index (exclusive) of the first complete subscript reference that
+    // opens at `open`: nested brackets nest, quoted or escaped text is
+    // skipped. `None` when the brackets never balance. All boundary bytes
+    // are ASCII, so the index is always a char boundary.
+    fn indirect_array_end(value: &str, open: usize) -> Option<usize> {
+        let bytes = value.as_bytes();
+        let mut depth = 0usize;
+        let mut quote: Option<u8> = None;
+        let mut escaped = false;
+        let mut i = open;
+        while i < bytes.len() {
+            let b = bytes[i];
+            // 0x27 is a single quote, 0x22 a double quote, 0x5c a backslash.
+            if escaped {
+                escaped = false;
+            } else if quote.is_some_and(|q| b == q) {
+                quote = None;
+            } else if quote.is_none() && (b == 0x27 || b == 0x22) {
+                quote = Some(b);
+            } else if quote.is_none() && b == 0x5c {
+                escaped = true;
+            } else if quote.is_none() && b == b'[' {
+                depth += 1;
+            } else if quote.is_none() && b == b']' && depth > 0 {
+                depth -= 1;
+                if depth == 0 {
+                    return Some(i + 1);
+                }
+            }
+            i += 1;
+        }
+        None
+    }
+
     /// The parameter `${!name...}` refers to: the value of `name` (an
     /// element when it carries a subscript). bash rejects an unset `name`
     /// ("invalid indirect expansion") and a value that is not a parameter
@@ -2673,33 +2707,36 @@ impl Interpreter {
                 .is_some_and(|f| f.is_ascii_alphabetic() || f == '_')
                 && c.all(|ch| ch.is_ascii_alphanumeric() || ch == '_')
         };
-        // TM-RES-INDIRECT: an indirect array target ends at the first `]`
-        // after `[` (real bash resolves `a[0]` from `a[0]:-x]` and ignores
-        // the rest). Truncating before validation keeps trailing
-        // operator/nesting text — including a nested `${!...}` — from ever
-        // reaching the desugar re-parse below, which re-entered indirect
-        // expansion without bound (`r='a[0]:-${!r}]'` overflowed the stack).
-        // Targets without a sub-`]` (plain names, `a[$i]`, `a[@]`) pass
-        // through unchanged.
-        let value = match value.find('[') {
-            Some(open) => match value[open..].find(']') {
-                Some(rel) => value[..open + rel + 1].to_string(),
-                None => value,
-            },
-            None => value,
+        // TM-RES-INDIRECT: a bracket indirect target is only valid when
+        // the first complete `[...]` reference consumes the entire value.
+        // Modern bash (5.x) rejects trailing operator/nesting text with
+        // `invalid variable name` (older 3.2 ignored it); rejection also
+        // keeps a nested `${!...}` from ever reaching the desugar re-parse
+        // below, which re-entered indirect expansion without bound
+        // (`r='a[0]:-${!r}]'` overflowed the stack). The boundary tracks
+        // nested brackets and quoted text, so valid nested subscripts
+        // (`a[b[0]]`) resolve whole, while `a[0]:-x]`, `a[0][1]`, and
+        // unbalanced shapes are refused. Plain names, `a[$i]`, and `a[@]`
+        // have no trailing text and pass through unchanged.
+        let valid_boundary = match value.find('[') {
+            Some(open) => {
+                Self::indirect_array_end(&value, open).is_some_and(|end| end == value.len())
+            }
+            None => true,
         };
-        let valid = ident(&value)
-            || (!value.is_empty() && value.bytes().all(|b| b.is_ascii_digit()))
-            || matches!(value.as_str(), "@" | "*" | "#" | "?" | "$" | "!" | "-")
-            || value.find('[').is_some_and(|open| {
-                ident(&value[..open])
-                    && value.ends_with(']')
-                    && value.len() > open + 2
-                    && !value.contains("$(")
-                    && !value.contains('`')
-                    && !value.contains("<(")
-                    && !value.contains(">(")
-            });
+        let valid = valid_boundary
+            && (ident(&value)
+                || (!value.is_empty() && value.bytes().all(|b| b.is_ascii_digit()))
+                || matches!(value.as_str(), "@" | "*" | "#" | "?" | "$" | "!" | "-")
+                || value.find('[').is_some_and(|open| {
+                    ident(&value[..open])
+                        && value.ends_with(']')
+                        && value.len() > open + 2
+                        && !value.contains("$(")
+                        && !value.contains('`')
+                        && !value.contains("<(")
+                        && !value.contains(">(")
+                }));
         if !valid {
             // THREAT[TM-DOS-130]: echo a bounded prefix of the bad target.
             let shown: String = value.chars().take(256).collect();

crates/bashkit/tests/integration/threat_model_tests.rs

@@ -10,8 +10,9 @@ use bashkit::{
     OverlayFs, SessionLimits, TraceEventDetails, TraceEventKind, TraceMode,
 };
 use std::path::Path;
+use std::process::{Command, Stdio};
 use std::sync::Arc;
-use std::time::Duration;
+use std::time::{Duration, Instant};
 
 // =============================================================================
 // 1. RESOURCE EXHAUSTION TESTS
@@ -20,14 +21,105 @@ use std::time::Duration;
 mod resource_exhaustion {
     use super::*;
 
-    /// TM-RES-INDIRECT: an indirect array target ends at the first `]`.
-    /// Trailing operator/nesting text is never re-expanded as code, so a
-    /// default operand cannot re-enter indirect expansion and exhaust the
-    /// stack (`r='a[0]:-${!r}]'`). Matches real bash: element-or-empty.
+    /// TM-DOS-135: child-process runner for hostile scripts.
+    /// Hostile reproducers never run in the test process: pre-fix they
+    /// overflowed the stack and aborted the whole runner. The CLI binary
+    /// sits next to the test binary's `deps` directory.
+    fn bashkit_cli_path() -> std::path::PathBuf {
+        let exe = std::env::current_exe().expect("current test executable");
+        let deps = exe.parent().expect("deps dir");
+        let profile = deps.parent().expect("profile dir");
+        profile.join(format!("bashkit{}", std::env::consts::EXE_SUFFIX))
+    }
+
+    struct BoundedRun {
+        timed_out: bool,
+        exit_code: Option<i32>,
+        stdout: String,
+        stderr: String,
+    }
+
+    /// Runs a script in a child CLI process under a wall-clock watchdog.
+    /// Darwin note: Rust std exposes no portable per-child CPU, stack, or
+    /// memory caps, so the watchdog kill plus the 8 KiB output cap are the
+    /// enforced bounds here. Records the real producer exit status.
+    fn run_cli_bounded(script: &str, limit: Duration) -> BoundedRun {
+        let mut child = Command::new(bashkit_cli_path())
+            .args(["-c", script])
+            .stdin(Stdio::null())
+            .stdout(Stdio::piped())
+            .stderr(Stdio::piped())
+            .spawn()
+            .expect("spawn bashkit CLI");
+        let start = Instant::now();
+        let timed_out = loop {
+            match child.try_wait().expect("poll child") {
+                Some(_) => break false,
+                None => {
+                    if start.elapsed() >= limit {
+                        let _ = child.kill();
+                        break true;
+                    }
+                    std::thread::sleep(Duration::from_millis(10));
+                }
+            }
+        };
+        let output = child.wait_with_output().expect("collect child output");
+        const CAP: usize = 8192;
+        let mut stdout = String::from_utf8_lossy(&output.stdout).into_owned();
+        let mut stderr = String::from_utf8_lossy(&output.stderr).into_owned();
+        stdout.truncate(CAP);
+        stderr.truncate(CAP);
+        BoundedRun {
+            timed_out,
+            exit_code: output.status.code(),
+            stdout,
+            stderr,
+        }
+    }
+
+    /// TM-DOS-135: hostile indirect cycles are refused in a bounded child.
+    /// Exact finding reproducer plus the mutual variant. Both run in a
+    /// child CLI under a wall-clock watchdog (never in the test runner:
+    /// pre-fix they overflowed the stack and aborted the process) and are
+    /// refused with `invalid variable name`, like modern bash (5.x); the
+    /// real producer exit status is asserted, never relabeled.
+    #[test]
+    fn indirect_hostile_cycles_rejected_in_bounded_child() {
+        for script in [
+            "unset a\nr='a[0]:-${!r}]'\necho \"${!r}\"",
+            "unset a\nr='a[0]:-${!s}]'\ns='a[0]:-${!r}]'\necho \"${!r}\"",
+        ] {
+            let run = run_cli_bounded(script, Duration::from_secs(20));
+            assert!(!run.timed_out, "hostile script hung: {script}");
+            assert_eq!(
+                run.exit_code,
+                Some(1),
+                "script: {script}, stdout: {}, stderr: {}",
+                run.stdout,
+                run.stderr
+            );
+            assert!(run.stdout.is_empty(), "stdout: {}", run.stdout);
+            assert!(
+                run.stderr.contains("invalid variable name"),
+                "stderr: {}",
+                run.stderr
+            );
+            assert!(run.stderr.len() < 1024, "stderr: {}", run.stderr);
+        }
+    }
+
+    /// TM-DOS-135: a bracket indirect target is refused unless the first
+    /// complete `[...]` reference consumes the whole value. Modern bash
+    /// (5.x) rejects trailing operators with `invalid variable name`;
+    /// refusal also keeps a nested `${!...}` from ever reaching the desugar
+    /// re-parse, which re-entered indirect expansion without bound.
+    /// Valid nested subscripts resolve whole.
     #[tokio::test]
-    async fn indirect_array_target_ends_at_first_close_bracket() {
-        // Trailing operator/nesting text is ignored, like real bash
-        // (verified: bash prints `out=[]`, `st=0` for every target below).
+    async fn indirect_array_boundary_rejects_trailing_operators() {
+        // Trailing operator/nesting text is refused, like bash 5.x. Older
+        // bash 3.2 ignored the tail, so no real-bash oracle is asserted
+        // for these shapes; the rejection itself is the contract.
         for target in [
             "a[0]:-injected]",
             "a[0]-injected]",
@@ -36,64 +128,84 @@ mod resource_exhaustion {
             "a[0]/x/injected]",
             "a[0][1]",
             "a[0]}${x:-injected}[0]",
+            "a[b[0]]:-D",
         ] {
             let mut bash = Bash::new();
             let result = bash
-                .exec(&format!(
-                    "r='{target}'\necho \"out=[${{!r}}]\"\necho \"st=$?\""
-                ))
+                .exec(&format!("a=(1 2)\nb=(0)\nr='{target}'\necho \"${{!r}}\""))
                 .await
                 .unwrap();
-            assert_eq!(result.stdout, "out=[]\nst=0\n", "target: {target}");
+            assert_ne!(result.exit_code, 0, "target accepted: {target}");
+            assert_eq!(result.exit_code, 1, "target: {target}");
+            assert!(result.stdout.is_empty(), "target: {target}");
             assert!(
-                result.stderr.is_empty(),
+                result.stderr.contains("invalid variable name"),
                 "target: {target}: {}",
                 result.stderr
             );
+            assert!(result.stderr.len() < 1024, "target: {target}");
         }
-        // Set element resolves through the truncated target (bash: `1`).
-        let mut bash = Bash::new();
-        let result = bash
-            .exec("a=(1 2)\nr='a[0]:-injected]'\necho \"out=[${!r}]\"\necho \"st=$?\"")
-            .await
-            .unwrap();
-        assert_eq!(result.stdout, "out=[1]\nst=0\n");
-        assert!(result.stderr.is_empty(), "stderr: {}", result.stderr);
-    }
 
-    /// TM-RES-INDIRECT: self-referential default operand terminates.
-    /// Pre-fix this exact reproducer overflowed the stack (SIGABRT).
-    #[tokio::test]
-    async fn indirect_self_cycle_default_operand_terminates() {
-        // Exact finding reproducer: unset element + self-cycle in default.
-        // Real bash prints `out=[]` with status 0 and never crashes.
+        // Nested subscripts resolve whole (bash: `two`/`two`).
         let mut bash = Bash::new();
         let result = bash
-            .exec("unset a\nr='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"")
+            .exec("a=(zero one two three)\nb=(2)\nr='a[b[0]]'\nprintf \"indirect=[%s]\\n\" \"${!r}\"\nprintf \"direct=[%s]\\n\" \"${a[b[0]]}\"")
             .await
             .unwrap();
-        assert_eq!(result.stdout, "out=[]\nst=0\n");
+        assert_eq!(result.stdout, "indirect=[two]\ndirect=[two]\n");
         assert!(result.stderr.is_empty(), "stderr: {}", result.stderr);
-        // Set element: the cycle operand is discarded, element wins.
+        // Associative references agree between direct and indirect use.
+        // Portable note: indexed cases above also run against real bash
+        // below; assoc arrays need bash 4+, so agreement is asserted here.
         let mut bash = Bash::new();
         let result = bash
-            .exec("a=(1 2)\nr='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"")
+            .exec("declare -A h=([k]=V [key]=W)\nkey=k\nr='h[$key]'\necho \"dollar=[${!r}] direct=[${h[$key]}]\"\nr='h[key]'\necho \"plain=[${!r}] direct=[${h[key]}]\"\nr='h[@]'\necho \"at=[${!r}] direct=[${h[@]}]\"")
             .await
             .unwrap();
-        assert_eq!(result.stdout, "out=[1]\nst=0\n");
-        assert!(result.stderr.is_empty(), "stderr: {}", result.stderr);
-        // Mutual cycle through two names terminates as well.
-        let mut bash = Bash::new();
-        let result = bash
-            .exec(
-                "unset a\nr='a[0]:-${!s}]'\ns='a[0]:-${!r}]'\necho \"out=[${!r}]\"\necho \"st=$?\"",
-            )
-            .await
-            .unwrap();
-        assert_eq!(result.stdout, "out=[]\nst=0\n");
+        assert_eq!(
+            result.stdout,
+            "dollar=[V] direct=[V]\nplain=[W] direct=[W]\nat=[V W] direct=[V W]\n"
+        );
         assert!(result.stderr.is_empty(), "stderr: {}", result.stderr);
     }
 
+    // Out of scope: `]` inside an assoc key is unsupported by the
+    // direct subscript parser as well (`declare` accepts it, reads
+    // misparse), so no indirect agreement is asserted for that shape.
+    // The boundary scan still tracks quotes so a complete reference
+    // stays whole if the parser ever learns quoted brackets.
+
+    /// TM-DOS-135: finite nested indexed indirects match real bash
+    /// byte-for-byte. Indexed arrays work on bash 3.2+, so this runs on
+    /// macOS and Linux CI alike.
+    #[tokio::test]
+    async fn indirect_nested_indexed_matches_real_bash() {
+        for script in [
+            "a=(zero one two three); b=(2); r='a[b[0]]'; printf 'indirect=[%s]\n' \"${!r}\"; printf 'direct=[%s]\n' \"${a[b[0]]}\"",
+            "a=(1 2); r='a[0]'; echo \"out=[${!r}]\"",
+            "a=(x y); r='a[@]'; echo \"out=[${!r}]\"",
+            "a=(x y); i=1; r='a[$i]'; echo \"out=[${!r}]\"",
+        ] {
+            let expected = Command::new("bash")
+                .args(["--noprofile", "--norc", "-c", script])
+                .output()
+                .expect("run real bash");
+            assert!(
+                expected.status.success(),
+                "real bash failed: {script}: {}",
+                String::from_utf8_lossy(&expected.stderr)
+            );
+            let mut harness = Bash::new();
+            let result = harness.exec(script).await.unwrap();
+            assert_eq!(
+                result.stdout,
+                String::from_utf8_lossy(&expected.stdout).into_owned(),
+                "script: {script}"
+            );
+            assert!(result.stderr.is_empty(), "stderr: {}", result.stderr);
+        }
+    }
+
     /// V1: Test that command limit prevents infinite execution
     #[tokio::test]
     async fn threat_infinite_commands_blocked() {