From 9ded7781a9bb13b5535962e3a8d26ea952b2ca8e Mon Sep 17 00:00:00 2001 From: Moe Ghasemi Date: Tue, 4 Aug 2026 18:10:07 -0700 Subject: [PATCH 1/2] 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 f88fc461f4d6..9ba59c9b0637 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 000000000000..0cd005fe9971 --- /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); +} From 3e074003f941a4c64391883ba3e01d173e5f2f5b Mon Sep 17 00:00:00 2001 From: Moe Ghasemi Date: Tue, 27 Oct 2026 14:49:11 -0700 Subject: [PATCH 2/2] module: test package config error propagation Cover pending exceptions in package JSON traversal and scope lookups, including failed-read recovery, and update the loader error expectation. Apply the C++ formatting requested by CI. Signed-off-by: Moe Ghasemi Assisted-by: GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 39bc512f-a84d-4c8b-a2f8-a2f25e9ee33f --- src/node_modules.cc | 6 +- test/parallel/test-module-loading-error.js | 4 +- ...t-module-package-json-error-propagation.js | 76 +++++++++++++++++++ 3 files changed, 81 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-module-package-json-error-propagation.js diff --git a/src/node_modules.cc b/src/node_modules.cc index 9ba59c9b0637..db600fc08ad3 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -264,9 +264,9 @@ const BindingData::PackageConfig* BindingData::GetPackageJSON( // 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)); + "Cannot read package config %s: %s", + path.data(), + uv_strerror(read_error)); return nullptr; } diff --git a/test/parallel/test-module-loading-error.js b/test/parallel/test-module-loading-error.js index 3496a4104df0..9dae18ed091a 100644 --- a/test/parallel/test-module-loading-error.js +++ b/test/parallel/test-module-loading-error.js @@ -90,7 +90,7 @@ assert.throws( assert.throws( () => { require('../fixtures/packages/is-dir'); }, common.isAIX ? { code: 'ERR_INVALID_PACKAGE_CONFIG' } : { - code: 'MODULE_NOT_FOUND', - message: /Cannot find module '\.\.\/fixtures\/packages\/is-dir'/ + code: 'ERR_ACCESS_DENIED', + message: /Cannot read package config .*package\.json: / } ); diff --git a/test/parallel/test-module-package-json-error-propagation.js b/test/parallel/test-module-package-json-error-propagation.js new file mode 100644 index 000000000000..70734d8ccd2b --- /dev/null +++ b/test/parallel/test-module-package-json-error-propagation.js @@ -0,0 +1,76 @@ +'use strict'; +// Flags: --expose-internals + +require('../common'); +const assert = require('node:assert'); +const fs = require('node:fs'); +const path = require('node:path'); +const { pathToFileURL } = require('node:url'); +const tmpdir = require('../common/tmpdir'); +const { + getNearestParentPackageJSON, + getRootPackageJSON, + getPackageScopeConfig, + getPackageType, +} = require('internal/modules/package_json_reader'); + +tmpdir.refresh(); + +const lookups = { + nearestParent: (file) => getNearestParentPackageJSON(file).data.type, + root: (file) => getRootPackageJSON(file).data.type, + scope: (file) => getPackageScopeConfig(pathToFileURL(file)).type, + type: (file) => getPackageType(pathToFileURL(file)), +}; + +for (const [name, lookup] of Object.entries(lookups)) { + for (const failure of ['invalid', 'directory']) { + const outer = tmpdir.resolve(`${name}-${failure}`); + const parent = name === 'root' ? + path.join(outer, 'node_modules', 'outer') : outer; + const inner = name === 'root' ? + path.join(parent, 'node_modules', 'inner') : path.join(parent, 'inner'); + const manifest = path.join(inner, 'package.json'); + const parentManifest = path.join(parent, 'package.json'); + const file = path.join(inner, 'lib', 'index.js'); + fs.mkdirSync(path.dirname(file), { recursive: true }); + + // Continuing past the first error would replace it with this parent's error. + fs.writeFileSync(parentManifest, '{'); + if (failure === 'invalid') { + fs.writeFileSync(manifest, '{'); + } else { + fs.mkdirSync(manifest); + + // Some platforms allow reading a directory. + let directoryReadError; + try { + fs.readFileSync(manifest); + } catch (err) { + directoryReadError = err; + } + if (!directoryReadError) { + continue; + } + assert.strictEqual(directoryReadError.code, 'EISDIR'); + } + + const expectedError = { + code: failure === 'invalid' ? + 'ERR_INVALID_PACKAGE_CONFIG' : 'ERR_ACCESS_DENIED', + message: new RegExp(RegExp.escape(path.toNamespacedPath(manifest))), + }; + assert.throws(() => lookup(file), expectedError); + + // A failed lookup must not cache the manifest as missing. + assert.throws(() => lookup(file), expectedError); + + if (failure === 'directory') { + fs.rmdirSync(manifest); + } + const config = JSON.stringify({ type: 'module', private: true }); + fs.writeFileSync(manifest, config); + fs.writeFileSync(parentManifest, config); + assert.strictEqual(lookup(file), 'module'); + } +}