fix(root): scope drive-relative destination guards - #97
Merged
Conversation
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. Co-authored-by: Yigtwxx <yigiterdogan023@gmail.com>
steipete
force-pushed
the
fix/drive-relative-scope
branch
from
August 2, 2026 22:19
1fee471 to
6797023
Compare
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(root): scope drive-relative destination guards This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
4 tasks
steipete
added a commit
that referenced
this pull request
Aug 2, 2026
Stress testing archive extraction found six defects. Drive-relative and NUL archive paths bypassed validation entirely, so a ZIP entry named C:secret.txt or nested/C:secret.txt was accepted — the archive-side sibling of the drive-relative aliasing closed for Root and FileStore in #97. Explicit zero limits silently became defaults, so maxEntries: 0 did not mean zero, and NaN disabled the bounded stream cap. The JavaScript ZIP reader and bounded reads accepted CRC and size mismatches. The JavaScript TAR reader ignored stripped entries when enforcing maxEntries while the native reader counted them. Duplicate names and post-strip output collisions behaved differently depending on which backend ran. Documentation implied secret readers enforce 0o600. They do not: writes enforce modes, and permission-checked reading is readSecureFile. Walk cycle prevention and failedDirs, secret hardlink and pinned-read ordering, secure temp name generation and mode-at-creation, and trash confinement were all verified clean.
steipete
added a commit
that referenced
this pull request
Aug 2, 2026
Stress testing path validation and root confinement found seven defects.
Intermediate symlinks bypassed the documented default reject policy, so
root.readText("alias/file.txt") returned contents through a directory symlink.
The policy was only ever enforced on the final component. device-path
precedence is deliberately preserved for lexically explicit unsafe namespaces
such as /dev/fd, which are themselves symlinks on common Linux hosts; ordinary
intermediate aliases now report symlink.
A deterministic parent-directory swap returned the replacement bytes instead of
path-mismatch; the pre-open path identity is now compared against the opened
descriptor. A raced FIFO could pin a read worker, so reads open nonblocking
where supported and reject it as not-file. Padded FileStore keys silently
aliased unpadded keys and now throw invalid-path.
sanitizeUntrustedFileName("CON .") still produced a Windows device name.
Followed intermediate symlink loops reported outside-workspace rather than the
documented symlink code. Overlong Root inputs were misclassified as escapes and
now report invalid-path.
Documentation claimed all absolute and .. inputs are rejected and that filename
sanitization preserves surrounding spaces; neither was true.
The drive-relative guarded and unguarded split introduced in #97 was verified
correct, with coverage extended across every Root wrapper, both move directions,
and all async and sync FileStore key methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This finishes #85 on top of current
mainwhile preserving all four commits and authorship from @Yigtwxx.splitSafeRelativePath()and apply the same portable identity rule to everyFileStorekey.resolve,openWritable,append,mkdir,write/createand their JSON wrappers,copyIn, and the destination ofmove.c:notes.txt: open/read variants,readAbsolute/reader,stat/exists/list/walk,remove, and the source ofmove.isDriveRelativePath()into the unexportedsafe-path-segmentmodule so@openclaw/fs-safe/pathgains no public API.Why this cut
The protected property is destination identity: two distinct untrusted names must not select the same newly resolved file on Windows. A Root read or inspection of an object already present does not create that identity, and rejecting it on POSIX would make a legal on-disk filename unreadable.
copyInis intentionally guarded because its first argument is a new destination. That means a caller using an unsanitizedpath.basename()can seeinvalid-pathfor a POSIX source namedc:foo.png; the caller should sanitize or explicitly map host basenames before choosing a portable destination. Quietly accepting the name would make staging host-dependent and could retarget it on Windows.move(from, to)treats its arguments differently for the same reason:fromidentifies an existing object and remains readable on POSIX, whiletocreates the new name and is guarded.Compatibility and docs
The changelog and Root, FileStore, and error references now describe exactly where drive-relative spellings are rejected and where legal POSIX drive-like filenames remain accepted. The entry credits @Yigtwxx.
Downstream sequencing note
In
openclaw/openclaw,validateAttachmentName()atsrc/agents/subagent-attachments.ts:155-173rejects separators, NUL/control characters,.,.., and.manifest.json, but not:. The untrusted subagent/ACP attachment name becomes a store key at line 329. With this fs-safe change, a drive-relative name now throwsinvalid-path, which is desirable fail-closed behavior; OpenClaw should add its own name check so the caller instead receives the domain-specificattachments_invalid_namefailure.Proof
origin/main(8926c33) regression: 2/2 tests failed.splitSafeRelativePath("C:evil")did not throw, andRoot.write("C:secret.txt", ...)resolved instead of rejecting.test/windows-path.test.tsplustest/api-coverage.test.ts— 33 passed, 2 platform skips.pnpm check— 63 files passed, 2 skipped; 683 tests passed, 32 skipped, including build and package validation.pnpm test:security— 5 files and 64 tests passed.git diff --check— clean.dist/path.d.tscontains noisDriveRelativePathdeclaration.gpt-5.6-sol, high reasoning) — clean, no accepted/actionable findings.