fix: recurse into sub-expressions in is_safe_expect_expr (0.2.1) (#2)

* Recurse into expect expression validation and add regression tests

* Recurse into expect expressions to reject nested unsafe and blocks

* chore: clarify expect validation scope, add regression tests, bump to 0.2.1

- Update comment and error message in validate_local_test_expects to
  accurately describe what is and isn't blocked: syntax-level injection
  vectors (block, unsafe, closure, control-flow) are rejected; call
  expressions to side-effectful functions are not blocked since .unit.spec
  files are treated as trusted input (same as body.rust)
- Add 4 regression tests for Field, Index, Unary, and Cast recursive arms
  to ensure sub-expression recursion is covered
- Track unbounded recursion finding in TODOS.md (no depth cap in
  is_safe_expect_expr; low-urgency given trusted input model)
- Bump version 0.2.0 → 0.2.1

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

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

Author: Spenquatch

CHANGELOG.md

@@ -1,5 +1,15 @@
 # Changelog
 
+## 0.2.1 - 2026-04-03
+
+### Security
+
+- **`is_safe_expect_expr` now recurses into sub-expressions** — Previously, the expression whitelist in `spec validate` only inspected the top-level AST node. A call like `f({ unsafe { ... } })` would pass because the outer `Call` was whitelisted without checking its arguments. All Arms (Binary, Call, MethodCall, Field, Index, Unary, Paren, Cast) now recurse into every sub-expression; `unsafe`, block, closure, and control-flow forms are rejected wherever they appear in the tree. Error message updated from "simple expression" framing to "block, unsafe, closure" framing to accurately describe what is and isn't blocked.
+
+### Testing
+
+- Added 4 regression tests covering recursion through Field, Index, Unary, and Cast arms.
+
 ## 0.2.0 - 2026-04-02
 
 ### Added

Cargo.lock

@@ -3,7 +3,7 @@ resolver = "3"
 members = ["spec-core", "spec-cli"]
 
 [workspace.package]
-version = "0.2.0"
+version = "0.2.1"
 edition = "2024"
 rust-version = "1.89.0"
 authors = ["Spec Authors"]

Cargo.toml

@@ -51,13 +51,17 @@
 ### Deferred from autoplan retrospective (2026-04-03)
 
 #### Security / Correctness (High Priority)
-- [ ] **Fix `is_safe_expect_expr` to recurse into sub-expressions** — Current implementation checks top-level expression variant but not children. `f({ unsafe { ... } })` passes because outer `Call` returns true without inspecting args. Fix: recurse into `Binary`, `Call`, `MethodCall`, `Field`, `Index`, `Unary`, `Cast` sub-expressions. Add tests: `expect_with_unsafe_block_in_call_arg_is_rejected`, `expect_with_block_in_binary_operand_is_rejected`. (validator.rs:148)
+- [x] **Fix `is_safe_expect_expr` to recurse into sub-expressions** — Completed fix/change-is_safe_expect_expr. `f({ unsafe { ... } })` now rejected. Added regression tests: `expect_with_unsafe_block_in_call_arg_is_rejected`, `expect_with_block_in_binary_operand_is_rejected`, `expect_with_unsafe_block_in_method_call_arg_is_rejected`. (validator.rs:148)
 
 #### Architecture Fixes (Medium Priority)
 - [ ] **Consolidate path-containment logic** — `clean_output_dir` uses `normalized_absolute_path` (lexical) and `ensure_output_marker` uses `canonicalize` (symlink-following). Divergent logic = future maintenance hazard. Extract single `safe_output_path(path) -> Result<PathBuf>` utility. (generator.rs, commands.rs)
 - [ ] **Add `local_tests[].id` uniqueness validation** — Duplicate ids within a unit → duplicate `fn test_{id}()` → compile error. Validate uniqueness in `validate_local_test_expects`. (validator.rs)
 - [ ] **Document `pub use generated::*` as required consuming-crate convention** — Internal deps generate `use crate::X` which only works if consuming crate re-exports generated modules at root. Document in README/DECISIONS.md. Not a code change.
 
+#### Architecture (Medium Priority)
+- [ ] **Defense-in-depth: validate local_tests[].expect at the sink** — `generate_code` (generator.rs:19) is a public library function that embeds `local_test.expect` verbatim with no validation. The CLI path always validates first via the loader, but a direct library API caller constructing a `ResolvedSpec` manually bypasses all expression validation. Consider: (a) validate at the `generate_code` sink, (b) use a newtype wrapper for validated expect strings, or (c) emit the generated assert!() from the validated syn::Expr AST instead of the raw string. Codex outside-voice finding, fix/change-is_safe_expect_expr review.
+- [ ] **Add recursion depth cap to `is_safe_expect_expr`** — The recursive AST walk has no depth limit. Deeply nested input like `((((x))))` or `!!!!!x` (100+ levels) could stack overflow during validation. Add a `depth: usize` parameter and return `false` above a threshold (~128). Low-urgency since `.unit.spec` files are trusted input, but the fix is trivial. Codex adversarial finding, fix/change-is_safe_expect_expr review.
+
 #### M3 Prerequisites (Design Spikes, Before Build)
 - [ ] **Define ICP: solo engineer vs. team coordination tool** — Changes M3 priority order completely. One paragraph, before M3 scoping.
 - [ ] **Force binary decision: commit generated output vs ephemeral** — Current hybrid (gitignored but required to compile) is unstable for real adopters. Decide and document in DECISIONS.md.

TODOS.md

@@ -130,14 +130,17 @@ fn validate_local_test_expects(spec: &LoadedSpec) -> Result<()> {
                 path: path.clone(),
             }
         })?;
-        // Only allow expression kinds that are safe to embed verbatim into assert!().
-        // Reject scope-creating forms (Block, Unsafe, Closure, If, Match, Loop, etc.)
-        // that could execute arbitrary code. A config lever for trusted workspaces is
-        // deferred to M3. See TODOS.md.
+        // Reject syntax-level injection vectors: block expressions, unsafe blocks,
+        // closures, and control-flow forms (If, Match, Loop, etc.) that introduce
+        // statements or scope inside assert!(). Note: call expressions to
+        // side-effectful functions (std::fs, std::process::Command, etc.) are not
+        // blocked — .unit.spec files are treated as trusted input, the same as
+        // body.rust. A config lever and defense-in-depth options are deferred to
+        // M3. See TODOS.md.
         if !is_safe_expect_expr(&expr) {
             return Err(SpecError::LocalTestExpectNotExpr {
                 id: test.id.clone(),
-                message: "expect must be a simple expression (binary, call, path, or literal); block, unsafe, closure, and control-flow expressions are not allowed".to_string(),
+                message: "expect must use only operators, function calls, and value access; block, unsafe, closure, and control-flow forms are not allowed".to_string(),
                 path: path.clone(),
             });
         }
@@ -147,14 +150,17 @@ fn validate_local_test_expects(spec: &LoadedSpec) -> Result<()> {
 
 fn is_safe_expect_expr(expr: &syn::Expr) -> bool {
     match expr {
-        syn::Expr::Binary(_)
-        | syn::Expr::Call(_)
-        | syn::Expr::MethodCall(_)
-        | syn::Expr::Path(_)
-        | syn::Expr::Lit(_)
-        | syn::Expr::Field(_)
-        | syn::Expr::Index(_)
-        | syn::Expr::Unary(_) => true,
+        syn::Expr::Binary(b) => is_safe_expect_expr(&b.left) && is_safe_expect_expr(&b.right),
+        syn::Expr::Call(c) => {
+            is_safe_expect_expr(&c.func) && c.args.iter().all(is_safe_expect_expr)
+        }
+        syn::Expr::MethodCall(m) => {
+            is_safe_expect_expr(&m.receiver) && m.args.iter().all(is_safe_expect_expr)
+        }
+        syn::Expr::Field(f) => is_safe_expect_expr(&f.base),
+        syn::Expr::Index(i) => is_safe_expect_expr(&i.expr) && is_safe_expect_expr(&i.index),
+        syn::Expr::Unary(u) => is_safe_expect_expr(&u.expr),
+        syn::Expr::Path(_) | syn::Expr::Lit(_) => true,
         syn::Expr::Paren(inner) => is_safe_expect_expr(&inner.expr),
         syn::Expr::Cast(c) => is_safe_expect_expr(&c.expr),
         // Block, Unsafe, Closure, If, Match, Loop, ForLoop, While, Async, etc. → rejected
@@ -779,8 +785,253 @@ pub fn apply_discount(subtotal: Decimal, rate: Decimal) -> Decimal {
         };
         let err = validate_semantic(&spec).unwrap_err().to_string();
         assert!(
-            err.contains("simple expression"),
+            err.contains("block, unsafe, closure"),
             "expected block expression to be rejected: {err}"
         );
     }
+
+    #[test]
+    fn expect_with_unsafe_block_in_call_arg_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_call_arg".to_string(),
+                    expect: "f(unsafe { true })".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in call arg to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_block_in_binary_operand_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "block_in_binary_operand".to_string(),
+                    expect: "true && { false }".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected block expression in binary operand to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_unsafe_block_in_method_call_arg_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_method_arg".to_string(),
+                    expect: "foo.bar(unsafe { true })".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in method call arg to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_unsafe_block_in_field_base_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_field_base".to_string(),
+                    expect: "(unsafe { foo }).field".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in field base to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_unsafe_block_in_index_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_index".to_string(),
+                    expect: "arr[unsafe { 0 }]".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in index to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_unsafe_block_in_unary_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_unary".to_string(),
+                    expect: "!(unsafe { true })".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in unary operand to be rejected: {err}"
+        );
+    }
+
+    #[test]
+    fn expect_with_unsafe_block_in_cast_is_rejected() {
+        use crate::types::{Body, Intent, LocalTest, SpecSource, SpecStruct};
+        let spec = LoadedSpec {
+            source: SpecSource {
+                file_path: "test.unit.spec".to_string(),
+                id: "pricing/apply_discount".to_string(),
+            },
+            spec: SpecStruct {
+                id: "pricing/apply_discount".to_string(),
+                kind: "function".to_string(),
+                intent: Intent {
+                    why: "Apply a discount.".to_string(),
+                },
+                contract: None,
+                deps: vec![],
+                imports: vec![],
+                body: Body {
+                    rust: "pub fn apply_discount() -> bool { true }".to_string(),
+                },
+                local_tests: vec![LocalTest {
+                    id: "unsafe_in_cast".to_string(),
+                    expect: "(unsafe { 0 }) as u64".to_string(),
+                }],
+                links: None,
+            },
+        };
+
+        let err = validate_semantic(&spec).unwrap_err().to_string();
+        assert!(
+            err.contains("block, unsafe, closure"),
+            "expected unsafe block in cast to be rejected: {err}"
+        );
+    }
 }