diff --git a/src/review/visual/actions-fallback.ts b/src/review/visual/actions-fallback.ts index ed8d7e2f8a..22feb1fe0f 100644 --- a/src/review/visual/actions-fallback.ts +++ b/src/review/visual/actions-fallback.ts @@ -239,27 +239,50 @@ const LOCAL_HEADER_SIZE = 30; // GitHub Actions artifacts hold at most a handful of files here (one per route x viewport); bound the walk // regardless of what a hostile/corrupt central directory claims, so a crafted entryCount can't spin forever. const MAX_ZIP_ENTRIES = 64; +const MAX_ARTIFACT_BYTES = 60 * 1024 * 1024; +const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a] as const; -async function inflateRawRaw(compressed: Uint8Array): Promise { +async function inflateRawRaw(compressed: Uint8Array, maxBytes: number): Promise { try { // The cast only narrows the TYPE for the UI workspace's stricter DOM-lib BodyInit/BlobPart, which excludes // SharedArrayBuffer from ArrayBufferLike -- `compressed` is always a view over a plain (never shared) - // ArrayBuffer here (subarray of bytes ultimately sourced from Response#arrayBuffer()), mirrors shot.ts's - // own `png as Uint8Array` cast for the identical reason. + // ArrayBuffer here (subarray of bytes ultimately sourced from Response#arrayBuffer()). const stream = new Blob([compressed as Uint8Array]).stream().pipeThrough(new DecompressionStream("deflate-raw")); - const buf = await new Response(stream).arrayBuffer(); - return new Uint8Array(buf); + const reader = stream.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + total += value.byteLength; + if (total > maxBytes) { + await reader.cancel().catch(() => undefined); + return null; + } + chunks.push(value); + } + const out = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + out.set(chunk, offset); + offset += chunk.byteLength; + } + return out; } catch { return null; } } +function isPng(bytes: Uint8Array): boolean { + return PNG_SIGNATURE.every((byte, index) => bytes[index] === byte); +} + /** Read every file entry out of a well-formed ZIP archive (method 0 = stored, or 8 = raw DEFLATE -- the only * two GitHub Actions' own artifact uploader produces). Anything else -- a truncated buffer, a bad signature, * an unsupported compression method, an offset past the buffer end -- degrades that ONE entry (or the whole * read) to being skipped/empty rather than throwing; this parses a REMOTE, only-indirectly-trusted byte * stream (the fork-built artifact), so every read here is bounds-checked before use. */ -export async function parseZipEntries(bytes: Uint8Array): Promise { +export async function parseZipEntries(bytes: Uint8Array, options: { maxEntryBytes?: number } = {}): Promise { try { if (bytes.byteLength < EOCD_MIN_SIZE) return []; const view = new DataView(bytes.buffer, bytes.byteOffset, bytes.byteLength); @@ -277,12 +300,14 @@ export async function parseZipEntries(bytes: Uint8Array): Promise { let centralDirOffset = view.getUint32(eocdOffset + 16, true); const entries: ZipEntry[] = []; const decoder = new TextDecoder(); + const maxEntryBytes = options.maxEntryBytes ?? MAX_ARTIFACT_BYTES; for (let i = 0; i < entryCount; i++) { if (centralDirOffset < 0 || centralDirOffset + CENTRAL_DIR_HEADER_SIZE > bytes.byteLength) break; if (view.getUint32(centralDirOffset, true) !== CENTRAL_DIR_SIGNATURE) break; const method = view.getUint16(centralDirOffset + 10, true); const compressedSize = view.getUint32(centralDirOffset + 20, true); + const uncompressedSize = view.getUint32(centralDirOffset + 24, true); const nameLen = view.getUint16(centralDirOffset + 28, true); const extraLen = view.getUint16(centralDirOffset + 30, true); const commentLen = view.getUint16(centralDirOffset + 32, true); @@ -304,8 +329,8 @@ export async function parseZipEntries(bytes: Uint8Array): Promise { const dataEnd = dataOffset + compressedSize; if (dataOffset >= 0 && dataEnd <= bytes.byteLength) { const compressed = bytes.subarray(dataOffset, dataEnd); - const data = method === 0 ? new Uint8Array(compressed) : method === 8 ? await inflateRawRaw(compressed) : null; - if (data) entries.push({ name, data }); + const data = uncompressedSize > maxEntryBytes ? null : method === 0 ? new Uint8Array(compressed) : method === 8 ? await inflateRawRaw(compressed, maxEntryBytes) : null; + if (data && data.byteLength <= maxEntryBytes) entries.push({ name, data }); } } centralDirOffset = nextCentralDirOffset; @@ -329,7 +354,6 @@ export type FallbackShot = { fileName: string; png: Uint8Array }; // Bounds a hostile/oversized artifact -- MAX_CONFIGURED_ROUTES (5, capture.ts) x 2 viewports x 2 themes, // rounded up, and a generous per-artifact byte cap (well above what ~20 full-page PNGs need in practice). const MAX_FALLBACK_SHOTS = 24; -const MAX_ARTIFACT_BYTES = 60 * 1024 * 1024; function githubApiHeaders(token: string): Headers { const headers = new Headers(); @@ -393,6 +417,7 @@ export async function fetchFallbackArtifactShots(params: { for (const entry of entries) { if (shots.length >= MAX_FALLBACK_SHOTS) break; if (!entry.name.toLowerCase().endsWith(".png")) continue; + if (!isPng(entry.data)) continue; shots.push({ fileName: entry.name, png: entry.data }); } return shots; diff --git a/test/unit/actions-fallback-webhook.test.ts b/test/unit/actions-fallback-webhook.test.ts index 738a3c40af..62cb509d4b 100644 --- a/test/unit/actions-fallback-webhook.test.ts +++ b/test/unit/actions-fallback-webhook.test.ts @@ -38,6 +38,13 @@ function concatBytes(parts: Uint8Array[]): Uint8Array { return out; } +// #4178: extracted .png entries are now validated against the real PNG magic-byte signature before being +// accepted, so a plain-text fixture ("desktop-bytes" etc.) no longer round-trips -- mirrors +// test/unit/actions-fallback.test.ts's own pngBytes() helper. +function pngBytes(label: string): Uint8Array { + return concatBytes([new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), new TextEncoder().encode(label)]); +} + /** Minimal single-entry-per-file ZIP builder (mirrors test/unit/actions-fallback.test.ts's own fixture, kept * file-local rather than shared since it's a small, self-contained test fixture, not production code). */ function buildZip(files: Array<{ name: string; data: Uint8Array }>): Uint8Array { @@ -154,8 +161,8 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => { await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe"); const zip = buildZip([ - { name: "root--desktop.png", data: new TextEncoder().encode("desktop-bytes") }, - { name: "root--mobile.png", data: new TextEncoder().encode("mobile-bytes") }, + { name: "root--desktop.png", data: pngBytes("desktop-bytes") }, + { name: "root--mobile.png", data: pngBytes("mobile-bytes") }, ]); vi.stubGlobal( @@ -191,7 +198,7 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => { const mobileObj = await env.REVIEW_AUDIT!.get(mobileKey); expect(desktopObj).not.toBeNull(); expect(mobileObj).not.toBeNull(); - expect(new TextDecoder().decode(await new Response(desktopObj!.body).arrayBuffer())).toBe("desktop-bytes"); + expect(new Uint8Array(await new Response(desktopObj!.body).arrayBuffer())).toEqual(pngBytes("desktop-bytes")); // The PR row still exists + is untouched in state -- the re-review ran without throwing. expect(await getPullRequest(env, "owner/fallback-repo", 55)).toMatchObject({ state: "open" }); @@ -203,7 +210,7 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => { // Only the desktop shot is present in the artifact -- the mobile one for the same route must be silently // skipped (no crash), while desktop still lands in R2. - const zip = buildZip([{ name: "root--desktop.png", data: new TextEncoder().encode("desktop-only") }]); + const zip = buildZip([{ name: "root--desktop.png", data: pngBytes("desktop-only") }]); vi.stubGlobal( "fetch", baseFetchStub({ @@ -245,7 +252,7 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => { payload: { patch: "@@\n+export default function App() { return null; }" }, }); - const zip = buildZip([{ name: "app--desktop.png", data: new TextEncoder().encode("app-desktop") }]); + const zip = buildZip([{ name: "app--desktop.png", data: pngBytes("app-desktop") }]); vi.stubGlobal( "fetch", baseFetchStub({ @@ -372,7 +379,7 @@ describe("workflow_run webhook -> actions_fallback storage (#4112)", () => { } as unknown as R2Bucket; env.REVIEW_AUDIT = failingAudit; await seedRepoAndPr(env, "cafebabecafebabecafebabecafebabecafebabe"); - const zip = buildZip([{ name: "root--desktop.png", data: new TextEncoder().encode("x") }]); + const zip = buildZip([{ name: "root--desktop.png", data: pngBytes("x") }]); vi.stubGlobal( "fetch", baseFetchStub({ diff --git a/test/unit/actions-fallback.test.ts b/test/unit/actions-fallback.test.ts index 2a01a82c1d..bfd7c79189 100644 --- a/test/unit/actions-fallback.test.ts +++ b/test/unit/actions-fallback.test.ts @@ -62,6 +62,10 @@ function concatBytes(parts: Uint8Array[]): Uint8Array { return out; } +function pngBytes(label: string): Uint8Array { + return concatBytes([new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), new TextEncoder().encode(label)]); +} + function buildZip(files: Array<{ name: string; data: Uint8Array; method: 0 | 8 }>): Uint8Array { const localParts: Uint8Array[] = []; const centralParts: Uint8Array[] = []; @@ -244,6 +248,10 @@ describe("parseZipEntries", () => { return { view: new DataView(zip.buffer, zip.byteOffset, zip.byteLength), centralDirStart, eocdStart }; } + function viewCompressedSize(zip: Uint8Array): number { + return new DataView(zip.buffer, zip.byteOffset, zip.byteLength).getUint32(18, true); + } + it("stops reading once a corrupted entry count walks the central directory past the buffer end", async () => { const zip = buildZip([{ name: "a.png", data: new TextEncoder().encode("A"), method: 0 }]); const { view, eocdStart } = singleEntryZipOffsets(zip, "a.png".length, 1); @@ -306,6 +314,18 @@ describe("parseZipEntries", () => { const entries = await parseZipEntries(zip); expect(entries).toEqual([]); }); + + it("skips a ZIP entry whose declared uncompressed size exceeds the configured cap", async () => { + const zip = buildZip([{ name: "big.png", data: new TextEncoder().encode("tiny"), method: 8 }]); + const { view, centralDirStart } = singleEntryZipOffsets(zip, "big.png".length, viewCompressedSize(zip)); + view.setUint32(centralDirStart + 24, 9, true); + await expect(parseZipEntries(zip, { maxEntryBytes: 8 })).resolves.toEqual([]); + }); + + it("stops inflating a DEFLATE entry once the output exceeds the configured cap", async () => { + const zip = buildZip([{ name: "bomb.png", data: new TextEncoder().encode("A".repeat(4096)), method: 8 }]); + await expect(parseZipEntries(zip, { maxEntryBytes: 1024 })).resolves.toEqual([]); + }); }); describe("dispatchVisualCaptureFallback", () => { @@ -471,8 +491,8 @@ describe("fetchFallbackArtifactShots", () => { it("lists, downloads, validates, and extracts PNG shots end to end", async () => { const zip = buildZip([ - { name: "root--desktop.png", data: new TextEncoder().encode("desktop-bytes"), method: 0 }, - { name: "root--mobile.png", data: new TextEncoder().encode("mobile-bytes"), method: 8 }, + { name: "root--desktop.png", data: pngBytes("desktop-bytes"), method: 0 }, + { name: "root--mobile.png", data: pngBytes("mobile-bytes"), method: 8 }, { name: "manifest.json", data: new TextEncoder().encode("{}"), method: 0 }, ]); stubSequence([ @@ -572,10 +592,21 @@ describe("fetchFallbackArtifactShots", () => { expect(shots).toEqual([]); }); + it("ignores .png entries whose decompressed bytes do not have a PNG signature", async () => { + const zip = buildZip([{ name: "root--desktop.png", data: new TextEncoder().encode("not really a png"), method: 0 }]); + stubSequence([ + () => Response.json({ artifacts: [{ id: 1, name: FALLBACK_ARTIFACT_NAME }] }), + () => new Response(null, { status: 302, headers: { location: "https://pipelines.actions.githubusercontent.com/x.zip" } }), + () => new Response(zip.buffer as ArrayBuffer, { status: 200 }), + ]); + const shots = await fetchFallbackArtifactShots({ token: "tok", repo: { owner: "acme", repo: "widgets" }, runId: 1 }); + expect(shots).toEqual([]); + }); + it("caps the number of returned shots even when the artifact holds more PNGs than the limit", async () => { const files = Array.from({ length: 30 }, (_, i) => ({ name: `route-${i}--desktop.png`, - data: new TextEncoder().encode(`shot-${i}`), + data: pngBytes(`shot-${i}`), method: 0 as const, })); const zip = buildZip(files);