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 <noreply@anthropic.com>

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;
 }

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}`);
   }
 }

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 {

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}]`);