feat(audit): Batch 21 — updater refactor under behavior pins (CL4, closes CC5)

14 pinning tests lock downloadAppUpdate's security invariants (redirect re-validation per hop, checksum refusal paths, SHA-256 verify + cleanup, truncation, idle-timeout, cancel in both phases, progress) with a scripted net.request fake; then the nested streaming block is extracted verbatim into streamInstallerToFile. downloadAppUpdate is now a linear pre-flight; B6 cancel-slot ownership stays with its module state via onCancelReady/onSettled. Pins green before and after. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-02 12:21:28 -04:00
parent d1e3168449
commit 34fbf36610
2 changed files with 438 additions and 66 deletions
@@ -274,58 +274,29 @@ function fetchTrustedText(
  })
 }

-/** Stream the installer to a temp file, pushing progress events to the renderer. */
-export async function downloadAppUpdate(url: string, wc: WebContents): Promise<AppUpdateDownload> {
-  if (!isTrustedDownloadUrl(url)) {
-    return { ok: false, error: 'Refused to download from an untrusted location.' }
-  }
-
-  // Derive a safe .exe filename from the URL; fall back to a fixed name.
-  const urlName = decodeURIComponent(new URL(url).pathname.split('/').pop() || '')
-  const safeName = /^[\w.\- ]+\.exe$/i.test(urlName) ? urlName : 'AeroFetch-Setup.exe'
-  const filePath = join(updateDir(), safeName)
-
-  // Resolve the published checksum up front (tiny, fails fast) so we never pull a
-  // ~300 MB installer we'd then refuse to run. The .sha256 sits next to the asset,
-  // so its URL is the installer URL + '.sha256' — still on the host-pinned origin.
-  const checksumUrl = (() => {
-    const u = new URL(url)
-    u.pathname += '.sha256'
-    return u.toString()
-  })()
-  // The renderer's Cancel button is live from the moment this is invoked, so the
-  // checksum phase must be cancelable too — without this, a Cancel during the
-  // fetch is a silent no-op and the full installer download proceeds anyway (B6).
-  // This early canceller only flags; nothing is on disk yet, so there's no teardown.
-  let canceledEarly = false
-  const earlyCancel = (): void => {
-    canceledEarly = true
-  }
-  cancelActiveDownload = earlyCancel
-  const settleEarly = (result: AppUpdateDownload): AppUpdateDownload => {
-    if (cancelActiveDownload === earlyCancel) cancelActiveDownload = null
-    return result
-  }
-
-  const sum = await fetchTrustedText(checksumUrl)
-  if (canceledEarly) return settleEarly({ ok: false, error: 'Update download canceled.' })
-  let expectedSha: string | null = null
-  if (sum.ok) {
-    expectedSha = extractSha256(sum.text, safeName)
-    if (!expectedSha) {
-      return settleEarly({ ok: false, error: "The update's checksum file is malformed." })
-    }
-  } else if (sum.status === 404) {
-    if (REQUIRE_CHECKSUM) {
-      return settleEarly({
-        ok: false,
-        error: `This release has no checksum (${safeName}.sha256) attached — refusing to install an unverified update.`
-      })
-    }
-  } else {
-    return settleEarly({ ok: false, error: `Couldn't fetch the update's checksum — ${sum.error}.` })
-  }
-
+/**
+ * Stream one HTTPS resource into a file with the updater's full download
+ * discipline (CL4's extracted helper; the once-wrapped event-emitter API CC5
+ * calls for): per-hop redirect re-validation against the trusted host, streaming
+ * SHA-256, idle-timeout, content-length truncation detection, write-stream
+ * backpressure, and single-point teardown that aborts the request and removes
+ * the partial file on every failure path.
+ *
+ * The caller owns the module-level cancel slot: `onCancelReady` hands it this
+ * download's cancel function once armed, and `onSettled` hands the same function
+ * back on completion so the caller can release the slot only if this download
+ * still owns it (the B6 ownership rule).
+ */
+function streamInstallerToFile(opts: {
+  url: string
+  filePath: string
+  /** published SHA-256 to verify against, or null to skip verification */
+  expectedSha: string | null
+  onProgress: (p: AppUpdateProgress) => void
+  onCancelReady: (cancel: () => void) => void
+  onSettled: (cancel: () => void) => void
+}): Promise<AppUpdateDownload> {
+  const { url, filePath, expectedSha, onProgress, onCancelReady, onSettled } = opts
  return new Promise<AppUpdateDownload>((resolve) => {
    let settled = false
    let fileStream: WriteStream | null = null
@@ -337,8 +308,7 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise<A
    const finish = (result: AppUpdateDownload): void => {
      if (settled) return
      settled = true
-      // Done — release the cancel slot, but only if this download still owns it (B6).
-      if (cancelActiveDownload === requestCancel) cancelActiveDownload = null
+      onSettled(requestCancel)
      if (idle) clearTimeout(idle)
      if (!result.ok) {
        request.abort()
@@ -351,11 +321,9 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise<A
    }

    // Expose this download's abort so cancelAppUpdate() (the update card's Cancel)
-    // can route through the same teardown — request.abort() + partial cleanup.
-    // Named so finish() can release the slot only when this download still owns it,
-    // and replaces the checksum-phase earlyCancel above (ownership moves here). (B6)
+    // can route through the same teardown — request.abort() + partial cleanup. (B6)
    const requestCancel = (): void => finish({ ok: false, error: 'Update download canceled.' })
-    cancelActiveDownload = requestCancel
+    onCancelReady(requestCancel)

    // net.request (not fetch) so we can re-validate the host on EVERY redirect
    // hop: Gitea 302s a release download to its attachment store, and undici's
@@ -408,14 +376,11 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise<A
          flow.pause()
          stream.once('drain', () => flow.resume?.())
        }
-        if (!wc.isDestroyed()) {
-          const progress: AppUpdateProgress = {
-            received,
-            total,
-            fraction: total ? received / total : undefined
-          }
-          wc.send(IpcChannels.appUpdateProgress, progress)
-        }
+        onProgress({
+          received,
+          total,
+          fraction: total ? received / total : undefined
+        })
      })

      response.on('end', () => {
@@ -452,6 +417,80 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise<A
  })
 }

+/** Stream the installer to a temp file, pushing progress events to the renderer. */
+export async function downloadAppUpdate(url: string, wc: WebContents): Promise<AppUpdateDownload> {
+  if (!isTrustedDownloadUrl(url)) {
+    return { ok: false, error: 'Refused to download from an untrusted location.' }
+  }
+
+  // Derive a safe .exe filename from the URL; fall back to a fixed name.
+  const urlName = decodeURIComponent(new URL(url).pathname.split('/').pop() || '')
+  const safeName = /^[\w.\- ]+\.exe$/i.test(urlName) ? urlName : 'AeroFetch-Setup.exe'
+  const filePath = join(updateDir(), safeName)
+
+  // Resolve the published checksum up front (tiny, fails fast) so we never pull a
+  // ~300 MB installer we'd then refuse to run. The .sha256 sits next to the asset,
+  // so its URL is the installer URL + '.sha256' — still on the host-pinned origin.
+  const checksumUrl = (() => {
+    const u = new URL(url)
+    u.pathname += '.sha256'
+    return u.toString()
+  })()
+  // The renderer's Cancel button is live from the moment this is invoked, so the
+  // checksum phase must be cancelable too — without this, a Cancel during the
+  // fetch is a silent no-op and the full installer download proceeds anyway (B6).
+  // This early canceller only flags; nothing is on disk yet, so there's no teardown.
+  let canceledEarly = false
+  const earlyCancel = (): void => {
+    canceledEarly = true
+  }
+  cancelActiveDownload = earlyCancel
+  const settleEarly = (result: AppUpdateDownload): AppUpdateDownload => {
+    if (cancelActiveDownload === earlyCancel) cancelActiveDownload = null
+    return result
+  }
+
+  const sum = await fetchTrustedText(checksumUrl)
+  if (canceledEarly) return settleEarly({ ok: false, error: 'Update download canceled.' })
+  let expectedSha: string | null = null
+  if (sum.ok) {
+    expectedSha = extractSha256(sum.text, safeName)
+    if (!expectedSha) {
+      return settleEarly({ ok: false, error: "The update's checksum file is malformed." })
+    }
+  } else if (sum.status === 404) {
+    if (REQUIRE_CHECKSUM) {
+      return settleEarly({
+        ok: false,
+        error: `This release has no checksum (${safeName}.sha256) attached — refusing to install an unverified update.`
+      })
+    }
+  } else {
+    return settleEarly({ ok: false, error: `Couldn't fetch the update's checksum — ${sum.error}.` })
+  }
+
+  // The streaming itself — redirects, hashing, timeout, truncation, teardown —
+  // lives in streamInstallerToFile (CL4). This function stays a linear
+  // pre-flight: trust-check → checksum resolution → stream → done. The cancel
+  // slot's B6 ownership dance stays here, next to the module state it guards:
+  // ownership moves from the checksum-phase earlyCancel above to the stream's
+  // cancel, and is released only if this download still owns the slot.
+  return streamInstallerToFile({
+    url,
+    filePath,
+    expectedSha,
+    onProgress: (progress) => {
+      if (!wc.isDestroyed()) wc.send(IpcChannels.appUpdateProgress, progress)
+    },
+    onCancelReady: (cancel) => {
+      cancelActiveDownload = cancel
+    },
+    onSettled: (cancel) => {
+      if (cancelActiveDownload === cancel) cancelActiveDownload = null
+    }
+  })
+}
+
 /** Launch a freshly-downloaded installer, then quit so it can replace the app. */
 export async function runAppUpdate(filePath: string): Promise<{ ok: boolean; error?: string }> {
  try {