diff --git a/.gitignore b/.gitignore index 285cf65..605e1fb 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ node_modules/ todos.json +*.tmp .DS_Store # Personal Claude Code overrides (not shared) diff --git a/src/cli.js b/src/cli.js index 85d7f28..bdcc4ba 100755 --- a/src/cli.js +++ b/src/cli.js @@ -11,8 +11,8 @@ function fail(message) { } function parseId(todos, arg) { + if (!/^\d+$/.test(arg ?? "")) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`); const id = Number(arg); - if (arg === undefined || !Number.isInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`); if (!has(todos, id)) fail(`No todo with id ${id}`); return id; } @@ -41,5 +41,11 @@ switch (cmd) { break; } -if (cmd !== "ls") save(FILE, todos); +if (cmd !== "ls") { + try { + save(FILE, todos); + } catch (err) { + fail(err.message); + } +} console.log(format(todos)); diff --git a/src/todo.js b/src/todo.js index c9e6e94..b1a4a65 100644 --- a/src/todo.js +++ b/src/todo.js @@ -1,4 +1,8 @@ -import { readFileSync, writeFileSync, renameSync, existsSync } from "node:fs"; +import { readFileSync, writeFileSync, renameSync, existsSync, realpathSync, statSync, chmodSync, rmSync } from "node:fs"; +import { randomBytes } from "node:crypto"; + +const isTodo = (t) => + t !== null && typeof t === "object" && Number.isInteger(t.id) && typeof t.text === "string" && typeof t.done === "boolean"; export function load(path) { if (!existsSync(path)) return []; @@ -9,18 +13,29 @@ export function load(path) { throw new Error(`Could not read ${path}: ${err.message}`); } if (!Array.isArray(todos)) throw new Error(`Could not read ${path}: expected a list of todos`); + const bad = todos.findIndex((t) => !isTodo(t)); + if (bad !== -1) throw new Error(`Could not read ${path}: entry ${bad + 1} is not a valid todo`); return todos; } -// Write to a temp file and rename, so an interrupted write never leaves `path` truncated. +// 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. export function save(path, todos) { - const tmp = `${path}.tmp`; - writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n"); - renameSync(tmp, path); + const exists = existsSync(path); + const target = exists ? realpathSync(path) : path; + const tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`; + try { + writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n"); + if (exists) chmodSync(tmp, statSync(target).mode); + renameSync(tmp, target); + } catch (err) { + rmSync(tmp, { force: true }); + throw new Error(`Could not save ${path}: ${err.message}`); + } } export function add(todos, text) { - const id = Math.max(0, ...todos.map((t) => t.id)) + 1; + const id = todos.reduce((max, t) => Math.max(max, t.id), 0) + 1; return [...todos, { id, text, done: false }]; } diff --git a/test/cli.test.js b/test/cli.test.js index 4e73029..46ead6c 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -1,9 +1,9 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { mkdtempSync, writeFileSync, readFileSync, existsSync } from "node:fs"; +import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; const CLI = fileURLToPath(new URL("../src/cli.js", import.meta.url)); @@ -36,6 +36,11 @@ test("add with no text fails without saving", () => { for (const [label, args, message] of [ ["missing id", [], /Invalid id: \(missing\)/], ["non-numeric id", ["abc"], /Invalid id: abc/], + ["hex id", ["0x1"], /Invalid id: 0x1/], + ["exponent id", ["1e0"], /Invalid id: 1e0/], + ["decimal id", ["1.0"], /Invalid id: 1\.0/], + ["padded id", [" 1 "], /Invalid id: 1 /], + ["empty id", [""], /Invalid id: \n/], ["unknown id", ["99"], /No todo with id 99/], ]) { for (const cmd of ["done", "rm"]) { @@ -59,6 +64,19 @@ test("unknown command prints usage even when the file is corrupt", () => { assert.match(res.stderr, /Unknown command: --help\nUsage:/); }); +test("a failed save gives a readable error instead of a stack trace", () => { + const file = tempFile(); + chmodSync(dirname(file), 0o500); + try { + const res = run(file, "add", "a"); + assert.equal(res.status, 1); + assert.match(res.stderr, /^Could not save /); + assert.doesNotMatch(res.stderr, /at .*\.js:\d+/); + } finally { + chmodSync(dirname(file), 0o700); + } +}); + test("a corrupt file gives a readable error instead of a stack trace", () => { const file = tempFile(); writeFileSync(file, "{ not json"); diff --git a/test/todo.test.js b/test/todo.test.js index 04beccd..1ddeea8 100644 --- a/test/todo.test.js +++ b/test/todo.test.js @@ -1,8 +1,8 @@ import { test } from "node:test"; import assert from "node:assert/strict"; -import { mkdtempSync, writeFileSync, existsSync } from "node:fs"; +import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, lstatSync, statSync, chmodSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { join, dirname } from "node:path"; import { load, save, add, has, complete, remove, format } from "../src/todo.js"; const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json"); @@ -42,12 +42,46 @@ test("load returns an empty list for a missing file", () => { assert.deepEqual(load(tempFile()), []); }); +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); +}); + test("save then load round-trips and leaves no temp file", () => { const file = tempFile(); const todos = add([], "a"); save(file, todos); assert.deepEqual(load(file), todos); - assert.equal(existsSync(`${file}.tmp`), false); + assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]); +}); + +test("save writes through a symlink instead of replacing it", () => { + const real = tempFile(); + const link = join(dirname(tempFile()), "link.json"); + save(real, []); + symlinkSync(real, link); + save(link, add([], "a")); + assert.equal(lstatSync(link).isSymbolicLink(), true); + assert.deepEqual(load(real).map((t) => t.text), ["a"]); +}); + +test("save keeps the file's permissions", () => { + const file = tempFile(); + save(file, []); + chmodSync(file, 0o600); + save(file, add([], "a")); + assert.equal(statSync(file).mode & 0o777, 0o600); +}); + +test("save reports a readable error and cleans up when it can't write", () => { + const file = tempFile(); + chmodSync(dirname(file), 0o500); + try { + assert.throws(() => save(file, []), /Could not save .*todos\.json/); + assert.deepEqual(readdirSync(dirname(file)), []); + } finally { + chmodSync(dirname(file), 0o700); + } }); test("load rejects invalid JSON with a readable error", () => { @@ -61,3 +95,11 @@ test("load rejects JSON that is not a list", () => { writeFileSync(file, "{}"); assert.throws(() => load(file), /expected a list of todos/); }); + +for (const entry of ["null", "1", '{"text":"a","done":false}', '{"id":"1","text":"a","done":false}']) { + test(`load rejects an invalid entry: ${entry}`, () => { + const file = tempFile(); + writeFileSync(file, `[${entry}]`); + assert.throws(() => load(file), /entry 1 is not a valid todo/); + }); +}