Skip to content

module: fail closed when reading package.json is denied - #65030

Open
moeghassi wants to merge 1 commit into
nodejs:mainfrom
moeghassi:module-fail-closed-denied-package-json
Open

module: fail closed when reading package.json is denied#65030
moeghassi wants to merge 1 commit into
nodejs:mainfrom
moeghassi:module-fail-closed-denied-package-json

Conversation

@moeghassi

Copy link
Copy Markdown

Summary

GetPackageJSON() in src/node_modules.cc treated every negative return
from ReadFileSync() identically: it negative-cached the path and reported the
manifest as absent. As a result a denied read (EACCES/EPERM, or an
anti-malware/EDR block that surfaces as a failed open) is indistinguishable from
ENOENT, so resolution silently falls back to index.js and the package loads
anyway.

This means an on-endpoint scanner cannot stop a require()/import of a
quarantined package by denying/quarantining its package.json — Node treats the
denied manifest as "no manifest" and runs the code.

Change

  • Only ENOENT and ENOTDIR now mean "there is no package.json here"
    (unchanged behavior, still negative-cached).
  • Any other read error is treated as a security-relevant signal and throws
    ERR_ACCESS_DENIED instead of falling back — so a denied/quarantined manifest
    aborts resolution for every package style (default index.js, "main", and
    "exports"), not just exports-based ones. The failure is intentionally
    not negative-cached so the deny isn't latched for the process lifetime.
  • TraverseParent and GetPackageScopeConfig now stop walking parent scopes
    when 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 → asserts
ERR_ACCESS_DENIED and that the index.js fallback did not run; skipped on
Windows and when running as root).

Notes for reviewers

  • This is a behavior change to module resolution (fail-open → fail-closed on
    non-ENOENT read errors). It may warrant a semver-major label: users with
    a legitimately unreadable ancestor directory (EACCES while walking up for
    package-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 type walks) if preferred.
  • cc @nodejs/loaders @nodejs/security-wg — feedback on the desired posture and
    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 of
truth here.

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>
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 5, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@moeghassi@nodejs-github-bot