From c7956c162b0c7d3cb842d636b2603ce8c2066031 Mon Sep 17 00:00:00 2001 From: sim Date: Sat, 12 Sep 2026 10:07:40 -0400 Subject: [PATCH] =?UTF-8?q?fix(#4652):=20a=20todo=20name=20must=20be=20a?= =?UTF-8?q?=20basename=20=E2=80=94=20containment=20alone=20cannot=20say=20?= =?UTF-8?q?that?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 -> /sibling.md escapes pending/, not the root a/../../b.md -> /b.md same sub/name.md -> /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 --- docs/CLI-TOOLS.md | 25 +++++++++++++------------ src/commands.cts | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 44 insertions(+), 16 deletions(-) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index ff979dbf0..0d863d6fe 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -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. -`` 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. +`` is a **basename inside the todos root**, not a path. A basename +guard runs first, before `` 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 diff --git a/src/commands.cts b/src/commands.cts index 362fffd3b..663e02b39 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -3490,6 +3490,30 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod const todosRoot = todosDir(cwd); const pendingDir = path.join(todosRoot, 'pending'); const completedDir = path.join(todosRoot, 'completed'); + + // #4652: containment against todosRoot only rejects paths that leave the + // root — it cannot express "a todo name is a basename, not a path" (see + // #4327). `../sibling.md`, `a/../../b.md`, and `sub/name.md` all resolve + // to a location inside todosRoot (or inside pending/) and would pass + // containment, yet none of them is a bare filename. Reject on basename + // shape FIRST, before any path is even joined — same predicate shape as + // findPhaseArtifact in check-command-router.cts. Checking both `/` and + // `\` explicitly (not just path.basename) matters on POSIX, where a + // literal backslash is just an ordinary filename character to + // path.basename but not to path.win32.basename or to the user's intent. + const rawFilename = filename as string; + if ( + rawFilename === '.' || + rawFilename === '..' || + rawFilename.includes('\0') || + rawFilename.includes('/') || + rawFilename.includes('\\') || + path.basename(rawFilename) !== rawFilename || + path.win32.basename(rawFilename) !== rawFilename + ) { + error(`todo name must be a plain filename inside the pending directory, not a path: ${rawFilename}`, ERROR_REASON.USAGE); + } + const sourcePath = path.join(pendingDir, filename as string); const targetPath = path.join(completedDir, filename as string); @@ -3509,10 +3533,13 @@ function cmdTodoComplete(cwd: string, filename: string | undefined, options: Tod error(`Todo not found: ${filename as string}`); } - // #4652: `.` and `..` resolve to the pending dir itself (which IS inside - // todosRoot, so containment passes) but are not a todo file — reject them - // the same way as any other invalid name instead of letting - // fs.readFileSync throw an uncaught EISDIR with an absolute-path stack trace. + // #4652: a name that IS a bare basename can still resolve to something that + // is not a regular file — a directory, symlink-to-directory, FIFO or socket + // sitting in pending/ under an ordinary-looking name. `.` and `..` no longer + // reach here (the basename guard above rejects them first), so this is not + // about traversal; it stops fs.readFileSync from throwing an uncaught EISDIR + // with an absolute-path stack trace where every sibling case gives a clean + // USAGE rejection. if (!fs.statSync(resolvedSource).isFile()) { error(`todo name is not a file: ${filename as string}`, ERROR_REASON.USAGE); }