Address review: gate ALL scaffolding entry points + robust name validation

Adversarial review found the first cut was incomplete (two blocking issues):

- `new` (cmd_new) is another user-name scaffolding entry point that was NOT guarded, so
  `specsync new "../../PWNED" --full` still escaped the project on every platform.
- The blocklist relied on `is_absolute()`, which misses Windows drive-relative names
  (`C:foo`): no separator, not absolute, so it passed — and on Windows `join` replaces
  the base, escaping `<specs_dir>/<name>/`.

Plus two follow-ups it surfaced: `wizard` had the same traversal gap (interactive, only
checked empty), and control characters/newlines were accepted (YAML frontmatter
injection / control-char dir names, in-project but unexpected).

Reworked into a single shared `validate_module_name` in commands/mod.rs (next to
`load_and_discover`) and gated ALL FOUR scaffolding entry points: `new`, `add-spec`,
`scaffold`, `wizard`. The validator now requires a single `Component::Normal` segment
with no raw separator and no control chars — platform-aware, so it also rejects Windows
drive-relative prefixes that `is_absolute()` misses, and blocks frontmatter injection.

Reproduced: `new "../../PWNED" --full` went from "files written outside project, exit 0"
to `invalid module name … / exit 1` with nothing created outside; a newline-injecting
name is refused.

Tests: `validate_module_name` unit tests (plain/unicode names ok; empty, separators,
`.`/`..`, absolute, control chars rejected; `#[cfg(windows)]` drive-relative rejected);
`scaffold_rejects_module_name_path_traversal` now also covers `new`. Documented the
shared export in commands.spec.md and the new error case in the new/wizard specs.
734 unit + 170 integration, fmt / clippy (bin) / self-check 100% (37851 LOC).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KDJxU4R8hUEuq1Y5jzft5m

Author: 0xLeif

specs/cmd_new/cmd_new.spec.md

@@ -55,6 +55,7 @@ Implements the `specsync new` command. Quick-creates a minimal spec with auto-de
 | Spec already exists | Exits 1 |
 | No source files found | Creates spec with empty `files:` and prints a ⚠ explaining that the `files:` list must be filled in before `check` passes |
 | Dir creation fails | Exits 1 |
+| Invalid module name (path separator, `.`/`..`, absolute/drive-relative, control chars) | Refused via `validate_module_name` before any write; prints `invalid module name …` and exits 1 (no path traversal) |
 
 ## Dependencies
 

specs/cmd_wizard/cmd_wizard.spec.md

@@ -54,7 +54,7 @@ Implements the `specsync wizard` command — an interactive TUI wizard for creat
 
 | Condition | Behavior |
 |-----------|----------|
-| Empty module name entered | Exits with code 1 |
+| Empty or unsafe module name entered (path separator, `.`/`..`, absolute/drive-relative, control chars) | Refused via `validate_module_name`; prints `invalid module name …` and exits 1 (no path traversal) |
 | Spec directory already exists | Prints error and exits 1 |
 | User cancels at confirmation | Exits cleanly with code 0 |
 | Directory creation fails | Exits with code 1 |

specs/commands/commands.spec.md

@@ -29,6 +29,7 @@ Shared command infrastructure used by all CLI subcommands. Provides config loadi
 | Function | Parameters | Returns | Description |
 |----------|-----------|---------|-------------|
 | `load_and_discover` | `root: &Path, allow_empty: bool` | `(SpecSyncConfig, Vec<PathBuf>)` | Load config and discover all spec files (excluding `_`-prefixed); exits if empty and `allow_empty` is false |
+| `validate_module_name` | `module_name: &str` | `Result<(), String>` | Validate a user-supplied module name for the scaffolding commands (`new`, `add-spec`, `scaffold`, `wizard`): must be a single plain path segment (one `Component::Normal`, no separators/`.`/`..`/absolute/drive-relative/control chars), preventing path traversal outside the project |
 | `filter_specs` | `root: &Path, spec_files: &[PathBuf], filters: &[String]` | `Vec<PathBuf>` | Filter spec files by user-provided names/paths (exact path, relative path, filename, module name); returns all if filters is empty |
 | `filter_by_status` | `spec_files: &[PathBuf], exclude: &[String], only: &[String]` | `Vec<PathBuf>` | Filter spec files by their frontmatter status field; supports exclude-list and allow-list modes |
 | `build_schema_columns` | `root: &Path, config: &SpecSyncConfig` | `HashMap<String, SchemaTable>` | Build column-level schema from migration files if `schema_dir` is configured |

src/commands/mod.rs

@@ -66,6 +66,37 @@ pub fn load_and_discover(root: &Path, allow_empty: bool) -> (types::SpecSyncConf
     (config, spec_files)
 }
 
+/// Validate a user-supplied module name used by the scaffolding commands
+/// (`new`, `add-spec`, `scaffold`, `wizard`). The name is written verbatim into paths
+/// like `<specs_dir>/<name>/<name>.spec.md` and joined onto source dirs, so an
+/// unvalidated name containing a path separator, `.`/`..`, or an absolute/drive-relative
+/// path would let scaffolding create files anywhere on disk (path traversal).
+///
+/// A valid name is a single plain path segment: exactly one `Component::Normal`, with no
+/// raw path separator and no control characters. The component check is platform-aware —
+/// it also rejects Windows drive-relative prefixes like `C:foo` that `Path::is_absolute`
+/// misses. Control characters are rejected so a name cannot inject into the generated
+/// YAML frontmatter or create a control-char directory. Returns `Err` (to be printed and
+/// exited on) rather than writing outside the project.
+pub(crate) fn validate_module_name(module_name: &str) -> Result<(), String> {
+    let single_normal_segment = {
+        let mut components = Path::new(module_name).components();
+        matches!(components.next(), Some(std::path::Component::Normal(_)))
+            && components.next().is_none()
+    };
+    let clean = !module_name.contains('/')
+        && !module_name.contains('\\')
+        && !module_name.chars().any(char::is_control);
+    if single_normal_segment && clean {
+        return Ok(());
+    }
+    Err(format!(
+        "invalid module name `{}`: use a single plain name — no path separators (`/`, `\\`), \
+         `.`/`..`, drive prefixes, absolute paths, or control characters",
+        module_name.escape_default()
+    ))
+}
+
 /// Filter spec files by user-provided spec names/paths.
 /// Matches against: exact file path, relative path, module name (from filename stem).
 /// Returns the full list if `filters` is empty.
@@ -702,3 +733,61 @@ pub fn create_drift_issues(
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::validate_module_name;
+
+    #[test]
+    fn validate_module_name_accepts_plain_names() {
+        for name in [
+            "auth",
+            "auth-service",
+            "user_profile",
+            "v2",
+            "a.b",
+            "Módulo",
+        ] {
+            assert!(
+                validate_module_name(name).is_ok(),
+                "`{name}` should be a valid module name"
+            );
+        }
+    }
+
+    #[test]
+    fn validate_module_name_rejects_traversal_and_injection() {
+        // Empty, path separators, parent/current refs, absolute paths, and control
+        // characters must all be refused — none may reach a filesystem join.
+        for name in [
+            "",
+            ".",
+            "..",
+            "../evil",
+            "../../PWNED/evil",
+            "a/b",
+            "a\\b",
+            "sub/mod",
+            "/tmp/abs",
+            "auth/", // trailing separator normalizes to one segment, still refused
+            "evil\nversion: 99", // newline → frontmatter injection
+            "tab\tname",
+            "null\0byte",
+        ] {
+            assert!(
+                validate_module_name(name).is_err(),
+                "`{}` must be rejected as an unsafe module name",
+                name.escape_default()
+            );
+        }
+    }
+
+    #[test]
+    #[cfg(windows)]
+    fn validate_module_name_rejects_windows_drive_relative() {
+        // `C:foo` has no separator and `is_absolute()` is false (drive-relative), but its
+        // components include a Prefix, so the single-Normal-segment check refuses it.
+        assert!(validate_module_name("C:foo").is_err());
+        assert!(validate_module_name("C:\\abs").is_err());
+    }
+}

src/commands/new.rs

@@ -7,8 +7,14 @@ use crate::config::load_config;
 use crate::exports;
 use crate::generator;
 
+use super::validate_module_name;
+
 /// Quick-create a minimal spec for a module with auto-detected source files.
 pub fn cmd_new(root: &Path, module_name: &str, full: bool) {
+    if let Err(e) = validate_module_name(module_name) {
+        eprintln!("{e}");
+        process::exit(1);
+    }
     let config = load_config(root);
     let specs_dir = root.join(&config.specs_dir);
     let spec_dir = specs_dir.join(module_name);

src/commands/scaffold.rs

@@ -7,29 +7,7 @@ use crate::config::load_config;
 use crate::generator;
 use crate::registry;
 
-/// Reject a module name that could escape the target directory. The name is written
-/// verbatim into paths like `<specs_dir>/<name>/<name>.spec.md` and joined onto source
-/// dirs, so a name containing a path separator, `.`/`..`, or an absolute/rooted path
-/// would let `add-spec`/`scaffold` create files anywhere on disk (path traversal). A
-/// module name must be a single path segment. Fails loud rather than writing outside
-/// the project.
-fn validate_module_name(module_name: &str) -> Result<(), String> {
-    if module_name.is_empty() {
-        return Err("module name must not be empty".to_string());
-    }
-    if module_name.contains('/')
-        || module_name.contains('\\')
-        || module_name == "."
-        || module_name == ".."
-        || Path::new(module_name).is_absolute()
-    {
-        return Err(format!(
-            "invalid module name `{module_name}`: use a single name without path \
-             separators (`/`, `\\`), `.`/`..`, or an absolute path"
-        ));
-    }
-    Ok(())
-}
+use super::validate_module_name;
 
 pub fn cmd_add_spec(root: &Path, module_name: &str) {
     if let Err(e) = validate_module_name(module_name) {
@@ -313,39 +291,3 @@ pub fn cmd_scaffold(
         }
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn validate_module_name_accepts_plain_names() {
-        for name in ["auth", "auth-service", "user_profile", "v2", "a.b"] {
-            assert!(
-                validate_module_name(name).is_ok(),
-                "`{name}` should be a valid module name"
-            );
-        }
-    }
-
-    #[test]
-    fn validate_module_name_rejects_path_traversal() {
-        // Anything that could escape `<specs_dir>/<name>/` must be refused.
-        for name in [
-            "",
-            "..",
-            ".",
-            "../evil",
-            "../../PWNED/evil",
-            "a/b",
-            "a\\b",
-            "/tmp/abs",
-            "sub/mod",
-        ] {
-            assert!(
-                validate_module_name(name).is_err(),
-                "`{name}` must be rejected as an unsafe module name"
-            );
-        }
-    }
-}

src/commands/wizard.rs

@@ -29,8 +29,8 @@ pub fn cmd_wizard(root: &Path) {
         .unwrap_or_else(|_| process::exit(0));
     let module_name = module_name.trim().to_string();
 
-    if module_name.is_empty() {
-        eprintln!("{} Module name cannot be empty", "Error:".red());
+    if let Err(e) = super::validate_module_name(&module_name) {
+        eprintln!("{} {e}", "Error:".red());
         process::exit(1);
     }
 

tests/integration.rs

@@ -5483,15 +5483,15 @@ fn scaffold_auto_detects_single_source_file() {
     );
 }
 
-/// `add-spec`/`scaffold` must refuse a module name that would escape the project
+/// `new`/`add-spec`/`scaffold` must refuse a module name that would escape the project
 /// (path traversal) rather than writing spec/companion files to an arbitrary location.
 #[test]
 fn scaffold_rejects_module_name_path_traversal() {
     let tmp = TempDir::new().unwrap();
     let root = tmp.path();
     write_config(root, "specs", &["src"]);
 
-    for sub in ["add-spec", "scaffold"] {
+    for sub in ["new", "add-spec", "scaffold"] {
         let output = specsync()
             .args([sub, "../../escape/evil", "--root", root.to_str().unwrap()])
             .output()