[PR #2365] [MERGED] fix(artifacts): read ASCII control bytes in legacy ZIP names as ASCII #5190

New issue

Closed

opened 2026-10-31 01:04:40 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:40 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2365
Author: @Chaoqi31
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/zip-cp437-control-names

📝 Commits (2)

e957ca4 fix(artifacts): read ASCII control bytes in legacy ZIP names as ASCII
28e843b test(artifacts): check extracted legacy ZIP control names on disk

📊 Changes

4 files changed (+93 additions, -2 deletions)

View changed files

📝 src/artifacts/SafeOutputTree.ts (+5 -0)
📝 src/artifacts/ZipArtifactReader.ts (+21 -2)
📝 tests/boundary/filesystem/artifactZipIntegrity.test.ts (+43 -0)
📝 tests/boundary/filesystem/safeOutputTree.test.ts (+24 -0)

📄 Description

Summary

ZIP member names without the UTF-8 flag kept zip.js's display glyphs for ASCII control bytes. A folder with a custom macOS icon, zipped with zip -r, holds Folder/Icon\r; REA listed and extracted it as Folder/Icon♪, a name that is not in the archive. Legacy names now decode bytes below 0x80 as ASCII, so the member keeps its real name in the inventory and on disk. Windows extraction destinations, which cannot hold such names, now refuse them with a typed path failure before creating anything.

No issue is linked. I found this while comparing inspect-artifact and extract-artifact with Python's zipfile on synthetic archives.

Problem and expected behavior

Smallest trigger: a ZIP entry whose name contains a byte in 0x01-0x1F or 0x7F and whose general-purpose flag lacks the UTF-8 bit. Info-ZIP zip leaves the flag clear for ASCII names, so Icon\r takes this path.

ZipArtifactReader passed names without the UTF-8 flag to zip.js's built-in CP437 decoder. That table maps 0x01-0x1F and 0x7F to IBM PC display glyphs (\r to ♪, \t to ○, DEL to ⌂). The Unicode CP437 mapping, which Python's zipfile uses, keeps 0x00-0x7F as ASCII. Expected: the logical path and the extracted file name are the bytes the archive stores, as they are for a directory target holding the same folder.

Change and scope

decodeZipEntryText, REA's existing zip.js decodeText hook, now decodes cp437 file names itself. Bytes below 0x80 decode as ASCII. The upper half uses the same CP437 table zip.js uses, so names such as é.txt keep their current decoding. UTF-8 names and comments are unchanged.

Windows file names cannot contain U+0001-U+001F. SafeOutputTree already refuses : on Windows destinations before creating anything; it now refuses these characters the same way, with an ArtifactReaderFailure of reason path. Before this, a directory component with such a character reached mkdir on Windows. UTF-8 flagged names could already carry these characters, so the refusal also covers them.

Out of scope: archives that store UTF-8 bytes without setting the flag still decode as CP437, as the ZIP specification requires. Other Windows naming rules (reserved device names, <>"|?*, trailing dots or spaces) are not checked by SafeOutputTree; src/adb/AdbHostFileName.ts already encodes them for adb pulls.

Contribution scope: not applicable (ordinary fix within existing boundaries).

Contract and boundary impact

Semantic owner and earliest changed stage: decodeZipEntryText in src/artifacts/ZipArtifactReader.ts, where ZIP names enter REA, and normalizeDestinationPath in src/artifacts/SafeOutputTree.ts, which owns destination naming rules.
Contribution scope fit and linked maintainer decision: ordinary fix; not applicable.
CLI and MCP/tool-catalog contract: no schema change. Logical paths change only for legacy names that contain 0x01-0x1F or 0x7F. Windows extraction of such names now fails with a typed path failure.
Provider, bridge, target-format, or platform compatibility: ZIP-family artifacts read through ZipArtifactReader; the destination refusal applies only when the destination platform is win32.
Evidence, artifact, provenance, or reconstruction contract: inventory paths and extracted names now match the archive's stored bytes. Existing path validation still applies to the decoded names.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

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.

The checked file change is extract-artifact writing members under its existing output root, now with their real names.

Evidence and regression coverage

Tests added or updated: tests/boundary/filesystem/artifactZipIntegrity.test.ts adds two cases to "preserves %s ZIP member names", Icon\r and the mixed name é\t\u007F.a, each without the UTF-8 flag. Each checks the inventory's logical path with a verified hash. Outside Windows, "extracts %s ZIP members under their stored names" extracts the same two names through extract_artifact and checks the reported relative_path, the directory listing, and the file contents. tests/boundary/filesystem/safeOutputTree.test.ts adds "refuses Windows control characters before creating %j" for a file name (Folder/Icon\r) and a directory name (Fold\ter/file.txt) on a win32 tree, and checks that nothing was created and rollback completes.
Base reproduction or other evidence: with ZipArtifactReader.ts and SafeOutputTree.ts restored from main (fd958808), the four new name cases and the two Windows cases fail; the other tests in both files pass, and one Linux-only test is skipped. A probe that replaces control characters with _ in normalizeDestinationPath fails only the two extraction cases.

User-visible CLI/MCP output (if applicable): a folder holding Icon\r and readme.txt, zipped with zip -qr icon.zip Folder.

--- python zipfile
[('Folder/', '0x0'), ('Folder/Icon\r', '0x0'), ('Folder/readme.txt', '0x0')]

Before (main), logical paths from rea inspect-artifact, then relative_path from rea extract-artifact and the extracted directory listing:

['.', 'Folder', 'Folder/Icon♪', 'Folder/readme.txt']
['Folder/Icon♪', 'Folder/readme.txt']
['readme.txt', 'Icon♪']

After:

['.', 'Folder', 'Folder/Icon\r', 'Folder/readme.txt']
['Folder/Icon\r', 'Folder/readme.txt']
['Icon\r', 'readme.txt']

Remaining proof gaps: I tested archives written by Info-ZIP zip, not archives from other producers. The Windows refusal is tested through SafeOutputTree.create(output, "win32") on macOS, not on a Windows host.

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

On the branch rebased onto main fd958808, with dependencies installed by npm ci: npm run check:fast passes.
npm run test:focused over 99 test files covering artifacts, archives, ZIP reading and safe output (tests/boundary/filesystem/, tests/boundary/providers/native/artifactProvider*, the MCP extraction suite and src/artifacts/). 918 pass and 2 are skipped.
The before/after CLI run above, with npm run build:cached on each checkout.
After 28e843b3, npm run check:fast passes on Node 24.18.0. npm run test:focused over artifactZipIntegrity.test.ts and safeOutputTree.test.ts passes 34 tests and skips 1.

Compatibility, safety, and release

Breaking changes or migration steps: none. Names that contained control glyphs now report the stored control characters, and Windows extraction refuses them instead of writing the glyph name.
Real Hopper/Ghidra, browser, or OS coverage: macOS, Info-ZIP zip 3.0 (Apple build) and Python zipfile as the oracle.
Package or release metadata impact: none.
Security, privacy, process, or containment review: decoded names still pass REA's existing archive path checks before extraction; containment is unchanged.

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/2365 **Author:** [@Chaoqi31](https://github.com/Chaoqi31) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/zip-cp437-control-names` --- ### 📝 Commits (2) - [`e957ca4`](https://github.com/morluto/rea/commit/e957ca46a0cf8a85d988ca0bf2b2df325ed32f16) fix(artifacts): read ASCII control bytes in legacy ZIP names as ASCII - [`28e843b`](https://github.com/morluto/rea/commit/28e843b36d773e329dea512a36edba41e11b4e8e) test(artifacts): check extracted legacy ZIP control names on disk ### 📊 Changes **4 files changed** (+93 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/SafeOutputTree.ts` (+5 -0) 📝 `src/artifacts/ZipArtifactReader.ts` (+21 -2) 📝 `tests/boundary/filesystem/artifactZipIntegrity.test.ts` (+43 -0) 📝 `tests/boundary/filesystem/safeOutputTree.test.ts` (+24 -0) </details> ### 📄 Description ## Summary ZIP member names without the UTF-8 flag kept zip.js's display glyphs for ASCII control bytes. A folder with a custom macOS icon, zipped with `zip -r`, holds `Folder/Icon\r`; REA listed and extracted it as `Folder/Icon♪`, a name that is not in the archive. Legacy names now decode bytes below 0x80 as ASCII, so the member keeps its real name in the inventory and on disk. Windows extraction destinations, which cannot hold such names, now refuse them with a typed path failure before creating anything. No issue is linked. I found this while comparing `inspect-artifact` and `extract-artifact` with Python's `zipfile` on synthetic archives. ## Problem and expected behavior Smallest trigger: a ZIP entry whose name contains a byte in 0x01-0x1F or 0x7F and whose general-purpose flag lacks the UTF-8 bit. Info-ZIP `zip` leaves the flag clear for ASCII names, so `Icon\r` takes this path. `ZipArtifactReader` passed names without the UTF-8 flag to zip.js's built-in CP437 decoder. That table maps 0x01-0x1F and 0x7F to IBM PC display glyphs (`\r` to `♪`, `\t` to `○`, DEL to `⌂`). The Unicode CP437 mapping, which Python's `zipfile` uses, keeps 0x00-0x7F as ASCII. Expected: the logical path and the extracted file name are the bytes the archive stores, as they are for a directory target holding the same folder. ## Change and scope `decodeZipEntryText`, REA's existing zip.js `decodeText` hook, now decodes `cp437` file names itself. Bytes below 0x80 decode as ASCII. The upper half uses the same CP437 table zip.js uses, so names such as `é.txt` keep their current decoding. UTF-8 names and comments are unchanged. Windows file names cannot contain U+0001-U+001F. `SafeOutputTree` already refuses `:` on Windows destinations before creating anything; it now refuses these characters the same way, with an `ArtifactReaderFailure` of reason `path`. Before this, a directory component with such a character reached `mkdir` on Windows. UTF-8 flagged names could already carry these characters, so the refusal also covers them. Out of scope: archives that store UTF-8 bytes without setting the flag still decode as CP437, as the ZIP specification requires. Other Windows naming rules (reserved device names, `<>"|?*`, trailing dots or spaces) are not checked by `SafeOutputTree`; `src/adb/AdbHostFileName.ts` already encodes them for adb pulls. Contribution scope: not applicable (ordinary fix within existing boundaries). ## Contract and boundary impact - Semantic owner and earliest changed stage: `decodeZipEntryText` in `src/artifacts/ZipArtifactReader.ts`, where ZIP names enter REA, and `normalizeDestinationPath` in `src/artifacts/SafeOutputTree.ts`, which owns destination naming rules. - Contribution scope fit and linked maintainer decision: ordinary fix; not applicable. - CLI and MCP/tool-catalog contract: no schema change. Logical paths change only for legacy names that contain 0x01-0x1F or 0x7F. Windows extraction of such names now fails with a typed path failure. - Provider, bridge, target-format, or platform compatibility: ZIP-family artifacts read through `ZipArtifactReader`; the destination refusal applies only when the destination platform is `win32`. - Evidence, artifact, provenance, or reconstruction contract: inventory paths and extracted names now match the archive's stored bytes. Existing path validation still applies to the decoded names. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. 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. - [x] 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. The checked file change is `extract-artifact` writing members under its existing output root, now with their real names. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/artifactZipIntegrity.test.ts` adds two cases to "preserves %s ZIP member names", `Icon\r` and the mixed name `é\t\u007F.a`, each without the UTF-8 flag. Each checks the inventory's logical path with a verified hash. Outside Windows, "extracts %s ZIP members under their stored names" extracts the same two names through `extract_artifact` and checks the reported `relative_path`, the directory listing, and the file contents. `tests/boundary/filesystem/safeOutputTree.test.ts` adds "refuses Windows control characters before creating %j" for a file name (`Folder/Icon\r`) and a directory name (`Fold\ter/file.txt`) on a `win32` tree, and checks that nothing was created and rollback completes. - Base reproduction or other evidence: with `ZipArtifactReader.ts` and `SafeOutputTree.ts` restored from `main` (fd958808), the four new name cases and the two Windows cases fail; the other tests in both files pass, and one Linux-only test is skipped. A probe that replaces control characters with `_` in `normalizeDestinationPath` fails only the two extraction cases. - User-visible CLI/MCP output (if applicable): a folder holding `Icon\r` and `readme.txt`, zipped with `zip -qr icon.zip Folder`. ``` --- python zipfile [('Folder/', '0x0'), ('Folder/Icon\r', '0x0'), ('Folder/readme.txt', '0x0')] ``` Before (`main`), logical paths from `rea inspect-artifact`, then `relative_path` from `rea extract-artifact` and the extracted directory listing: ``` ['.', 'Folder', 'Folder/Icon♪', 'Folder/readme.txt'] ['Folder/Icon♪', 'Folder/readme.txt'] ['readme.txt', 'Icon♪'] ``` After: ``` ['.', 'Folder', 'Folder/Icon\r', 'Folder/readme.txt'] ['Folder/Icon\r', 'Folder/readme.txt'] ['Icon\r', 'readme.txt'] ``` - Remaining proof gaps: I tested archives written by Info-ZIP `zip`, not archives from other producers. The Windows refusal is tested through `SafeOutputTree.create(output, "win32")` on macOS, not on a Windows host. 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 - On the branch rebased onto `main` fd958808, with dependencies installed by `npm ci`: `npm run check:fast` passes. - `npm run test:focused` over 99 test files covering artifacts, archives, ZIP reading and safe output (`tests/boundary/filesystem/`, `tests/boundary/providers/native/artifactProvider*`, the MCP extraction suite and `src/artifacts/`). 918 pass and 2 are skipped. - The before/after CLI run above, with `npm run build:cached` on each checkout. - After 28e843b3, `npm run check:fast` passes on Node 24.18.0. `npm run test:focused` over `artifactZipIntegrity.test.ts` and `safeOutputTree.test.ts` passes 34 tests and skips 1. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Names that contained control glyphs now report the stored control characters, and Windows extraction refuses them instead of writing the glyph name. - Real Hopper/Ghidra, browser, or OS coverage: macOS, Info-ZIP `zip` 3.0 (Apple build) and Python `zipfile` as the oracle. - Package or release metadata impact: none. - Security, privacy, process, or containment review: decoded names still pass REA's existing archive path checks before extraction; containment is unchanged. ## 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-10-31 01:04:40 +00:00