[PR #4187] [CLOSED] fix(binary): confine app program directory to its bundle #6117

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

📋 Pull Request Information

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

Base: main ← Head: fix/app-bundle-symlink-escape


📝 Commits (2)

  • fa89760 fix(binary): keep app bundle paths within selected root
  • ef4d8a2 fix(binary): confine app program directory to its bundle

📊 Changes

3 files changed (+128 additions, -4 deletions)

View changed files

📝 src/application/AppBundleExecutable.test.ts (+74 -0)
📝 src/application/AppBundleExecutable.ts (+17 -4)
📝 tests/boundary/filesystem/binaryTarget.test.ts (+37 -0)

📄 Description

Summary

Fixes #4181. Prevent app-bundle resolution from admitting an executable outside the selected canonical bundle.

Problem and expected behavior

A Contents/MacOS symlink to an external directory could pass because the resolver only checked whether the canonical executable was inside the canonical program directory. The program directory itself must first be inside the canonical bundle.

Change and scope

  • Resolve the canonical bundle and program-directory paths; reject an escaping program directory before resolving its executable.
  • Retain the existing executable-within-program-directory check.
  • Preserve existing Info.plist symlink and path semantics; readable symlinks remain inspectable.
  • Focused local target-resolution bug fix; no new provider, capability, execution, or CLI/MCP contract. Maintainer scope decision: not applicable.

Contract and boundary impact

  • Semantic owner and earliest changed stage: resolveAppBundleExecutable, before provider selection.
  • Contribution scope fit and linked maintainer decision: existing local bundle-path admission bug; no new capability, not applicable.
  • CLI and MCP/tool-catalog contract: no schema or registration changes; invalid program-directory escapes return the existing typed BinaryTargetError algebra.
  • Provider/platform compatibility: internal path aliases are allowed when canonical paths remain inside the selected bundle; external program directories are rejected.
  • Evidence/provenance: preserves caller-selected paths and existing plist inspection behavior.
  • Generated metadata/package impact: none.

Expected effects:

  • 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: injected-filesystem tests cover directory escape short-circuiting and an allowed bundle alias; a real-filesystem boundary test covers an escaping Contents/MacOS symlink. The existing provider test for a readable Info.plist symlink remains passing.
  • Base reproduction or other evidence: the issue report supplies the /usr/bin reproduction. Source inspection confirms the missing comparison between canonical program directory and bundle root; the base revision was not separately rerun here.
  • User-visible CLI/MCP output: no contract change; no separate CLI/MCP transcript.
  • Remaining proof gaps: the real symlink fixture is skipped on Windows; injectable path-resolution tests run cross-platform. No third-party app bundle was used.

Validation performed

  • npm run check:fast — passed (typecheck and lint).
  • npm run test:focused -- tests/boundary/providers/native/artifactProvider.directoryProbes.test.ts tests/boundary/filesystem/binaryTarget.test.ts — 75 passed.
  • npm run test:local -- src/application/AppBundleExecutable.test.ts src/application/AppBundleOpenStepDepth.test.ts — 7 passed.
  • Earlier CI run 38778819108 had 1 failed test (preserves plist inspection for a readable Info.plist symlink), with 1,642 passed and 47 skipped. That exposed an unintended change in the first patch; the current patch restores the existing plist behavior, and the affected suites pass locally. CI run 38783329065 for commit ef4d8a2 passed: all five test shards and the selected package, source, static, and required checks succeeded; unselected docs and real-provider lanes were skipped.

Compatibility, safety, and release

  • Breaking changes or migration steps: none intended.
  • Real Hopper/Ghidra, browser, or OS coverage: no real provider; filesystem fixtures and injected path resolution only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: rejects a program directory outside the selected canonical bundle before resolving the executable; does not execute the target.

Author readiness

Human owner: @gongfpp (review and readiness pending)

  • 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/4187 **Author:** [@gongfpp](https://github.com/gongfpp) **Created:** 10/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/app-bundle-symlink-escape` --- ### 📝 Commits (2) - [`fa89760`](https://github.com/morluto/rea/commit/fa89760f9fe4de5e7121716e398b1e8fd9ddd3b6) fix(binary): keep app bundle paths within selected root - [`ef4d8a2`](https://github.com/morluto/rea/commit/ef4d8a2ae113746c1f36ec766581e3d38a017be1) fix(binary): confine app program directory to its bundle ### 📊 Changes **3 files changed** (+128 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/application/AppBundleExecutable.test.ts` (+74 -0) 📝 `src/application/AppBundleExecutable.ts` (+17 -4) 📝 `tests/boundary/filesystem/binaryTarget.test.ts` (+37 -0) </details> ### 📄 Description ## Summary Fixes #4181. Prevent app-bundle resolution from admitting an executable outside the selected canonical bundle. ## Problem and expected behavior A `Contents/MacOS` symlink to an external directory could pass because the resolver only checked whether the canonical executable was inside the canonical program directory. The program directory itself must first be inside the canonical bundle. ## Change and scope - Resolve the canonical bundle and program-directory paths; reject an escaping program directory before resolving its executable. - Retain the existing executable-within-program-directory check. - Preserve existing `Info.plist` symlink and path semantics; readable symlinks remain inspectable. - Focused local target-resolution bug fix; no new provider, capability, execution, or CLI/MCP contract. Maintainer scope decision: not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: `resolveAppBundleExecutable`, before provider selection. - Contribution scope fit and linked maintainer decision: existing local bundle-path admission bug; no new capability, not applicable. - CLI and MCP/tool-catalog contract: no schema or registration changes; invalid program-directory escapes return the existing typed `BinaryTargetError` algebra. - Provider/platform compatibility: internal path aliases are allowed when canonical paths remain inside the selected bundle; external program directories are rejected. - Evidence/provenance: preserves caller-selected paths and existing plist inspection behavior. - Generated metadata/package impact: none. Expected effects: - [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: injected-filesystem tests cover directory escape short-circuiting and an allowed bundle alias; a real-filesystem boundary test covers an escaping `Contents/MacOS` symlink. The existing provider test for a readable `Info.plist` symlink remains passing. - Base reproduction or other evidence: the issue report supplies the `/usr/bin` reproduction. Source inspection confirms the missing comparison between canonical program directory and bundle root; the base revision was not separately rerun here. - User-visible CLI/MCP output: no contract change; no separate CLI/MCP transcript. - Remaining proof gaps: the real symlink fixture is skipped on Windows; injectable path-resolution tests run cross-platform. No third-party app bundle was used. ## Validation performed - `npm run check:fast` — passed (typecheck and lint). - `npm run test:focused -- tests/boundary/providers/native/artifactProvider.directoryProbes.test.ts tests/boundary/filesystem/binaryTarget.test.ts` — 75 passed. - `npm run test:local -- src/application/AppBundleExecutable.test.ts src/application/AppBundleOpenStepDepth.test.ts` — 7 passed. - Earlier CI run `38778819108` had 1 failed test (`preserves plist inspection for a readable Info.plist symlink`), with 1,642 passed and 47 skipped. That exposed an unintended change in the first patch; the current patch restores the existing plist behavior, and the affected suites pass locally. CI run 38783329065 for commit ef4d8a2 passed: all five test shards and the selected package, source, static, and required checks succeeded; unselected docs and real-provider lanes were skipped. ## Compatibility, safety, and release - Breaking changes or migration steps: none intended. - Real Hopper/Ghidra, browser, or OS coverage: no real provider; filesystem fixtures and injected path resolution only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: rejects a program directory outside the selected canonical bundle before resolving the executable; does not execute the target. ## Author readiness Human owner: @gongfpp (review and readiness pending) - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:12: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#6117
No description provided.