From 82e4e032dfe97b7912cebc2812e1e9fc41c1eb33 Mon Sep 17 00:00:00 2001 From: Afloat <132809511+Afloat16@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:13:24 +0800 Subject: [PATCH] fix(artifacts): complete positional ZIP reads (#4414) fix(artifacts): complete ZIP positional reads --- src/artifacts/ZipArtifactReader.test.ts | 162 +++++++++++++++++++++++- src/artifacts/ZipArtifactReader.ts | 18 ++- 2 files changed, 177 insertions(+), 3 deletions(-) diff --git a/src/artifacts/ZipArtifactReader.test.ts b/src/artifacts/ZipArtifactReader.test.ts index e3486a16d0..702ae67475 100644 --- a/src/artifacts/ZipArtifactReader.test.ts +++ b/src/artifacts/ZipArtifactReader.test.ts @@ -9,11 +9,12 @@ import { Uint8ArrayWriter, ZipWriter, } from "@zip.js/zip.js"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; import { ZipArtifactReader } from "./ZipArtifactReader.js"; +import { openRegularFile } from "../filesystem/RegularFile.js"; import { MINIMUM_ARTIFACT_EXPANDED_BYTES } from "../domain/artifactExpansionBudget.js"; /** Small real ZIP carriers with forged sizes; metadata-only controls, never decompressed. */ @@ -53,6 +54,165 @@ const enumerate = async (reader: ZipArtifactReader): Promise => { return files; }; +const admittedZip = async () => { + const directory = await createTestTempDirectory("rea-zip-short-read-"); + const path = join(directory, "valid.zip"); + const content = "Complete ZIP member bytes, including café."; + const writer = new ZipWriter(new Uint8ArrayWriter()); + await writer.add("member.txt", new TextReader(content)); + await writeFile(path, await writer.close()); + const handle = await openRegularFile(path, { symlinks: "reject" }); + // Keep the actual descriptor, but select the positional overload used by + // the ZIP adapter so the spy does not infer FileHandle.read's options form. + const positional: { + read( + buffer: Buffer, + offset: number, + length: number, + position: number, + ): Promise<{ bytesRead: number; buffer: Buffer }>; + } = handle; + return { path, content, handle, positional, initial: await handle.stat() }; +}; + +describe("ZipArtifactReader positional reads", () => { + it("enumerates and extracts a real ZIP when each descriptor read returns a short prefix", async () => { + const fixture = await admittedZip(); + const nativeRead = fixture.positional.read.bind(fixture.handle); + const reads = vi + .spyOn(fixture.positional, "read") + .mockImplementation(async (...args) => { + const [buffer, offset, length, position] = args; + return nativeRead(buffer, offset, Math.min(length, 3), position); + }); + const reader = new ZipArtifactReader( + fixture.path, + "zip", + undefined, + fixture, + ); + try { + const entries = []; + for await (const entry of reader.entries()) entries.push(entry); + expect(entries.map(({ path }) => path)).toEqual(["member.txt"]); + const entry = entries[0]; + if (entry === undefined) throw new Error("Expected a ZIP member"); + const chunks: Buffer[] = []; + for await (const chunk of await reader.open(entry)) + chunks.push(Buffer.from(chunk)); + expect(Buffer.concat(chunks)).toEqual(Buffer.from(fixture.content)); + expect(reads.mock.calls.length).toBeGreaterThan(3); + } finally { + reads.mockRestore(); + await reader.close(); + await fixture.handle.close(); + } + }); + + it("reports premature EOF inside the admitted extent as an integrity failure", async () => { + const fixture = await admittedZip(); + const nativeRead = fixture.positional.read.bind(fixture.handle); + let calls = 0; + const reads = vi + .spyOn(fixture.positional, "read") + .mockImplementation(async (...args) => { + const [buffer, offset, length, position] = args; + if (calls++ > 0) return { buffer, bytesRead: 0 }; + return nativeRead(buffer, offset, Math.min(length, 3), position); + }); + const reader = new ZipArtifactReader( + fixture.path, + "zip", + undefined, + fixture, + ); + try { + await expect(enumerate(reader)).rejects.toMatchObject({ + reason: "integrity", + }); + expect(calls).toBe(2); + } finally { + reads.mockRestore(); + await reader.close(); + await fixture.handle.close(); + } + }); + + it("keeps metadata allocation admission ahead of short reads", async () => { + const fixture = await admittedZip(); + const reads = vi.spyOn(fixture.positional, "read"); + const reader = new ZipArtifactReader(fixture.path, "zip", 2, fixture); + try { + await expect(enumerate(reader)).rejects.toMatchObject({ + reason: "limit", + }); + expect(reads).not.toHaveBeenCalled(); + } finally { + reads.mockRestore(); + await reader.close(); + await fixture.handle.close(); + } + }); + + it("does not publish an entry cancelled during short metadata reads", async () => { + const fixture = await admittedZip(); + const controller = new AbortController(); + const nativeRead = fixture.positional.read.bind(fixture.handle); + const reads = vi + .spyOn(fixture.positional, "read") + .mockImplementation(async (...args) => { + const [buffer, offset, length, position] = args; + const result = await nativeRead( + buffer, + offset, + Math.min(length, 3), + position, + ); + controller.abort(); + return result; + }); + const reader = new ZipArtifactReader( + fixture.path, + "zip", + undefined, + fixture, + ); + const iterator = reader.entries(controller.signal)[Symbol.asyncIterator](); + try { + await expect(iterator.next()).rejects.toMatchObject({ + reason: "cancelled", + }); + } finally { + await iterator.return?.(); + reads.mockRestore(); + await reader.close(); + await fixture.handle.close(); + } + }); + + it("rejects a changed admitted descriptor before reading ZIP bytes", async () => { + const fixture = await admittedZip(); + await writeFile(fixture.path, Buffer.alloc(0)); + const reads = vi.spyOn(fixture.positional, "read"); + const reader = new ZipArtifactReader( + fixture.path, + "zip", + undefined, + fixture, + ); + try { + await expect(enumerate(reader)).rejects.toMatchObject({ + reason: "integrity", + }); + expect(reads).not.toHaveBeenCalled(); + } finally { + reads.mockRestore(); + await reader.close(); + await fixture.handle.close(); + } + }); +}); + describe("ZipArtifactReader decompression budget", () => { it("refuses an archive whose entries declare more than its size can justify", async () => { const directory = await createTestTempDirectory("rea-zip-budget-bomb-"); diff --git a/src/artifacts/ZipArtifactReader.ts b/src/artifacts/ZipArtifactReader.ts index 763192af3d..61f58b9ae2 100644 --- a/src/artifacts/ZipArtifactReader.ts +++ b/src/artifacts/ZipArtifactReader.ts @@ -144,8 +144,22 @@ class NodeFileReader extends Reader { `ZIP metadata read requires ${readBytes} bytes, exceeding the ${this.maximumReadBytes}-byte read budget`, ); const bytes = Buffer.alloc(readBytes); - const read = await handle.read(bytes, 0, bytes.length, index); - return bytes.subarray(0, read.bytesRead); + let offset = 0; + while (offset < bytes.length) { + const { bytesRead } = await handle.read( + bytes, + offset, + bytes.length - offset, + index + offset, + ); + if (bytesRead === 0) + throw new ArtifactReaderFailure( + "integrity", + `ZIP source ended before its admitted extent: ${this.path}`, + ); + offset += bytesRead; + } + return bytes; } async closeHandle(): Promise {