From 089fc94e16538c5def7ad4005f846c488e3acaaa Mon Sep 17 00:00:00 2001 From: christian-westerkam Date: Wed, 21 Oct 2026 00:33:46 +0200 Subject: [PATCH] Address review round 2: duplicate ids, read-only files, dangling links, safe ids - load() rejects duplicate ids (left by the old add bug) and ids that aren't safe positive integers - save() refuses files the user can't write, and follows dangling symlinks to their target - CLI rejects ids beyond Number.MAX_SAFE_INTEGER - Skip permission-based tests when running as root Co-Authored-By: Claude Opus 5.5 --- src/cli.js | 2 +- src/todo.js | 34 ++++++++++++++++++++++++++++------ test/cli.test.js | 3 ++- test/todo.test.js | 36 ++++++++++++++++++++++++++++++++++-- 4 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/cli.js b/src/cli.js index bdcc4ba..9a0a914 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 (!/^\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; } diff --git a/src/todo.js b/src/todo.js index b1a4a65..3cb1518 100644 --- a/src/todo.js +++ b/src/todo.js @@ -1,8 +1,24 @@ -import { readFileSync, writeFileSync, renameSync, existsSync, realpathSync, statSync, chmodSync, rmSync } from "node:fs"; +import { + readFileSync, writeFileSync, renameSync, existsSync, lstatSync, readlinkSync, statSync, chmodSync, accessSync, rmSync, constants, +} from "node:fs"; +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. +const isId = (id) => Number.isSafeInteger(id) && id > 0 && id < Number.MAX_SAFE_INTEGER; + const isTodo = (t) => - t !== null && typeof t === "object" && Number.isInteger(t.id) && typeof t.text === "string" && typeof t.done === "boolean"; + 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 []; @@ -15,21 +31,27 @@ export function load(path) { 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`); + const seen = new Set(); + const dup = todos.find((t) => seen.size === seen.add(t.id).size); + if (dup) throw new Error(`Could not read ${path}: id ${dup.id} is used more than once; renumber it by hand`); 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. export function save(path, todos) { - const exists = existsSync(path); - const target = exists ? realpathSync(path) : path; - const tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`; + let tmp; try { + const target = resolveLinks(path); + const exists = existsSync(target); + if (exists) accessSync(target, constants.W_OK); + tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`; writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n"); if (exists) chmodSync(tmp, statSync(target).mode); renameSync(tmp, target); } catch (err) { - rmSync(tmp, { force: true }); + if (tmp) rmSync(tmp, { force: true }); throw new Error(`Could not save ${path}: ${err.message}`); } } diff --git a/test/cli.test.js b/test/cli.test.js index 46ead6c..350b05a 100644 --- a/test/cli.test.js +++ b/test/cli.test.js @@ -41,6 +41,7 @@ for (const [label, args, message] of [ ["decimal id", ["1.0"], /Invalid id: 1\.0/], ["padded id", [" 1 "], /Invalid id: 1 /], ["empty id", [""], /Invalid id: \n/], + ["unsafe id", ["99999999999999999999"], /Invalid id: 99999999999999999999/], ["unknown id", ["99"], /No todo with id 99/], ]) { for (const cmd of ["done", "rm"]) { @@ -64,7 +65,7 @@ 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", () => { +test("a failed save gives a readable error instead of a stack trace", { skip: process.getuid?.() === 0 && "root ignores file permissions" }, () => { const file = tempFile(); chmodSync(dirname(file), 0o500); try { diff --git a/test/todo.test.js b/test/todo.test.js index 1ddeea8..ea23327 100644 --- a/test/todo.test.js +++ b/test/todo.test.js @@ -6,6 +6,7 @@ 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"); +const isRoot = process.getuid?.() === 0; test("add appends an open todo", () => { const todos = add([], "buy milk"); @@ -65,6 +66,29 @@ test("save writes through a symlink instead of replacing it", () => { assert.deepEqual(load(real).map((t) => t.text), ["a"]); }); +test("load rejects duplicate ids left by the old add() bug", () => { + const file = tempFile(); + writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }])); + assert.throws(() => load(file), /id 2 is used more than once/); +}); + +test("save through a dangling symlink creates the target and keeps the link", () => { + const dir = dirname(tempFile()); + const link = join(dir, "link.json"); + symlinkSync("real.json", link); + save(link, add([], "a")); + assert.equal(lstatSync(link).isSymbolicLink(), true); + assert.deepEqual(load(join(dir, "real.json")).map((t) => t.text), ["a"]); +}); + +test("save refuses a read-only file", { skip: isRoot && "root ignores file permissions" }, () => { + const file = tempFile(); + save(file, []); + chmodSync(file, 0o444); + assert.throws(() => save(file, add([], "a")), /Could not save .*todos\.json/); + assert.deepEqual(load(file), []); +}); + test("save keeps the file's permissions", () => { const file = tempFile(); save(file, []); @@ -73,7 +97,7 @@ test("save keeps the file's permissions", () => { assert.equal(statSync(file).mode & 0o777, 0o600); }); -test("save reports a readable error and cleans up when it can't write", () => { +test("save reports a readable error and cleans up when it can't write", { skip: isRoot && "root ignores file permissions" }, () => { const file = tempFile(); chmodSync(dirname(file), 0o500); try { @@ -96,7 +120,15 @@ test("load rejects JSON that is not a list", () => { 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}']) { +for (const entry of [ + "null", + "1", + '{"text":"a","done":false}', + '{"id":"1","text":"a","done":false}', + '{"id":0,"text":"a","done":false}', + '{"id":-1,"text":"a","done":false}', + `{"id":${Number.MAX_SAFE_INTEGER},"text":"a","done":false}`, +]) { test(`load rejects an invalid entry: ${entry}`, () => { const file = tempFile(); writeFileSync(file, `[${entry}]`);