From 76650f9f49228fef3b89afecb3497fdb1316f1e1 Mon Sep 17 00:00:00 2001
From: Ryan McMillan <ryan@twinpines.io>
Date: Wed, 10 Jun 2026 19:49:13 -0500
Subject: [PATCH] Validate task ids and reject dot-segment sync paths

Defense-in-depth on top of encodeURIComponent for sync task-id paths:

- taskPathSegment now rejects "." / ".." / empty ids, which encodeURIComponent
  leaves untouched and URL normalization would otherwise collapse (e.g.
  /tasks/../context -> /context).
- parseTasksJsonl validates any present id against /^[a-f0-9]{32}$/, so a
  hostile .delega/tasks.jsonl is rejected at parse time. Absent ids (new local
  tasks created on push) are still allowed.

Tests cover path-confusing/malformed ids through the parser and dot-segment
ids through taskPathSegment. 7/7 pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 src/commands/sync.ts | 17 ++++++++++++++++-
 test/sync.test.mjs   | 23 +++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/src/commands/sync.ts b/src/commands/sync.ts
index 7ccbbd6..e7bbc43 100644
--- a/src/commands/sync.ts
+++ b/src/commands/sync.ts
@@ -73,8 +73,18 @@ const SYNC_DIR = ".delega";
 const SYNC_CONFIG = "config.json";
 const TASKS_FILE = "tasks.jsonl";
 
+// Server task ids are 32 lowercase hex chars. Locally authored records may have
+// no id yet (created on push), but any id that *is* present must match.
+const TASK_ID_RE = /^[a-f0-9]{32}$/;
+
 export function taskPathSegment(id: string | number): string {
-  return encodeURIComponent(String(id));
+  const encoded = encodeURIComponent(String(id));
+  // encodeURIComponent leaves "." and ".." untouched, so an id of "." or ".."
+  // would still collapse a path via URL normalization. Refuse those outright.
+  if (encoded === "" || encoded === "." || encoded === "..") {
+    throw new Error(`Refusing to build a task path from unsafe id: ${JSON.stringify(String(id))}`);
+  }
+  return encoded;
 }
 
 function isPlainObject(value: unknown): value is Record<string, unknown> {
@@ -113,6 +123,11 @@ export function parseTasksJsonl(raw: string): TaskSyncRecord[] {
       if (typeof parsed.content !== "string" || !parsed.content.trim()) {
         throw new Error(`Line ${index + 1} must include content`);
       }
+      if (parsed.id !== undefined && parsed.id !== null) {
+        if (typeof parsed.id !== "string" || !TASK_ID_RE.test(parsed.id)) {
+          throw new Error(`Line ${index + 1} has an invalid task id in .delega/tasks.jsonl`);
+        }
+      }
       return parsed as unknown as TaskSyncRecord;
     });
 }
diff --git a/test/sync.test.mjs b/test/sync.test.mjs
index bb9b20a..8db6286 100644
--- a/test/sync.test.mjs
+++ b/test/sync.test.mjs
@@ -48,6 +48,21 @@ test("parseTasksJsonl rejects non-object and content-less lines", () => {
   assert.throws(() => parseTasksJsonl('{"id":"a"}\n'), /Line 1 must include content/);
 });
 
+test("parseTasksJsonl rejects path-confusing and malformed task ids", () => {
+  const valid = "a".repeat(32);
+  // A well-formed server id is accepted; absent id is allowed (created on push).
+  assert.equal(parseTasksJsonl(`{"id":"${valid}","content":"ok"}\n`)[0].id, valid);
+  assert.equal(parseTasksJsonl('{"content":"new local task"}\n')[0].id, undefined);
+  // Path-traversal / query-injection ids from a hostile tasks.jsonl are rejected.
+  for (const id of ["..", ".", "../agents", "abc/../def", "task?x=y", "AAAA", ""]) {
+    assert.throws(
+      () => parseTasksJsonl(`${JSON.stringify({ id, content: "x" })}\n`),
+      /Line 1 has an invalid task id/,
+      `id ${JSON.stringify(id)} should be rejected`,
+    );
+  }
+});
+
 test("diffSyncRecords separates local, remote, changed, and clean buckets", () => {
   const diff = diffSyncRecords(
     [
@@ -74,3 +89,11 @@ test("taskPathSegment encodes task IDs before URL path construction", () => {
   assert.equal(taskPathSegment("../agents?x=y"), "..%2Fagents%3Fx%3Dy");
   assert.equal(taskPathSegment("task/with?query=true"), "task%2Fwith%3Fquery%3Dtrue");
 });
+
+test("taskPathSegment refuses dot-segment ids that survive URL encoding", () => {
+  // encodeURIComponent leaves these untouched; URL normalization would then
+  // collapse "/tasks/../context" to "/context", so they must be rejected.
+  for (const id of [".", ".."]) {
+    assert.throws(() => taskPathSegment(id), /unsafe id/);
+  }
+});