fix: bridge colon-tainted bin paths so hoisted bins resolve in lifecycle scripts - #281
Open
shauryagangrade wants to merge 1 commit into
Open
fix: bridge colon-tainted bin paths so hoisted bins resolve in lifecycle scripts#281shauryagangrade wants to merge 1 commit into
shauryagangrade wants to merge 1 commit into
Conversation
…cle scripts
When any ancestor directory of a project contains the POSIX PATH delimiter
(':' on unix), set-path would emit a node_modules/.bin entry with an
embedded ':' that the shell splits into two entries, so hoisted binaries
like tsc could not be found (exit 127).
Since a ':' cannot be escaped in a PATH entry, present a colon-free symlink
mirror instead: for each colon-containing node_modules dir, create a
node_modules symlink under a tmp root and put the mirrored .bin path on
PATH. Symlinking node_modules (not .bin) preserves the relative
/../<pkg> resolution used by bin shims.
POSIX-only (Windows PATH delimiter is ';' and drive letters contain ':').
Degrades to the existing warning when the tmpdir itself contains the
delimiter. No behavior change for colon-free projects.
Fixes npm/cli#9910
3 tasks
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 npm/cli#9910
Problem
When any ancestor directory of a project contains the POSIX PATH delimiter
(
:on unix),@npmcli/run-scriptbuilds a lifecycle-script PATH whosenode_modules/.binentry contains an embedded:. The shell splits thatentry at the
:into two bogus entries, so hoisted binaries liketsccannot be found (
tsc: command not found, exit 127).Repro (faithful to the issue):
Running a workspace lifecycle script that calls
tscfails with exit 127.Renaming the parent to
parent-directoryfixes it, confirming the PATHdelimiter collision as the root cause.
Why not a trivial fix
A
:cannot be escaped inside a PATH entry on POSIX — there is no escapingin PATH. The current code already knows about this (it emits
Path contains delimiter ... may not behave as expected), but provides no workaround.Approach: symlink bridge
Since a colon-containing dir cannot be put on PATH, present a colon-free
symlink mirror instead:
node_modulesdir, create<tmp>/npm-run-script-bridge-<uid>-<hash>/<nmhash>/node_modulesas asymlink to the real
node_modules..binpath on PATH instead of the real one.We symlink
node_modulesitself (not.bin) because hoisted bin entriesresolve their real target relative to the
.bindir(
$basedir/../<pkg>/...). Symlinkingnode_moduleskeeps that relativeresolution landing in the real project tree, so existing bins keep working.
Guards
;and drive letters (C:)contain
:— the bridge would false-positive. Guarded onsep === '/' && delimiter === ':'.:.Implementation
lib/bin-bridge.js(new):createBinBridge(projectPath, opts)returns apath-mapping function with lazy tmp-root creation, deterministic
per-user/per-project hashing,
0700perms, and a parallel-race guard.lib/set-path.js: route both thebinPathsentries and the walk-upnode_modules/.binentries through the bridge; only warn when a path isunbridgeable.
Tests
test/bin-bridge.js(new): unit tests for guards/identity/determinism plusintegration tests that execute a shim resolving relative to its new location
through the bridge, and
setPATH-level assertions that the emitted PATH iscolon-free.
npm testpasses (100% coverage, lint clean).Changelog / semver
fix:→ patch release, per CONTRIBUTING.md.