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); +}