fix: abort scan-from-url downloads on cancel; cap download size (#136)

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 |

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,

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);
   }
 }

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;