fix(plan-canvas): validate feedback IPC locator and request shape

scripts/hooks/plan-canvas-pending.js

@@ -33,6 +33,34 @@ const path = require('path');
 // server is wedged. Falling back to the state file keeps delivery working.
 const SERVER_TIMEOUT_MS = 1000;
 const MAX_ITEMS_REPORTED = 20;
+const SESSION_KEY_PATTERN = /^[a-f0-9]{12}$/;
+const LOOPBACK_HOST = '127.0.0.1';
+
+function validServerPort(port) {
+  return Number.isInteger(port) && port >= 1 && port <= 65535;
+}
+
+function validSessionKey(key) {
+  return typeof key === 'string' && SESSION_KEY_PATTERN.test(key);
+}
+
+// Resolve the Claude profile directory the same way everywhere.
+//
+// Duplicated rather than imported on purpose: hooks ship in managed installs
+// that carry top-level `scripts/` without `scripts/lib/` (see #3259), so a
+// require into lib/ would make the hook unloadable there. `~` is expanded the
+// way `scripts/lib/agent-data-home.js` expands it, so a profile written as
+// `~/.claude-work` resolves to one place rather than two.
+function claudeConfigDir() {
+  const configured = process.env.CLAUDE_CONFIG_DIR;
+  const trimmed = configured ? String(configured).trim() : '';
+  if (!trimmed) return path.join(os.homedir(), '.claude');
+  if (trimmed.startsWith('~')) {
+    const remainder = trimmed.slice(1).replace(/^[/\\]+/, '');
+    return remainder ? path.join(os.homedir(), remainder) : os.homedir();
+  }
+  return path.resolve(trimmed);
+}
 
 // Resolve the Claude profile directory the same way everywhere.
 //
@@ -70,9 +98,11 @@ function readState() {
 function readServerPort() {
   try {
     const info = JSON.parse(fs.readFileSync(path.join(stateDir(), 'server.json'), 'utf8'));
-    return Number.isInteger(info.port) ? info.port : null;
-  } catch {
-    return null;
+    return info && !Array.isArray(info) && validServerPort(info.port) ? info.port : undefined;
+  } catch (error) {
+    // A missing locator permits offline delivery; malformed metadata does not
+    // establish that the server is down, so leave its queue untouched.
+    return error.code === 'ENOENT' ? null : undefined;
   }
 }
 
@@ -89,7 +119,7 @@ function isInside(dir, file) {
 function pendingSessions(state, cwd, env = process.env) {
   const scopeAll = String(env.ECC_PLAN_CANVAS_STOP_SCOPE || '').trim().toLowerCase() === 'all';
   return Object.values((state && state.sessions) || {})
-    .filter(session => session && session.status !== 'ended')
+    .filter(session => session && validSessionKey(session.key) && typeof session.file === 'string' && session.file.length > 0 && session.status !== 'ended')
     .filter(session => Array.isArray(session.pendingFeedback) && session.pendingFeedback.length > 0)
     .filter(session => (scopeAll ? true : isInside(cwd, session.file)))
     .sort((a, b) => String(a.updatedAt || '').localeCompare(String(b.updatedAt || '')));
@@ -101,16 +131,25 @@ function pendingSessions(state, cwd, env = process.env) {
  * makes /api/await return immediately instead of long polling.
  */
 function drainViaServer(port, key) {
+  // Expected local IPC: server.json locates the bound loopback port; the only
+  // file-derived query value is a non-secret 12-hex artifact identifier.
+  // Validate at the HTTP boundary as well as during session selection.
+  if (!validServerPort(port) || !validSessionKey(key)) return Promise.resolve(null);
   return new Promise(resolve => {
     const req = http.request(
       {
-        host: '127.0.0.1',
+        host: LOOPBACK_HOST,
         port,
         method: 'GET',
-        path: `/api/await?key=${encodeURIComponent(key)}&timeoutMs=0`,
+        path: `/api/await?key=${key}&timeoutMs=0`,
         agent: false
       },
       res => {
+        if (res.statusCode !== 200) {
+          res.resume();
+          resolve(null);
+          return;
+        }
         let data = '';
         res.on('data', chunk => {
           data += chunk;
@@ -232,7 +271,9 @@ async function run(rawInput) {
   const sessions = pendingSessions(state, payload.cwd || process.cwd());
   if (sessions.length === 0) return passThrough;
 
-  const delivered = await collectDeliveries(sessions, readServerPort());
+  const port = readServerPort();
+  if (port === undefined) return passThrough;
+  const delivered = await collectDeliveries(sessions, port);
   if (delivered.length === 0) return passThrough;
 
   return {
@@ -241,4 +282,4 @@ async function run(rawInput) {
   };
 }
 
-module.exports = { run, pendingSessions, describeItem, buildReason, drainViaFile };
+module.exports = { run, pendingSessions, describeItem, buildReason, drainViaFile, drainViaServer };

tests/hooks/plan-canvas-pending-hook.test.js

@@ -11,6 +11,9 @@ const assert = require('assert');
 const fs = require('fs');
 const os = require('os');
 const path = require('path');
+const http = require('http');
+const { createSessionStore } = require('../../scripts/lib/plan-canvas/sessions');
+const { createPlanCanvasServer } = require('../../scripts/lib/plan-canvas/server');
 
 const HOOK = path.join(__dirname, '..', '..', 'scripts', 'hooks', 'plan-canvas-pending.js');
 
@@ -177,6 +180,118 @@ async function runTests() {
     assert.strictEqual((await hook.run('{}')).exitCode, 0);
   })) passed++; else failed++;
 
+  if (await test('actual loopback server drains only the current project using an opaque fixed-shape URL', async () => {
+    const root = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'plan-canvas-ipc-')));
+    const stateDir = path.join(root, 'state');
+    const project = path.join(root, 'project');
+    const other = path.join(root, 'other');
+    fs.mkdirSync(project); fs.mkdirSync(other);
+    const artifact = path.join(project, 'plan.md');
+    const otherArtifact = path.join(other, 'plan.md');
+    fs.writeFileSync(artifact, '# Plan'); fs.writeFileSync(otherArtifact, '# Other');
+    const store = createSessionStore({ stateDir });
+    const own = store.open(artifact).session;
+    const foreign = store.open(otherArtifact).session;
+    store.queueFeedback(own.key, [{ kind: 'chat', text: 'OWN_FEEDBACK' }]);
+    store.queueFeedback(foreign.key, [{ kind: 'chat', text: 'OTHER_FEEDBACK' }]);
+    const canvas = createPlanCanvasServer({ store, idleTimeoutMs: 0 });
+    const requests = [];
+    canvas.server.prependListener('request', req => requests.push({ path: req.url, host: req.headers.host }));
+    let closed = false;
+    try {
+      const bound = await canvas.listen(0);
+      assert.ok(bound.port > 0, 'ephemeral listen0 is recorded as its actual assigned port');
+      fs.writeFileSync(path.join(stateDir, 'server.json'), JSON.stringify({ pid: process.pid, port: bound.port, version: 'test' }));
+      const hook = loadHook(stateDir);
+      const raw = JSON.stringify({ cwd: project });
+      const result = await hook.run(raw);
+      assert.strictEqual(JSON.parse(result.stdout).decision, 'block');
+      assert.ok(result.stdout.includes('OWN_FEEDBACK'));
+      assert.ok(!result.stdout.includes('OTHER_FEEDBACK'));
+      assert.deepStrictEqual(store.get(own.key).pendingFeedback, []);
+      assert.strictEqual(store.get(foreign.key).pendingFeedback.length, 1);
+      assert.deepStrictEqual(requests, [{ path: `/api/await?key=${own.key}&timeoutMs=0`, host: `127.0.0.1:${bound.port}` }]);
+      assert.match(own.key, /^[a-f0-9]{12}$/);
+      assert.strictEqual((await hook.run(raw)).stdout, raw, 'empty queue passes through');
+      await canvas.close(); closed = true;
+      // Stale but valid locator fails open, preserving the queued feedback.
+      store.queueFeedback(own.key, [{ kind: 'chat', text: 'OFFLINE_FEEDBACK' }]);
+      assert.strictEqual((await hook.run(raw)).stdout, raw);
+      assert.strictEqual(readPending(stateDir, own.key).length, 1);
+      fs.unlinkSync(path.join(stateDir, 'server.json'));
+      const fallback = await hook.run(raw);
+      assert.ok(JSON.parse(fallback.stdout).reason.includes('OFFLINE_FEEDBACK'));
+      assert.deepStrictEqual(readPending(stateDir, own.key), [], 'missing locator still drains offline file queue');
+      assert.strictEqual(readPending(stateDir, foreign.key).length, 1, 'offline fallback preserves other project queue');
+    } finally {
+      if (!closed) await canvas.close();
+      fs.rmSync(root, { recursive: true, force: true });
+    }
+  })) passed++; else failed++;
+
+  if (await test('HTTP errors and redirects fail open without following another destination', async () => {
+    const root = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'plan-canvas-http-failure-')));
+    const stateDir = path.join(root, 'state');
+    const artifact = path.join(root, 'plan.md');
+    fs.writeFileSync(artifact, '# Plan');
+    let status = 302;
+    let requests = 0;
+    const server = http.createServer((_req, res) => {
+      requests++;
+      res.writeHead(status, { 'Content-Type': 'application/json', Location: 'http://example.invalid/should-not-follow' });
+      res.end(JSON.stringify({ status: 'feedback', items: [{ kind: 'chat', text: 'UNTRUSTED_ERROR_BODY' }] }));
+    });
+    try {
+      await new Promise(resolve => server.listen(0, '127.0.0.1', resolve));
+      const port = server.address().port;
+      writeState(stateDir, { aaaaaaaaaaaa: sessionRecord('aaaaaaaaaaaa', artifact, [{ kind: 'chat', text: 'KEEP' }]) });
+      fs.writeFileSync(path.join(stateDir, 'server.json'), JSON.stringify({ port }));
+      const hook = loadHook(stateDir);
+      const raw = JSON.stringify({ cwd: root });
+      for (status of [302, 500]) {
+        assert.strictEqual((await hook.run(raw)).stdout, raw);
+        assert.strictEqual(readPending(stateDir, 'aaaaaaaaaaaa').length, 1);
+      }
+      assert.strictEqual(requests, 2, 'each attempt makes one loopback request; redirects are not followed');
+    } finally {
+      await new Promise(resolve => server.close(resolve));
+      fs.rmSync(root, { recursive: true, force: true });
+    }
+  })) passed++; else failed++;
+
+  if (await test('invalid keys and malformed locator ports make no HTTP request and preserve feedback', async () => {
+    const root = fs.realpathSync.native(fs.mkdtempSync(path.join(os.tmpdir(), 'plan-canvas-invalid-ipc-')));
+    const stateDir = path.join(root, 'state');
+    const artifact = path.join(root, 'plan.md');
+    fs.writeFileSync(artifact, '# Plan');
+    const hook = loadHook(stateDir);
+    const originalRequest = http.request;
+    let calls = 0;
+    http.request = () => { calls++; throw new Error('unexpected HTTP request'); };
+    try {
+      for (const port of [0, -1, 65536, 1.5, '4321', null]) {
+        writeState(stateDir, { aaaaaaaaaaaa: sessionRecord('aaaaaaaaaaaa', artifact, [{ kind: 'chat', text: 'KEEP' }]) });
+        fs.writeFileSync(path.join(stateDir, 'server.json'), JSON.stringify({ port }));
+        const raw = JSON.stringify({ cwd: root });
+        assert.strictEqual((await hook.run(raw)).stdout, raw);
+        assert.strictEqual(readPending(stateDir, 'aaaaaaaaaaaa').length, 1);
+        assert.strictEqual(await hook.drainViaServer(port, 'aaaaaaaaaaaa'), null);
+      }
+      fs.writeFileSync(path.join(stateDir, 'server.json'), JSON.stringify({ port: 4321 }));
+      for (const key of ['bad?key=SECRET&other=1', 'A'.repeat(12), 'a'.repeat(13), 'a'.repeat(100000), {}, null]) {
+        writeState(stateDir, { aaaaaaaaaaaa: sessionRecord(key, artifact, [{ kind: 'chat', text: 'KEEP' }]) });
+        const raw = JSON.stringify({ cwd: root });
+        assert.strictEqual((await hook.run(raw)).stdout, raw);
+        assert.strictEqual(readPending(stateDir, 'aaaaaaaaaaaa').length, 1);
+        assert.strictEqual(await hook.drainViaServer(4321, key), null);
+      }
+      assert.strictEqual(calls, 0);
+    } finally {
+      http.request = originalRequest;
+      fs.rmSync(root, { recursive: true, force: true });
+    }
+  })) passed++; else failed++;
+
   if (originalStateDir === undefined) delete process.env.ECC_PLAN_CANVAS_STATE_DIR;
   else process.env.ECC_PLAN_CANVAS_STATE_DIR = originalStateDir;