From 34fbf366104715db59f02c72f99af4d8bf5a5339 Mon Sep 17 00:00:00 2001 From: debont80 Date: Thu, 2 Jul 2026 12:21:28 -0400 Subject: [PATCH] =?UTF-8?q?feat(audit):=20Batch=2021=20=E2=80=94=20updater?= =?UTF-8?q?=20refactor=20under=20behavior=20pins=20(CL4,=20closes=20CC5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/main/updater.ts | 171 +++++++++++------- test/updaterDownload.test.ts | 333 +++++++++++++++++++++++++++++++++++ 2 files changed, 438 insertions(+), 66 deletions(-) create mode 100644 test/updaterDownload.test.ts diff --git a/src/main/updater.ts b/src/main/updater.ts index ea0bf3e..ade8d97 100644 --- a/src/main/updater.ts +++ b/src/main/updater.ts @@ -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 { - 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 { + const { url, filePath, expectedSha, onProgress, onCancelReady, onSettled } = opts return new Promise((resolve) => { let settled = false let fileStream: WriteStream | null = null @@ -337,8 +308,7 @@ export async function downloadAppUpdate(url: string, wc: WebContents): Promise { 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 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 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 { + 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 { diff --git a/test/updaterDownload.test.ts b/test/updaterDownload.test.ts new file mode 100644 index 0000000..7957ad4 --- /dev/null +++ b/test/updaterDownload.test.ts @@ -0,0 +1,333 @@ +/** + * Behavior-pinning tests for downloadAppUpdate (CL4) — written BEFORE extracting + * the streamToFile helper so the security-critical invariants are locked at the + * public boundary and must survive the refactor unchanged: + * + * - untrusted initial URL refused (no request made) + * - per-hop redirect re-validation on BOTH the checksum fetch and the installer + * - missing (404) checksum refused; malformed checksum refused + * - SHA-256 verified against the published digest; mismatch deletes the partial + * - truncation (received < content-length) detected; partial deleted + * - idle timeout aborts a stalled download + * - cancel during the checksum phase and mid-download both settle as canceled + * - progress events reach the WebContents + */ +import { describe, it, expect, vi, beforeEach, afterEach, afterAll } from 'vitest' +import { EventEmitter } from 'node:events' +import { mkdtempSync, rmSync, existsSync, readFileSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { createHash } from 'node:crypto' + +const TMP = mkdtempSync(join(tmpdir(), 'aerofetch-updater-test-')) + +// --- electron + settings mocks ---------------------------------------------- + +class FakeResponse extends EventEmitter { + constructor( + public statusCode: number, + public headers: Record = {} + ) { + super() + } +} + +class FakeRequest extends EventEmitter { + headers: Record = {} + ended = false + aborted = false + redirectsFollowed = 0 + constructor(public url: string) { + super() + } + setHeader(k: string, v: string): void { + this.headers[k] = v + } + end(): void { + this.ended = true + } + abort(): void { + this.aborted = true + } + followRedirect(): void { + this.redirectsFollowed++ + } +} + +type Script = (req: FakeRequest) => void +const requestScripts: Script[] = [] +const madeRequests: FakeRequest[] = [] + +vi.mock('electron', () => ({ + app: { getPath: () => TMP, getVersion: () => '0.5.0' }, + net: { + request: (opts: { url: string }) => { + const req = new FakeRequest(opts.url) + madeRequests.push(req) + const script = requestScripts.shift() + if (script) queueMicrotask(() => script(req)) + return req + } + }, + shell: {} +})) + +vi.mock('../src/main/settings', () => ({ + getSettings: () => ({ updateToken: '' }) +})) + +import { downloadAppUpdate, cancelAppUpdate } from '../src/main/updater' +import type { WebContents } from 'electron' + +// --- helpers ------------------------------------------------------------------ + +const HOST = 'https://gitea.netbird.zimspace.uk:5938' +const ASSET = `${HOST}/debont80/AeroFetch/releases/download/v9.9.9/AeroFetch-Setup-9.9.9.exe` +const INSTALLER = Buffer.from('FAKE-INSTALLER-BYTES-'.repeat(64)) +const GOOD_SHA = createHash('sha256').update(INSTALLER).digest('hex') + +function fakeWc(): { wc: WebContents; sends: unknown[] } { + const sends: unknown[] = [] + const wc = { + isDestroyed: () => false, + send: (_ch: string, p: unknown) => { + sends.push(p) + } + } as unknown as WebContents + return { wc, sends } +} + +/** Script the checksum (.sha256) fetch to answer 200 with the given text. */ +function checksumOk(text: string): Script { + return (req) => { + const res = new FakeResponse(200) + req.emit('response', res) + res.emit('data', Buffer.from(text)) + res.emit('end') + } +} + +/** Wait for the fire-and-forget partial unlink to land. */ +async function expectGone(path: string): Promise { + for (let i = 0; i < 20 && existsSync(path); i++) await new Promise((r) => setTimeout(r, 25)) + expect(existsSync(path)).toBe(false) +} + +const installerPath = join(TMP, 'AeroFetch-Setup-9.9.9.exe') + +beforeEach(() => { + requestScripts.length = 0 + madeRequests.length = 0 +}) + +afterEach(() => { + vi.useRealTimers() + try { + rmSync(installerPath, { force: true }) + } catch { + /* ignore */ + } +}) + +afterAll(() => { + rmSync(TMP, { recursive: true, force: true }) +}) + +// --- the pins ------------------------------------------------------------------- + +describe('downloadAppUpdate (CL4 behavior pins)', () => { + it('refuses an untrusted initial URL without making any request', async () => { + const { wc } = fakeWc() + const r = await downloadAppUpdate('https://evil.example/x.exe', wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/untrusted/i) + expect(madeRequests.length).toBe(0) + }) + + it('downloads, verifies the SHA-256, and reports progress (happy path)', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER.subarray(0, 512)) + res.emit('data', INSTALLER.subarray(512)) + res.emit('end') + }) + const { wc, sends } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r).toEqual({ ok: true, filePath: installerPath }) + expect(readFileSync(installerPath).equals(INSTALLER)).toBe(true) + expect(sends.length).toBeGreaterThan(0) + expect(sends.at(-1)).toMatchObject({ received: INSTALLER.length, total: INSTALLER.length }) + }) + + it('rejects a checksum mismatch and deletes the partial', async () => { + const wrong = 'a'.repeat(64) + requestScripts.push(checksumOk(`${wrong} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER) + res.emit('end') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/checksum/i) + await expectGone(installerPath) + }) + + it('detects truncation against content-length and deletes the partial', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length * 2) }) + req.emit('response', res) + res.emit('data', INSTALLER) // half of what was promised + res.emit('end') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/incomplete/i) + await expectGone(installerPath) + }) + + it('refuses to follow an installer redirect off the trusted host', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + req.emit('redirect', 302, 'GET', 'https://evil.example/attachments/x.exe') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/redirect.*untrusted/i) + const installerReq = madeRequests[1]! + expect(installerReq.redirectsFollowed).toBe(0) + expect(installerReq.aborted).toBe(true) + }) + + it('follows an on-host redirect (per-hop re-validation passes)', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + req.emit('redirect', 302, 'GET', `${HOST}/attachments/abc123`) + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER) + res.emit('end') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(true) + expect(madeRequests[1]!.redirectsFollowed).toBe(1) + }) + + it('refuses a checksum-fetch redirect off the trusted host', async () => { + requestScripts.push((req) => { + req.emit('redirect', 302, 'GET', 'https://evil.example/x.sha256') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/checksum/i) + expect(madeRequests.length).toBe(1) // never reached the installer request + }) + + it('refuses to install when the release has no checksum attached (404)', async () => { + requestScripts.push((req) => { + req.emit('response', new FakeResponse(404)) + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/no checksum/i) + expect(madeRequests.length).toBe(1) + }) + + it('refuses a malformed checksum file (no 64-hex digest)', async () => { + requestScripts.push(checksumOk('not a digest at all\n')) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/malformed/i) + expect(madeRequests.length).toBe(1) + }) + + it('a cancel during the checksum phase settles as canceled before any installer request', async () => { + requestScripts.push((req) => { + // Cancel lands while the checksum fetch is in flight. + cancelAppUpdate() + const res = new FakeResponse(200) + req.emit('response', res) + res.emit('data', Buffer.from(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + res.emit('end') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/canceled/i) + expect(madeRequests.length).toBe(1) + }) + + it('a cancel mid-download aborts, settles as canceled, and deletes the partial', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER.subarray(0, 512)) + cancelAppUpdate() + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/canceled/i) + expect(madeRequests[1]!.aborted).toBe(true) + await expectGone(installerPath) + }) + + it('times out a stalled download and deletes the partial', async () => { + vi.useFakeTimers() + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER.subarray(0, 512)) + // …and then nothing: the stream stalls. + }) + const { wc } = fakeWc() + const pending = downloadAppUpdate(ASSET, wc) + await vi.advanceTimersByTimeAsync(61_000) + const r = await pending + expect(r.ok).toBe(false) + expect(r.error).toMatch(/stalled/i) + expect(madeRequests[1]!.aborted).toBe(true) + vi.useRealTimers() + await expectGone(installerPath) + }) + + it('fails on an HTTP error status from the installer download', async () => { + requestScripts.push(checksumOk(`${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`)) + requestScripts.push((req) => { + req.emit('response', new FakeResponse(503)) + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(false) + expect(r.error).toMatch(/503/) + }) + + it('matches the checksum line for THIS asset in a combined checksum file', async () => { + const other = 'b'.repeat(64) + requestScripts.push( + checksumOk(`${other} SomethingElse.exe\n${GOOD_SHA} AeroFetch-Setup-9.9.9.exe\n`) + ) + requestScripts.push((req) => { + const res = new FakeResponse(200, { 'content-length': String(INSTALLER.length) }) + req.emit('response', res) + res.emit('data', INSTALLER) + res.emit('end') + }) + const { wc } = fakeWc() + const r = await downloadAppUpdate(ASSET, wc) + expect(r.ok).toBe(true) + }) +})