From 9c6c2a4f61c4a7bd628afdb363c878e8371a5163 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() 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 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: c9e5a501-399c-443b-9241-2c71246b185f --- src/node_modules.cc | 36 +++++--- ...test-require-package-json-access-denied.js | 91 +++++++++++++++++++ 2 files changed, 115 insertions(+), 12 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 f88fc461f4d..9ba59c9b063 100644 --- a/src/node_modules.cc +++ b/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; 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 00000000000..0cd005fe997 --- /dev/null +++ b/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); +}