fix(security): classify dangerous Windows sandbox binds first#63074
fix(security): classify dangerous Windows sandbox binds first#63074luoyanglang wants to merge 4 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a security classification bug in Confidence Score: 5/5Safe to merge — the reordering fix is correct, the new test validates the changed behavior, and no regressions are introduced. The change is small and surgical: a 4-line block moved 8 lines down. Both the old and new code always throw for dangerous binds; the only difference is the kind field, which is now accurate for Windows paths. The new test directly exercises the fixed path. No P0/P1 findings. No files require special attention.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4219d19d5
ℹ️ 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".
| expect(getBlockedBindReason("C:\\Users\\tester\\.docker\\config.json:/mnt/test:ro")).toEqual( | ||
| expect.objectContaining({ | ||
| kind: "targets", | ||
| blockedPath: "C:/Users/tester/.docker", |
There was a problem hiding this comment.
Make Windows bind-path expectation platform-aware
This new assertion hard-codes blockedPath: "C:/Users/tester/.docker", but home roots are derived via resolveRequiredHomeDir(...)->path.resolve(...); on Linux/macOS, HOME="C:\\Users\\tester" resolves under the current working directory, so getBlockedBindReason(...) remains kind: "non_absolute" instead of targets. Since this test is unconditional, non-Windows CI lanes can fail even though the Windows-specific behavior is fine; gate this case to Windows or compute the expected path through the same normalization path used by production code.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dedf04ea4
ℹ️ 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".
| kind: "targets", | ||
| blockedPath: "C:/Users/tester/.docker", | ||
| }), |
There was a problem hiding this comment.
Make Windows bind expectation platform-aware
This assertion assumes getBlockedBindReason will classify a Windows-style source as kind: "targets", but on non-Windows runners resolveRequiredHomeDir uses host path.resolve semantics, so HOME="C:\\Users\\tester" normalizes under the POSIX cwd and the bind remains kind: "non_absolute". That makes this test fail in Linux/macOS CI even though the Windows-specific behavior is fine; gate the case to Windows or derive the expected blocked path through the same normalization path used by production code.
Useful? React with 👍 / 👎.
0dedf04 to
f9e4715
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e40fc89dcb
ℹ️ 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".
| expect(getBlockedBindReason("C:\\Users\\tester\\.docker\\config.json:/mnt/test:ro")).toEqual( | ||
| expect.objectContaining({ | ||
| kind: "targets", | ||
| blockedPath: "C:/Users/tester/.docker", | ||
| }), |
There was a problem hiding this comment.
Make Windows-home bind assertion host-aware
This assertion is not portable across CI hosts: when this test runs on Linux/macOS, the home roots are derived through host path.resolve, so a Windows-style HOME value like C:\Users\tester is resolved under the POSIX cwd and does not match C:/Users/tester/.docker, causing getBlockedBindReason(...) to return kind: "non_absolute" instead of targets. Fresh evidence: on Linux, path.resolve("C:\\Users\\tester") resolves to /workspace/openclaw/C:\\Users\\tester, which explains the mismatch. This makes the new regression test fail off Windows unless the expectation is gated by platform or computed via the same normalization path as production code.
Useful? React with 👍 / 👎.
b98b344 to
3b219e7
Compare
|
Codex review: found issues before merge. Summary Reproducibility: yes. Current-main source shows the non-absolute return runs before blocked-path classification, and the audit collector then downgrades that path to the warn check instead of the critical dangerous-bind check. Next step before merge Security Review findings
Review detailsBest possible solution: Land or replace this with a security-reviewed sandbox fix that classifies Windows home credential bind sources as critical dangerous binds, with a changelog entry, while leaving generic Windows bind acceptance to the broader Windows bind work. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source shows the non-absolute return runs before blocked-path classification, and the audit collector then downgrades that path to the warn check instead of the critical dangerous-bind check. Is this the best way to solve the issue? Yes for the narrow bug. Checking normalized blocked paths before the fallback and adding raw Windows home roots is the maintainable direction; broad Windows bind support should remain separate in #42174. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 9772ce6ce975. |
Summary
Testing