diff --git a/src/cli.js b/src/cli.js index 9a0a914..638c469 100755 --- a/src/cli.js +++ b/src/cli.js @@ -12,7 +12,7 @@ function fail(message) { function parseId(todos, arg) { const id = Number(arg); - if (!/^\d+$/.test(arg ?? "") || !Number.isSafeInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`); + if (!/^[1-9]\d*$/.test(arg ?? "") || !Number.isSafeInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`); if (!has(todos, id)) fail(`No todo with id ${id}`); return id; } @@ -30,7 +30,11 @@ switch (cmd) { case "add": { const text = args.join(" ").trim(); if (!text) fail(`Missing todo text\n${USAGE}`); - todos = add(todos, text); + try { + todos = add(todos, text); + } catch (err) { + fail(err.message); + } break; } case "done": diff --git a/src/todo.js b/src/todo.js index 3cb1518..5d3f754 100644 --- a/src/todo.js +++ b/src/todo.js @@ -4,22 +4,12 @@ import { import { dirname, resolve } from "node:path"; import { randomBytes } from "node:crypto"; -// Ids are positive and stay below MAX_SAFE_INTEGER so add()'s max + 1 is always exact. +// Ids are positive and below MAX_SAFE_INTEGER, so max + 1 in add() is always exact. const isId = (id) => Number.isSafeInteger(id) && id > 0 && id < Number.MAX_SAFE_INTEGER; const isTodo = (t) => t !== null && typeof t === "object" && isId(t.id) && typeof t.text === "string" && typeof t.done === "boolean"; -// Follow symlinks by hand so a dangling link resolves to the file it points at. -function resolveLinks(path) { - let target = path; - for (let hops = 0; lstatSync(target, { throwIfNoEntry: false })?.isSymbolicLink(); hops++) { - if (hops === 40) throw new Error("too many levels of symbolic links"); - target = resolve(dirname(target), readlinkSync(target)); - } - return target; -} - export function load(path) { if (!existsSync(path)) return []; let todos; @@ -37,18 +27,27 @@ export function load(path) { return todos; } -// Write a uniquely named temp file next to the real file (following symlinks), copy over -// its permissions, then rename it into place, so an interrupted write never truncates it. -// A file the user can't write (e.g. chmod 444) is refused, as a plain write would be. +// Atomic save: write a uniquely named temp file next to the real file, then rename it into +// place, so an interrupted write never truncates the list. +// - Symlinks are followed by hand, so a dangling link gets its target created, not replaced. +// - The temp file is created with the real file's permissions, so a private list is never +// readable by others, even mid-write. A file the user can't write (chmod 444) is refused. +// - Trade-offs of renaming: the directory must be writable, hard links to the old file are +// not updated, and the new file is owned by whoever ran the save. export function save(path, todos) { let tmp; try { - const target = resolveLinks(path); - const exists = existsSync(target); - if (exists) accessSync(target, constants.W_OK); + let target = path; + for (let hops = 0; lstatSync(target, { throwIfNoEntry: false })?.isSymbolicLink(); hops++) { + if (hops === 40) throw new Error("too many levels of symbolic links"); + target = resolve(dirname(target), readlinkSync(target)); + } + const stat = statSync(target, { throwIfNoEntry: false }); + if (stat) accessSync(target, constants.W_OK); + const mode = stat ? stat.mode & 0o777 : 0o666; tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`; - writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n"); - if (exists) chmodSync(tmp, statSync(target).mode); + writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n", { mode, flag: "wx" }); + if (stat) chmodSync(tmp, mode); // undo the umask, so the mode matches exactly renameSync(tmp, target); } catch (err) { if (tmp) rmSync(tmp, { force: true }); @@ -58,6 +57,7 @@ export function save(path, todos) { export function add(todos, text) { const id = todos.reduce((max, t) => Math.max(max, t.id), 0) + 1; + if (!isId(id)) throw new Error("No ids left: the highest id is already the largest allowed"); return [...todos, { id, text, done: false }]; } diff --git a/test/cli.test.js b/test/cli.test.js index 350b05a..82d60d2 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -25,6 +25,30 @@ test("ls with no file prints the empty message and writes nothing", () => { assert.equal(existsSync(file), false); }); +test("no command lists the todos", () => { + const file = tempFile(); + run(file, "add", "a"); + const res = run(file); + assert.equal(res.status, 0); + assert.equal(res.stdout.trim(), "[ ] 1. a"); +}); + +test("add trims surrounding whitespace from the text", () => { + const file = tempFile(); + run(file, "add", " buy", "milk "); + assert.deepEqual(JSON.parse(readFileSync(file, "utf8")), [{ id: 1, text: "buy milk", done: false }]); +}); + +test("add fails readably when no ids are left, without saving", () => { + const file = tempFile(); + const before = JSON.stringify([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }]); + writeFileSync(file, before); + const res = run(file, "add", "b"); + assert.equal(res.status, 1); + assert.match(res.stderr, /^No ids left/); + assert.equal(readFileSync(file, "utf8"), before); +}); + test("add with no text fails without saving", () => { const file = tempFile(); const res = run(file, "add", " "); @@ -42,6 +66,8 @@ for (const [label, args, message] of [ ["padded id", [" 1 "], /Invalid id: 1 /], ["empty id", [""], /Invalid id: \n/], ["unsafe id", ["99999999999999999999"], /Invalid id: 99999999999999999999/], + ["leading-zero id", ["01"], /Invalid id: 01/], + ["zero id", ["0"], /Invalid id: 0/], ["unknown id", ["99"], /No todo with id 99/], ]) { for (const cmd of ["done", "rm"]) { diff --git a/test/todo.test.js b/test/todo.test.js index ea23327..ff67b4d 100644 --- a/test/todo.test.js +++ b/test/todo.test.js @@ -43,6 +43,10 @@ test("load returns an empty list for a missing file", () => { assert.deepEqual(load(tempFile()), []); }); +test("add refuses an id that load() would reject", () => { + assert.throws(() => add([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }], "b"), /No ids left/); +}); + test("add handles a very large list", () => { const todos = Array.from({ length: 200_000 }, (_, i) => ({ id: i + 1, text: "x", done: false })); assert.equal(add(todos, "y").at(-1).id, 200_001);