Greptile Summary

This PR closes an npm_execpath injection vulnerability where a workspace .env could redirect the bundled runtime dep installer to execute an attacker-controlled script. The fix applies three independent layers: blocking NPM_EXECPATH in BLOCKED_WORKSPACE_DOTENV_KEYS, stripping both casings from the install env in createBundledRuntimeDepsInstallEnv, and removing the env read entirely from resolveBundledRuntimeDepsNpmRunner. All three layers are exercised by the updated test suite using strict .toEqual assertions.

Confidence Score: 5/5

Safe to merge — the fix is correct, all-casing variants are handled, and tests are strict.

All three mitigating layers are implemented correctly. Case normalization is consistent (toUpperCase() in the dotenv blocker, toLowerCase() in the install-env stripper). The runner no longer reads npm_execpath at all. Production call sites always pass the stripped installEnv to the runner. No regressions or missing guards identified.

No files require special attention.

_{Reviews (2): Last reviewed commit: "fix: address PR review feedback" | Re-trigger Greptile}

@greptile review

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by both MEMBER author association and the maintainer label, so it needs explicit maintainer judgment. It also is not obsolete: current main still lacks the proposed hardening for npm_execpath in workspace .env, install env creation, and npm runner resolution.

Best possible solution:

Keep this PR open for maintainer/security review. The best path is to land it or an equivalent patch that blocks NPM_EXECPATH from workspace .env, strips all casing variants from the bundled runtime dependency install environment, makes runner selection ignore npm_execpath, and preserves the added regression tests, while deciding whether the PATH fallback concern belongs in this PR or a follow-up.

What I checked:

• Protected maintainer item: The provided GitHub context marks the PR author association as MEMBER and includes the protected maintainer label. The cleanup policy says to keep such items open even when the technical direction looks plausible.
• Current main still reads npm_execpath: resolveBundledRuntimeDepsNpmRunner still reads env.npm_execpath, validates only by isNpmCliPath, and puts that path first in the npm CLI candidate list. (src/plugins/bundled-runtime-deps.ts:1290, ba722fd1265a)
• Install env is not stripped on main: createBundledRuntimeDepsInstallEnv returns the npm install environment without deleting lowercase or uppercase npm_execpath; the install path then passes that env into runner resolution and spawn. (src/plugins/bundled-runtime-deps.ts:1266, ba722fd1265a)
• Workspace dotenv blocklist is missing the key: BLOCKED_WORKSPACE_DOTENV_KEYS does not include NPM_EXECPATH, while workspace dotenv loading applies this blocklist before copying values into process.env. (src/infra/dotenv.ts:13, ba722fd1265a)
• Existing tests reflect pre-fix behavior: Current tests still assert Windows npm_execpath can be used as the npm CLI path and do not include the PR's proposed uppercase/lowercase workspace dotenv blocking coverage. (src/plugins/bundled-runtime-deps.test.ts:107, ba722fd1265a)
• Security review scope: The provided PR file list is limited to the dotenv and bundled-runtime-deps source/tests, with no workflows, lockfiles, dependency sources, package metadata, or release scripts. The Aisle bot comment raises a related PATH fallback concern in the same runner, so this should stay open for maintainer security review rather than automated cleanup. (bb45373f19ba)

Remaining risk / open question:

• The PR is security-sensitive and has a bot-raised PATH fallback concern that maintainers should explicitly accept, defer, or fold into this patch.
• If this PR is closed without an equivalent replacement, current main still has the described npm_execpath trust path.

Codex review notes: model gpt-5.5, reasoning high; reviewed against ba722fd1265a.

Addressed in c8f2df4dcb7296396e40de07f047d9da92dc2720.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	PATH hijacking leading to RCE when executing Node-adjacent npm shim on POSIX

1. 🟠 PATH hijacking leading to RCE when executing Node-adjacent `npm` shim on POSIX

Property	Value
Severity	High
CWE	CWE-426
Location	src/plugins/bundled-runtime-deps.ts:1291-1303

Description

resolveBundledRuntimeDepsNpmRunner falls back on POSIX to executing a Node-adjacent npm file directly (e.g. /opt/node/bin/npm). Many npm binaries in Node distributions are shim scripts with a shebang like #!/usr/bin/env node.

Because /usr/bin/env node resolves node via PATH, and the returned runner does not set/override PATH (nor does it force the known-safe process.execPath), an attacker who can influence the environment PATH (e.g., CI/workspace env, parent process env) can cause the shim to execute an attacker-controlled node binary.

Impact:

• The process spawns npm to install deps; hijacking node gives arbitrary code execution with the privileges of the running process.

Vulnerable code:

const npmBinCandidates = [
  pathImpl.resolve(nodeDir, "npm"),
  pathImpl.resolve(nodeDir, "../bin/npm"),
];
...
return {
  command: npmBinPath,
  args: params.npmArgs,
};

Recommendation

Avoid executing the npm shim directly on POSIX, or ensure it cannot resolve node from an attacker-controlled PATH.

Safer options:

1. Prefer running the npm CLI via the known Node executable (like the Windows branch already does):

​// Find npm-cli.js and always run it via execPath
return {
  command: execPath,
  args: [npmCliPath, ...params.npmArgs],
};

2. If you must execute the npm shim, prefix PATH with nodeDir (or set it to a minimal safe PATH) so /usr/bin/env node resolves to the expected Node:

const env = params.env ?? process.env;
const pathKey = Object.keys(env).find(k => k.toLowerCase() === "path") ?? "PATH";
const currentPath = env[pathKey];
return {
  command: npmBinPath,
  args: params.npmArgs,
  env: {
    ...env,
    [pathKey]: typeof currentPath === "string" && currentPath.length
      ? `${nodeDir}${path.delimiter}${currentPath}`
      : nodeDir,
  },
};

Also consider validating that npmBinPath is not a shim relying on /usr/bin/env, or directly invoking ${nodeDir}/node with the resolved npm-cli.js path.

---

Analyzed PR: #73262 at commit bb45373

_{Last updated on: 2026-04-28T10:53:37Z}

Addressed in c8f2df4dcb7296396e40de07f047d9da92dc2720.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by both MEMBER author association and the maintainer label, so it needs explicit maintainer judgment. It also is not obsolete: current main still lacks the proposed hardening for npm_execpath in workspace .env, install env creation, and npm runner resolution.

Best possible solution:

Keep this PR open for maintainer/security review. The best path is to land it or an equivalent patch that blocks NPM_EXECPATH from workspace .env, strips all casing variants from the bundled runtime dependency install environment, makes runner selection ignore npm_execpath, and preserves the added regression tests, while deciding whether the PATH fallback concern belongs in this PR or a follow-up.

What I checked:

• Protected maintainer item: The provided GitHub context marks the PR author association as MEMBER and includes the protected maintainer label. The cleanup policy says to keep such items open even when the technical direction looks plausible.
• Current main still reads npm_execpath: resolveBundledRuntimeDepsNpmRunner still reads env.npm_execpath, validates only by isNpmCliPath, and puts that path first in the npm CLI candidate list. (src/plugins/bundled-runtime-deps.ts:1290, ba722fd1265a)
• Install env is not stripped on main: createBundledRuntimeDepsInstallEnv returns the npm install environment without deleting lowercase or uppercase npm_execpath; the install path then passes that env into runner resolution and spawn. (src/plugins/bundled-runtime-deps.ts:1266, ba722fd1265a)
• Workspace dotenv blocklist is missing the key: BLOCKED_WORKSPACE_DOTENV_KEYS does not include NPM_EXECPATH, while workspace dotenv loading applies this blocklist before copying values into process.env. (src/infra/dotenv.ts:13, ba722fd1265a)
• Existing tests reflect pre-fix behavior: Current tests still assert Windows npm_execpath can be used as the npm CLI path and do not include the PR's proposed uppercase/lowercase workspace dotenv blocking coverage. (src/plugins/bundled-runtime-deps.test.ts:107, ba722fd1265a)
• Security review scope: The provided PR file list is limited to the dotenv and bundled-runtime-deps source/tests, with no workflows, lockfiles, dependency sources, package metadata, or release scripts. The Aisle bot comment raises a related PATH fallback concern in the same runner, so this should stay open for maintainer security review rather than automated cleanup. (bb45373f19ba)

Remaining risk / open question:

• The PR is security-sensitive and has a bot-raised PATH fallback concern that maintainers should explicitly accept, defer, or fold into this patch.
• If this PR is closed without an equivalent replacement, current main still has the described npm_execpath trust path.

Codex review notes: model gpt-5.5, reasoning high; reviewed against ba722fd1265a.

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.
_{Aisle supplements but does not replace security review.}

Analyzed PR: #73262 at commit 8e331b2

_{Last updated on: 2026-04-28T12:53:27Z}