From 9759adff47f1fcaefc1ca39b3b0f1a806d58da77 Mon Sep 17 00:00:00 2001 From: Moe Ghasemi Date: Tue, 4 Aug 2026 18:10:07 -0700 Subject: [PATCH] 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 --- src/node_modules.cc | 36 ++++++++-- ...test-require-package-json-access-denied.js | 67 +++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-require-package-json-access-denied.js diff --git a/src/node_modules.cc b/src/node_modules.cc index 717d84a4f89a..4c8999659100 100644 --- a/src/node_modules.cc +++ b/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; diff --git a/test/parallel/test-require-package-json-access-denied.js b/test/parallel/test-require-package-json-access-denied.js new file mode 100644 index 000000000000..f4c761630b1a --- /dev/null +++ b/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);