Fix: reject path-traversal module names in add-spec/scaffold - #316
Conversation
`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
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
✅ Corvin says...
_
<(^\ .oO(Caw! ^v^)
|/(\
\(\\
" "\\
"Caw! Your code sparkles like a dropped french fry."
CI Summary
| Check | Status |
|---|---|
| Validate action.yml | ✅ Passed |
| Dependency Audit | ✅ Passed |
| Code Coverage | ✅ Passed |
| Format Check | ✅ Passed |
| Docs Site | ✅ Passed |
| Spec Validation | ✅ Passed |
| Tests (build, test, clippy) | ✅ Passed |
| VS Code Extension | ✅ Passed |
📋 Spec Validation Details
✅ SpecSync: Passed
| Metric | Value |
|---|---|
| Specs checked | 60 |
| Passed | 60 |
| Errors | 0 |
| Warnings | 0 |
| File coverage | 100% (76/76) |
| LOC coverage | 100% (37814/37814) |
Generated by specsync · Run specsync check --format github to reproduce
Powered by corvid-pet
…ation 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
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
✅ Corvin says...
_
<(^\ .oO(Caw! ^v^)
|/(\
\(\\
" "\\
"Caw! Your code sparkles like a dropped french fry."
CI Summary
| Check | Status |
|---|---|
| Validate action.yml | ✅ Passed |
| Dependency Audit | ✅ Passed |
| Code Coverage | ✅ Passed |
| Format Check | ✅ Passed |
| Docs Site | ✅ Passed |
| Spec Validation | ✅ Passed |
| Tests (build, test, clippy) | ✅ Passed |
| VS Code Extension | ✅ Passed |
📋 Spec Validation Details
✅ SpecSync: Passed
| Metric | Value |
|---|---|
| Specs checked | 60 |
| Passed | 60 |
| Errors | 0 |
| Warnings | 0 |
| File coverage | 100% (76/76) |
| LOC coverage | 100% (37851/37851) |
Generated by specsync · Run specsync check --format github to reproduce
Powered by corvid-pet
|
Re-review after fixes: READY, 90%, zero blocking. Both prior blocking issues confirmed fixed:
All four scaffolding entry points ( Accepted follow-ups (non-blocking, no action needed here):
|
Medium-tier audit finding (#14) — a path traversal in spec scaffolding.
Problem
cmd_add_specandcmd_scaffoldwrote the user-suppliedmodule_nameverbatim 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 root:The spec file and its companion directory landed outside the project root. Path traversal writing files to arbitrary locations.
Fix
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 andexit 1, before any filesystem write. Both entry points are gated. Legitimate names (auth,auth-service,user_profile) are unaffected.Tests
validate_module_nameaccepts plain names, rejects separators /../ absolute / empty.scaffold_rejects_module_name_path_traversal— bothadd-specandscaffoldwith../../escape/evilfail loud with "invalid module name" and write nothing outside the project root.cmd_scaffold.spec.md, updated testing.md.cargo fmt --checkclean,cargo clippy --bin specsync -- -D warningsclean, self-check 60/60 at 100%.🤖 Generated with Claude Code