Repository navigation
fix(test): compare install VM paths in one spelling so symlinked tmpdirs pass - #1120
Merged
benvinegar merged 2 commits intoSep 23, 2026
Merged
Conversation
Contributor
|
PR author is not in the allowed authors list. |
|
@shashank-100 is attempting to deploy a commit to the Modem Team on Vercel. A member of the Team first needs to authorize it. |
Member
|
Thank you! |
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
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.
Problem
assertSafeInstallVmRuntimePathcanonicalizes the repo root but not the target, then compares them:On macOS the harness builds fixtures under
os.tmpdir()=/var/folders/..., where/varis a symlink to/private/var. SoallowedRootis/private/var/.../tmp/install-vmwhileresolvedis/var/.../tmp/install-vm/cache.path.relativebetween the two spellings yields an absolute escape path, and a harness-owned directory is rejected as foreign:The same asymmetry corrupts the symlink-ancestor walk below it, which bases its cursor on
physicalRepoRootwhile iterating segments taken relative to the non-physicalresolved.Impact: three tests fail on macOS, so
bun run testcannot go green locally on a Mac:install VM contract > allows cleaning only real harness-owned paths and rejects symlink ancestorsdisposable VM shell > stages only curated examples and deterministic benchmark patches by defaultdisposable VM shell > runs a fresh host build before adding Hunk to the staged shell inputA symlinked
$HOMEreproduces it on Linux; CI presumably runs where/varis real, which is why it is green there.Approach
Rewrite the target onto the repo's physical prefix and use that for the containment check and the symlink walk, so both sides are compared in one spelling.
Crucially, only the prefix is canonicalized. Every segment below the repo root is left verbatim, because resolving those would silently disarm the guard this function exists to enforce: a symlink inside the tree would be collapsed by
realpathSyncbefore the symlink-ancestor walk could see it. A plainrealpathSync(target)fails for that reason and one more — it throws on a leaf that does not exist yet, which callers legitimately pass.rebaseOnPhysicalRepoRoottries the caller's spelling of the repo root and then the physical one, and rebases the first that contains the target. A target outside both is returned untouched so the containment check rejects it.Non-goals: no change to what is considered owned, no new permitted root, and no relaxation of any rejection.
Tests
Three tests added in
test/cli/install-vm/contract.test.ts:accepts owned paths when the repo sits under a symlinked ancestor— reaches the repo through a symlinked ancestor and asserts both an existing leaf and a not-yet-created one are accepted and returned in the caller's spelling, while a path outside the runtime tree still throws.rejects a symlink inside the tree even when the full path exists— a symlink redirecting withintmp/install-vmis refused in all three shapes: as the leaf, with an existing tail, and with a missing tail. The middle case is the one a prefix-only canonicalization protects; resolving the full target would accept it.rejects a target reached through a symlink that escapes the harness tree— pins the escape rejections: a child through an escaping symlink,../..traversal, and an outside directory all throw, and the file outside survives.Verified by differential run rather than assertion alone. Against
mainthese fail 4; against the first revision of this branch they fail 2; with the current fix, 19 pass and 0 fail. Each new test therefore fails against at least one broken implementation.Each rejection now asserts which guard fired (
"symlink ancestor"vs"outside") instead of a bare.toThrow(), so a change that swaps one rejection for another is visible.Commands run
mainfails 6 on this machine; this branch fails 2, neither related:Jujutsu source reading > logs unexpected source failures...— pre-existing, fails on cleanmain, andpackages/hunk-jjis not in this diff.hunk.dev install script > resolves through Hunk and falls back directly to GitHub— a 5s network timeout. Passes in isolation on both this branch and cleanmain; it imports nothing this diff touches.Platforms
Fixed and verified on macOS (darwin 27.0.0), which is where the bug reproduces. The change is
node:path/node:fsonly and adds no platform branch; the new tests build fixtures withpath.joinandmkdtempSync. Behavior wheretmpdir()has no symlinked ancestor is unchanged, since canonicalizing an already-physical path is a no-op — that is the Linux CI case, and its coverage is the existing suite.Symlink creation is unprivileged on macOS and Linux but needs elevation or Developer Mode on Windows. The two new tests call
symlinkSync, matching the existing test directly above them, so this introduces no new Windows constraint.Changeset
Empty — test-harness only, nothing user-visible.
Review history
The first revision of this branch canonicalized the whole target through its nearest existing ancestor. Code review caught that this collapsed symlinks above that point, so
alias/sub(a symlink resolving inside the tree, with every segment existing) was accepted wheremainrejected it — a caller'srmSyncwould then have traversed it. It did not permit escaping the tree, since containment still rejected symlinks resolving outside, but it weakened the no-symlink policy. Fixed in7239571fby canonicalizing only the repo-root prefix, with the case added as a regression test.🤖 Generated with Claude Code