diff --git a/scripts/hooks/plan-canvas-pending.js b/scripts/hooks/plan-canvas-pending.js index b9b7dee4b..63727e436 100644 --- a/scripts/hooks/plan-canvas-pending.js +++ b/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 }; diff --git a/tests/hooks/plan-canvas-pending-hook.test.js b/tests/hooks/plan-canvas-pending-hook.test.js index 43545bc98..ddf4b3977 100644 --- a/tests/hooks/plan-canvas-pending-hook.test.js +++ b/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;