[PR #6937] [MERGED] fix(zip): validate trailing archive record candidates #7459

Closed
opened 2026-11-01 21:48:00 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6937
Author: @ShuaixinHuang
Created: 10/24/2026
Status: ✅ Merged
Merged: 10/24/2026
Merged by: @morluto

Base: main ← Head: codex/fix-zip-comment-record-detection


📝 Commits (4)

  • 3e1bdc2 fix(zip): validate trailing archive record candidates
  • ba51d51 merge: synchronize upstream plist CI repair
  • 47898cb chore: sync upstream plist validation and lifecycle fixes
  • 95162ed chore: sync upstream native and launcher test contracts

📊 Changes

3 files changed (+71 additions, -46 deletions)

View changed files

📝 src/domain/zipPackageFormat.ts (+36 -39)
📝 tests/acceptance/applications/mobileDirectoryComponents.test.ts (+5 -3)
📝 tests/boundary/filesystem/zipDetection.test.ts (+30 -4)

📄 Description

Summary

Recognize a ZIP after leading script bytes when its legal archive comment contains an EOCD-like sequence. Validate candidate directories before accepting the final record candidate, so comment data cannot hide the real record.

Problem and expected behavior

The locator previously chose the last signature whose comment-length field reached EOF and stopped scanning. If that sequence was comment data, failed directory validation rejected the whole valid archive instead of trying the genuine earlier record. This is a newly reproduced format finding; no existing issue is linked.

Change and scope

Move the existing candidate validation into the existing backward scan and continue after an invalid candidate. The same 65,557-byte maximum tail window and single-disk, nonempty, non-Zip64 restrictions remain. No new archive family, extraction authority, allowance or parser abstraction.

Synchronization note: the normal pre-commit hook also simplifies one equivalent encrypted-ZIP fixture options expression in tests/acceptance/applications/mobileDirectoryComponents.test.ts; it does not add a product behavior change.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the readiness and merge checklists
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/domain/zipPackageFormat.ts, trailing-ZIP detection before root classification and binary-target admission.
  • Contribution scope fit and linked maintainer decision: Ordinary correctness fix within existing boundaries; separate maintainer decision not applicable.
  • CLI and MCP/tool-catalog contract: Existing ZIP roots with leading bytes are recognized despite signature-like comment data. Public CLI/MCP schemas and supported archive families are unchanged.
  • Provider, bridge, target-format, or platform compatibility: See scope and platform verification below; no new provider.
  • Evidence, artifact, provenance, or reconstruction contract: Verified member identities and the explicit non-entry-byte limitation remain. Comments and script stubs are not invented as inventory entries.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): Reads the already selected archive through the existing positional-read boundary. No source execution, extraction-policy or cleanup-ownership change.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): None; no generated output, package or installation change.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: Expand the existing leading-byte matrix in tests/boundary/filesystem/zipDetection.test.ts for both offset conventions, with and without an EOCD-like legal comment. Preserve classifier, target admission and verified-member identity assertions plus the existing malformed/symlink controls.
  • Base reproduction or other evidence: On base cc18e9ad, both new comment cases fail: 2 failed, 23 passed, 1 existing Windows symlink skip. With the correction: 25 passed, the same 1 platform skip. An independent @zip.js/zip.js reader accepts the plain and commented fixtures and reads a.txt in both; the source locator changes from undefined to the correct leading byte count 17 for the commented input.
  • User-visible CLI/MCP output (if applicable): Root classification and parseBinaryTarget identify the commented script-prefixed fixture as ZIP; inventory members match the equivalent plain ZIP.
  • Remaining proof gaps: Synthetic valid ZIP fixtures on Windows and an independent ZIP reader. No real self-extracting executable was launched. Zip64, split and empty trailing archives remain outside this locator. No performance improvement claim.

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

Synchronized with upstream b5c42dfc, including #6945, for six shared CI failures: two stale bound-target error expectations and four launcher/package fixture cases. The two source cases reproduce before synchronization (2 failed, 3 passed). After synchronization, all six previously failing cases pass; bound-target permissions and exact corrupted-catalog/CRLF/bytecode rejection assertions remain. Extended validation on the synchronized ZIP tree ran the complete metadata, MCP, TAR, plist and ZIP files: 231 passed, 22 existing platform skips, and one Windows-local catalog-recording timeout. An unchanged isolated rerun passed in 24 seconds; the original timeout remains recorded and does not establish whole-suite success. Real macOS plist CLI tests were skipped on Windows. The six plist boundary files still pass (64 passed, 3 existing platform skips). On this PR tree, 25 ZIP tests passed, with the existing Windows symlink case skipped; npm run check and npm run docs:check passed. Full new-head Linux CI must qualify the synchronized change.

Original fix baseline cc18e9ad; synchronized upstream b5c42dfc; current head 95162edd09.

  • npm run test:focused -- tests/boundary/filesystem/zipDetection.test.ts — 25 passed, 1 existing Windows symlink skip on the independent final branch.
  • Independent ZIP reader and source-locator probe — both valid fixtures readable, leading byte count preserved.
  • npm run check — all six tasks, module boundaries and architecture guards passed.
  • Formatting, git diff --check and normal commit/push hooks passed.
  • Local checks do not establish complete Linux CI success; consult the current-head Checks tab.

Compatibility, safety, and release

  • Breaking changes or migration steps: No migration. Invalid candidates now permit continued detection; the original supported-format and directory/local-header validation rules are retained.

  • Real Hopper/Ghidra, browser, or OS coverage: Pinned Node 24.22.1 on Windows x64, synthetic archive classification/admission/inventory. No executable or native provider run.

  • Package or release metadata impact: None.

  • Security, privacy, process, or containment review: Reads the already selected archive through the existing positional-read boundary. No source execution, extraction-policy or cleanup-ownership change.

Author readiness

Human owner: @ShuaixinHuang

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • 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/6937 **Author:** [@ShuaixinHuang](https://github.com/ShuaixinHuang) **Created:** 10/24/2026 **Status:** ✅ Merged **Merged:** 10/24/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `codex/fix-zip-comment-record-detection` --- ### 📝 Commits (4) - [`3e1bdc2`](https://github.com/morluto/rea/commit/3e1bdc2dbf4f16e0cd9e3ccd652cb877bd4ec19b) fix(zip): validate trailing archive record candidates - [`ba51d51`](https://github.com/morluto/rea/commit/ba51d515d458e38dbc97d29236dccb140bd3c24b) merge: synchronize upstream plist CI repair - [`47898cb`](https://github.com/morluto/rea/commit/47898cb95065222916c88e533c565837d4a6b221) chore: sync upstream plist validation and lifecycle fixes - [`95162ed`](https://github.com/morluto/rea/commit/95162edd0924708460edbd9faa23322aed4a3903) chore: sync upstream native and launcher test contracts ### 📊 Changes **3 files changed** (+71 additions, -46 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/zipPackageFormat.ts` (+36 -39) 📝 `tests/acceptance/applications/mobileDirectoryComponents.test.ts` (+5 -3) 📝 `tests/boundary/filesystem/zipDetection.test.ts` (+30 -4) </details> ### 📄 Description ## Summary Recognize a ZIP after leading script bytes when its legal archive comment contains an EOCD-like sequence. Validate candidate directories before accepting the final record candidate, so comment data cannot hide the real record. ## Problem and expected behavior The locator previously chose the last signature whose comment-length field reached EOF and stopped scanning. If that sequence was comment data, failed directory validation rejected the whole valid archive instead of trying the genuine earlier record. This is a newly reproduced format finding; no existing issue is linked. ## Change and scope Move the existing candidate validation into the existing backward scan and continue after an invalid candidate. The same 65,557-byte maximum tail window and single-disk, nonempty, non-Zip64 restrictions remain. No new archive family, extraction authority, allowance or parser abstraction. Synchronization note: the normal pre-commit hook also simplifies one equivalent encrypted-ZIP fixture options expression in `tests/acceptance/applications/mobileDirectoryComponents.test.ts`; it does not add a product behavior change. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the readiness and merge checklists unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/domain/zipPackageFormat.ts`, trailing-ZIP detection before root classification and binary-target admission. - Contribution scope fit and linked maintainer decision: Ordinary correctness fix within existing boundaries; separate maintainer decision not applicable. - CLI and MCP/tool-catalog contract: Existing ZIP roots with leading bytes are recognized despite signature-like comment data. Public CLI/MCP schemas and supported archive families are unchanged. - Provider, bridge, target-format, or platform compatibility: See scope and platform verification below; no new provider. - Evidence, artifact, provenance, or reconstruction contract: Verified member identities and the explicit non-entry-byte limitation remain. Comments and script stubs are not invented as inventory entries. - Target selection, authority, upstream behavior, and cleanup details (if applicable): Reads the already selected archive through the existing positional-read boundary. No source execution, extraction-policy or cleanup-ownership change. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): None; no generated output, package or installation change. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: Expand the existing leading-byte matrix in tests/boundary/filesystem/zipDetection.test.ts for both offset conventions, with and without an EOCD-like legal comment. Preserve classifier, target admission and verified-member identity assertions plus the existing malformed/symlink controls. - Base reproduction or other evidence: On base cc18e9ad, both new comment cases fail: 2 failed, 23 passed, 1 existing Windows symlink skip. With the correction: 25 passed, the same 1 platform skip. An independent @zip.js/zip.js reader accepts the plain and commented fixtures and reads a.txt in both; the source locator changes from undefined to the correct leading byte count 17 for the commented input. - User-visible CLI/MCP output (if applicable): Root classification and parseBinaryTarget identify the commented script-prefixed fixture as ZIP; inventory members match the equivalent plain ZIP. - Remaining proof gaps: Synthetic valid ZIP fixtures on Windows and an independent ZIP reader. No real self-extracting executable was launched. Zip64, split and empty trailing archives remain outside this locator. No performance improvement claim. 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 Synchronized with upstream `b5c42dfc`, including #6945, for six shared CI failures: two stale bound-target error expectations and four launcher/package fixture cases. The two source cases reproduce before synchronization (2 failed, 3 passed). After synchronization, all six previously failing cases pass; bound-target permissions and exact corrupted-catalog/CRLF/bytecode rejection assertions remain. Extended validation on the synchronized ZIP tree ran the complete metadata, MCP, TAR, plist and ZIP files: 231 passed, 22 existing platform skips, and one Windows-local catalog-recording timeout. An unchanged isolated rerun passed in 24 seconds; the original timeout remains recorded and does not establish whole-suite success. Real macOS plist CLI tests were skipped on Windows. The six plist boundary files still pass (64 passed, 3 existing platform skips). On this PR tree, 25 ZIP tests passed, with the existing Windows symlink case skipped; `npm run check` and `npm run docs:check` passed. Full new-head Linux CI must qualify the synchronized change. Original fix baseline cc18e9ad; synchronized upstream b5c42dfc; current head 95162edd0924708460edbd9faa23322aed4a3903. - `npm run test:focused -- tests/boundary/filesystem/zipDetection.test.ts` — 25 passed, 1 existing Windows symlink skip on the independent final branch. - Independent ZIP reader and source-locator probe — both valid fixtures readable, leading byte count preserved. - `npm run check` — all six tasks, module boundaries and architecture guards passed. - Formatting, git diff --check and normal commit/push hooks passed. - Local checks do not establish complete Linux CI success; consult the current-head Checks tab. ## Compatibility, safety, and release - Breaking changes or migration steps: No migration. Invalid candidates now permit continued detection; the original supported-format and directory/local-header validation rules are retained. - Real Hopper/Ghidra, browser, or OS coverage: Pinned Node 24.22.1 on Windows x64, synthetic archive classification/admission/inventory. No executable or native provider run. - Package or release metadata impact: None. - Security, privacy, process, or containment review: Reads the already selected archive through the existing positional-read boundary. No source execution, extraction-policy or cleanup-ownership change. ## Author readiness Human owner: @ShuaixinHuang - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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-11-01 21:48:00 +00:00
Sign in to join this conversation.
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#7459
No description provided.