From 3c575611ee1b9709f3cb58397e0d7440426b3da6 Mon Sep 17 00:00:00 2001 From: heznpc Date: Thu, 2 Jul 2026 21:04:03 +0900 Subject: [PATCH] fix: bound capture helper failures --- .github/workflows/ci.yml | 4 +-- src/launch.js | 18 ++++++++++-- src/serve.js | 59 ++++++++++++++++++++++++++++------------ src/video.js | 53 +++++++++++++++++++++++++++++------- 4 files changed, 102 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b35748b..ce98762 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,9 +17,9 @@ jobs: # skip Playwright's browser download to keep CI fast. PLAYWRIGHT_SKIP_BROWSER_DOWNLOAD: '1' steps: - - uses: actions/checkout@v6 + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 - - uses: actions/setup-node@v6 + - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6 with: node-version: 22 cache: 'npm' diff --git a/src/launch.js b/src/launch.js index 8fd61d3..a6eceee 100644 --- a/src/launch.js +++ b/src/launch.js @@ -59,9 +59,21 @@ async function launchWithExtension({ extensionDir, viewport, recordVideoDir, rec const context = await chromium.launchPersistentContext(userDataDir, launchOpts); // Wait for the service worker so we can read the extension ID off its URL. - let [sw] = context.serviceWorkers(); - if (!sw) sw = await context.waitForEvent('serviceworker', { timeout: 15_000 }); - const extensionId = sw.url().split('/')[2]; + // A content-script-only extension (no MV3 worker) or a slow worker makes this + // reject — clean up the open context and temp profile before rethrowing so a + // failed launch never leaks a live Chromium process or a temp dir. + let extensionId; + try { + let [sw] = context.serviceWorkers(); + if (!sw) sw = await context.waitForEvent('serviceworker', { timeout: 15_000 }); + extensionId = sw.url().split('/')[2]; + } catch (err) { + await context.close().catch(() => {}); + if (userDataDir && fs.existsSync(userDataDir)) { + fs.rmSync(userDataDir, { recursive: true, force: true }); + } + throw err; + } return { context, extensionId, userDataDir }; } diff --git a/src/serve.js b/src/serve.js index 2c98d0c..8f1e390 100644 --- a/src/serve.js +++ b/src/serve.js @@ -48,25 +48,50 @@ function serveDirectory(dir, opts = {}) { const root = path.resolve(dir); return new Promise((resolve) => { const server = http.createServer((req, res) => { - const urlPath = decodeURIComponent((req.url || '/').split('?')[0]); - // Path-traversal sanitizer (CodeQL js/path-injection): normalize, then - // strip any leading `../` segments so the join can't escape `root`. - const rel = path.normalize(urlPath === '/' ? '/index.html' : urlPath).replace(/^(\.\.(\/|\\|$))+/, ''); - let filePath = path.join(root, rel); - if (!fs.existsSync(filePath) || fs.statSync(filePath).isDirectory()) { - filePath = opts.fallback ? path.join(root, opts.fallback) : null; - } - if (!filePath || !fs.existsSync(filePath)) { - res.writeHead(404, { 'Content-Type': 'text/plain' }); - res.end('Not found'); + // A synchronous throw inside this handler would become an uncaughtException + // that kills the whole capture process, so guard the entire body. Malformed + // percent-encoding (decodeURIComponent), a TOCTOU race between existsSync and + // read, or a permission/EACCES error must all yield an HTTP status, not a crash. + let urlPath; + try { + urlPath = decodeURIComponent((req.url || '/').split('?')[0]); + } catch { + res.writeHead(400, { 'Content-Type': 'text/plain' }); + res.end('Bad request'); return; } - const ext = path.extname(filePath).toLowerCase(); - res.writeHead(200, { - 'Content-Type': CONTENT_TYPES[ext] || 'application/octet-stream', - 'Content-Security-Policy': FIXTURE_CSP, - }); - res.end(fs.readFileSync(filePath)); + try { + // Path-traversal sanitizer (CodeQL js/path-injection): normalize, then + // strip any leading `../` segments so the join can't escape `root`. + const rel = path.normalize(urlPath === '/' ? '/index.html' : urlPath).replace(/^(\.\.(\/|\\|$))+/, ''); + let filePath = path.join(root, rel); + if (!fs.existsSync(filePath) || fs.statSync(filePath).isDirectory()) { + filePath = opts.fallback ? path.join(root, opts.fallback) : null; + } + if (!filePath || !fs.existsSync(filePath)) { + res.writeHead(404, { 'Content-Type': 'text/plain' }); + res.end('Not found'); + return; + } + const ext = path.extname(filePath).toLowerCase(); + res.writeHead(200, { + 'Content-Type': CONTENT_TYPES[ext] || 'application/octet-stream', + 'Content-Security-Policy': FIXTURE_CSP, + }); + res.end(fs.readFileSync(filePath)); + } catch (err) { + // readFileSync can throw after writeHead(200) already flushed headers, so + // only write a new status when none has been sent — otherwise abort the socket. + if (!res.headersSent) { + res.writeHead(err && err.code === 'ENOENT' ? 404 : 500, { 'Content-Type': 'text/plain' }); + res.end('Server error'); + } else { + res.destroy(); + } + } + }); + server.on('clientError', (err, socket) => { + if (socket.writable) socket.end('HTTP/1.1 400 Bad Request\r\n\r\n'); }); server.listen(0, '127.0.0.1', () => { const { port } = server.address(); diff --git a/src/video.js b/src/video.js index 543dc89..7741b63 100644 --- a/src/video.js +++ b/src/video.js @@ -20,6 +20,37 @@ const INSTALL_HINT = '`apt-get install -y ffmpeg`; GitHub ubuntu runners already have it) or set ' + "SHOTKIT_FFMPEG to a binary. Playwright's bundled ffmpeg cannot encode H.264."; +// Bound each ffmpeg invocation so a wedged encoder (bad input, odd codec path, +// a binary that hangs on stdin) can't block the whole capture run forever. +// Override for legitimately long encodes via SHOTKIT_FFMPEG_TIMEOUT_MS. +function ffmpegTimeoutMs(env = process.env) { + const raw = Number(env.SHOTKIT_FFMPEG_TIMEOUT_MS); + return Number.isFinite(raw) && raw > 0 ? raw : 120_000; +} + +/** + * Run ffmpeg with a bounded timeout, turning a timeout into a clear error + * (execFileSync otherwise throws a raw ETIMEDOUT/signal error on `.killed`). + */ +function runFfmpeg(bin, args, timeoutMs) { + try { + execFileSync(bin, args, { + stdio: ['ignore', 'ignore', 'inherit'], + timeout: timeoutMs, + killSignal: 'SIGKILL', + }); + } catch (err) { + if (err && (err.killed || err.code === 'ETIMEDOUT')) { + throw new Error( + `shotkit: ffmpeg timed out after ${Math.round(timeoutMs / 1000)}s — the encoder may be ` + + 'wedged. Raise SHOTKIT_FFMPEG_TIMEOUT_MS for legitimately long encodes.', + { cause: err }, + ); + } + throw err; + } +} + /** * Locate a usable ffmpeg. Returns the binary path/name, or null. * @param {NodeJS.ProcessEnv} [env] @@ -28,7 +59,14 @@ function findFfmpeg(env = process.env) { for (const bin of [env.SHOTKIT_FFMPEG, 'ffmpeg']) { if (!bin) continue; try { - const r = spawnSync(bin, ['-version'], { stdio: ['ignore', 'pipe', 'ignore'], encoding: 'utf8', env }); + // `-version` returns near-instantly; a small timeout guards against a + // resolved binary that wedges (e.g. hangs waiting on stdin). + const r = spawnSync(bin, ['-version'], { + stdio: ['ignore', 'pipe', 'ignore'], + encoding: 'utf8', + env, + timeout: 10_000, + }); if (r.status === 0 && /ffmpeg version/i.test(r.stdout || '')) return bin; } catch (_e) { /* try the next candidate */ @@ -132,14 +170,13 @@ function postProcessDemo({ webmPath, mp4, trim, crop, zoom, thumbnail, log, env const bin = findFfmpeg(env); if (!bin) throw new Error(`demo mp4/trim/crop/zoom/thumbnail requested but ${INSTALL_HINT}`); + const timeoutMs = ffmpegTimeoutMs(env); const produced = []; let finalVideoPath = webmPath; if (mp4 || crop || zoom) { const crf = typeof mp4 === 'object' && mp4.crf != null ? mp4.crf : undefined; const mp4Path = webmPath.replace(/\.webm$/, '.mp4'); - execFileSync(bin, buildFfmpegArgs({ input: webmPath, output: mp4Path, trim, crf, crop, zoom }), { - stdio: ['ignore', 'ignore', 'inherit'], - }); + runFfmpeg(bin, buildFfmpegArgs({ input: webmPath, output: mp4Path, trim, crf, crop, zoom }), timeoutMs); produced.push(mp4Path); finalVideoPath = mp4Path; const notes = ['H.264']; @@ -150,9 +187,7 @@ function postProcessDemo({ webmPath, mp4, trim, crop, zoom, thumbnail, log, env } else if (trim) { // Trim-only: stream-copy to a sibling temp file, then swap in place. const tmp = `${webmPath}.trim.webm`; - execFileSync(bin, buildFfmpegArgs({ input: webmPath, output: tmp, trim, copy: true }), { - stdio: ['ignore', 'ignore', 'inherit'], - }); + runFfmpeg(bin, buildFfmpegArgs({ input: webmPath, output: tmp, trim, copy: true }), timeoutMs); fs.renameSync(tmp, webmPath); log(`✓ ${path.basename(webmPath)} trimmed in place`); } @@ -163,9 +198,7 @@ function postProcessDemo({ webmPath, mp4, trim, crop, zoom, thumbnail, log, env const thumbPath = typeof thumbnail === 'object' && thumbnail.name ? path.join(path.dirname(webmPath), thumbnail.name) : webmPath.replace(/\.webm$/, '-thumbnail.png'); - execFileSync(bin, buildThumbnailArgs({ input: finalVideoPath, output: thumbPath, at }), { - stdio: ['ignore', 'ignore', 'inherit'], - }); + runFfmpeg(bin, buildThumbnailArgs({ input: finalVideoPath, output: thumbPath, at }), timeoutMs); // ffmpeg exits 0 even when `at` seeks past the end of a (trimmed) clip, // writing no file. Only record the thumbnail when it was actually produced, // so the manifest never references a phantom asset.