Skip to content

fix: lazy PKG_NATIVE_CACHE_PATH eval + integrity check for cached .node files #228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
olegcS wants to merge 1 commit into from
Closed

fix: lazy PKG_NATIVE_CACHE_PATH eval + integrity check for cached .node files #228

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions prelude/bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2187,7 +2187,10 @@ function payloadFileSync(pointer) {
// - Windows: C:\Users\John\.cache
// Custom example: /opt/myapp/cache or C:\myapp\cache
// Native addons will be extracted to: <PKG_NATIVE_CACHE_BASE>/pkg/<hash>
const PKG_NATIVE_CACHE_BASE =
//
// Note: We capture the initial value as the default, but re-read process.env inside
// process.dlopen so that runtime changes to PKG_NATIVE_CACHE_PATH take effect.
Copy link
Copy Markdown
Member

@robertsLando robertsLando Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security concern: Re-reading process.env.PKG_NATIVE_CACHE_PATH at each dlopen call means any code (including a compromised dependency) that runs before a native addon require() can redirect where addons are extracted to. This turns an immutable, bootstrap-time config into a mutable runtime setting, opening the door to arbitrary directory creation and file writes at attacker-chosen paths.

The current behavior — capturing the value once before any user code runs — is the safer design. If runtime override is truly needed, consider a freeze-after-first-use pattern instead.

const PKG_NATIVE_CACHE_DEFAULT =
Comment on lines 2189 to +2193
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment still refers to <PKG_NATIVE_CACHE_BASE>/pkg/<hash>, but the code now uses cacheBase and PKG_NATIVE_CACHE_DEFAULT. Please update the wording to match the current variables/env var behavior to avoid misleading users reading this section.

Copilot uses AI. Check for mistakes.
process.env.PKG_NATIVE_CACHE_PATH || path.join(homedir(), '.cache');

function revertMakingLong(f) {
Expand All @@ -2209,7 +2212,9 @@ function payloadFileSync(pointer) {
// the hash is needed to be sure we reload the module in case it changes
const hash = createHash('sha256').update(moduleContent).digest('hex');

const tmpFolder = path.join(PKG_NATIVE_CACHE_BASE, 'pkg', hash);
// Re-read PKG_NATIVE_CACHE_PATH at each call so runtime changes take effect
const cacheBase = process.env.PKG_NATIVE_CACHE_PATH || PKG_NATIVE_CACHE_DEFAULT;
const tmpFolder = path.join(cacheBase, 'pkg', hash);
Comment on lines +2215 to +2217
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces new runtime behavior (lazy PKG_NATIVE_CACHE_PATH evaluation and integrity verification for cached .node files) but there are no accompanying tests. Please add/extend a test to cover: (1) setting process.env.PKG_NATIVE_CACHE_PATH at runtime before first native addon load, and (2) detecting a tampered cached .node file and forcing re-extraction.

Copilot generated this review using guidance from repository custom instructions.

fs.mkdirSync(tmpFolder, { recursive: true });

Expand Down Expand Up @@ -2237,7 +2242,17 @@ function payloadFileSync(pointer) {
} else {
const tmpModulePath = path.join(tmpFolder, moduleBaseName);

if (!fs.existsSync(tmpModulePath)) {
if (fs.existsSync(tmpModulePath)) {
// Verify cached file integrity against snapshot content.
// The folder name encodes the expected hash, but the file inside could
// have been replaced (e.g. by a local user to inject malicious code).
const cachedContent = fs.readFileSync(tmpModulePath);
const cachedHash = createHash('sha256').update(cachedContent).digest('hex');
if (cachedHash !== hash) {
// Cached file was tampered with or corrupted — re-extract from snapshot
Comment on lines +2249 to +2252
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existsSync -> readFileSync/hash flow can throw if the cached file is deleted/replaced between the calls (or if permissions/locks prevent reading). Consider wrapping the integrity check in a try/catch and falling back to re-extracting from the snapshot (or treating read errors as a cache miss) to avoid crashing in these race/error cases.

Suggested change
const cachedContent = fs.readFileSync(tmpModulePath);
const cachedHash = createHash('sha256').update(cachedContent).digest('hex');
if (cachedHash !== hash) {
// Cached file was tampered with or corrupted — re-extract from snapshot
try {
const cachedContent = fs.readFileSync(tmpModulePath);
const cachedHash = createHash('sha256')
.update(cachedContent)
.digest('hex');
if (cachedHash !== hash) {
// Cached file was tampered with or corrupted — re-extract from snapshot
fs.copyFileSync(modulePath, tmpModulePath);
}
} catch (_) {
// Treat read/hash failures as a cache miss and restore from snapshot.

Copilot uses AI. Check for mistakes.
fs.copyFileSync(modulePath, tmpModulePath);
}
} else {
fs.copyFileSync(modulePath, tmpModulePath);
}

Expand Down