module: report unreadable package.json

Project: nodejs/node | Language: C++/JavaScript | Concepts: error-handling, testing-strategy, api-design | Difficulty: mid
Link: https://github.com/nodejs/node/pull/65223

The problem

If a package.json file exists but cannot be read (for example, due to permission errors), Node.js previously treated it as if it did not exist at all. This meant that important fields like "exports" and "type" were silently ignored, and module resolution could fall back to unintended files, potentially breaking package boundaries or security expectations.

Why it's non-trivial

A naive fix might simply throw on any read error, but some errors (like file not found or not a directory) are expected and should not be treated as fatal. Only genuine read failures (e.g., permission denied) should surface as errors. The fix must distinguish between these cases and align behavior with how parse errors are already handled.

The approach

1. Distinguish error types when reading package.json:

-  if (ReadFileSync(&package_config.raw_json, path.data()) < 0) {
+  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));
+      return nullptr;
+    }
+
+    // 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});

What this does:

• Checks the result of reading package.json.
• If the error is "file not found", "not a directory", or "is a directory", it is treated as "no package config here" (the old behavior).
• For any other error (e.g., permission denied), throws an error with a clear message, surfacing the underlying problem instead of silently ignoring it.

2. Add a regression test to ensure proper error reporting:

+fs.chmodSync(depPackageJson, 0o000);
+
+try {
+  const child = spawnSync(process.execPath, [entry], { encoding: 'utf8' });
+  // The read failure must be reported rather than resolving to index.js.
+  assert.doesNotMatch(child.stdout, /decoy/);
+  assert.match(child.stderr, /Cannot read package config/);
+  assert.notStrictEqual(child.status, 0);
+} finally {
+  fs.chmodSync(depPackageJson, 0o644);
+}

What this does:

• Makes package.json unreadable.
• Runs a child process to import the package.
• Asserts that the error is reported (stderr contains the expected message), and that fallback to the wrong file does not occur.
• Restores file permissions in a finally block to ensure cleanup even if the test fails.

3. Test both readable and unreadable cases:

+// Sanity check: the export resolves while the package config is readable.
+{
+  const child = spawnSync(process.execPath, [entry], { encoding: 'utf8' });
+  assert.strictEqual(child.stdout.trim(), 'real');
+  assert.strictEqual(child.status, 0, child.stderr);
+}

What this does:

• Verifies that with normal permissions, the correct export is resolved and the process exits successfully.

From the review thread

aduh95:
The status check should always come last as it's the least informative one. Also, we should ensure the last chmodSync is called even if there's a test failure

try {
  const child = spawnSync(process.execPath, [entry], { encoding: 'utf8' });
  // The read failure must be reported rather than resolving to index.js.
  assert.doesNotMatch(child.stdout, /decoy/);
  assert.match(child.stderr, /Cannot read package config/);
  assert.notStrictEqual(child.status, 0);
} finally {
  fs.chmodSync(depPackageJson, 0o644);
}

bitpshr:
Checked the commit before the resolver moved to C++ (f13dbfd43, parent 4ec085b240). The JS read() there treated an undefined return from internalModuleReadJSON() as "does not exist" with no throw, and only a parse failure threw ERR_INVALID_PACKAGE_CONFIG. InternalModuleReadJSON() in node_file.cc bailed with a bare return for any uv_fs_open() failure, so EACCES came back as undefined just like ENOENT.

So the silent fallback looks like it predates the C++ move rather than being a regression from it, which fits there never being coverage. Happy to be wrong if you remember an earlier version that threw, I only went back as far as the refactor.

Takeaways

• Always distinguish between expected and unexpected errors when reading files; only treat truly exceptional cases as fatal.
• Surface errors to the user when silent fallback could lead to incorrect or surprising behavior, especially in module/package resolution.
• Ensure tests not only cover the "happy path" but also simulate and assert on error conditions, including proper cleanup after test execution.