diff --git a/CHANGES/2026-07-02_fix-abort-scan-from-url-download.md b/CHANGES/2026-07-02_fix-abort-scan-from-url-download.md new file mode 100644 index 0000000..70eb2d6 --- /dev/null +++ b/CHANGES/2026-07-02_fix-abort-scan-from-url-download.md @@ -0,0 +1,54 @@ +# fix: abort scan-from-url downloads on cancel; cap download size + +**Date:** 2026-07-02 +**Type:** Fix + +## Intent + +Closes #136. Cancelling a scan-from-url job did not abort the in-flight +download: `runFetch` only checked `job._cancelFn()` *after* +`downloadToTemp(url)` resolved, so a cancelled job kept streaming the whole +tarball to disk. There was also no download size cap or backstop for +runaway/chunked responses. + +### Prompts summary +1. Implement GitHub issue #136: wire an AbortController from `cancelJob` + through to the fetch + stream pipeline, add a 2 GB download cap, clean up + the partial temp file on abort/failure, and cover both with unit tests. + +## Changes + +### `src/remoteFetch.js` +- `downloadToTemp(url, opts = {})` now accepts `opts.signal` (AbortSignal) + and passes it to both `fetch()` and the stream `pipeline()`, so an abort + tears the download down immediately. +- Added exported `MAX_DOWNLOAD_BYTES` (2 GB). A `content-length` header over + the cap rejects up front with `download too large (...)`; a byte-counting + Transform enforces the same cap during streaming, covering chunked + responses without a content-length. +- On any failure (abort, cap, HTTP error) the temp dir is removed via + `cleanupTemp` before rethrowing, so the partial file never leaks. An abort + surfaces as a normalized `download cancelled` error. + +### `src/scanJobs.js` +- `makeJob` base shape gains `_abort: null`. +- `runFetch` creates an `AbortController`, stores it on `job._abort`, passes + its signal to `downloadToTemp`, and clears it once the download settles. + A cancel-triggered abort finishes the job as `cancelled`, not `failed`. +- `cancelJob` calls `job._abort.abort()` after setting the cancel flag, so an + in-flight download stops immediately. + +### `tests/node/remoteFetch.test.js` +- New `downloadToTemp abort and size cap` suite: (a) aborting an in-flight + download rejects with `download cancelled` and leaves no + `papastud-fetch-*` temp dir behind; (b) a response advertising a + `content-length` over the cap rejects with `download too large` without + downloading a body. + +## Files modified + +| File | Change | +|------|--------| +| `src/remoteFetch.js` | Abort signal support, 2 GB cap, temp-dir cleanup on failure | +| `src/scanJobs.js` | Wire AbortController through job lifecycle; cancel aborts download | +| `tests/node/remoteFetch.test.js` | Abort + size-cap unit tests | diff --git a/src/remoteFetch.js b/src/remoteFetch.js index 28b9d62..0955be6 100644 --- a/src/remoteFetch.js +++ b/src/remoteFetch.js @@ -22,9 +22,14 @@ const os = require('os'); const path = require('path'); const crypto = require('crypto'); const { spawnSync } = require('child_process'); -const { Readable } = require('stream'); +const { Readable, Transform } = require('stream'); const { pipeline } = require('stream/promises'); +// Hard cap on a scan-from-url download. CI screenshot tarballs are tens of +// megabytes; anything past this is a mistake (or abuse) and we bail before +// filling the disk. +const MAX_DOWNLOAD_BYTES = 2 * 1024 * 1024 * 1024; // 2 GB + // Parse and validate a fetch URL. Returns the URL object; throws on a // malformed or non-http(s) URL. Shared by the route handler (fail fast with a // 400) and downloadToTemp (the security boundary). @@ -37,18 +42,56 @@ function validateFetchUrl(url) { return parsed; } +// Counts bytes flowing through and errors once the cap is exceeded, so +// chunked responses without a content-length header are capped too. +function byteCapTransform(limit) { + let seen = 0; + return new Transform({ + transform(chunk, _enc, cb) { + seen += chunk.length; + if (seen > limit) { + cb(new Error(`download too large (exceeds ${limit} bytes)`)); + return; + } + cb(null, chunk); + }, + }); +} + // Stream a remote tarball to a temp file. Returns the temp file path. -// Throws on a non-http(s) URL or a non-2xx response. -async function downloadToTemp(url) { +// Throws on a non-http(s) URL, a non-2xx response, a download exceeding +// MAX_DOWNLOAD_BYTES, or an abort via opts.signal (as 'download cancelled'). +// On any failure the temp dir is removed before rethrowing. +async function downloadToTemp(url, opts = {}) { validateFetchUrl(url); - const res = await fetch(url, { redirect: 'follow' }); - if (!res.ok) throw new Error(`download failed: HTTP ${res.status}`); - if (!res.body) throw new Error('download failed: empty response body'); - - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'papastud-fetch-')); - const tarFile = path.join(dir, 'result.tar'); - await pipeline(Readable.fromWeb(res.body), fs.createWriteStream(tarFile)); - return tarFile; + const { signal } = opts; + let res = null; + let tarFile = null; + try { + res = await fetch(url, { redirect: 'follow', signal }); + if (!res.ok) throw new Error(`download failed: HTTP ${res.status}`); + if (!res.body) throw new Error('download failed: empty response body'); + const contentLength = Number(res.headers.get('content-length')); + if (Number.isFinite(contentLength) && contentLength > MAX_DOWNLOAD_BYTES) { + throw new Error(`download too large (${contentLength} bytes, cap is ${MAX_DOWNLOAD_BYTES})`); + } + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'papastud-fetch-')); + tarFile = path.join(dir, 'result.tar'); + await pipeline( + Readable.fromWeb(res.body), + byteCapTransform(MAX_DOWNLOAD_BYTES), + fs.createWriteStream(tarFile), + { signal } + ); + return tarFile; + } catch (e) { + cleanupTemp(tarFile); // never leak a partial download + // Drop the connection if the body wasn't fully consumed (e.g. size-cap bail). + try { res?.body?.cancel()?.catch(() => {}); } catch {} + if (signal?.aborted) throw new Error('download cancelled'); + throw e; + } } // An entry is unsafe if it is absolute or escapes the extraction root. @@ -155,6 +198,7 @@ function cleanupTemp(tarFile) { } module.exports = { + MAX_DOWNLOAD_BYTES, validateFetchUrl, downloadToTemp, listBuildMembers, diff --git a/src/scanJobs.js b/src/scanJobs.js index 2cae7c8..4df9346 100644 --- a/src/scanJobs.js +++ b/src/scanJobs.js @@ -47,6 +47,7 @@ function makeJob(project, status, extra = {}) { error: null, _cancelFn: () => cancelled, _cancel: () => { cancelled = true; }, + _abort: null, // AbortController while a download is in flight _finished_at: null, ...extra, }; @@ -98,6 +99,9 @@ function cancelJob(jobId) { const job = jobs.get(jobId); if (!job) return false; job._cancel(); + // Abort an in-flight download immediately instead of letting it stream to + // completion before the cancel flag is even checked. + if (job._abort) job._abort.abort(); // A job parked awaiting the compat-mismatch confirmation will never resume, // so finalize it and drop its downloaded tarball now. if (job.status === 'needs_confirmation') finishJob(job, 'cancelled'); @@ -197,8 +201,11 @@ function startScanFromUrl(project, url) { async function runFetch(jobId, project, url) { const job = jobs.get(jobId); if (!job) return; + const ac = new AbortController(); + job._abort = ac; try { - const tarFile = await remoteFetch.downloadToTemp(url); + const tarFile = await remoteFetch.downloadToTemp(url, { signal: ac.signal }); + job._abort = null; job._tarFile = tarFile; if (job._cancelFn()) return finishJob(job, 'cancelled'); job.status = 'extracting'; @@ -216,6 +223,9 @@ async function runFetch(jobId, project, url) { } doExtractAndScan(jobId, project, tarFile, compat.modules); } catch (e) { + job._abort = null; + // A cancel-triggered abort is not a failure — surface it as 'cancelled'. + if (job._cancelFn() && ac.signal.aborted) return finishJob(job, 'cancelled'); finishJob(job, 'failed', e.message); } } diff --git a/tests/node/remoteFetch.test.js b/tests/node/remoteFetch.test.js index e9ab697..69206ff 100644 --- a/tests/node/remoteFetch.test.js +++ b/tests/node/remoteFetch.test.js @@ -123,6 +123,68 @@ describe('remoteFetch pure helpers', () => { }); }); +describe('downloadToTemp abort and size cap', () => { + function listFetchTemps() { + return fs.readdirSync(os.tmpdir()).filter(n => n.startsWith('papastud-fetch-')); + } + + function startServer(handler) { + return new Promise(resolve => { + const srv = http.createServer(handler); + srv.listen(0, '127.0.0.1', () => { + resolve([srv, `http://127.0.0.1:${srv.address().port}/result.tar`]); + }); + }); + } + + async function stopServer(srv) { + srv.closeAllConnections?.(); + await new Promise(r => srv.close(r)); + } + + it('aborting an in-flight download rejects and leaves no temp dir', async () => { + const before = new Set(listFetchTemps()); + // Stream some bytes, then hang — only an abort can end this download. + const [srv, url] = await startServer((req, res) => { + res.writeHead(200, { 'Content-Type': 'application/x-tar' }); + res.write('partial-tar-bytes'); + }); + try { + const ac = new AbortController(); + const download = remoteFetch.downloadToTemp(url, { signal: ac.signal }); + // Let the download get past the headers and start streaming. + await new Promise(r => setTimeout(r, 150)); + ac.abort(); + await assert.rejects(download, /download cancelled/); + const leaked = listFetchTemps().filter(n => !before.has(n)); + assert.deepEqual(leaked, [], 'partial download temp dir must be cleaned up'); + } finally { + await stopServer(srv); + } + }); + + it('rejects a content-length over the cap without downloading the body', async () => { + const before = new Set(listFetchTemps()); + const [srv, url] = await startServer((req, res) => { + // Advertise a huge body; never send it — the header check must bail first. + res.writeHead(200, { + 'Content-Type': 'application/x-tar', + 'Content-Length': String(remoteFetch.MAX_DOWNLOAD_BYTES + 1), + }); + // No body write ever happens, so flush the buffered header explicitly — + // otherwise the client never receives it and fetch hangs on headers. + res.flushHeaders(); + }); + try { + await assert.rejects(remoteFetch.downloadToTemp(url), /download too large/); + const leaked = listFetchTemps().filter(n => !before.has(n)); + assert.deepEqual(leaked, []); + } finally { + await stopServer(srv); + } + }); +}); + describe('POST /api/projects/:id/scan-from-url', () => { let tmpdir, projectRoot, server, baseUrl, tarServer, tarUrl, tarFile;