module: fail closed when reading package.json is denied

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>

Author: moeghassi

src/node_modules.cc

@@ -111,10 +111,26 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON(
   PackageConfig package_config{};
   package_config.file_path = path;
   // No need to exclude BOM since simdjson will skip it.
-  if (ReadFileSync(&package_config.raw_json, path.data()) < 0) {
-    // Add `nullopt` to the package config cache so that we don't
-    // need to open and attempt to read this path again
-    binding_data->package_configs_.insert({std::string(path), std::nullopt});
+  int read_err = ReadFileSync(&package_config.raw_json, path.data());
+  if (read_err < 0) {
+    // A missing file (ENOENT) or a non-directory path component (ENOTDIR)
+    // legitimately means "there is no package.json here". Treat it as absent
+    // and cache the negative result so we don't try to open this path again.
+    if (read_err == UV_ENOENT || read_err == UV_ENOTDIR) {
+      binding_data->package_configs_.insert({std::string(path), std::nullopt});
+      return nullptr;
+    }
+    // Any other failure (e.g. EACCES/EPERM, or an anti-malware/EDR deny that
+    // surfaces as a blocked read) is a security-relevant signal, not proof of
+    // absence. Fail closed: throw instead of silently falling back to the
+    // legacy `index.js` resolution, so a denied or quarantined manifest cannot
+    // be treated as "no package.json" and let the package load anyway.
+    // Intentionally not negative-cached so the deny isn't latched for the
+    // lifetime of the process.
+    THROW_ERR_ACCESS_DENIED(realm->isolate(),
+                            "Cannot read package config %s: %s",
+                            path.data(),
+                            uv_strerror(read_err));
     return nullptr;
   }
 
@@ -325,6 +341,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;
     }
@@ -435,6 +457,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,67 @@
+'use strict';
+
+// This test verifies that when reading a package's `package.json` fails with a
+// non-ENOENT error (e.g. EACCES, or an anti-malware/EDR deny that surfaces as a
+// blocked read), the module loader fails closed with ERR_ACCESS_DENIED instead
+// of treating the manifest as absent and silently falling back to `index.js`.
+//
+// It relies on chmod(0) to make the file unreadable, which only produces EACCES
+// for a non-root user on POSIX systems, so it is skipped on Windows and when
+// running as root.
+
+const common = require('../common');
+
+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 assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+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');
+}
+
+assert.throws(
+  () => require(pkgDir),
+  (err) => {
+    assert.strictEqual(err.code, 'ERR_ACCESS_DENIED');
+    return true;
+  },
+  'requiring a package whose package.json read is denied must fail closed');
+
+// The index.js fallback must NOT have executed.
+assert.strictEqual(
+  fs.existsSync(markerPath), false,
+  'index.js fallback ran despite the manifest being denied');
+
+// Restore perms so tmpdir cleanup can remove the file.
+fs.chmodSync(pkgJsonPath, 0o644);