fix(sessions): refuse a traversal id before the recursive delete runs

`deleteSession` ends in `rm -rf <root>/<id>`, and the id is a string the
caller was handed. `..` passes the `^[a-zA-Z0-9._-]+$` check that both
this path and the thread store rely on — it is spelled entirely in
characters an id may legitimately contain — and `join(root, '..')` is the
directory above the sessions root.

Every in-tree caller validates first: `ProtocolRuntime.deleteThread`
404s on a thread that does not exist, and `CanonicalThreadStore.delete`
runs `FileThreadStore.delete` (which validates) before the session
projection. So this is not reachable today. It is also a recursive
delete resolved from an untrusted string, one refactor away from being
the only check, and AGENTS.md asks for adversarial tests exactly here.

`deleteSession` now rejects a malformed id before removing anything, and
`validThreadId` stops admitting `.` and `..` — harmless while every path
built from an id had a suffix appended, not harmless now that one
reaches a directory removal.

The fixture nests its sessions root deep enough that every hostile id
still resolves inside the temp directory: confirming this bug by
deleting the guard walked `rm -rf` up into $TMPDIR, which is not
something a test should be able to do by accident.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: oratis

CHANGELOG.md

@@ -29,7 +29,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   existing `threadManagement` capability, with the local writer kept as the
   fallback for a sidecar too old to know the method.
 
-
 ### 🔒 Security
 
 - **A sub-agent did not inherit the file contract.** The `Task` delegation
@@ -60,6 +59,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   thread removes both its protocol snapshot and its canonical session
   projection: `list` reads both, so removing one left the row reappearing on the
   next refresh as an empty session that could not be opened.
+- `deleteSession` refuses a session id that is not a single path segment, before
+  removing anything. It ends in a recursive delete of `<root>/<id>`, and `..` —
+  the id that resolves to the directory _above_ the sessions root — is spelled
+  entirely in characters an id may legitimately contain, so the character-class
+  check both it and the thread store relied on admitted it. Every in-tree caller
+  validates first; a delete this destructive should not depend on that.
 - **`Grep` over a single file no longer prefixes every line with a colon.**
   ripgrep omits the filename when the search path is one _file_ — there is
   nothing to disambiguate — so its `--null` output carries no NUL, and rejoining

apps/server/src/store.ts

@@ -9,6 +9,11 @@ import type { ThreadSnapshot, ThreadStore } from '@deepcode/protocol';
 import { historyFromThread } from './runtime-executor.js';
 
 function validThreadId(threadId: string): boolean {
+  // `.` and `..` are spelled entirely in characters an id may legitimately
+  // contain, so the class alone admits them. Harmless while every path built
+  // from an id had a suffix appended; not harmless now that `delete` reaches a
+  // recursive removal of `<root>/<id>`.
+  if (threadId === '.' || threadId === '..') return false;
   return /^[a-zA-Z0-9._-]+$/.test(threadId);
 }
 

packages/core/src/sessions/storage.test.ts

@@ -1,9 +1,10 @@
-import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises';
+import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
 import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import {
   appendMessage,
+  deleteSession,
   listSessions,
   newSessionId,
   readMessages,
@@ -259,3 +260,56 @@ describe('session storage', () => {
     await expect(readFile(files.jsonlPath, 'utf8')).rejects.toThrow();
   });
 });
+
+// `deleteSession` ends in a recursive `rm` of `<root>/<id>`, so the id is a path
+// segment resolved from a string the caller received. AGENTS.md asks for
+// adversarial tests where a mistake destroys user work; this is that place.
+describe('deleteSession', () => {
+  let root: string;
+  beforeEach(async () => {
+    root = await mkdtemp(join(tmpdir(), 'deepcode-delete-'));
+  });
+  afterEach(async () => {
+    await rm(root, { recursive: true, force: true });
+  });
+
+  it('removes every file the session owns', async () => {
+    const id = newSessionId();
+    await writeMeta(root, { id, cwd: '/w', createdAt: 'x', updatedAt: 'x' });
+    await appendMessage(root, id, { role: 'user', content: [{ type: 'text', text: 'hi' }] });
+    const files = sessionFiles(root, id);
+
+    await deleteSession(root, id);
+
+    await expect(readFile(files.metaPath, 'utf8')).rejects.toThrow();
+    await expect(readFile(files.jsonlPath, 'utf8')).rejects.toThrow();
+    expect(await listSessions(root)).toHaveLength(0);
+  });
+
+  it('is not an error when the files are already gone', async () => {
+    await expect(deleteSession(root, newSessionId())).resolves.toBeUndefined();
+  });
+
+  it.each(['..', '.', '', '../..', 'a/b', '../escape'])(
+    'refuses the id %j instead of resolving it into a path',
+    async (id) => {
+      // `..` is the one that matters: it is spelled entirely in characters an
+      // id may legitimately contain, so a character-class check alone admits
+      // it — and `join(sessionsRoot, '..')` is the directory above it.
+      //
+      // The sessions root is nested deep enough that every id here still
+      // resolves inside `root`. Without that, a run with the guard removed
+      // walks `rm -rf` up out of the fixture and into the system temp
+      // directory — which is how this was confirmed, and not something a test
+      // should be able to do by accident.
+      const sessionsRoot = join(root, 'deep', 'deeper', 'sessions');
+      await mkdir(sessionsRoot, { recursive: true });
+      const guarded = join(root, 'keep-me.txt');
+      await writeFile(guarded, 'not mine to delete', 'utf8');
+
+      await expect(deleteSession(sessionsRoot, id)).rejects.toThrow(/invalid id/);
+
+      expect(await readFile(guarded, 'utf8')).toBe('not mine to delete');
+    },
+  );
+});

packages/core/src/sessions/storage.ts

@@ -68,6 +68,24 @@ export interface SessionFiles {
   snapshotsDir: string;
 }
 
+/**
+ * A session id that is safe to interpolate into a path.
+ *
+ * `deleteSession` removes a whole directory recursively, so the id has to be a
+ * single path segment and cannot be a traversal. `..` is the one that matters:
+ * it is composed entirely of characters an id may legitimately contain, so a
+ * character-class check alone lets it through — and `join(root, '..')` is the
+ * parent of the sessions root.
+ */
+function validSessionId(sessionId: string): boolean {
+  return (
+    sessionId !== '' &&
+    sessionId !== '.' &&
+    sessionId !== '..' &&
+    /^[a-zA-Z0-9._-]+$/.test(sessionId)
+  );
+}
+
 /**
  * Irreversibly remove every file belonging to one session.
  *
@@ -79,8 +97,16 @@ export interface SessionFiles {
  * Missing files are not an error. The caller has already established that the
  * session exists, and a delete that fails partway through because one of five
  * paths was already gone leaves the user unable to finish it.
+ *
+ * A malformed id *is* an error, and is refused before anything is removed. The
+ * callers in-tree all validate first, so this can only fire on a programming
+ * mistake — which is exactly when a recursive delete resolved from an untrusted
+ * string must not proceed on the strength of somebody else having checked.
  */
 export async function deleteSession(root: string, sessionId: string): Promise<void> {
+  if (!validSessionId(sessionId)) {
+    throw new Error(`Refusing to delete session with invalid id: ${JSON.stringify(sessionId)}`);
+  }
   const files = sessionFiles(root, sessionId);
   for (const path of [
     files.jsonlPath,