From 103bbb272aba7f369a8f1fa7cea2b38655a2ecff Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Fri, 28 Aug 2026 17:38:33 -0400 Subject: [PATCH] fix(plan-canvas): sanitize PDF failures --- docs/testing/plan-canvas-pdf-export.tdd.md | 6 +- scripts/lib/plan-canvas/server.js | 14 ++++- scripts/plan-canvas.js | 21 +++++-- tests/integration/plan-canvas-e2e.test.js | 16 ++++++ tests/scripts/plan-canvas.test.js | 65 +++++++++++++++++----- 5 files changed, 98 insertions(+), 24 deletions(-) diff --git a/docs/testing/plan-canvas-pdf-export.tdd.md b/docs/testing/plan-canvas-pdf-export.tdd.md index e2bc2ca06..38e849628 100644 --- a/docs/testing/plan-canvas-pdf-export.tdd.md +++ b/docs/testing/plan-canvas-pdf-export.tdd.md @@ -24,9 +24,9 @@ artifact as a real PDF file without sending the plan to an external converter. - RED: the focused server suite produced 30 passes and 2 failures because the Canvas had no Download PDF control or PDF endpoint. -- GREEN: renderer unit tests pass 7/7, Plan Canvas server tests pass 34/34, - and the end-to-end review workflow passes 12/12. -- FULL SUITE: the final review-hardened implementation passes all 4,008 +- GREEN: renderer unit tests pass 7/7, Plan Canvas server tests pass 35/35, + and the end-to-end review workflow passes 13/13. +- FULL SUITE: the final review-hardened implementation passes all 4,010 discovered tests; hosted security reruns are recorded on PR #2894. - COVERAGE: `npm run coverage` passes 4,003/4,003 with 88.97% statements, 80.58% branches, 94.22% functions, and 88.97% lines. The Plan Canvas diff --git a/scripts/lib/plan-canvas/server.js b/scripts/lib/plan-canvas/server.js index c93135531..7963980c7 100644 --- a/scripts/lib/plan-canvas/server.js +++ b/scripts/lib/plan-canvas/server.js @@ -433,8 +433,18 @@ function createPlanCanvasServer({ }); return sendPdf(res, pdf); } catch (error) { - const statusCode = error.code === 'PDF_BROWSER_NOT_FOUND' ? 503 : 500; - return sendJson(res, statusCode, { error: error.message, code: error.code || 'PDF_EXPORT_FAILED' }); + const code = error.code || 'PDF_EXPORT_FAILED'; + log(`[plan-canvas] PDF export failed (${code}): ${error.stack || error.message}`); + if (code === 'PDF_BROWSER_NOT_FOUND') { + return sendJson(res, 503, { + error: 'PDF export requires Google Chrome, Chromium, or Microsoft Edge; configure ECC_PLAN_CANVAS_CHROME_PATH if auto-discovery cannot find it', + code + }); + } + return sendJson(res, 500, { + error: 'PDF export failed; check the Plan Canvas server log for details', + code: 'PDF_EXPORT_FAILED' + }); } finally { pdfSnapshot = null; pdfExportActive = false; diff --git a/scripts/plan-canvas.js b/scripts/plan-canvas.js index 9c0a33855..195607b56 100755 --- a/scripts/plan-canvas.js +++ b/scripts/plan-canvas.js @@ -179,19 +179,20 @@ async function withServerStartLock(port, task, { const lockPort = validatePort(port); const startedAt = Date.now(); let socket = null; + let socketError = null; while (true) { try { socket = await new Promise((resolve, reject) => { const candidate = dgramImpl.createSocket('udp4'); const onError = error => { - candidate.close(); + try { candidate.close(); } catch { /* failed bind has no open handle */ } reject(error); }; candidate.once('error', onError); candidate.bind(lockPort, DEFAULT_HOST, () => { candidate.removeListener('error', onError); - candidate.on('error', () => {}); + candidate.on('error', error => { socketError = error; }); candidate.unref(); resolve(candidate); }); @@ -206,11 +207,23 @@ async function withServerStartLock(port, task, { } } + let result; try { - return await task(); + result = await task(); } finally { - if (socket) socket.close(); + if (socket) { + await new Promise(resolve => { + try { socket.close(resolve); } catch { resolve(); } + }); + } } + if (socketError) { + const error = new Error(`Plan Canvas startup lock failed on port ${port}: ${socketError.message}`); + error.code = 'PLAN_CANVAS_START_LOCK_FAILED'; + error.cause = socketError; + throw error; + } + return result; } function serverIsCompatible(health) { diff --git a/tests/integration/plan-canvas-e2e.test.js b/tests/integration/plan-canvas-e2e.test.js index a534e5f71..467aff10b 100644 --- a/tests/integration/plan-canvas-e2e.test.js +++ b/tests/integration/plan-canvas-e2e.test.js @@ -17,6 +17,7 @@ */ const assert = require('assert'); +const { EventEmitter } = require('events'); const fs = require('fs'); const http = require('http'); const os = require('os'); @@ -169,6 +170,21 @@ async function main() { ); }); + await test('port-scoped startup lock propagates socket failures', async () => { + class FailingLockSocket extends EventEmitter { + bind(_port, _host, callback) { setImmediate(callback); } + unref() {} + close(callback) { if (callback) setImmediate(callback); } + } + const socket = new FailingLockSocket(); + await assert.rejects( + withServerStartLock(port + 4, async () => { + socket.emit('error', new Error('simulated UDP failure')); + }, { dgramImpl: { createSocket: () => socket }, timeoutMs: 2000 }), + error => error.code === 'PLAN_CANVAS_START_LOCK_FAILED' && error.message.includes('simulated UDP failure') + ); + }); + await test('concurrent opens serialize replacement of a same-version legacy server', async () => { const legacyPort = port + 1; const legacyStateDir = path.join(tmp, 'legacy-state'); diff --git a/tests/scripts/plan-canvas.test.js b/tests/scripts/plan-canvas.test.js index b460e7ef7..ca3635994 100644 --- a/tests/scripts/plan-canvas.test.js +++ b/tests/scripts/plan-canvas.test.js @@ -104,6 +104,8 @@ async function main() { const pdfRequests = []; let holdPdfExport = false; let releasePdfExport = null; + let pdfFailure = null; + const serverLogs = []; let idleFired = false; const canvas = createPlanCanvasServer({ store, @@ -112,9 +114,11 @@ async function main() { idleTimeoutMs: 0, pdfExporter: async options => { pdfRequests.push(options); + if (pdfFailure) throw pdfFailure; if (holdPdfExport) await new Promise(resolve => { releasePdfExport = resolve; }); return { buffer: Buffer.from('%PDF-1.4\n%%EOF\n'), filename: 'demo.pdf' }; }, + log: line => serverLogs.push(line), onIdleShutdown: () => { idleFired = true; } @@ -260,22 +264,53 @@ async function main() { holdPdfExport = true; const snapshot = '

Already rendered diagram

Local SVG'; const first = request(port, 'POST', `/api/session/${key}/pdf`, { body: { html: snapshot } }); - await waitFor(() => typeof releasePdfExport === 'function'); - const printable = await request(port, 'GET', `/artifact/${key}/?pdf=1`); - assert.ok(printable.body.includes('Already rendered diagram')); - assert.ok(printable.body.includes('Local SVG')); - assert.ok(printable.headers['content-security-policy'].includes("script-src 'none'")); - const overloaded = await request(port, 'GET', `/api/session/${key}/pdf`); - assert.strictEqual(overloaded.statusCode, 429); - assert.strictEqual(overloaded.headers['retry-after'], '1'); - assert.deepStrictEqual(jsonBody(overloaded), { - error: 'another PDF export is already in progress', - code: 'PDF_EXPORT_BUSY' - }); - releasePdfExport(); + try { + await waitFor(() => typeof releasePdfExport === 'function'); + const printable = await request(port, 'GET', `/artifact/${key}/?pdf=1`); + assert.ok(printable.body.includes('Already rendered diagram')); + assert.ok(printable.body.includes('Local SVG')); + assert.ok(printable.headers['content-security-policy'].includes("script-src 'none'")); + const overloaded = await request(port, 'GET', `/api/session/${key}/pdf`); + assert.strictEqual(overloaded.statusCode, 429); + assert.strictEqual(overloaded.headers['retry-after'], '1'); + assert.deepStrictEqual(jsonBody(overloaded), { + error: 'another PDF export is already in progress', + code: 'PDF_EXPORT_BUSY' + }); + } finally { + const release = releasePdfExport; + holdPdfExport = false; + releasePdfExport = null; + if (release) release(); + } assert.strictEqual((await first).statusCode, 200); - holdPdfExport = false; - releasePdfExport = null; + })) passed++; else failed++; + + if (await test('PDF failures log diagnostics without disclosing local paths', async () => { + try { + const rendererError = new Error('Chromium failed at /Users/private/browser-profile'); + rendererError.code = 'PDF_EXPORT_FAILED'; + pdfFailure = rendererError; + const failed = await request(port, 'GET', `/api/session/${key}/pdf`); + assert.strictEqual(failed.statusCode, 500); + assert.deepStrictEqual(jsonBody(failed), { + error: 'PDF export failed; check the Plan Canvas server log for details', + code: 'PDF_EXPORT_FAILED' + }); + assert.ok(!failed.body.includes('/Users/private')); + assert.ok(serverLogs.some(line => line.includes('/Users/private/browser-profile'))); + + const browserError = new Error('missing override /Users/private/Chrome'); + browserError.code = 'PDF_BROWSER_NOT_FOUND'; + pdfFailure = browserError; + const missing = await request(port, 'GET', `/api/session/${key}/pdf`); + assert.strictEqual(missing.statusCode, 503); + assert.strictEqual(jsonBody(missing).code, 'PDF_BROWSER_NOT_FOUND'); + assert.ok(jsonBody(missing).error.includes('ECC_PLAN_CANVAS_CHROME_PATH')); + assert.ok(!missing.body.includes('/Users/private')); + } finally { + pdfFailure = null; + } })) passed++; else failed++; if (await test('browser client uses finite polling instead of one permanent connection per canvas', async () => {