module: fail closed when reading package.json is denied - #65030
Open
moeghassi wants to merge 1 commit into
Open
Conversation
GetPackageJSON() treated every negative return from ReadFileSync() the same way: it cached a negative result and reported the manifest as absent. A denied read (EACCES/EPERM, or an anti-malware/EDR block that surfaces as a failed open) was therefore indistinguishable from ENOENT, so resolution silently fell back to index.js and the package loaded anyway. This makes it impossible for an on-endpoint scanner to stop a require()/import of a quarantined package by denying its manifest. Only ENOENT and ENOTDIR now mean "no package.json here". Any other read error is treated as a security-relevant signal and throws ERR_ACCESS_DENIED instead of falling back, so a denied or quarantined manifest aborts resolution for every package style (default index.js, "main", and "exports"). The failure is intentionally not negative-cached so the deny is not latched for the lifetime of the process. The two parent-scope walks (TraverseParent and GetPackageScopeConfig) now stop when an exception is pending so the thrown error propagates instead of being swallowed by continuing up the tree. Signed-off-by: Moe Ghasemi <moeghasemi@microsoft.com>
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.
Summary
GetPackageJSON()insrc/node_modules.cctreated every negative returnfrom
ReadFileSync()identically: it negative-cached the path and reported themanifest as absent. As a result a denied read (
EACCES/EPERM, or ananti-malware/EDR block that surfaces as a failed open) is indistinguishable from
ENOENT, so resolution silently falls back toindex.jsand the package loadsanyway.
This means an on-endpoint scanner cannot stop a
require()/importof aquarantined package by denying/quarantining its
package.jsonβ Node treats thedenied manifest as "no manifest" and runs the code.
Change
ENOENTandENOTDIRnow mean "there is nopackage.jsonhere"(unchanged behavior, still negative-cached).
ERR_ACCESS_DENIEDinstead of falling back β so a denied/quarantined manifestaborts resolution for every package style (default
index.js,"main", and"exports"), not justexports-based ones. The failure is intentionallynot negative-cached so the deny isn't latched for the process lifetime.
TraverseParentandGetPackageScopeConfignow stop walking parent scopeswhen an exception is pending, so the thrown error propagates instead of being
swallowed by continuing up the tree.
Adds a POSIX regression test (
chmod(0)the manifest β assertsERR_ACCESS_DENIEDand that theindex.jsfallback did not run; skipped onWindows and when running as root).
Notes for reviewers
non-
ENOENTread errors). It may warrant a semver-major label: users witha legitimately unreadable ancestor directory (
EACCESwhile walking up forpackage-type detection) would now get a thrown error instead of silent
fallback. I'm happy to scope the throw more narrowly (e.g. only the target
package's own manifest, not ancestor
typewalks) if preferred.scope is very welcome.
I was unable to run a full local build to compile-check (the environment lacks
the ClangCL toolset required for the current
main); CI will be the source oftruth here.