Skip to content

fix(launcher): verify PATH ancestry and reject temporary app bundles - #7022

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/upstream-launcher-path-20261013
Oct 13, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:codex/upstream-launcher-path-20261013

Conversation

@luvs01

@luvs01 luvs01 commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Validate the original POSIX PATH ancestry, symlink hops, canonical identity, owner and writable modes before probing a Bun executable. Shared sticky temporary directories no longer qualify as trusted ancestry.
  • Refuse persistent terminal-command registration from raw or canonical temporary app bundles. An ordinary ancestor supplied through TMPDIR does not by itself make a permanent installation temporary.
  • Carry the focused change from source commit 48c6928e75697ab2d10888bc69ae6b3f6370e463 onto dev commit 6da2774369aa32552e318e333902499683d053bc, preserving the launcher contract and regression coverage.

Verification

  • Windows, pinned test runner Bun 1.4.3: bun test --isolate --parallel=1 tests/cli/ocx-launcher-runtime.test.ts tests/ci-workflows/bun-runtime.test.ts initially produced 87 pass, 6 POSIX-only skip and 6 timeout failures with the default 5-second test limit.
  • The repository CI batch script specifies --timeout 60000. Re-running the affected Node launcher PATH fallback activation group with that same setting passed all 7 cases. Product probe deadlines and test assertions were unchanged. All 93 applicable cases therefore passed across these scoped runs; this is not a claim of one clean full-suite run.
  • bun run typecheck, bun run structure:check, and git diff --check passed. bun run privacy:scan passed for the materialized tracked files, including every changed file; the local checkout is sparse.
  • The source revision's exact-head CI completed Linux 4, Windows 9 and macOS 3 test shards, plus the Linux desktop formatting, Clippy, Rust tests and packaged-shell checks. Those results are source-revision evidence, not CI for this new commit.
  • Rust was not recompiled locally because this machine has no current Cargo/Rust toolchain. Local POSIX execution and the full suite were not repeated: focused Windows coverage and the existing unchanged-source CI evidence were used while preparing several independent changes without another large toolchain installation. New-head CI and maintainer review remain required; keep this PR in Draft.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>

Summary by CodeRabbit

  • Bug Fixes
    • POSIX Bun executables found through PATH now undergo stricter checks of file ownership, permissions, and symlink paths before they can be used.
    • Persistent bundle registration now rejects bundles in recognized system or operating-system temporary locations, including qualifying custom TMPDIR locations. Ordinary application folders remain eligible with unrelated TMPDIR overrides.
  • Documentation
    • Updated installation and launcher guidance to explain the stricter POSIX executable checks and temporary-location restrictions.

Validate original and resolved POSIX PATH ownership, modes and symlink hops before executing a Bun candidate. Reject raw and canonical temporary app bundles while retaining permanent installations beneath ordinary TMPDIR overrides.

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 21d25079-2771-47c8-97d7-a721a3829cb1
📥 Commits

Reviewing files that changed from the base of the PR and between 7a40fb0 and d38c8ed.

📒 Files selected for processing (3)
  • desktop/src-tauri/src/cli_command_posix.rs
  • docs-site/src/content/docs/getting-started/installation.md
  • docs-site/src/content/docs/ko/getting-started/installation.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

The change adds component-aware checks for temporary bundle locations and stricter POSIX validation for Bun candidates found through PATH. It also updates related documentation and tests.

Changes: Persistent bundle location checks

Layer / File(s) Summary
Temporary bundle detection
desktop/src-tauri/src/cli_command_posix.rs, structure/desktop-terminal-command.md
stable_bundle checks raw and canonical paths for recognized temporary roots, /Volumes, and exact AppTranslocation components. A canonicalized TMPDIR counts as temporary only under the stated conditions. Tests cover root matching, translocation components, custom TMPDIR values, and stable bundle fixtures.

POSIX Bun candidate validation

Layer / File(s) Summary
POSIX candidate path trust
src/lib/bun-path-runtime.d.mts, src/lib/bun-path-runtime.mjs, structure/runtime.md
The filesystem interface adds lstat, readlink, geteuid, and ownership data. POSIX candidates are checked across path components and symlink expansions, with a limit of 40 expansions. Windows candidates bypass these POSIX checks.
Candidate validation coverage
tests/cli/ocx-launcher-runtime.test.ts, tests/ci-workflows/bun-runtime.test.ts, docs-site/src/content/docs/getting-started/installation.md, docs-site/src/content/docs/ko/getting-started/installation.md
Tests cover ownership, writable components, symlinks, case normalization, and trusted fixture roots. The installation guidance describes the POSIX candidate checks.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant PATH as PATH candidate lookup
  participant Trust as trustedPosixCandidate
  participant FS as POSIX filesystem
  participant Launcher as Bun launcher
  PATH->>Trust: Submit POSIX candidate
  Trust->>FS: Inspect components with lstat and readlink
  FS-->>Trust: Return ownership, modes, types, and link targets
  Trust->>FS: Resolve candidate and compare canonical path
  FS-->>Trust: Return canonical path
  Trust-->>Launcher: Accept or reject candidate
Loading
Merge Risk: ⚪ Minimal · up to d38c8

No actionable regression is established in the reviewed changes. The reported test concern applies outside the fixture’s stated checkout precondition; normal exact-head CI should still complete before merge.

Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 38.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 5 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely summarizes the two primary changes: validating PATH ancestry for launcher candidates and rejecting temporary app bundles.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 38.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 5 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 13, 2026
@lidge-jun
lidge-jun marked this pull request as ready for review October 13, 2026 09:15
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T09:24:16.779532Z 7a40fb0 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔇 Additional comments (8)
desktop/src-tauri/src/cli_command_posix.rs (1)

97-97: LGTM!

Also applies to: 108-108, 129-129, 138-138, 196-196, 197-197, 198-198, 206-206, 254-254

structure/desktop-terminal-command.md (1)

30-33: LGTM!

src/lib/bun-path-runtime.mjs (3)

35-38: Handle .. after a symlink expansion with the parent of the expanded path.

Line 36 sets current to posix.dirname(current). A symlink is expanded into components that restart from /, so current is the physical walked path at that point. POSIX semantics resolve .. against the physical directory, and this code does the same. The walk therefore matches kernel resolution for ... Line 69 also checks the walked path against realpath. A divergence would cause rejection, so the check fails closed.


114-123: LGTM!


21-75: 🎯 Functional Correctness

The repository defines rejection of group/world-writable ancestors, including shared sticky directories, as intentional. The comment’s Homebrew/Nix examples do not establish a supported layout that violates this policy. No change is supported by the inspected evidence.

src/lib/bun-path-runtime.d.mts (1)

8-17: LGTM!

structure/runtime.md (1)

123-123: LGTM!

tests/ci-workflows/bun-runtime.test.ts-28-47 (1)

28-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Duplicate fixture-root helper.

trustedPathRoot (Lines 35-46) repeats pathFixtureRoot in tests/cli/ocx-launcher-runtime.test.ts (Lines 398-406). Both helpers use the same base path and mode. Extract one shared helper into tests/helpers/ so that the two trust assumptions cannot drift apart.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: d896d8aa-d6bb-4f9a-8443-7d86b0ad5d9f
📥 Commits

Reviewing files that changed from the base of the PR and between d934632 and 7a40fb0.

📒 Files selected for processing (7)
  • desktop/src-tauri/src/cli_command_posix.rs
  • src/lib/bun-path-runtime.d.mts
  • src/lib/bun-path-runtime.mjs
  • structure/desktop-terminal-command.md
  • structure/runtime.md
  • tests/ci-workflows/bun-runtime.test.ts
  • tests/cli/ocx-launcher-runtime.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a40fb0370

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/lib/bun-path-runtime.mjs
Comment thread desktop/src-tauri/src/cli_command_posix.rs
@lidge-jun

Copy link
Copy Markdown
Owner

Thanks @luvs01! Maintainer review is done and approved at d38c8ed; I'll squash-merge once CI is green on that exact head. Each new push restarts the full CI queue (currently ~1h), so please hold further pushes on this PR unless something is broken.

@lidge-jun

Copy link
Copy Markdown
Owner

Thanks @luvs01! Reviewed: PATH Bun fallback now validates every original and symlink-expanded ancestor (root/effective-user owned, not group/world writable), and Desktop refuses persistent terminal-command registration from temporary or translocated bundles. Users with a group-writable PATH ancestor can still set OPENCODEX_BUN_PATH; installation docs explain the rule.

Maintainer integration into dev under MAINTAINERS.md (admin dev-only PR bypass). Exact-head CI evidence: every check green on d38c8ede710a2c80b30b310eb8408aee8d2e28cf (test 1–4/4, Windows shards where applicable, enforce-target), all review threads resolved, clean merge onto current dev.

@lidge-jun
lidge-jun merged commit 935c15d into lidge-jun:dev Oct 13, 2026
52 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants