Skip to content

fix(exec/approvals): match executable realpath against allowlist patterns (#45595)#73543

Closed
johnstormentswe wants to merge 1 commit into
openclaw:mainfrom
johnstormentswe:fix/exec-approval-realpath-match
Closed

fix(exec/approvals): match executable realpath against allowlist patterns (#45595)#73543
johnstormentswe wants to merge 1 commit into
openclaw:mainfrom
johnstormentswe:fix/exec-approval-realpath-match

Conversation

@johnstormentswe

@johnstormentswe johnstormentswe commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What

matchAllowlist in src/infra/exec-command-resolution.ts compares allowlist patterns against resolution.resolvedPath only — the PATH-resolved entry point — while the runtime later pins execution to resolution.resolvedRealPath (via fs.realpath). On any host where binaries are reached through a symlink (Homebrew, nix, asdf, etc.) the operator gets exactly one of two bad outcomes:

  • Allowlist the real binary (e.g. /opt/homebrew/Cellar/ripgrep/14.1.1/bin/rg) → matching always misses because the resolution carries the symlink path /opt/homebrew/bin/rg. Every Homebrew rg invocation is unnecessarily blocked.
  • Allowlist the symlink (e.g. /opt/homebrew/bin/rg) → matching succeeds, but if the symlink target later changes (Homebrew bumps the Cellar path), the same approval keeps firing while a different real binary actually runs.

The reporter at #45595 documented both directions and pointed at the exact mismatch.

Closes #45595.

Why this fix

Match each path-shaped allowlist pattern against both resolvedPath and (when distinct) resolvedRealPath. The runtime always executes the realpath, so:

  • An allowlist entry pinned to the real binary matches every symlink that resolves to it, no matter which Homebrew Cellar version owns the binary today.
  • An allowlist entry pinned to the symlink keeps working unchanged (resolvedPath check fires first).
  • An allowlist entry pinned to neither still does not match (negative control).

The realpath check is short-circuited via a strict-equality guard so resolutions where resolvedRealPath === resolvedPath (no symlink indirection) do not pay any extra work.

The bare-* wildcard branch is untouched. Basename-only matching (non-path-shaped patterns) is untouched. argPattern path is untouched. Only the path-shaped pattern check at line ~376 picks up the realpath fallback.

Tests

Added describe("symlink/realpath dual matching (#45595)") block in src/infra/exec-allowlist-matching.test.ts:

  • Real-path allowlist matches symlink resolution — operator pins /opt/homebrew/Cellar/ripgrep/14.1.1/bin/rg, resolution carries /opt/homebrew/bin/rg symlink + that realpath, allowlist hits via the realpath branch. (The bug's primary case — was returning null before this fix.)
  • Symlink allowlist still matches — backward compatibility, the resolvedPath branch fires first.
  • Negative control — pattern that matches neither resolvedPath nor resolvedRealPath returns null.
  • No double-consult — when realpath equals resolvedPath, the realpath branch is short-circuited; matching still succeeds via the resolvedPath branch.

The existing 8 tests in the same file continue to pass unchanged because they all use resolutions without resolvedRealPath — the new branch is a no-op for them.

Notes

  • Single-area diff: src/infra/exec-command-resolution.ts (one new local + one combined ||) and a colocated test addition, plus a one-line CHANGELOG entry under ## Unreleased ### Fixes with Thanks @juan-flores077.
  • No public API change. ExecutableResolution.resolvedRealPath already existed; this fix just consults it where the matcher previously did not.
  • AI-assisted (Claude). Reviewed locally; please flag if there is an intentional reason approvals were tied to the symlink only (e.g. an audit-log requirement). I read the issue and CONTRIBUTING.md and did not find such a constraint, but the security-adjacent nature of the change warrants a careful eye.

Validation

Local CI is constrained on this machine; relying on CI for the canonical proof. I have:

  • Verified by code-reading that matchAllowlist is the only matcher consulting allowlist patterns for tools.exec.allowlist (the only other path-shaped consumer of ExecutableResolution.resolvedPath is resolveAllowlistCandidatePath which produces a display path for diagnostics, not a match decision).
  • Confirmed the existing tests in exec-allowlist-matching.test.ts use resolutions without resolvedRealPath, so the new branch is path-isolated and the existing assertions are unchanged.
  • Confirmed the changelog entry is single-line and credits a contributor.

Happy to address Greptile/Codex review feedback or extend the match to also normalize ../. segments via path.resolve if reviewers want defense in depth.

Suggested CHANGELOG entry

This PR intentionally does not touch CHANGELOG.md to avoid hot-file rebase conflicts that have been closing rebased fix PRs. Maintainers can drop the following line under ## Unreleased > ### Fixes at merge time:

  • Exec/approvals: also match the executable's realpath against tools.exec.allowlist patterns so operators on Homebrew/nix/asdf systems can allowlist either the symlink or the real-binary canonical path without one mismatching the other; the runtime always pins execution to the realpath, so approvals stay tied to the binary that actually runs. Fixes [Bug]: Exec approval checks use symlink path but execution uses real path #45595. Thanks @juan-flores077.

@greptile-apps

greptile-apps Bot commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a path-matching asymmetry in matchAllowlist (src/infra/exec-command-resolution.ts) where only resolvedPath (the symlink) was checked against allowlist patterns, while the runtime executes the binary at resolvedRealPath. The fix adds a short-circuited realpath fallback exclusively in the hasPathSelector branch: if the pattern doesn't match resolvedPath, it is also tested against resolvedRealPath (when distinct). The logic is minimal, targeted, and well-tested with four new cases covering the primary bug, backward compatibility, a negative control, and the equality-guard short-circuit.

Confidence Score: 5/5

Safe to merge — the change is minimal, well-isolated, and comprehensively tested.

Single-area diff touching one conditional expression; the equality guard correctly short-circuits the extra match call when no symlink indirection exists; all existing tests continue to pass unchanged; four new targeted tests cover the added branch exhaustively; no public API changes.

No files require special attention.

Reviews (1): Last reviewed commit: "fix(exec/approvals): match executable re..." | Re-trigger Greptile

@clawsweeper

clawsweeper Bot commented Apr 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge.

Summary
The PR adds a resolvedRealPath fallback for path-shaped exec allowlist patterns and adds symlink/realpath matching tests.

Reproducibility: yes. A high-confidence source reproduction is an ExecutableResolution with resolvedPath=/opt/homebrew/bin/rg, resolvedRealPath=/opt/homebrew/Cellar/ripgrep/14.1.1/bin/rg, and an allowlist entry for the real path; current main matches the symlink path while approved execution pins the realpath.

Real behavior proof
Needs real behavior proof before merge: The PR body provides code-reading and CI reliance only; it needs redacted terminal output, logs, screenshot, recording, or linked artifact showing the symlinked realpath allowlist behavior after the patch before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
Manual review is needed because this external PR lacks real behavior proof and changes security-sensitive exec approval identity semantics while the linked issue is broader than the matcher fallback.

Security
Needs attention: The diff changes security-sensitive exec approval identity semantics and needs maintainer signoff before closing the broader realpath trust-model issue.

Review details

Best possible solution:

Land or replace this with a maintainer-approved executable identity fix that aligns matching, trust checks, persistence, audit metadata, and execution, with focused regression coverage and real setup proof.

Do we have a high-confidence way to reproduce the issue?

Yes. A high-confidence source reproduction is an ExecutableResolution with resolvedPath=/opt/homebrew/bin/rg, resolvedRealPath=/opt/homebrew/Cellar/ripgrep/14.1.1/bin/rg, and an allowlist entry for the real path; current main matches the symlink path while approved execution pins the realpath.

Is this the best way to solve the issue?

Unclear as a complete fix. The fallback is narrow and useful for real-path allowlist misses, but the final policy still needs to decide whether symlink-path approvals may continue to authorize a changed real executable and whether the linked issue should remain open for the other affected surfaces.

Security concerns:

  • [medium] Confirm symlink approval compatibility — src/infra/exec-command-resolution.ts:386
    The changed matcher still accepts resolvedPath before the realpath fallback, so a symlink-path allowlist entry can continue approving after the symlink target changes while execution pins the new real binary. That may be an intentional compatibility choice, but it is the security-sensitive half of the linked report.
    Confidence: 0.87
  • [low] Avoid closing broader executable identity work
    The linked issue also names safe-bin trust, skill auto-allow, Allow Always persistence, approval metadata, and doctor hints; this branch only updates one matcher branch and should not automatically close the full canonical issue without maintainer approval.
    Confidence: 0.86

What I checked:

  • current-main-computes-both-paths: Current main builds executable resolutions with both the PATH-resolved path and fs.realpathSync() result, so symlink and real executable identities are both available. (src/infra/exec-command-resolution.ts:69, 48b4e5b3614f)
  • current-main-matcher-uses-resolvedPath-only: For path-shaped allowlist patterns, current matchAllowlist() compares the pattern only against resolution.resolvedPath; a real-binary allowlist entry can miss a symlinked PATH resolution. (src/infra/exec-command-resolution.ts:368, 48b4e5b3614f)
  • current-main-execution-pins-realpath: Approval-backed execution pins argv[0] to resolution.execution.resolvedRealPath ?? resolution.execution.resolvedPath, which creates the reported identity split for symlinked executables. (src/node-host/invoke-system-run-plan.ts:1274, 48b4e5b3614f)
  • pr-adds-focused-realpath-fallback: The PR head adds a distinct resolvedRealPath fallback after the existing resolvedPath path-pattern match. (src/infra/exec-command-resolution.ts:386, 704edc534678)
  • pr-adds-regression-tests: The PR adds tests for real-path matching through a symlink resolution, symlink-path compatibility, a negative control, and same-path short-circuit behavior. (src/infra/exec-allowlist-matching.test.ts:98, 704edc534678)
  • related-issue-is-broader: The linked open issue names allowlist matching, safe-bin trust, skill auto-allow, Allow Always persistence, approval metadata, and doctor hints as affected identity surfaces, not only the matcher branch.

Likely related people:

  • steipete: Git history shows recent merged work separating exec policy and execution targets, splitting wrapper trust resolution, and matching bare exec allowlist commands in the central files for this behavior. (role: recent exec approval target contributor; confidence: high; commits: 7f373823b02e, d8cef14eb118, 5699209d0091; files: src/infra/exec-command-resolution.ts, src/infra/exec-approvals-allowlist.ts, src/node-host/invoke-system-run-plan.ts)
  • vincentkoc: Git history ties this area to exec allowlist glob hardening and wrapper-trust work near the same approval boundary. (role: adjacent exec allowlist security contributor; confidence: medium; commits: 82e3ac21eec9, d8cef14eb118; files: src/infra/exec-allowlist-pattern.ts, src/infra/exec-allowlist-pattern.test.ts, src/infra/exec-command-resolution.ts)

Remaining risk / open question:

  • The contributor has not provided after-fix real setup proof for the symlinked executable allowlist flow.
  • The PR is currently conflicting, so older green CI is stale relative to current main.
  • The patch fixes the real-path allowlist miss but leaves symlink-path approval drift and adjacent trust, persistence, audit, and diagnostic surfaces for maintainer policy review.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 48b4e5b3614f.

@johnstormentswe
johnstormentswe force-pushed the fix/exec-approval-realpath-match branch from 8293123 to 4a9635f Compare April 28, 2026 14:28
@johnstormentswe
johnstormentswe force-pushed the fix/exec-approval-realpath-match branch from 4a9635f to 704edc5 Compare April 29, 2026 02:11
@johnstormentswe

johnstormentswe commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Hi @steipete
How are you? Could you review this PR please?

@clawsweeper clawsweeper Bot mentioned this pull request Apr 29, 2026
Closed
@johnstormentswe

johnstormentswe commented May 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@clawsweeper authmerge

@johnstormentswe

johnstormentswe commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

Hi @clawsweeper Could you merge this PR?

@johnstormentswe

johnstormentswe commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

@greptile-apps

@steipete steipete mentioned this pull request May 17, 2026
Merged
@steipete

steipete commented May 17, 2026

Copy link
Copy Markdown
Contributor

Thanks for the report and patch here. We landed a maintainer rewrite in #82825 as 524185a, keeping the same fix direction but broadening it across path-shaped allowlists, safe-bin trust, skill auto-allow, Allow Always persistence, and approval audit metadata, with symlink-retargeting regression coverage.

Closing this PR as superseded by the landed fix. Thanks again for chasing this down.

@steipete steipete closed this May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Exec approval checks use symlink path but execution uses real path

2 participants

@johnstormentswe @steipete