fix(root): create writable-open parents through the guarded mkdir helper#61
Open
yetval wants to merge 1 commit into
Open
fix(root): create writable-open parents through the guarded mkdir helper#61yetval wants to merge 1 commit into
yetval wants to merge 1 commit into
Conversation
openWritableFileInRoot created the destination's parent directories with a single recursive fs.mkdir, which follows symlinks in components that already exist. The only protection was a guard on the nearest existing ancestor, and adding a symlink child to that ancestor changes neither its dev/ino nor its realpath, so both guard assertions passed while the mkdir walked out of the root. Route the parent creation through mkdirPathComponentsWithGuards, matching mkdirPathFallback and runPinnedWriteFallback. It creates one component at a time and realpath-checks each one against the root before descending, so a parent swapped to an out-of-root symlink during the call is rejected instead of followed.
clawsweeper
Bot
added
proof: sufficient
|
Codex review: needs maintainer review before merge. Reviewed July 26, 2026, 12:33 PM ET / 16:33 UTC. ClawSweeper reviewWhat this changesThis PR replaces recursive parent creation for Merge readinessKeep open for maintainer review: the production change reuses the canonical guarded mkdir path and the PR provides credible real macOS race proof, but it also changes the observable parent-file failure from raw Likely related people
Priority: P1 Review scores
Verification
How this fits togetherThe root write subsystem accepts an untrusted root-relative destination path and prepares its parent directories before opening a writable file. That directory-preparation step feeds the final file open and must preserve confinement even while another same-user process changes path components. flowchart LR
A[Untrusted relative path] --> B[Root path resolution]
B --> C[Alias escape check]
C --> D[Guarded parent creation]
D --> E[Real-path boundary check]
E --> F[Writable file handle]
G[Concurrent parent swap] --> D
Decision needed
Why: The security route is narrow and matches an existing primitive, but repository policy identifies public error codes as compatibility surfaces and does not establish that raw Node error behavior may be changed without maintainer intent. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the guarded per-component mkdir route, retain the race regression test, and merge only after explicitly accepting the normalized Do we have a high-confidence way to reproduce the issue? Yes. The PR supplies a concrete current-main macOS reproduction through the public compiled Is this the best way to solve the issue? Unclear. Reusing the existing guarded per-component mkdir helper is the narrowest maintainable security fix, but maintainers must first confirm that its AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8cced2ab6264. LabelsLabel changes:
Label 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
|
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
root().append()androot().openWritable()(andcopyIn()when the Python helper is off or unavailable) create the destination's parent directories with a single recursivefs.mkdir. If a component of that parent path is replaced with a symlink pointing outside the root while the call is in flight, the mkdir follows it and creates directories outside the root.The caller is not told. The post-open boundary check still fires, so the call throws
not-foundoroutside-workspaceand the caller believes the operation was blocked, while the directories created outside the root persist. That gap between "this was rejected" and "state was left behind outside the root" is the user-visible problem.Scope, stated precisely: what escapes is unauthorized directory creation, plus a 0-byte file on the
copyInfallback. Attacker-controlled content does not land outside the root, because the post-openisPathInside(rootWithSep, realPath)check plus the cleanup path catch that. This is not arbitrary write and not a full sandbox escape.The trigger is the same-user parent-swap race the library already defends against elsewhere, and which
SECURITY.mdand the guarded primitive treat as in scope.Root cause
src/root-impl.ts:825-830inopenWritableFileInRoot, before this change:Node's recursive
mkdirfollows symlinks in components that already exist. The only protection here is a guard on the nearest existing ancestor, and that guard compares that ancestor's dev/ino and realpath. Adding a symlink child to that ancestor changes neither, so both the pre-mutation and post-mutation guard assertions pass while the mkdir walks out of the root.src/guarded-mkdir.ts:60-62documents this exact hazard as the reason the guarded primitive exists:A pre-existing out-of-root symlink parent is already rejected by
assertNoPathAliasEscape. Only the swap window between that check and the mkdir is exposed, which is why this needs a race to reach.Fix
mkdirPathComponentsWithGuardscreates one component at a time, guards each parent across the mutation, and realpath-checks every created segment against the canonical root before descending. A parent swapped to an out-of-root symlink mid-call is rejected instead of followed. In-root symlinked components stay allowed, so the behavior added in #39 is preserved.Why this is the right boundary
openWritableFileInRootthat did not use it.mkdirPathFallback(src/root-impl.ts:1361) andrunPinnedWriteFallback(src/pinned-write.ts:262) already route through it, so this converges the writable-open path onto the same primitive rather than adding a second mechanism.write()andcreate()on POSIX were already safe: their parent creation goes throughmkdirPathComponentsWithGuardsor the fd-relative Python helper. This change makesappend(),openWritable(), and thecopyIn()fallback match them.mkdirPathComponentsWithGuardsalready returns the resolved real parent, but the existingfs.realpath(resolved)andisPathInsidehandling below the call site keeps working unchanged for in-root symlinked parents, so no further adjustment was needed and the diff stays at the one call site.EEXISTand the new code surfacesFsSafeError("not-file"). That matches what the pinned write path already returns for the same situation, so it makes the two configurations agree rather than diverge.writeMissingFileFallback(src/root-impl.ts:1600) has the same rawfs.mkdir(..., { recursive: true })with no guard around it at all. It is reachable only on Windows (writeFileInRootroutes towriteFileFallbackonly whenprocess.platform === "win32"), where the equivalent swap would use a directory junction. I have no Windows environment to reproduce or verify it on, so I left it out rather than ship an unverified change on a platform I cannot exercise. Happy to follow up separately, or to fold it in here if you would rather have both in one change.Verification
pnpm lint:file-sizeandpnpm lint:fs-boundary: clean.pnpm build: clean.pnpm test: 46 files, 501 passed, 4 skipped.test/write-boundary-bypass.test.tsfails on pristine main 8cced2a (out-of-rootlogsdirectory observed) and passes with this patch.Real behavior proof
Behavior addressed:
root().append()withmkdirenabled creates directories outside the root when a parent component is swapped to an out-of-root symlink during the call, while the caller receives an error and believes the operation was blocked.Real environment tested: the compiled package (
dist/index.js) built from pristine main 8cced2a and from this patch at 498ed24, driven through the publicroot()API with default options and defaultFS_SAFE_PYTHON_MODE=auto. Everything is real on-disk state on macOS: a real temporary workspace root, a real sibling victim directory outside it, and a separate forked OS process flipping<root>/databetween a plain directory and a symlink to the victim. Nothing is stubbed and there is no external service involved.Exact steps or command run after this patch:
node proof.mjs <dist> <label>, which opensroot(<workspace>), forks the attacker process, then callsawait r.append("data/logs/2026/07/app.log", "entry\n")in a bounded loop and walks the victim directory outside the root. Identical script, identical inputs, run against each build.Evidence after fix:
Observed result after fix: on pristine main the 13th call left the directory tree
logs/2026/07/outside the root while reportingnot-foundto the caller; with this patch 2000 calls against the same live swapping process left nothing outside the root and the rejection reason became the accuratenot-file: directory component must be a directory.What was not tested: Windows was not exercised, so the sibling
writeMissingFileFallbackpath noted above is unverified and unchanged. The proof is a timing race, so the "before" run reproduces probabilistically (13 attempts on this run, 5 to 20 typical); the "after" run is bounded at 2000 attempts rather than proven exhaustively. No published package was installed; the compiled output of each tree was loaded directly.