fix(resolution): keep the existence probe inside the project root (#1631) - #1632
Open
maxmilian wants to merge 1 commit into
Open
fix(resolution): keep the existence probe inside the project root (#1631)#1632maxmilian wants to merge 1 commit into
maxmilian wants to merge 1 commit into
Conversation
…lbymchenry#1631) `fileExists` fell back to `path.join(projectRoot, filePath)`, which does not clamp, while relative-import resolution hands it paths carrying `../` segments — so a crafted import in an indexed file made the resolver stat arbitrary absolute paths. Nothing outside the root is read (the content sinks are guarded separately) and no edge is produced, but the probe is an existence oracle driven by repository content. Contain the fallback lexically. A path outside the root can never be an indexed project file, and the `knownFiles` set consulted first already answered for every path that is, so refusing is the correct result rather than a new restriction. Lexical containment only, via a new `lexicalPathWithinRoot` split out of `validatePathWithinRoot`: this is a per-candidate hot path, and the symlink half costs two `realpathSync` calls per probe — measured at ~106-153us/call against ~1.5us before, a ~70-100x regression. It is also the wrong check here, since indexing deliberately follows in-root symlinks whose targets live outside the root (colbymchenry#935). With the lexical guard the probe costs ~1.9us/call. Co-Authored-By: Claude <noreply@anthropic.com>
maxmilian
marked this pull request as ready for review
August 28, 2026 04:49
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.
Fixes #1631.
fileExistsfell back topath.join(this.projectRoot, filePath), which does not clamp, while relative-import resolution hands it paths carrying../segments. A crafted relative import in an indexed file therefore made the resolverstatarbitrary absolute paths. Nothing outside the root is read (the content sinks are guarded separately byvalidatePathWithinRoot) and no edge is produced, but the probe is an existence oracle driven by repository content — and every other layer contains paths, so this one quietly not doing it is surprising on its own.The
knownFilesset is consulted first, so any indexed file resolves regardless of where it lives; only the filesystem fallback needs bounding. A path outside the root can never be an indexed project file, so returningfalseis the correct answer rather than a new restriction.Why lexical containment, and not
validatePathWithinRootThe issue suggests
validatePathWithinRoot(root, p, { allowSymlinkEscape: true }). That is right about the intent but wrong about the cost here:fileExistsruns per import candidate, andvalidatePathWithinRootdoes tworealpathSynccalls per invocation. Measured over 20,000 calls:validatePathWithinRootThe missing-file column is worse because the realpath pair goes through the
ENOENTthrow path, and building the exception dominates.It is also the wrong check semantically. The symlink half exists to stop an in-root symlink whose real target escapes the root (#527) — but indexing deliberately follows exactly those symlinks (#935), which is why
allowSymlinkEscapeexists in the first place. On this path only the lexical../escape should be refused.So this splits the lexical half out as
lexicalPathWithinRootand hasvalidatePathWithinRootcall it, rather than duplicating the containment rule. Its doc comment says plainly that it is not a substitute for the full check on any path whose contents get served.Tests
__tests__/resolution-fileexists-containment.test.ts, three cases:falsecan only come from the guard) — this is the one that fails before the changeFull suite: 179 files / 3055 tests passing (3052 on
mainplus these three).npm run buildclean.Reported and diagnosed by @ErQrYfkrju, who deliberately kept it out of #1630 so it wouldn't land as a single-language guarantee.