fix: handle Windows path separators in workspace root validation#30766
Conversation
🔒 Aisle Security Analysis✅ We scanned this PR and did not find any security vulnerabilities. Aisle supplements but does not replace security review. Analyzed PR: #30766 at commit Last updated on: 2026-03-02T21:42:11Z |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e9cae5702
ℹ️ 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".
| if (process.platform === "win32") { | ||
| const rootForCompare = normalizeWindowsPathForComparison(rootResolved); | ||
| const targetForCompare = normalizeWindowsPathForComparison(resolved); | ||
| const relative = path.win32.relative(rootForCompare, targetForCompare); |
There was a problem hiding this comment.
Return relative path using original path casing
In the Windows branch, relative is computed from rootForCompare and targetForCompare, but those values are lowercased by normalizeWindowsPathForComparison(). This makes the returned relative path lowercased as well (for example Foo\\Bar.txt becomes foo\\bar.txt), so callers may end up reading/writing a different path than requested when running in case-sensitive Windows directories (or with tooling that depends on case preservation). Keep normalized strings for boundary validation, but compute the returned relative path from rootResolved and resolved.
Useful? React with 👍 / 👎.
Greptile SummaryFixes Windows path separator handling for workspace root validation by normalizing paths before comparison. The approach correctly addresses case-insensitivity and separator differences on Windows. Key Changes:
Issues Found:
Confidence Score: 2/5
Last reviewed commit: 8e9cae5 |
| // On Windows, use normalized path comparison to handle case-insensitivity and separators. | ||
| if (process.platform === "win32") { | ||
| const rootForCompare = normalizeWindowsPathForComparison(rootResolved); | ||
| const targetForCompare = normalizeWindowsPathForComparison(resolved); | ||
| const relative = path.win32.relative(rootForCompare, targetForCompare); | ||
| if (relative === "" || relative === ".") { | ||
| if (options?.allowRoot) { | ||
| return ""; | ||
| } | ||
| throw new Error(`Path escapes workspace root: ${candidate}`); | ||
| } | ||
| if (relative.startsWith("..") || path.win32.isAbsolute(relative)) { | ||
| throw new Error(`Path escapes workspace root: ${candidate}`); | ||
| } | ||
| return relative; |
There was a problem hiding this comment.
relative path is calculated from normalized (lowercase) paths, which loses the original case
The normalized paths should only be used for security validation (checking if path escapes), but the final returned relative path should be calculated from the original rootResolved and resolved to preserve case:
| // On Windows, use normalized path comparison to handle case-insensitivity and separators. | |
| if (process.platform === "win32") { | |
| const rootForCompare = normalizeWindowsPathForComparison(rootResolved); | |
| const targetForCompare = normalizeWindowsPathForComparison(resolved); | |
| const relative = path.win32.relative(rootForCompare, targetForCompare); | |
| if (relative === "" || relative === ".") { | |
| if (options?.allowRoot) { | |
| return ""; | |
| } | |
| throw new Error(`Path escapes workspace root: ${candidate}`); | |
| } | |
| if (relative.startsWith("..") || path.win32.isAbsolute(relative)) { | |
| throw new Error(`Path escapes workspace root: ${candidate}`); | |
| } | |
| return relative; | |
| // On Windows, use normalized path comparison to handle case-insensitivity and separators. | |
| if (process.platform === "win32") { | |
| const rootForCompare = normalizeWindowsPathForComparison(rootResolved); | |
| const targetForCompare = normalizeWindowsPathForComparison(resolved); | |
| const relativeForCheck = path.win32.relative(rootForCompare, targetForCompare); | |
| if (relativeForCheck === "" || relativeForCheck === ".") { | |
| if (options?.allowRoot) { | |
| return ""; | |
| } | |
| throw new Error(`Path escapes workspace root: ${candidate}`); | |
| } | |
| if (relativeForCheck.startsWith("..") || path.win32.isAbsolute(relativeForCheck)) { | |
| throw new Error(`Path escapes workspace root: ${candidate}`); | |
| } | |
| // Return relative path from original resolved paths to preserve case | |
| return path.win32.relative(rootResolved, resolved); | |
| } |
While Windows filesystems are case-insensitive and will function correctly either way, returning lowercase paths could cause inconsistencies in error messages, logs, and string comparisons.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 886-900
Comment:
relative path is calculated from normalized (lowercase) paths, which loses the original case
The normalized paths should only be used for security validation (checking if path escapes), but the final returned relative path should be calculated from the original `rootResolved` and `resolved` to preserve case:
```suggestion
// On Windows, use normalized path comparison to handle case-insensitivity and separators.
if (process.platform === "win32") {
const rootForCompare = normalizeWindowsPathForComparison(rootResolved);
const targetForCompare = normalizeWindowsPathForComparison(resolved);
const relativeForCheck = path.win32.relative(rootForCompare, targetForCompare);
if (relativeForCheck === "" || relativeForCheck === ".") {
if (options?.allowRoot) {
return "";
}
throw new Error(`Path escapes workspace root: ${candidate}`);
}
if (relativeForCheck.startsWith("..") || path.win32.isAbsolute(relativeForCheck)) {
throw new Error(`Path escapes workspace root: ${candidate}`);
}
// Return relative path from original resolved paths to preserve case
return path.win32.relative(rootResolved, resolved);
}
```
While Windows filesystems are case-insensitive and will function correctly either way, returning lowercase paths could cause inconsistencies in error messages, logs, and string comparisons.
How can I resolve this? If you propose a fix, please make it concise.| // Normalize Windows paths for consistent comparison (handles case-insensitivity and separators). | ||
| function normalizeWindowsPathForComparison(input: string): string { | ||
| let normalized = path.win32.normalize(input); | ||
| if (normalized.startsWith("\\\\?\\")) { | ||
| normalized = normalized.slice(4); | ||
| if (normalized.toUpperCase().startsWith("UNC\\")) { | ||
| normalized = `\\\\${normalized.slice(4)}`; | ||
| } | ||
| } | ||
| return normalized.replaceAll("/", "\\").toLowerCase(); | ||
| } |
There was a problem hiding this comment.
duplicates normalizeWindowsPathForComparison from src/infra/path-guards.ts:6-15
Export this function from path-guards.ts and import it here to avoid duplication and ensure both implementations stay in sync.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 866-876
Comment:
duplicates `normalizeWindowsPathForComparison` from `src/infra/path-guards.ts:6-15`
Export this function from `path-guards.ts` and import it here to avoid duplication and ensure both implementations stay in sync.
How can I resolve this? If you propose a fix, please make it concise.|
Thanks for tackling this Windows workspace-root issue. Deferring for now: the normalization approach looks directionally right, but we still need focused regression tests that specifically cover separator/case combinations and root-relative edge cases in |
8e9cae5 to
cda2e2a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cda2e2adf6
ℹ️ 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 rootForCompare = normalizeWindowsPathForComparison(rootResolved); | ||
| const targetForCompare = normalizeWindowsPathForComparison(resolvedCandidate); | ||
| const relative = path.win32.relative(rootForCompare, targetForCompare); |
There was a problem hiding this comment.
Preserve original casing in Windows relative paths
relative is computed from rootForCompare and targetForCompare, which are lowercased by normalizeWindowsPathForComparison(), so the returned path loses original casing (for example, Memory\\Log.txt becomes memory\\log.txt). That can break reads/writes in case-sensitive Windows directories (or any downstream tooling that relies on case-preserving paths), even when the path is inside the workspace. Use the normalized strings only for the boundary check and compute the returned relative path from the original resolved paths instead. Fresh evidence: this commit now does the lowercased comparison-based path.win32.relative(...) in toRelativePathUnderRoot.
Useful? React with 👍 / 👎.
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
* fix(agents): normalize windows workspace path boundary checks (openclaw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> (cherry picked from commit ef89b48) * fix: bypass proxy for CDP localhost connections (openclaw#31219) When HTTP_PROXY / HTTPS_PROXY / ALL_PROXY environment variables are set, CDP connections to localhost/127.0.0.1 can be incorrectly routed through the proxy (e.g. via global-agent or undici proxy dispatcher), causing browser control to fail. Fix: - New cdp-proxy-bypass module with utilities for direct localhost connections - WebSocket (ws) CDP connections: pass explicit http.Agent to bypass any global proxy agent patching - fetch-based CDP probes: wrap in withNoProxyForLocalhost() to temporarily set NO_PROXY for the duration of the call - Playwright connectOverCDP: wrap in withNoProxyForLocalhost() since Playwright reads env vars internally - 13 new tests covering getDirectAgentForCdp, hasProxyEnv, and withNoProxyForLocalhost (env save/restore, error recovery) (cherry picked from commit c96234b) * fix(mattermost): pass mediaLocalRoots through reply delivery (openclaw#44021) Merged via squash. Prepared head SHA: 856f11f Co-authored-by: LyleLiu666 <31182860+LyleLiu666@users.noreply.github.com> Co-authored-by: mukhtharcm <56378562+mukhtharcm@users.noreply.github.com> Reviewed-by: @mukhtharcm (cherry picked from commit c965049) * fix(test): reduce startup-heavy hotspot retention (openclaw#52381) Extract applyMSTeamsWebhookTimeouts + constants from monitor.ts into standalone webhook-timeouts.ts module and update test import. Subset of upstream dbd26e4 — fork takes only the msteams-local changes. (cherry picked from commit dbd26e4) * fix(ci): split redact snapshot schema coverage (cherry picked from commit 5841e3b) * fix(logging): make logger import browser-safe (cherry picked from commit df3a190) * fix: resume orphaned subagent sessions after SIGUSR1 reload Closes openclaw#47711 After a SIGUSR1 gateway reload aborts in-flight subagent LLM calls, the gateway now scans for orphaned sessions and sends a synthetic resume message to restart their work. Also makes the deferral timeout configurable via gateway.reload.deferralTimeoutMs (default: 5 minutes, up from 90s). (cherry picked from commit 304703f) * fix(gateway): pin plugin webhook route registry (openclaw#47902) (cherry picked from commit a69f619) * fix: validate edge tts output file is non-empty before reporting success (openclaw#43385) thanks @Huntterxx Merged after review.\n\nSmall, scoped fix: treat 0-byte Edge TTS output as failure so provider fallback can continue. (cherry picked from commit 946c24d) * fix(config): regenerate base schema + add label after deferralTimeoutMs port Follow-up to cherry-pick 304703f (fix: resume orphaned subagent sessions after SIGUSR1 reload): - Regenerate src/config/schema.base.generated.ts (picks up new gateway.reload.deferralTimeoutMs field + help text) - Add FIELD_LABELS entry for gateway.reload.deferralTimeoutMs (schema.help.quality test enforces label parity) - Update infra-runtime timeout test to advance 5m instead of 90s (default raised by the fix) --------- Co-authored-by: Shawn <118158941+kevinWangSheng@users.noreply.github.com> Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com> Co-authored-by: Marcus Widing <widing.marcus@gmail.com> Co-authored-by: Lyle <31182860+LyleLiu666@users.noreply.github.com> Co-authored-by: Vincent Koc <vincentkoc@ieee.org> Co-authored-by: Altay <altay@uinaf.dev> Co-authored-by: Joey Krug <joeykrug@gmail.com> Co-authored-by: Peter Steinberger <peter@steipete.me> Co-authored-by: Hiago Silva <97215740+Huntterxx@users.noreply.github.com>
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
…aw#30766) Co-authored-by: Tak Hoffman <781889+Takhoffman@users.noreply.github.com>
Summary
Fixes #30761 - False positive "Path escapes workspace root" error on Windows when writing to
memory/subdirectory.Problem
The
toRelativePathInRootfunction inpi-tools.read.tswas incorrectly rejecting valid paths inside the workspace root on Windows due to inconsistent path separator handling and case-sensitivity issues.On Windows:
/or\\\?\The original code used
path.relative()andpath.isAbsolute()without normalizing for Windows-specific path characteristics, causing paths likeC:\Users\<user>\openclaw-new\memoryto be incorrectly flagged as escaping the workspace rootC:\Users\<user>\openclaw-new.Solution
This fix normalizes Windows paths before comparison, matching the approach already used in
isPathInside()frompath-guards.ts:path.win32.normalize()for proper Windows path handling\\?\) if presentTesting
The fix has been verified to compile correctly. The existing test suite in
fs-safe.test.tscovers the workspace boundary validation logic.Changes
normalizeWindowsPathForComparison()helper functiontoRelativePathInRoot()to use Windows-specific normalization when running on Windows platform