Fix: reject path-traversal module names in add-spec/scaffold (#316)

* Fix: reject path-traversal module names in `add-spec`/`scaffold`

`cmd_add_spec` and `cmd_scaffold` wrote the user-supplied module name verbatim into
paths (`<specs_dir>/<name>/<name>.spec.md`, and joined onto source dirs), with no
validation. A name containing `../` or an absolute path escaped the project: e.g.
`specsync add-spec "../../PWNED/evil"` created `evil.spec.md` and a companion directory
OUTSIDE the project root (and then panicked mid-way, exit 101). Path traversal writing
files to arbitrary locations.

Added `validate_module_name`: a module name must be a single path segment — empty, path
separators (`/`, `\`), `.`/`..`, and absolute paths are refused with a clear error and
exit 1, before any filesystem write. Gated both entry points. Legitimate names
(`auth`, `auth-service`, `user_profile`) are unaffected.

Reproduced: the traversal above went from "files written outside project + exit 101" to
`invalid module name … / exit 1` with nothing created outside.

Tests: `validate_module_name` unit tests (accepts plain names, rejects
separators/`..`/absolute/empty); `scaffold_rejects_module_name_path_traversal`
integration test (both commands fail loud, nothing escapes root). Documented the new
invariant + error cases in the cmd_scaffold spec. 734 unit + 170 integration, fmt /
clippy (bin) / self-check 100% (37814 LOC).

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

* 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

---------

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

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_scaffold/cmd_scaffold.spec.md

@@ -34,6 +34,7 @@ Implements `specsync add-spec` and `specsync scaffold` commands. Creates new spe
 2. `cmd_scaffold` supports custom templates and auto-appends to registry
 3. Neither overwrites existing specs
 4. Companion files (tasks.md, context.md, requirements.md, testing.md) are always generated with guided starter content; design.md is generated only when `companions.design` is enabled in config
+5. Both validate `module_name` as a single path segment before any filesystem write — a name containing a path separator (`/`, `\`), `.`/`..`, or an absolute path is refused with exit 1, so scaffolding can never create files outside the project (no path traversal)
 
 ## Behavioral Examples
 
@@ -50,6 +51,8 @@ Implements `specsync add-spec` and `specsync scaffold` commands. Creates new spe
 | Spec exists | Early return |
 | Dir creation fails | Exits 1 |
 | Custom template dir missing | Falls back to built-in |
+| Module name with path separator / `..` / absolute path | Refused before any write; prints `invalid module name …` and exits 1 |
+| Empty module name | Refused; exits 1 |
 
 ## Dependencies
 

specs/cmd_scaffold/testing.md

@@ -6,7 +6,8 @@ spec: cmd_scaffold.spec.md
 
 | Area | Command | Assertions To Watch |
 |------|---------|---------------------|
-| `src/commands/scaffold.rs` | cargo test commands::scaffold | No inline `#[cfg(test)]` module; add focused coverage for `cmd_add_spec`, `cmd_scaffold`, source auto-detection, and registry auto-registration before risky changes |
+| `src/commands/scaffold.rs` | cargo test commands::scaffold | Inline tests cover `validate_module_name` (accepts plain names, rejects separators/`..`/absolute); still add coverage for source auto-detection and registry auto-registration before risky changes |
+| `tests/integration.rs` | cargo test --test integration scaffold_rejects_module_name_path_traversal | `add-spec`/`scaffold` with a traversal name (`../../escape/evil`) exit non-zero with "invalid module name" and write nothing outside the project root |
 | `tests/integration.rs` | cargo test --test integration generate_creates_companion_files | Exercises the same `generator::generate_companion_files_for_spec` path scaffold uses to emit companions |
 | `tests/integration.rs` | cargo test --test integration companion_files_not_overwritten_on_regenerate | Confirms existing companions are not clobbered when re-running on an existing spec |
 
@@ -27,6 +28,7 @@ spec: cmd_scaffold.spec.md
 | Spec exists | Early return | Keep or add a focused assertion before changing this behavior |
 | Dir creation fails | Exits 1 | Keep or add a focused assertion before changing this behavior |
 | Custom template dir missing | Falls back to built-in | Keep or add a focused assertion before changing this behavior |
+| Module name with path separator / `..` / absolute / empty | Refused before any write; exits 1 with "invalid module name" (no path traversal) | Asserted by `scaffold_rejects_module_name_path_traversal` + `commands::scaffold::tests`; keep the guard first in both entry points |
 
 ## Reviewer Checklist
 

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,7 +7,13 @@ use crate::config::load_config;
 use crate::generator;
 use crate::registry;
 
+use super::validate_module_name;
+
 pub fn cmd_add_spec(root: &Path, module_name: &str) {
+    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);
@@ -184,6 +190,10 @@ pub fn cmd_scaffold(
     dir: Option<PathBuf>,
     template: Option<PathBuf>,
 ) {
+    if let Err(e) = validate_module_name(module_name) {
+        eprintln!("{e}");
+        process::exit(1);
+    }
     let config = load_config(root);
     let specs_dir = dir.unwrap_or_else(|| root.join(&config.specs_dir));
     let spec_dir = specs_dir.join(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,6 +5483,37 @@ fn scaffold_auto_detects_single_source_file() {
     );
 }
 
+/// `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 ["new", "add-spec", "scaffold"] {
+        let output = specsync()
+            .args([sub, "../../escape/evil", "--root", root.to_str().unwrap()])
+            .output()
+            .unwrap();
+        assert!(
+            !output.status.success(),
+            "`{sub}` with a traversal module name must fail loud"
+        );
+        let stderr = String::from_utf8_lossy(&output.stderr);
+        assert!(
+            stderr.contains("invalid module name"),
+            "`{sub}` should report an invalid module name; stderr: {stderr}"
+        );
+    }
+
+    // The guard runs before any filesystem write, so nothing escaped the project root.
+    assert!(
+        !root.parent().unwrap().join("escape").exists(),
+        "a spec/companion escaped the project root"
+    );
+}
+
 /// The original repro: a spec with a warning gets recorded in the hash cache,
 /// then `check --fix` (without --force) reports "All specs unchanged" and
 /// fixes nothing. An explicit --fix must never be a silent no-op.