Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions specs/cmd_new/cmd_new.spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions specs/cmd_scaffold/cmd_scaffold.spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
4 changes: 3 additions & 1 deletion specs/cmd_scaffold/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |

Expand All @@ -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

Expand Down
2 changes: 1 addition & 1 deletion specs/cmd_wizard/cmd_wizard.spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
1 change: 1 addition & 0 deletions specs/commands/commands.spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
89 changes: 89 additions & 0 deletions src/commands/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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());
}
}
6 changes: 6 additions & 0 deletions src/commands/new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 10 additions & 0 deletions src/commands/scaffold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/commands/wizard.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
31 changes: 31 additions & 0 deletions tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading