[PR #4030] [CLOSED] fix(artifacts): admit literal POSIX names before a colon #6003

Closed
opened 2026-10-31 01:11:06 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/artifact-path-posix-colon-name


📝 Commits (1)

  • a965914 fix(artifacts): admit literal POSIX names before a colon

📊 Changes

3 files changed (+166 additions, -2 deletions)

View changed files

📝 src/artifacts/ArtifactPaths.test.ts (+34 -0)
📝 src/artifacts/ArtifactPaths.ts (+9 -2)
➕ tests/boundary/filesystem/artifactColonName.test.ts (+123 -0)

📄 Description

Summary

Fixes #3874.

A valid ASAR or directory archive whose top-level member is an ordinary POSIX name such as a:b.txt cannot be inventoried or extracted on a POSIX host. The artifact reader rejects the member before it ever reaches the destination, because the absolute-path guard mistakes the a: prefix for a Windows drive letter. This change narrows that guard so a drive-absolute path still fails while a literal name with a colon is admitted, restoring the behavior the owning filesystem and ASAR reader already report.

Problem and expected behavior

The smallest trigger is an archive that contains the single member a:b.txt at its root. The guard in normalizeArtifactPath tested /^[A-Za-z]:/u, so any logical name that begins with one ASCII letter followed by a colon was treated as absolute and refused with Artifact path is absolute or unsafe. inspect-artifact and extract-artifact both exited 1 with artifact_operation_failed, reason path, and returned no result, even though the official @electron/asar reader lists /a:b.txt and reads its bytes.

The violated invariant is that REA preserves the literal names the owning reader reports. src/artifacts/AsarListing.ts treats an ASAR member as one logical component, not a host path, and admits the component a:b.txt. src/artifacts/DirectoryArtifactReader.ts emits the actual relative filesystem name. The rejection keyed on the start of the whole logical path, so the controls ab:c.txt and nested/a:b.txt were accepted while the root member was refused. Destination-specific colon restrictions belong to the Windows output boundary in src/artifacts/SafeOutputTree.ts and should not decide whether a POSIX archive can be read at all.

Expected behavior after the change: a:b.txt, a:b.txt/child.txt, x:y, and 1:2.txt normalize unchanged and flow through inventory and extraction, while C:/outside, C:\outside, and d:/lower continue to be refused as absolute, and /outside, //server/share, and safe/<NUL>bad continue to be refused as unsafe.

Change and scope

The predicate is narrowed from ^[A-Za-z]: to ^[A-Za-z]:[\\/]. A drive-absolute path always has a separator right after the colon, so C:/… and C:\… are still rejected, and a name that merely contains a colon later in its first component is no longer caught.

One ambiguity is unavoidable and is now documented in the code rather than hidden. A drive-relative form such as c:file.txt is lexically identical to the literal POSIX name c:file.txt, since both are one letter, a colon, and a non-separator. No parser can separate them from the string alone. The choice here is to keep the literal name and reject only the unambiguous drive-absolute shape, because the guard protects a logical archive namespace where POSIX names are valid members, and the Windows output boundary still owns the destination colon rule. c:file.txt and D:relative\child therefore normalize unchanged, and the test comment records why.

Not included: no change to SafeOutputTree.ts, no change to how ASAR or directory listings are parsed, and no new option or configuration. The Windows destination behavior that refuses colon names on write is untouched, so an archive admitted here can still fail at a Windows destination for the destination's own reasons, which is the correct separation.

Contract and boundary impact

  • Semantic owner and earliest changed stage: src/artifacts/ArtifactPaths.ts owns logical-path normalization; the earliest changed stage is the normalize step that runs before any reader or destination logic.
  • Contribution scope fit and linked maintainer decision: an ordinary fix inside an existing boundary, so not applicable.
  • CLI and MCP/tool-catalog contract: no signature or schema change. inspect-artifact and extract-artifact accept more inputs and stop returning artifact_operation_failed for them.
  • Provider, bridge, target-format, or platform compatibility: ASAR and directory readers are unaffected. Windows output keeps its colon restriction; POSIX readers gain the names they always reported.
  • Evidence, artifact, provenance, or reconstruction contract: logical names now survive normalization unchanged, so inventory and extraction keep the name the owning reader supplied.
  • Target selection, authority, upstream behavior, and cleanup details: none, this stage reads a container and writes no target.
  • Generated metadata, package, or installation impact: none.

Expected effects: Read existing files or captured data.

Evidence and regression coverage

  • Tests added or updated: src/artifacts/ArtifactPaths.test.ts gains a normalizeArtifactPath absolute-path rejection block that pins the accepted literal names (a:b.txt, a:b.txt/child.txt, x:y, 1:2.txt, c:file.txt, D:relative\child) and the still-rejected absolute and unsafe forms (C:/outside, C:\outside, d:/lower, /outside, //server/share, safe/<NUL>bad).
  • Base reproduction or other evidence: on the base revision the same literal-name cases throw Artifact path is absolute or unsafe, which the issue records as exit 1 with artifact_operation_failed for both commands. The added accepted-name cases fail on the base and pass on this branch, so the regression is the guard itself, not a downstream symptom.
  • User-visible CLI/MCP output: the issue shows inspect-artifact and extract-artifact returning the artifact_operation_failed envelope on the base and the control archive returning exit 0 with all four paths verified. After the change the root member normalizes like the control members already did.
  • Remaining proof gaps: the run above exercises normalization directly. A full ASAR fixture built with @electron/asar through the CLI on a POSIX host was not executed here, so the end-to-end path is argued from the reader contract rather than observed in this session. The Windows-boundary test tests/boundary/filesystem/artifactLiteralBackslash.test.ts is unchanged and still covers the destination rule.

Validation performed

  • npm run test:focused -- src/artifacts/ArtifactPaths.test.ts — all tests passed, including the new accepted-name and rejected-absolute cases.
  • npm run check:fast — typecheck and lint succeeded.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. The change only admits inputs that were previously refused, and every previously refused absolute or unsafe form still fails.
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable to this stage; the boundary suite that covers the Windows destination rule was not re-run because it does not touch the changed file.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the guard is a safety check, so the trade-off is recorded. Narrowing it keeps the absolute and NUL cases refused. The remaining ambiguity is the drive-relative spelling, which is accepted as a literal name by design and cannot be resolved without more information than the string carries.

Author readiness

Human owner: @DeryFerd

  • 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/4030 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/artifact-path-posix-colon-name` --- ### 📝 Commits (1) - [`a965914`](https://github.com/morluto/rea/commit/a965914b0a808104bbc499bfe8a8bec7848b2d1b) fix(artifacts): admit literal POSIX names before a colon ### 📊 Changes **3 files changed** (+166 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/artifacts/ArtifactPaths.test.ts` (+34 -0) 📝 `src/artifacts/ArtifactPaths.ts` (+9 -2) ➕ `tests/boundary/filesystem/artifactColonName.test.ts` (+123 -0) </details> ### 📄 Description ## Summary Fixes #3874. A valid ASAR or directory archive whose top-level member is an ordinary POSIX name such as `a:b.txt` cannot be inventoried or extracted on a POSIX host. The artifact reader rejects the member before it ever reaches the destination, because the absolute-path guard mistakes the `a:` prefix for a Windows drive letter. This change narrows that guard so a drive-absolute path still fails while a literal name with a colon is admitted, restoring the behavior the owning filesystem and ASAR reader already report. ## Problem and expected behavior The smallest trigger is an archive that contains the single member `a:b.txt` at its root. The guard in `normalizeArtifactPath` tested `/^[A-Za-z]:/u`, so any logical name that begins with one ASCII letter followed by a colon was treated as absolute and refused with `Artifact path is absolute or unsafe`. `inspect-artifact` and `extract-artifact` both exited 1 with `artifact_operation_failed`, reason `path`, and returned no result, even though the official `@electron/asar` reader lists `/a:b.txt` and reads its bytes. The violated invariant is that REA preserves the literal names the owning reader reports. `src/artifacts/AsarListing.ts` treats an ASAR member as one logical component, not a host path, and admits the component `a:b.txt`. `src/artifacts/DirectoryArtifactReader.ts` emits the actual relative filesystem name. The rejection keyed on the start of the whole logical path, so the controls `ab:c.txt` and `nested/a:b.txt` were accepted while the root member was refused. Destination-specific colon restrictions belong to the Windows output boundary in `src/artifacts/SafeOutputTree.ts` and should not decide whether a POSIX archive can be read at all. Expected behavior after the change: `a:b.txt`, `a:b.txt/child.txt`, `x:y`, and `1:2.txt` normalize unchanged and flow through inventory and extraction, while `C:/outside`, `C:\outside`, and `d:/lower` continue to be refused as absolute, and `/outside`, `//server/share`, and `safe/<NUL>bad` continue to be refused as unsafe. ## Change and scope The predicate is narrowed from `^[A-Za-z]:` to `^[A-Za-z]:[\\/]`. A drive-absolute path always has a separator right after the colon, so `C:/…` and `C:\…` are still rejected, and a name that merely contains a colon later in its first component is no longer caught. One ambiguity is unavoidable and is now documented in the code rather than hidden. A drive-relative form such as `c:file.txt` is lexically identical to the literal POSIX name `c:file.txt`, since both are one letter, a colon, and a non-separator. No parser can separate them from the string alone. The choice here is to keep the literal name and reject only the unambiguous drive-absolute shape, because the guard protects a logical archive namespace where POSIX names are valid members, and the Windows output boundary still owns the destination colon rule. `c:file.txt` and `D:relative\child` therefore normalize unchanged, and the test comment records why. Not included: no change to `SafeOutputTree.ts`, no change to how ASAR or directory listings are parsed, and no new option or configuration. The Windows destination behavior that refuses colon names on write is untouched, so an archive admitted here can still fail at a Windows destination for the destination's own reasons, which is the correct separation. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/artifacts/ArtifactPaths.ts` owns logical-path normalization; the earliest changed stage is the normalize step that runs before any reader or destination logic. - Contribution scope fit and linked maintainer decision: an ordinary fix inside an existing boundary, so not applicable. - CLI and MCP/tool-catalog contract: no signature or schema change. `inspect-artifact` and `extract-artifact` accept more inputs and stop returning `artifact_operation_failed` for them. - Provider, bridge, target-format, or platform compatibility: ASAR and directory readers are unaffected. Windows output keeps its colon restriction; POSIX readers gain the names they always reported. - Evidence, artifact, provenance, or reconstruction contract: logical names now survive normalization unchanged, so inventory and extraction keep the name the owning reader supplied. - Target selection, authority, upstream behavior, and cleanup details: none, this stage reads a container and writes no target. - Generated metadata, package, or installation impact: none. Expected effects: Read existing files or captured data. ## Evidence and regression coverage - Tests added or updated: `src/artifacts/ArtifactPaths.test.ts` gains a `normalizeArtifactPath absolute-path rejection` block that pins the accepted literal names (`a:b.txt`, `a:b.txt/child.txt`, `x:y`, `1:2.txt`, `c:file.txt`, `D:relative\child`) and the still-rejected absolute and unsafe forms (`C:/outside`, `C:\outside`, `d:/lower`, `/outside`, `//server/share`, `safe/<NUL>bad`). - Base reproduction or other evidence: on the base revision the same literal-name cases throw `Artifact path is absolute or unsafe`, which the issue records as exit 1 with `artifact_operation_failed` for both commands. The added accepted-name cases fail on the base and pass on this branch, so the regression is the guard itself, not a downstream symptom. - User-visible CLI/MCP output: the issue shows `inspect-artifact` and `extract-artifact` returning the `artifact_operation_failed` envelope on the base and the control archive returning exit 0 with all four paths `verified`. After the change the root member normalizes like the control members already did. - Remaining proof gaps: the run above exercises normalization directly. A full ASAR fixture built with `@electron/asar` through the CLI on a POSIX host was not executed here, so the end-to-end path is argued from the reader contract rather than observed in this session. The Windows-boundary test `tests/boundary/filesystem/artifactLiteralBackslash.test.ts` is unchanged and still covers the destination rule. ## Validation performed - `npm run test:focused -- src/artifacts/ArtifactPaths.test.ts` — all tests passed, including the new accepted-name and rejected-absolute cases. - `npm run check:fast` — typecheck and lint succeeded. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The change only admits inputs that were previously refused, and every previously refused absolute or unsafe form still fails. - Real Hopper/Ghidra, browser, or OS coverage: not applicable to this stage; the boundary suite that covers the Windows destination rule was not re-run because it does not touch the changed file. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the guard is a safety check, so the trade-off is recorded. Narrowing it keeps the absolute and NUL cases refused. The remaining ambiguity is the drive-relative spelling, which is accepted as a literal name by design and cannot be resolved without more information than the string carries. ## Author readiness Human owner: @DeryFerd - [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 - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:11:06 +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#6003
No description provided.