fix(#4652): a todo name must be a basename — containment alone cannot say that
The verification checkpoint caught a real design gap, not a flaky test. Confining sourcePath/targetPath within todosRoot correctly rejects `../../escaped`, which leaves the root. It does NOT reject these, because they all land inside it: ../sibling.md -> <todosRoot>/sibling.md escapes pending/, not the root a/../../b.md -> <todosRoot>/b.md same sub/name.md -> <pendingDir>/sub/name.md inside pending/, but nested Three committed tests asserted these must be rejected and were right: the design says "a todo name is a basename, not a path", and #4327 requires the resolved path stay inside "the todos root (pending and completed subdirs)". Containment against a root is structurally incapable of expressing "basename" — it answers "is this inside?", and all three are. The wrong tool was reaching for the wrong question. A basename guard now runs BEFORE any path is joined: reject on a `/` or `\` separator, on a path.basename / path.win32.basename mismatch, on `.` / `..`, and on a NUL byte. Both separators are checked explicitly because on POSIX a literal backslash is an ordinary filename character to path.basename but not to path.win32.basename or to the user's intent — this repo has a documented bug class for exactly that asymmetry. Same predicate shape as findPhaseArtifact in check-command-router.cts, so the two agree. Containment is kept as defense-in-depth rather than replaced. The basename guard is the specific rule; containment is the backstop. Message wording matters here and is deliberate: `sub/name.md` does NOT escape its allowed directory, so reusing the escape message would have stated something false. It now says the name must be a plain filename, not a path. docs/CLI-TOOLS.md corrected again, in the opposite direction from last time. The previous revision said an absolute filename is "folded under the root" and 404s — true then, false now: the basename guard rejects it before any join happens. Two corrections to one paragraph in one phase is the cost of documenting behavior while it is still moving; the paragraph now matches the shipped code. Verified through the real CLI, not by calling the built function directly: all eight rejection cases produce the new USAGE message; `ok.md` still completes and moves to completed/; `missing.md` still gives "Todo not found". Also corrected the now-stale comment above the isFile() check — it described `.`/`..` reaching that line, which the basename guard now prevents. The check itself stays: a bare basename can still name a directory, FIFO or socket in pending/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1177,18 +1177,19 @@ from `todos/pending/` to `todos/completed/` and upserts `completed:` and
|
||||
`status: completed` inside the file's frontmatter block. Unknown flags are
|
||||
rejected loudly.
|
||||
|
||||
`<filename>` is a **basename inside the todos root**, not a path. A value that
|
||||
resolves outside that root — a traversal like `../../escaped` or an embedded
|
||||
separator like `sub/name.md` — is rejected as a usage error **before** any file
|
||||
is read or moved (#4327). An absolute path is handled differently: it is
|
||||
**folded under the todos root** (Node's `path.join` does not let a later
|
||||
absolute segment escape a prior one), so it cannot reach a file outside the
|
||||
root — it simply fails with the ordinary "Todo not found" error unless a file
|
||||
of that joined name happens to exist under `todos/pending/`; it is not
|
||||
rejected as a containment violation. The check covers both halves of the move,
|
||||
so neither the source nor the destination can land outside the root, and
|
||||
`--dry-run` is rejected on the same terms rather than previewing a resolved
|
||||
outside path.
|
||||
`<filename>` is a **basename inside the todos root**, not a path. A basename
|
||||
guard runs first, before `<filename>` is joined onto any directory: a value
|
||||
containing an embedded separator (either `/` or `\`, e.g. `sub/name.md` or
|
||||
`sub\name.md`), a value whose own basename differs from itself (e.g.
|
||||
`a/../../b.md`, `../sibling.md`), a bare `.` or `..`, an absolute path (e.g.
|
||||
`/etc/passwd`), or a NUL byte is rejected as a usage error **before** any file
|
||||
is read or moved (#4327, #4652). A traversal that only escapes the `pending`/
|
||||
`completed` subdirectory without leaving the todos root (`../sibling.md`) is
|
||||
caught by this same guard, not by containment. Containment against the todos
|
||||
root still runs afterward as defense-in-depth for the resolved source and
|
||||
target paths, so neither half of the move can land outside the root. The
|
||||
check covers both halves of the move, and `--dry-run` is rejected on the same
|
||||
terms rather than previewing a resolved outside path.
|
||||
|
||||
```bash
|
||||
# UAT audit — scan all phases for unresolved items
|
||||
|
||||
Reference in New Issue
Block a user