fix(node-host): allow absolute-path native binaries through approval binder#66731
Conversation
Greptile SummaryThis PR narrows Confidence Score: 5/5
Reviews (2): Last reviewed commit: "test(node-host): use stable native binar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fce6fea6f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd8d73ded1
ℹ️ 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".
tmimmanuel
force-pushed
the
fix/node-host-absolute-binary-approval-binding
branch
from
dd8d73d to
47b6e87
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47b6e87ead
ℹ️ 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".
tmimmanuel
force-pushed
the
fix/node-host-absolute-binary-approval-binding
branch
from
47b6e87 to
2985cec
Compare
|
@obviyus Please review. |
obviyus
left a comment
obviyus
left a comment
There was a problem hiding this comment.
Root cause checks out.
shellPayloadNeedsStableBinding() was still fail-closing any shell payload whose first token resolved to a file, which wrongly lumped native binaries in with mutable script operands. The new header probe in src/node-host/invoke-system-run-plan.ts narrows that to actual script-like payloads, keeps the fail-closed behavior on probe errors / empty files / weak PE matches, and the regressions in src/node-host/invoke-system-run-plan.test.ts cover the important branches.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2217c4738
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1955a457d
ℹ️ 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".
obviyus
force-pushed
the
fix/node-host-absolute-binary-approval-binding
branch
from
e1955a4 to
af0e160
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af0e160ee9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650dbdd11e
ℹ️ 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".
obviyus
left a comment
obviyus
left a comment
There was a problem hiding this comment.
Verified the node-host approval binder bug where absolute-path shell payload binaries were being treated like unsafe interpreter/script payloads and denied before allowlist approval.
Maintainer follow-up: added regressions and tightened the shell-payload path so unknown headers, relative binaries, writable paths, and symlinked writable targets all stay fail-closed.
Local gate: pnpm test src/node-host/invoke-system-run-plan.test.ts
|
Landed on main. Thanks @tmimmanuel. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77edb54b17
ℹ️ 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".
) (thanks @tmimmanuel) * fix(node-host): allow absolute-path native binaries through approval binder * test(node-host): cover binary binder edge cases * test(node-host): use stable native binary fixture * fix(ci): restore fail-closed race handling * refactor(node-host): distill approval binding regressions * fix(node-host): fail closed on unknown shell payload headers * fix: land node-host approval binding for native binaries (openclaw#66731) (thanks @tmimmanuel) * fix: keep relative shell binary payloads fail-closed (openclaw#66731) (thanks @tmimmanuel) * fix: keep shell binary bypass on stable paths only (openclaw#66731) (thanks @tmimmanuel) * fix: fail closed on symlinked shell binary targets (openclaw#66731) (thanks @tmimmanuel) --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us>
) (thanks @tmimmanuel) * fix(node-host): allow absolute-path native binaries through approval binder * test(node-host): cover binary binder edge cases * test(node-host): use stable native binary fixture * fix(ci): restore fail-closed race handling * refactor(node-host): distill approval binding regressions * fix(node-host): fail closed on unknown shell payload headers * fix: land node-host approval binding for native binaries (openclaw#66731) (thanks @tmimmanuel) * fix: keep relative shell binary payloads fail-closed (openclaw#66731) (thanks @tmimmanuel) * fix: keep shell binary bypass on stable paths only (openclaw#66731) (thanks @tmimmanuel) * fix: fail closed on symlinked shell binary targets (openclaw#66731) (thanks @tmimmanuel) --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us>
2 participants
Summary
Fix the node-host approval binder so absolute-path native binaries are not rejected as unsafe interpreter/runtime commands.
Before this change,
tools.execwithhost=nodecould fail for commands like/usr/bin/whoamibecause the shell-payload binder treated any existing file at argv[0] as requiring stable script-style binding. That was correct for mutable scripts, but wrong for native binaries.This patch keeps the fail-closed behavior for likely script files while allowing native executables through without requiring a mutable file operand.
Fixes #66524.
What changed
src/node-host/invoke-system-run-plan.tsshellPayloadNeedsStableBinding()to distinguish:src/node-host/invoke-system-run-plan.test.tsfor:/bin/sh -lc /usr/bin/whoamisucceeds/bin/sh -lc <script-path>still fails closedWhy
The previous logic rejected all shell payloads whose first token resolved to an existing file, which unintentionally blocked absolute-path binaries on node hosts. The
env /usr/bin/whoamiworkaround reported in #66524 demonstrated that the rejection was overly broad.This change narrows the rejection surface to mutable script-like targets rather than all file-backed commands.
Risk
Low to medium.
The patch only affects the shell-payload stable-binding heuristic. It preserves the existing stricter behavior for script/interpreter cases and does not change allowlist, approval, or execution policy flows.
Testing
Added regression tests for:
Local verification:
git diff --checkpassed