module: fail closed when reading package.json is denied

GetPackageJSON() now reports unreadable manifests, but treats a
directory named package.json as absent and reports other read failures
as ERR_INVALID_PACKAGE_CONFIG.

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 parent-scope walks and FindNodeModulesPackage now stop when an
exception is pending so the thrown error propagates instead of being
swallowed by continuing up the tree. Cover directory manifests, retry
after a failed read, and unreadable files with regression tests.

Signed-off-by: Moe Ghasemi <moeghasemi@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c9e5a501-399c-443b-9241-2c71246b185f

Author: moeghassi

src/node_modules.cc

@@ -258,18 +258,15 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
   // No need to exclude BOM since simdjson will skip it.
   int read_error = ReadFileSync(&package_config.raw_json, path.data());
   if (read_error < 0) {
-    // No file at this path, a path component that is not a directory, or a
-    // "package.json" that is itself a directory all mean there is no package
-    // config here. Any other failure means a package.json is present but
-    // could not be read. Treating that as absent silently drops fields such
-    // as "exports" and "type", which can resolve a specifier to a different
-    // file, so surface the read error instead of continuing.
-    if (read_error != UV_ENOENT && read_error != UV_ENOTDIR &&
-        read_error != UV_EISDIR) {
-      THROW_ERR_INVALID_PACKAGE_CONFIG(realm->isolate(),
-                                       "Cannot read package config %s: %s.",
-                                       path.data(),
-                                       uv_strerror(read_error));
+    // Only a missing file or a non-directory path component means no config.
+    // Other failures, including a directory named package.json, must stop
+    // resolution rather than silently allowing index.js fallback. Do not cache
+    // these failures, since read access may be restored later.
+    if (read_error != UV_ENOENT && read_error != UV_ENOTDIR) {
+      THROW_ERR_ACCESS_DENIED(realm->isolate(),
+                             "Cannot read package config %s: %s",
+                             path.data(),
+                             uv_strerror(read_error));
       return nullptr;
     }
 
@@ -406,6 +403,12 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
 
     auto package_json =
         GetPackageJSON(realm, ConvertPathToUTF8(package_json_path), nullptr);
+    // A denied/quarantined manifest (or an invalid package config) throws;
+    // stop walking so the pending exception propagates instead of being
+    // swallowed by continuing up the tree.
+    if (realm->isolate()->HasPendingException()) [[unlikely]] {
+      return nullptr;
+    }
     if (package_json != nullptr) {
       return package_json;
     }
@@ -459,6 +462,9 @@ const BindingData::PackageConfig* BindingData::FindNodeModulesPackage(
 
       auto package_json =
           GetPackageJSON(realm, ConvertPathToUTF8(package_json_path), nullptr);
+      if (realm->isolate()->HasPendingException()) [[unlikely]] {
+        return nullptr;
+      }
       if (package_json != nullptr) {
         if (!package_json->is_private.value_or(false)) {
           // A publishable (non-private) package encloses the file.
@@ -598,6 +604,12 @@ void BindingData::GetPackageScopeConfig(
     error_context.specifier = resolved.ToString();
     auto package_json =
         GetPackageJSON(realm, file_path_buf.ToStringView(), &error_context);
+    // A denied/quarantined manifest (or an invalid package config) throws;
+    // stop walking so the pending exception propagates instead of being
+    // swallowed by continuing up the tree.
+    if (realm->isolate()->HasPendingException()) [[unlikely]] {
+      return;
+    }
     if (package_json != nullptr) {
       if constexpr (return_only_type) {
         Local<Value> value;

test/parallel/test-require-package-json-access-denied.js

@@ -0,0 +1,91 @@
+'use strict';
+
+// This test verifies that when reading a package's `package.json` fails with a
+// non-ENOENT/ENOTDIR error (e.g. EACCES, EISDIR, or an anti-malware/EDR deny),
+// the module loader fails closed with ERR_ACCESS_DENIED instead
+// of treating the manifest as absent and silently falling back to `index.js`.
+//
+// The unreadable-file case relies on chmod(0), which only produces EACCES
+// for a non-root user on POSIX systems.
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+{
+  const dirPkg = path.join(tmpdir.path, 'node_modules', 'directory-manifest');
+  const manifest = path.join(dirPkg, 'package.json');
+  const marker = path.join(tmpdir.path, 'directory-loaded.marker');
+  fs.mkdirSync(manifest, { recursive: true });
+  fs.writeFileSync(
+    path.join(dirPkg, 'index.js'),
+    `require('fs').writeFileSync(${JSON.stringify(marker)}, 'loaded');\n`);
+
+  // Some platforms can read directories, so only test this error when the
+  // filesystem actually reports EISDIR.
+  let directoryReadError;
+  try {
+    fs.readFileSync(manifest);
+  } catch (err) {
+    directoryReadError = err;
+  }
+  if (directoryReadError) {
+    assert.strictEqual(directoryReadError.code, 'EISDIR');
+    assert.throws(() => require(dirPkg), { code: 'ERR_ACCESS_DENIED' });
+    assert.strictEqual(fs.existsSync(marker), false);
+
+    // A failed read must not be cached as an absent package config.
+    fs.rmdirSync(manifest);
+    fs.writeFileSync(manifest, '{"main":"real.js"}');
+    fs.writeFileSync(path.join(dirPkg, 'real.js'), 'module.exports = 42;\n');
+    assert.strictEqual(require(dirPkg), 42);
+    assert.strictEqual(fs.existsSync(marker), false);
+  }
+}
+
+if (common.isWindows) {
+  common.skip('chmod(0) does not produce EACCES on Windows');
+}
+if (!process.getuid || process.getuid() === 0) {
+  common.skip('cannot produce EACCES as root');
+}
+
+const pkgDir = path.join(tmpdir.path, 'node_modules', 'evil');
+fs.mkdirSync(pkgDir, { recursive: true });
+
+// A default-`index.js` package: before the fix, a denied manifest was swallowed
+// (index.js fallback) and this code would run anyway.
+const indexPath = path.join(pkgDir, 'index.js');
+const markerPath = path.join(tmpdir.path, 'loaded.marker');
+fs.writeFileSync(
+  indexPath,
+  `require('fs').writeFileSync(${JSON.stringify(markerPath)}, 'loaded');\n`);
+
+const pkgJsonPath = path.join(pkgDir, 'package.json');
+fs.writeFileSync(pkgJsonPath, JSON.stringify({ name: 'evil', version: '1.0.0' }));
+
+// Deny reads of the manifest to simulate a quarantine/anti-malware block.
+fs.chmodSync(pkgJsonPath, 0o000);
+
+// Sanity check: the deny actually took effect (otherwise the test is moot).
+try {
+  fs.readFileSync(pkgJsonPath);
+  common.skip('environment allowed reading a chmod(0) file');
+} catch (err) {
+  assert.strictEqual(err.code, 'EACCES');
+}
+
+try {
+  assert.throws(
+    () => require(pkgDir),
+    { code: 'ERR_ACCESS_DENIED' },
+    'requiring a package whose package.json read is denied must fail closed');
+
+  assert.strictEqual(fs.existsSync(markerPath), false);
+} finally {
+  fs.chmodSync(pkgJsonPath, 0o644);
+}