morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #1081] [CLOSED] fix(artifacts): bound ZIP entry extraction to the 64 MiB per-entry budget #2629

New issue

Closed

opened 2026-10-17 21:08:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-17 21:08:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1081
Author: @DeryFerd
Created: 10/8/2026
Status: ❌ Closed

Base: main ← Head: fix/zip-entry-budget

📝 Commits (1)

a0a7644 fix(artifacts): bound ZIP entry extraction to the 64 MiB per-entry budget

📊 Changes

2 files changed (+108 additions, -0 deletions)

View changed files

➕ src/artifacts/ZipArtifactReader.test.ts (+98 -0)
📝 src/artifacts/ZipArtifactReader.ts (+10 -0)

📄 Description

Summary

ZipArtifactReader decompressed every regular ZIP entry without any ceiling. The declared size was already surfaced to inventory as declaredSize, but open() never enforced it, so a small compressed archive could drive unbounded decompression through the reader's internal stream and exhaust memory during extraction. This PR rejects entries over 64 MiB at open() with the reader's existing typed failure, before any decompression starts.

There is no linked issue. I did not find an open or closed issue tracking this; the nearby work is #594 (interrupt ZIP extraction under backpressure), #1013 (overlap check performance), and #847 (path constraint diagnostics), none of which add a size budget.

Problem and expected behavior

Smallest trigger: a crafted ZIP whose single file entry decompresses past 64 MiB (the fixture in this PR is a few hundred bytes on disk after compression). On the base revision, reader.open(entry) resolves a stream and entry.getData() decompresses the full payload with only backpressure applied:

AssertionError: promise resolved "PassThrough{ ... }" instead of rejecting

The violated invariant is the resource bound on untrusted target bytes: a value the format itself declares (uncompressedSize) should gate allocation before the work happens, not after. Expected behavior: an oversized entry refuses extraction with a typed, caller-visible error that names the limit, while inventory still lists the entry and its declared size so observation stays complete.

The caller sees the existing limit projection end to end:

{
  "reason": "limit",
  "message": "ZIP entry exceeds the 64 MiB per-entry limit: huge.bin"
}

ArtifactProvider passes the reason through as ArtifactOperationError, and analysisErrorProjection already maps reason: "limit" to category truncated with truncated: true, the same path the asset-catalog and per-document decode limits use today.

Change and scope

I added MAX_ENTRY_BYTES = 64 * 1024 * 1024 in ZipArtifactReader and one check in open() after the existing directory, symlink, and encrypted-entry rejections: when stored.uncompressedSize exceeds the budget, open() rejects with ArtifactReaderFailure("limit", ...) instead of resolving the extraction stream. The number is not invented: KeyedArchiveReader, AppleAssetCatalogAnalysis, and InterfaceBuilderAnalysis all use 64 MiB per-document budgets, and InterfaceBuilderAnalysis already words its error as a "64 MiB per-document decode limit", so this lines the ZIP reader up with its siblings.

The check enforces the declared size, which is sufficient because zip.js 2.15 already verifies streamed output against the declaration (I confirmed this by patching a fixture header to lie about its size; getData fails with "Invalid uncompressed size"). Classic zip bombs declare their true huge size, so refusing the declaration at open() stops them before inflate runs. Inventory behavior is untouched by design: oversized entries are still enumerated with their real declaredSize, and only extraction refuses them.

Intentionally not included: an aggregate budget across an archive's total extracted bytes (consumers such as extraction already apply their own totals, and a second aggregate belongs with that layer), and any change to path validation, which #847 already reports precisely.

Contract and boundary impact

Semantic owner and earliest changed stage: artifacts adapter; src/artifacts/ZipArtifactReader.ts owns the check, and the effect starts at reader.open(), before any bytes are decompressed.
CLI and MCP/tool-catalog contract: no input or output schema changes and no catalog edits. inventory_artifact output is unchanged; extraction of a single entry over 64 MiB now fails with reason limit (projected as category truncated, truncated: true) instead of streaming an unbounded payload.
Provider, bridge, target-format, or platform compatibility: the reader serves the zip, ipa, apk, msix, and appx formats, so all of them gain the same per-entry budget. No provider or bridge code changed, and the check is plain number comparison with no platform behavior.
Evidence, artifact, provenance, or reconstruction contract: inventory observation is preserved (a test asserts the oversized entry still appears with its declared size); digests, provenance, and artifact identity handling are untouched.
Process execution, authorization, cleanup, or containment impact: none.
Generated metadata (docs/product-catalog.json), package, or installation impact: none; no contract descriptions changed, so no catalog regeneration is part of this PR.

Evidence and regression coverage

Tests added or updated: src/artifacts/ZipArtifactReader.test.ts with three cases: an over-budget entry is rejected with ArtifactReaderFailure reason limit, an entry within the budget still extracts to the expected content, and inventory still observes an over-budget entry with its declared size.
Base reproduction or other evidence: on the base revision the first test fails with "promise resolved instead of rejecting" (the stream was created); with the check it rejects before any decompression. The declared-size guarantee was probed directly against the pinned zip.js version with a patched header, which produced "Invalid uncompressed size".
User-visible CLI/MCP output (if applicable): the limit failure message shown above, surfaced through the existing truncated category for limit-class errors.
Remaining proof gaps: none for this extraction path. The aggregate-archive budget is deliberately out of scope and noted above; if maintainers prefer a different per-entry ceiling than the 64 MiB used by the sibling readers, the change is the single constant.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run typecheck — passed, 0 errors.
npx vitest run src/artifacts/ZipArtifactReader.test.ts tests/boundary/filesystem/artifactZipIntegrity.test.ts --coverage.enabled=false — 2 files, 6 tests passed.
npx vitest run src/artifacts tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/filesystem/zipDetection.test.ts --coverage.enabled=false — 10 files, 49 tests passed, 1 skipped; no regressions in the reader, extraction, or integrity suites.
Two AppleAssetCatalogAnalysis tests fail on this base revision with and without the branch applied (reproduced with the change stashed), so they are pre-existing and unrelated.
npx oxlint src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts — 0 errors.
npx oxfmt --check src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts — clean.
Contract and generated metadata did not change, so no catalog or docs generation gates were touched.
Real Hopper/Ghidra, browser, or other real-provider lanes: not run; no provider, bridge, or browser code changed.

Compatibility, safety, and release

Breaking changes or migration steps: extraction of a single entry over 64 MiB previously proceeded without a bound and now returns a typed limit error. Archives whose largest entry exceeds 64 MiB will refuse extraction; inventory still lists them. No migration steps; worth one line in the release notes as a resource-safety tightening, and the ceiling is a single constant if a different budget is preferred.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider or platform-specific code changed, and the new tests build their fixtures with zip.js in the standard suite.
Package or release metadata impact: none; no dependency, lockfile, or generated metadata changes.
Security, privacy, process, or containment review: closes an unbounded decompression path reachable from any untrusted ZIP-family archive the operator analyzes, replacing it with a pre-allocation refusal that follows the existing typed-limit convention.

Review checklist

The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1081 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/8/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/zip-entry-budget` --- ### 📝 Commits (1) - [`a0a7644`](https://github.com/morluto/rea/commit/a0a764492782a40ede92e79eaeb09ac603280620) fix(artifacts): bound ZIP entry extraction to the 64 MiB per-entry budget ### 📊 Changes **2 files changed** (+108 additions, -0 deletions) <details> <summary>View changed files</summary> ➕ `src/artifacts/ZipArtifactReader.test.ts` (+98 -0) 📝 `src/artifacts/ZipArtifactReader.ts` (+10 -0) </details> ### 📄 Description ## Summary `ZipArtifactReader` decompressed every regular ZIP entry without any ceiling. The declared size was already surfaced to inventory as `declaredSize`, but `open()` never enforced it, so a small compressed archive could drive unbounded decompression through the reader's internal stream and exhaust memory during extraction. This PR rejects entries over 64 MiB at `open()` with the reader's existing typed failure, before any decompression starts. There is no linked issue. I did not find an open or closed issue tracking this; the nearby work is #594 (interrupt ZIP extraction under backpressure), #1013 (overlap check performance), and #847 (path constraint diagnostics), none of which add a size budget. ## Problem and expected behavior Smallest trigger: a crafted ZIP whose single file entry decompresses past 64 MiB (the fixture in this PR is a few hundred bytes on disk after compression). On the base revision, `reader.open(entry)` resolves a stream and `entry.getData()` decompresses the full payload with only backpressure applied: ``` AssertionError: promise resolved "PassThrough{ ... }" instead of rejecting ``` The violated invariant is the resource bound on untrusted target bytes: a value the format itself declares (`uncompressedSize`) should gate allocation before the work happens, not after. Expected behavior: an oversized entry refuses extraction with a typed, caller-visible error that names the limit, while inventory still lists the entry and its declared size so observation stays complete. The caller sees the existing limit projection end to end: ```json { "reason": "limit", "message": "ZIP entry exceeds the 64 MiB per-entry limit: huge.bin" } ``` `ArtifactProvider` passes the reason through as `ArtifactOperationError`, and `analysisErrorProjection` already maps `reason: "limit"` to category `truncated` with `truncated: true`, the same path the asset-catalog and per-document decode limits use today. ## Change and scope I added `MAX_ENTRY_BYTES = 64 * 1024 * 1024` in `ZipArtifactReader` and one check in `open()` after the existing directory, symlink, and encrypted-entry rejections: when `stored.uncompressedSize` exceeds the budget, `open()` rejects with `ArtifactReaderFailure("limit", ...)` instead of resolving the extraction stream. The number is not invented: `KeyedArchiveReader`, `AppleAssetCatalogAnalysis`, and `InterfaceBuilderAnalysis` all use 64 MiB per-document budgets, and `InterfaceBuilderAnalysis` already words its error as a "64 MiB per-document decode limit", so this lines the ZIP reader up with its siblings. The check enforces the declared size, which is sufficient because zip.js 2.15 already verifies streamed output against the declaration (I confirmed this by patching a fixture header to lie about its size; `getData` fails with "Invalid uncompressed size"). Classic zip bombs declare their true huge size, so refusing the declaration at `open()` stops them before inflate runs. Inventory behavior is untouched by design: oversized entries are still enumerated with their real `declaredSize`, and only extraction refuses them. Intentionally not included: an aggregate budget across an archive's total extracted bytes (consumers such as extraction already apply their own totals, and a second aggregate belongs with that layer), and any change to path validation, which #847 already reports precisely. ## Contract and boundary impact - Semantic owner and earliest changed stage: artifacts adapter; `src/artifacts/ZipArtifactReader.ts` owns the check, and the effect starts at `reader.open()`, before any bytes are decompressed. - CLI and MCP/tool-catalog contract: no input or output schema changes and no catalog edits. `inventory_artifact` output is unchanged; extraction of a single entry over 64 MiB now fails with reason `limit` (projected as category `truncated`, `truncated: true`) instead of streaming an unbounded payload. - Provider, bridge, target-format, or platform compatibility: the reader serves the `zip`, `ipa`, `apk`, `msix`, and `appx` formats, so all of them gain the same per-entry budget. No provider or bridge code changed, and the check is plain number comparison with no platform behavior. - Evidence, artifact, provenance, or reconstruction contract: inventory observation is preserved (a test asserts the oversized entry still appears with its declared size); digests, provenance, and artifact identity handling are untouched. - Process execution, authorization, cleanup, or containment impact: none. - Generated metadata (`docs/product-catalog.json`), package, or installation impact: none; no contract descriptions changed, so no catalog regeneration is part of this PR. ## Evidence and regression coverage - Tests added or updated: `src/artifacts/ZipArtifactReader.test.ts` with three cases: an over-budget entry is rejected with `ArtifactReaderFailure` reason `limit`, an entry within the budget still extracts to the expected content, and inventory still observes an over-budget entry with its declared size. - Base reproduction or other evidence: on the base revision the first test fails with "promise resolved instead of rejecting" (the stream was created); with the check it rejects before any decompression. The declared-size guarantee was probed directly against the pinned zip.js version with a patched header, which produced "Invalid uncompressed size". - User-visible CLI/MCP output (if applicable): the `limit` failure message shown above, surfaced through the existing `truncated` category for limit-class errors. - Remaining proof gaps: none for this extraction path. The aggregate-archive budget is deliberately out of scope and noted above; if maintainers prefer a different per-entry ceiling than the 64 MiB used by the sibling readers, the change is the single constant. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run typecheck` — passed, 0 errors. - `npx vitest run src/artifacts/ZipArtifactReader.test.ts tests/boundary/filesystem/artifactZipIntegrity.test.ts --coverage.enabled=false` — 2 files, 6 tests passed. - `npx vitest run src/artifacts tests/boundary/filesystem/artifactZipIntegrity.test.ts tests/boundary/filesystem/zipDetection.test.ts --coverage.enabled=false` — 10 files, 49 tests passed, 1 skipped; no regressions in the reader, extraction, or integrity suites. - Two `AppleAssetCatalogAnalysis` tests fail on this base revision with and without the branch applied (reproduced with the change stashed), so they are pre-existing and unrelated. - `npx oxlint src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts` — 0 errors. - `npx oxfmt --check src/artifacts/ZipArtifactReader.ts src/artifacts/ZipArtifactReader.test.ts` — clean. - Contract and generated metadata did not change, so no catalog or docs generation gates were touched. - Real Hopper/Ghidra, browser, or other real-provider lanes: not run; no provider, bridge, or browser code changed. ## Compatibility, safety, and release - Breaking changes or migration steps: extraction of a single entry over 64 MiB previously proceeded without a bound and now returns a typed limit error. Archives whose largest entry exceeds 64 MiB will refuse extraction; inventory still lists them. No migration steps; worth one line in the release notes as a resource-safety tightening, and the ceiling is a single constant if a different budget is preferred. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider or platform-specific code changed, and the new tests build their fixtures with zip.js in the standard suite. - Package or release metadata impact: none; no dependency, lockfile, or generated metadata changes. - Security, privacy, process, or containment review: closes an unbounded decompression path reachable from any untrusted ZIP-family archive the operator analyzes, replacing it with a pre-allocation refusal that follows the existing typed-limit convention. ## Review checklist - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-17 21:08:23 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-10-31 01:05:27 +00:00

[PR #2511] [MERGED] fix(ci): ignore generated website translation staging #5286

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#2629

No description provided.

Rows
Columns