fix(path): reject drive-relative untrusted relative paths - #85
Conversation
splitSafeRelativePath() rejected absolute paths via path.win32.isAbsolute(),
which reports false for the drive-relative spelling "C:name". The segment was
returned unchanged and path.resolve() then consumed the drive prefix, so on
Windows "C:secret.txt" and "secret.txt" resolved to the same file. Measured
with path.win32.resolve("C:\root", ...):
["C:evil"] -> C:\root\evil
["a","C:b"] -> C:\root\a\b
["a","D:b"] -> D:\a\b
Reject any segment starting with a drive letter, checked per segment because
a drive-relative spelling anywhere in the path aliases the whole prefix. The
check is unconditional, matching the existing path.win32.isAbsolute() guard.
|
Codex review: found issues before merge. Reviewed August 2, 2026, 2:19 PM ET / 18:19 UTC. ClawSweeper reviewWhat this changesReject Windows drive-relative spellings such as Merge readinessKeep this PR open. It fixes the previously identified raw-input bypasses and has credible Windows runtime proof, but it still accidentally expands the published Priority: P1 Review scores
Verification
How this fits together
flowchart LR
A[Untrusted path input] --> B[Safe path helper]
A --> C[Root or file-store API]
B --> D[Segment validation]
C --> E[Raw Root input validation]
D --> F[Constrained path resolution]
E --> F
F --> G[Read write or metadata operation]
Decision needed
Why: The Windows alias is source-proven, but choosing portable strict rejection versus a Windows-only guard defines a public compatibility policy that cannot be resolved mechanically. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep a shared drive-relative predicate internal to the guarded filesystem implementation, then adopt the portable strict rejection only if the repository owner explicitly wants one cross-platform key contract; otherwise gate the rejection to Windows and document that platform distinction. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source establishes the unsafe resolution sequence, and the PR supplies a Windows before/after real-disk reproduction across the affected APIs. Is this the best way to solve the issue? No. The validation placement now covers the demonstrated ingress paths, but the shared predicate should not become a public path-helper API and the portable compatibility policy needs owner approval. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 2477f5681f68. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (8 earlier review cycles)
|
|
Two notes before review. The red Node 24 Windows leg is a pre-existing flake, not this branch. Node 22 on That exact test also failed on This change is a pure string guard in Ordering with #84. Both PRs add a bullet under a |
The first commit guarded splitSafeRelativePath(), but Root never calls it: assertValidRootRelativePath() only checked for NUL bytes, and resolvePathInRoot() then ran path.resolve(root, "C:secret.txt"), which consumes the drive prefix. Every Root method, and every fileStore() key that delegates to one (open, the read variants, remove, exists), still aliased. Measured on Windows before this change, with the new regression test: AssertionError: promise resolved to a ReadResult instead of rejecting + realPath: <tmp>\fs-safe-drive-relative-delegates-XXXXXX\secret.txt Move the predicate into isDriveRelativePath() and apply it in assertValidRootRelativePath(), the funnel every Root method reaches directly or through resolvePathInRoot(). The regex now excludes drive-absolute spellings such as C:\root\file.txt, which Root accepts today when they stay inside the root, and a positive test pins that. The per-segment rule stays in splitSafeRelativePath(), where segments become separate path.resolve() arguments and an embedded "a/C:b" really does alias. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfSdVPKK9F2JvDqpkzS3ND
|
Pushed The hole is the Measured on the previous head
What changed
One deliberate narrowing: the pattern is Where I did not follow the recommendationRouting every file-store key through The asymmetry that remains is embedded segments ( Happy to extend the rejection to embedded segments if you prefer the stricter single contract, but it widens the compatibility surface on POSIX, where CoverageIn
ValidationWindows 11, Node v22.20.0, pnpm 10.34.5: The acceptance criteria list is covered: |
readAbsolute() and reader() enter through readPathInRoot(), which resolved the raw input against the root before any validation ran. On Windows that consumed a drive-relative prefix, so C:secret.txt reached readFileInRoot() as secret.txt and aliased the plain key the rest of the branch already rejected. Validate the raw input first. The check is the drive-relative one only, so drive-absolute paths inside the root keep working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ah1kwRo32A6sGU1tbMFEFk
|
Pushed The hole
const candidatePath = path.isAbsolute(params.filePath)
? path.resolve(params.filePath)
: path.resolve(rootDir, params.filePath);
const relativePath = path.relative(rootDir, candidatePath);
Measured on the previous head
The fix
The narrowed pattern CoverageIn
No new test file, no platform skip — the guard is unconditional, so both run on all six legs. ValidationWindows 11, Node v22.20.0: That lock failure is the same pre-existing Windows flake described above — a separate-process arbitration test that has been failing intermittently on On the ownership decisionStill yours to make, and it is worth stating plainly: rejecting The alternative is gating the guard on If you would rather keep POSIX permissive, say so and I will move the check behind a platform gate and adjust the tests and docs accordingly — it is a small change either way, but it should be your call rather than mine. |
The drive-relative absolute-path test opened a root at the raw mkdtemp() result, which is /var/... on macOS and an 8.3 short name on Windows CI. openRoot() resolves the root, so the unresolved path the test passed back in read as outside-workspace and failed every non-Linux leg. main fails an equivalent repro the same way, so this is the test skipping the realpath convention the rest of the suite follows, not a regression in the drive-relative rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Dpz7Mnp2xh7WHWHVAnPWtK
path.win32.isAbsolute() reports false for the drive-relative spelling C:name while path.resolve() still consumes the prefix, so C:secret.txt aliased secret.txt on Windows and C:.. slipped past the .. check, letting splitSafeRelativePath() violate its own documented postcondition. The rejection applies where a path is created or resolved: safe-relative parsing, every FileStore key, Root resolve, write, create, append, openWritable, mkdir, copyIn, and the destination of move. It does not apply to operations on an object that already exists — reads, stat, exists, list, walk, remove, and the source of move — because c:notes.txt is a legal POSIX filename and refusing to read a file that exists is collateral rather than containment. isDriveRelativePath stays internal and is absent from the published ./path subpath. Supersedes #85. Co-authored-by: Yigtwxx <yigiterdogan023@gmail.com>
|
Landed as eda9a61 via #97 — thank you, and your four commits are credited on the squash. Your analysis was exactly right, including the parts that are easy to miss. You argued for an unconditional guard rather than gating on One change before landing. The guard had also been placed in So the rejection now applies where a path is created or resolved: safe-relative parsing, every I also moved Worth flagging downstream: in |
What Problem This Solves
Fixes an issue where consumers passing untrusted relative paths through
splitSafeRelativePath()/resolveSafeRelativePath()would have two distinct inputs resolve to the same file on Windows, because a drive-relative spelling such asC:secret.txtwas accepted as an ordinary segment.The affected surface is path validation, and through it the file stores. This is a deliberate compatibility change: input that previously resolved silently now throws
invalid-path. That is called out here rather than at the bottom becauseCONTRIBUTING.mdnames path handling as a public compatibility surface.splitSafeRelativePath()(src/path.ts:138-161) rejects NUL bytes, backslashes, absolute paths,//prefixes, and..segments. Absolute detection usespath.win32.isAbsolute(), which returnsfalseforC:name— that spelling is relative to the drive's own working directory, not to the root. The segment is therefore returned unchanged, andresolveSafeRelativePath()hands it topath.resolve(), which does consume the drive prefix.Measured on Node v22.20.0 / win32:
path.posix.resolve()keeps these as literal segments, so the aliasing bites on Windows only — but the divergence between the two platforms is itself the problem for anything replicating store contents across hosts.Two consequences beyond the aliasing:
"C:.."passes the..check as the single segmentC:.., so the function violated its own postcondition (relative path must not contain '..') and returned a..-bearing segment to any caller that joins segments itself.resolveSafeRelativePath()'s trailingisPathInside()still caught the escape, so this was a contract gap rather than a traversal.src/file-store.ts:111-120routes untrusted store keys throughassertRelativePath(), which rewrites\to/before validation. That makes"C:\\x"safe (it becomesC:/x, whichpath.win32.isAbsolute()catches) while"C:evil"and"a\\C:b"pass straight through — so the wholefileStore/fileStoreSynckey surface was affected.Why This Change Was Made
One guard in
src/path.ts: any segment matching/^[A-Za-z]:/is rejected with the existinginvalid-pathcode.Design decisions:
["a","C:b"] -> C:\root\a\babove: a drive-relative spelling anywhere in the path aliases the whole prefix, and"./C:evil"has its.filtered out before resolution. A leading-segment-only check would leave the hole open.FsSafeErrorCodeunion,docs/types.md, or any option shape. The message follows its two siblings in the same loop (relative path must not contain '..',relative path must not be absolute)...check so"C:.."reports the drive-letter cause, and the function can no longer return a..-bearing segment.path.win32.isAbsolute()on every host, so its contract is "reject what is unsafe on any supported platform." Gating onprocess.platformwould make a store key valid on Linux and a boundary violation on Windows, which is worse for anyone syncing store contents across hosts.ab93382.Non-goal: broader colon handling (NTFS alternate data streams,
file.txt:stream) is a separate concern and is not touched here.Compatibility scan:
grep -rnE '"[A-Za-z]:[^\\/]' src/ test/ docs/returns nothing — no in-package caller passes a drive-relative relative path. Externally this would break a consumer deliberately using single-letter-plus-colon filename prefixes as store keys. Those are already illegal filenames on Windows, and on Windows the previous behavior wrote them to the wrong file, so the breakage surfaces a bug rather than removing a working feature.User Impact
fileStore,fileStoreSync, and any direct user of@openclaw/fs-safe/pathnow reject drive-relative keys instead of silently aliasing them:Before this change the second call resolved to the same file and overwrote it. A caller that maintained its own allow/deny bookkeeping per relative path could be fed the
C:-prefixed spelling to reach a key it had already decided to protect.No API, option, default, export, or error code was added or removed.
Evidence
Reproduction on
main, from the new store-level regression test:The pre-fix run does not merely fail to throw — it returns the path of
secret.txt, confirming the second write landed on the first key's file.Regression coverage added to
test/windows-path.test.ts(three tests) andtest/api-coverage.test.ts(existing rejection array extended in place, no new lines):C:evil,c:evil,C:,C:..,C:evil/sub,a/C:b,./C:evil,a/D:blogs/2026-08-02T10:30:00Z.logstill parses normally, so the anchored single-letter pattern does not reject timestamped filenamesmkdtempreal-disk store case quoted aboveNone are platform-skipped. The guard is unconditional, so the assertions hold on all six CI matrix legs; only the pre-fix failure mode was Windows-specific.
Validation on Windows 11, Node v22.20.0, pnpm 10.34.5:
One note on the local run:
test/file-lock-reentrancy.test.tsfailed once withEPERMon a.lockfile during an earlier full-suite pass, then passed 4/4 in isolation and in the clean re-run above. It is unrelated to this change —src/sidecar-lock.tsdoes not use either function touched here — and looks like a Windows file-handle race from the separate-process arbitration test.CHANGELOG.mdupdated when release-relevantUpdate —
2a10498readAbsolute()andreader()bypassed the guard:readPathInRoot()resolved the raw input against the root before validating it, soC:secret.txtreached the downstream check already flattened tosecret.txt.Fixed by validating the raw input at the top of
readPathInRoot(), before the firstpath.resolve(). Drive-absolute in-root paths are unaffected.Pre-fix, on head
7024025with the two entry points added to the rejection table:Coverage now spans 15 entry points, and the positive case pins
readAbsolute()andreader()on an absolute in-root path.Validation on Windows 11, Node v22.20.0:
The portable-versus-Windows-only compatibility decision is still open for the repository owner; see the discussion comment.