Prefer non-user writeable paths by optimol · Pull Request #54346 · openclaw/openclaw

optimol · 2026-03-25T07:56:47Z

Summary

Describe the problem and fix in 2–5 bullets: Prefer non-user writeable paths.

Problem:
Why it matters:
What changed:
What did NOT change (scope boundary):

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Closes #
Related #
This PR fixes a bug or regression

Root Cause / Regression History (if applicable)

For bug fixes or regressions, explain why this happened, not just what changed. Otherwise write N/A. If the cause is unclear, write Unknown.

Root cause:
Missing detection / guardrail:
Prior context (git blame, prior PR, issue, or refactor if known):
Why this regressed now:
If unknown, what was ruled out:

Regression Test Plan (if applicable)

For bug fixes or regressions, name the smallest reliable test coverage that should have caught this. Otherwise write N/A.

Coverage level that should have caught this:
- Unit test
- Seam / integration test
- End-to-end test
- Existing coverage already sufficient
Target test or file:
Scenario the test should lock in:
Why this is the smallest reliable guardrail:
Existing test that already covers this (if any):
If no new test is added, why not:

User-visible / Behavior Changes

List user-visible changes (including defaults/config).
If none, write None.

Security Impact (required)

New permissions/capabilities? (Yes/No)
Secrets/tokens handling changed? (Yes/No)
New/changed network calls? (Yes/No)
Command/tool execution surface changed? (Yes/No)
Data access scope changed? (Yes/No)
If any Yes, explain risk + mitigation:

Repro + Verification

Environment

OS:
Runtime/container:
Model/provider:
Integration/channel (if any):
Relevant config (redacted):

Steps

Expected

Actual

Evidence

Attach at least one:

Failing test/log before + passing after
Trace/log snippets
Screenshot/recording
Perf numbers (if relevant)

Human Verification (required)

What you personally verified (not just CI), and how:

Verified scenarios:
Edge cases checked:
What you did not verify:

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.

Compatibility / Migration

Backward compatible? (Yes/No) Yes-ish, installs that only put ffmpeg/ffprobe/openssl under user-writable dirs will no longer be picked up, but that's intended.
Config/env changes? (Yes/No) No
Migration needed? (Yes/No) No
If yes, exact upgrade steps:

Failure Recovery (if this breaks)

How to disable/revert this change quickly:
Files/config to restore:
Known bad symptoms reviewers should watch for:

Risks and Mitigations

List only real risks for this PR. Add/remove entries as needed. If none, write None.

Risk:
- Mitigation:

greptile-apps · 2026-03-25T07:59:30Z

Greptile Summary

This PR hardens the binary execution surface by (1) reordering PATH construction so system-managed directories (/usr/bin, /usr/local/bin, etc.) always come before user-writable locations, and (2) introducing resolveSystemBin, a new utility that resolves ffmpeg, ffprobe, and openssl to absolute paths exclusively within a curated list of trusted system directories instead of relying on the ambient PATH.

Key changes:

src/infra/path-env.ts: System dirs moved to prepend; user-writable dirs (mise shims, pnpm, bun, yarn, brew env-relative paths) moved to append, preventing them from shadowing system binaries in child-process PATH.
src/infra/resolve-system-bin.ts (new): Trusted-dir resolver with per-platform logic (macOS, Linux, NixOS, Windows). Results are cached for the process lifetime. One bug: the resolved cache is keyed only on name and ignores opts?.extraDirs, so a call with extraDirs that finds a binary outside the trusted set will poison the cache for all subsequent callers that omit extraDirs.
src/media/ffmpeg-exec.ts and src/infra/tls/gateway.ts: ffprobe, ffmpeg, and openssl are now resolved via resolveSystemBin instead of bare string names, so they can never be intercepted by a user-controlled PATH entry.

Confidence Score: 4/5

Safe to merge after addressing the extraDirs cache-key bug in resolve-system-bin.ts; all production callers currently omit extraDirs so the risk is latent rather than live, but the API is exported and the fix is straightforward.
The PATH-reordering and resolveSystemBin changes are clean and well-tested. The one concrete logic bug (cache keyed only on name, not on extraDirs) could allow a future caller to unintentionally bypass the trusted-dirs guarantee for subsequent callers in the same process. No production code currently uses extraDirs, so this is a latent rather than active risk, warranting a 4 rather than a lower score.
src/infra/resolve-system-bin.ts — cache key should incorporate extraDirs to prevent cache poisoning.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/infra/resolve-system-bin.ts
Line: 116-143

Comment:
**`extraDirs` poisons the cache for callers without `extraDirs`**

The resolved cache is keyed only on `name`, ignoring `opts?.extraDirs`. If a first caller invokes `resolveSystemBin("mytool", { extraDirs: ["/some/non-system/path"] })` and the binary is found there, the result gets cached under `"mytool"`. Every subsequent call to `resolveSystemBin("mytool")` without `extraDirs` will return that cached non-trusted path, silently bypassing the security guarantee the function is designed to enforce.

Concrete scenario:
1. Caller A: `resolveSystemBin("ffmpeg", { extraDirs: ["/home/user/.local/bin"] })` → caches `/home/user/.local/bin/ffmpeg` under key `"ffmpeg"`.
2. Caller B: `resolveSystemBin("ffmpeg")` → returns `/home/user/.local/bin/ffmpeg` from cache, defeating PATH-hijack protection.

The fix is to include a stable representation of `extraDirs` in the cache key, or to avoid caching results that originate from outside `getTrustedDirs()`. The existing `extraDirs` test doesn't catch this because `_resetResolveSystemBin` clears the cache in `beforeEach`.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "prefer system paths" | Re-trigger Greptile}

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f06458a67d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 096fab8696

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddbb498f07

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87116e9826

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1931ccff0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1f5465591

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab22d5b098

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

jacobtomlinson · 2026-03-27T11:29:29Z

CI passed on 37c7542. 5fc39f9 only added comments. Merging.

* infra: trust system binary roots * infra: isolate windows install root overrides * infra: narrow windows reg lookup * browser: restore windows executable comments --------- Co-authored-by: Jacob Tomlinson <jtomlinson@nvidia.com>

openclaw-barnacle Bot added the size: M label Mar 25, 2026

greptile-apps Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

Comment thread src/infra/resolve-system-bin.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

openclaw-barnacle Bot added size: L and removed size: M labels Mar 25, 2026

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

Comment thread src/infra/resolve-system-bin.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

chatgpt-codex-connector Bot reviewed Mar 25, 2026

View reviewed changes

Comment thread src/infra/resolve-system-bin.ts Outdated

openclaw-barnacle Bot added the gateway Gateway runtime label Mar 26, 2026

optimol changed the title ~~prefer system paths~~ prefer trusted paths Mar 26, 2026

openclaw-barnacle Bot added size: XL and removed size: L labels Mar 27, 2026

infra: trust system binary roots

ab22d5b

jacobtomlinson force-pushed the anm/main/pathfix/0 branch from 692714f to ab22d5b Compare March 27, 2026 10:14

chatgpt-codex-connector Bot reviewed Mar 27, 2026

View reviewed changes

Comment thread src/infra/windows-install-roots.ts Outdated

jacobtomlinson added 3 commits March 27, 2026 10:27

infra: isolate windows install root overrides

e750b54

infra: narrow windows reg lookup

37c7542

browser: restore windows executable comments

5fc39f9

jacobtomlinson changed the title ~~prefer trusted paths~~ Prefer non-user writeable paths Mar 27, 2026

jacobtomlinson merged commit c40884d into openclaw:main Mar 27, 2026
39 of 42 checks passed

github-actions Bot mentioned this pull request Mar 27, 2026

📡 Upstream Digest — 2026-03-27 12:43 UTC curtismercier/openclaw-mods#380

Open

clawsweeper Bot mentioned this pull request May 2, 2026

fix: block SystemRoot/WINDIR in workspace .env and harden reg.exe path resolution [AI-assisted] #74454

Merged

9 tasks

Uh oh!

Conversation

optimol commented Mar 25, 2026 • edited by jacobtomlinson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause / Regression History (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Failure Recovery (if this breaks)

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Mar 25, 2026

Greptile Summary

Confidence Score: 4/5

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

jacobtomlinson commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

optimol commented Mar 25, 2026 •

edited by jacobtomlinson

Loading

jacobtomlinson commented Mar 27, 2026 •

edited

Loading