fix(review): bound fallback artifact inflation

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<Uint8Array | null> {
+async function inflateRawRaw(compressed: Uint8Array, maxBytes: number): Promise<Uint8Array | null> {
   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<ArrayBuffer>` cast for the identical reason.
+    // ArrayBuffer here (subarray of bytes ultimately sourced from Response#arrayBuffer()).
     const stream = new Blob([compressed as Uint8Array<ArrayBuffer>]).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<ZipEntry[]> {
+export async function parseZipEntries(bytes: Uint8Array, options: { maxEntryBytes?: number } = {}): Promise<ZipEntry[]> {
   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<ZipEntry[]> {
     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<ZipEntry[]> {
         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;

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({

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