docs(gateway): add Windows no-Docker hardening fallback guide#38932
docs(gateway): add Windows no-Docker hardening fallback guide#38932fantasyengineercdream wants to merge 3 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR adds a docs-only Windows host hardening guide (
Confidence Score: 3/5
Last reviewed commit: 51d2bf1 |
| runas /user:$env:COMPUTERNAME\openclaw_bot "powershell -NoProfile -NoExit -Command `"` | ||
| $env:OPENCLAW_STATE_DIR='$State'; ` | ||
| $env:OPENCLAW_CONFIG_PATH='$State\openclaw.json'; ` | ||
| Set-Location -LiteralPath '$Workspace'; ` | ||
| openclaw gateway`"" | ||
| ``` |
There was a problem hiding this comment.
$env: variables expanded by calling session, breaking inner assignments
This runas command wraps the inner PowerShell command in a double-quoted string. PowerShell expands $env:OPENCLAW_STATE_DIR and $env:OPENCLAW_CONFIG_PATH in the outer session before passing the string to runas. Since those variables are almost certainly empty in the outer session, the resulting inner command becomes something like:
='D:\OpenClawWorkspace\.openclaw-state'; `
='D:\OpenClawWorkspace\.openclaw-state\openclaw.json'; `
which is invalid PowerShell syntax and will fail silently or with a parse error in the child session. The $State and $Workspace variables (plain PS variables, not $env:) are correctly expanded by the outer session and should remain as-is. Only the $env: variable names on the left-hand side of the assignments need to be escaped.
Fix by escaping the inner $env: names with a backtick so they are not expanded by the outer session:
| runas /user:$env:COMPUTERNAME\openclaw_bot "powershell -NoProfile -NoExit -Command `"` | |
| $env:OPENCLAW_STATE_DIR='$State'; ` | |
| $env:OPENCLAW_CONFIG_PATH='$State\openclaw.json'; ` | |
| Set-Location -LiteralPath '$Workspace'; ` | |
| openclaw gateway`"" | |
| ``` | |
| runas /user:$env:COMPUTERNAME\openclaw_bot "powershell -NoProfile -NoExit -Command `"` | |
| `$env:OPENCLAW_STATE_DIR='$State'; ` | |
| `$env:OPENCLAW_CONFIG_PATH='$State\openclaw.json'; ` | |
| Set-Location -LiteralPath '$Workspace'; ` | |
| openclaw gateway`"" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/gateway/windows-no-docker-hardening.md
Line: 131-136
Comment:
**`$env:` variables expanded by calling session, breaking inner assignments**
This `runas` command wraps the inner PowerShell command in a double-quoted string. PowerShell expands `$env:OPENCLAW_STATE_DIR` and `$env:OPENCLAW_CONFIG_PATH` **in the outer session** before passing the string to `runas`. Since those variables are almost certainly empty in the outer session, the resulting inner command becomes something like:
```
='D:\OpenClawWorkspace\.openclaw-state'; `
='D:\OpenClawWorkspace\.openclaw-state\openclaw.json'; `
```
which is invalid PowerShell syntax and will fail silently or with a parse error in the child session. The `$State` and `$Workspace` variables (plain PS variables, not `$env:`) are correctly expanded by the outer session and should remain as-is. Only the `$env:` variable names on the left-hand side of the assignments need to be escaped.
Fix by escaping the inner `$env:` names with a backtick so they are not expanded by the outer session:
```suggestion
runas /user:$env:COMPUTERNAME\openclaw_bot "powershell -NoProfile -NoExit -Command `"`
`$env:OPENCLAW_STATE_DIR='$State'; `
`$env:OPENCLAW_CONFIG_PATH='$State\openclaw.json'; `
Set-Location -LiteralPath '$Workspace'; `
openclaw gateway`""
```
How can I resolve this? If you propose a fix, please make it concise.| ```powershell | ||
| $Parent = "D:\" | ||
| Get-ChildItem -LiteralPath $Parent -Directory | | ||
| Where-Object { $_.FullName -ne "D:\OpenClawWorkspace" } | | ||
| ForEach-Object { | ||
| icacls $_.FullName /remove:d "$Principal" /T /C | ||
| } | ||
| ``` |
There was a problem hiding this comment.
$Principal undefined if code block is run in isolation
$Principal is defined in the first rollback code block (line 175), but this second rollback block references it without redefining it. If a reader copies and runs only this snippet (a common pattern when following step-by-step docs), $Principal will be an empty string in PowerShell, so icacls ... /remove:d "" will silently no-op or emit an unhelpful error, leaving the deny ACEs in place.
Repeating the definition at the top of this block makes it self-contained:
| ```powershell | |
| $Parent = "D:\" | |
| Get-ChildItem -LiteralPath $Parent -Directory | | |
| Where-Object { $_.FullName -ne "D:\OpenClawWorkspace" } | | |
| ForEach-Object { | |
| icacls $_.FullName /remove:d "$Principal" /T /C | |
| } | |
| ``` | |
| $Principal = "$env:COMPUTERNAME\openclaw_bot" | |
| $Parent = "D:\" | |
| Get-ChildItem -LiteralPath $Parent -Directory | | |
| Where-Object { $_.FullName -ne "D:\OpenClawWorkspace" } | | |
| ForEach-Object { | |
| icacls $_.FullName /remove:d "$Principal" /T /C | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/gateway/windows-no-docker-hardening.md
Line: 183-190
Comment:
**`$Principal` undefined if code block is run in isolation**
`$Principal` is defined in the first rollback code block (line 175), but this second rollback block references it without redefining it. If a reader copies and runs only this snippet (a common pattern when following step-by-step docs), `$Principal` will be an empty string in PowerShell, so `icacls ... /remove:d ""` will silently no-op or emit an unhelpful error, leaving the deny ACEs in place.
Repeating the definition at the top of this block makes it self-contained:
```suggestion
$Principal = "$env:COMPUTERNAME\openclaw_bot"
$Parent = "D:\"
Get-ChildItem -LiteralPath $Parent -Directory |
Where-Object { $_.FullName -ne "D:\OpenClawWorkspace" } |
ForEach-Object {
icacls $_.FullName /remove:d "$Principal" /T /C
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51d2bf1e63
ℹ️ 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".
| $Parent = "D:\" | ||
| Get-ChildItem -LiteralPath $Parent -Directory | | ||
| Where-Object { $_.FullName -ne "D:\OpenClawWorkspace" } | |
There was a problem hiding this comment.
Use selected workspace parent in rollback ACL cleanup
The rollback instructions hardcode D:\ and D:\OpenClawWorkspace even though earlier steps present the workspace as configurable (for example, “such as D:\OpenClawWorkspace”). If a user chose a different workspace location, this cleanup loop will skip the directories where denies were actually added in step 4, leaving restrictive ACLs behind and making the rollback incomplete.
Useful? React with 👍 / 👎.
9e6b341 to
3027dcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3027dcf5fe
ℹ️ 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".
| intentionally blocked: | ||
|
|
||
| ```powershell | ||
| runas /user:$env:COMPUTERNAME\openclaw_bot "powershell -NoProfile -Command `"Get-ChildItem -LiteralPath '$env:USERPROFILE'`"" |
There was a problem hiding this comment.
Verify denied access against a path this guide blocks
The forbidden-path check currently probes '$env:USERPROFILE', which is usually inaccessible to other non-admin users on Windows even without the sibling-deny ACLs from step 4, so this test can report a “pass” while your intended boundary is not actually enforced. In practice this gives false confidence: users can complete verification even if the deny loop was skipped or mis-targeted; the check should target one of the explicitly denied sibling directories (or a user-provided blocked path) to validate the hardening steps this page introduced.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: found issues before merge. Summary Reproducibility: yes. for the PR docs defect. Source inspection of PR head shows the deny loop blocks sibling directories under the workspace parent, while the verification command checks the caller-expanded user profile path instead of a path that the guide explicitly denied. Next step before merge Security Review findings
Review detailsBest possible solution: Publish the fallback only after the verification targets an explicitly denied sibling or configured blocked path, links/navigation are rebased onto current docs, and secops approves the threat-model wording. Do we have a high-confidence way to reproduce the issue? Yes for the PR docs defect. Source inspection of PR head shows the deny loop blocks sibling directories under the workspace parent, while the verification command checks the caller-expanded user profile path instead of a path that the guide explicitly denied. Is this the best way to solve the issue? No, not as-is. The docs direction is plausible, but the verification needs to test the hardening step itself and the branch should be rebased into the current docs structure before security-owner review. Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 605e89468ebf. |
Summary
Change Type
Scope
Why this is worth taking
This PR is valuable even though it is docs-only:
Linked Issue/PR
User-visible / Behavior Changes
Security Impact
NoNoNoNoNoRepro + Verification
Environment
Steps
OPENCLAW_STATE_DIR/OPENCLAW_CONFIG_PATH.Expected
Actual
Evidence
Reference implementation and validation material:
Human Verification
Compatibility / Migration
YesNoNoFailure Recovery
docs/gateway/sandboxing.md,docs/platforms/windows.md, and the new doc page.Risks and Mitigations
Risk: users may overestimate the security boundary.
Mitigation: the guide repeatedly states that this is a weaker host hardening fallback, not a container boundary.
Risk: reviewers may read this as a proposal to bless native Windows as the default path.
Mitigation: the PR keeps WSL2 and Docker as the primary recommendation and adds this only as a documented fallback.
AI-assisted transparency
YesHuman-verified on a working Windows reference setup