fix: address open CodeQL findings#11609
Merged
Merged
Conversation
* Reject prototype-polluting keys (`__proto__`, `constructor`, `prototype`) before dynamic manifest property writes in `convertEnginesRuntimeToDependencies`, `updateProjectManifestObject`, and `removeDeps`. * Replace the super-linear-backtracking `semverRegex` in the npm-resolver 404 hint with an O(n) `stripTrailingSemverSuffix` scanner. * Swap the `\n+$` regex in `renderDependentsTree` for a constant-time trailing newline trim. * Properly escape `listenPath` and message values in the test IPC server's generated `node -e` scripts (no more `JSON.stringify(...).slice(1, -1)`). * Use `replaceAll` in the oidcProvenance test's URL-encoding stand-in.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds prototype-pollution guards to manifest writes, replaces a super-linear semver regex with an O(n) scanner for registry-404 hints, introduces on-disk TestIpcServer helper scripts with safe shell quoting, replaces regex newline trimming with a loop helper, and updates tests and changeset metadata. ChangesSecurity Fixes and Performance Optimizations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple open CodeQL security alerts across pnpm’s codebase, covering prototype-pollution guards, ReDoS-safe string handling, and safer construction of test IPC shell scripts.
Changes:
- Replaces potentially prototype-polluting dynamic dependency writes/deletes with guarded or literal-key updates in manifest-related code paths.
- Removes/avoids potentially super-linear regex patterns (notably semver detection in 404 hints and newline trimming in tree rendering).
- Refactors
TestIpcServerscript generation to avoid theJSON.stringify(...).slice(1, -1)pattern by introducing awrapNodeEvalhelper.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| resolving/npm-resolver/src/fetch.ts | Replaces semver regex matching with an O(n) suffix parser using semver.valid(). |
| releasing/commands/test/publish/oidcProvenance.test.ts | Uses replaceAll() for more complete URL encoding in tests. |
| pkg-manifest/utils/src/updateProjectManifestObject.ts | Skips prototype-polluting keys before manifest dependency writes. |
| pkg-manifest/utils/src/convertEnginesRuntimeToDependencies.ts | Uses literal keys in dependency assignment to avoid prototype-polluting assignment alerts. |
| installing/deps-installer/src/uninstall/removeDeps.ts | Filters prototype-polluting keys before dependency deletes and avoids repeated dynamic indexing. |
| deps/inspection/list/src/renderDependentsTree.ts | Replaces trailing-newline regex with a linear scan helper. |
| .changeset/codeql-security-fixes.md | Adds a changeset describing the CodeQL security fixes. |
| utils/test-ipc-server/src/TestIpcServer.ts | Refactors generated scripts and adds wrapNodeEval() to escape code passed to node -e. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* `removeDeps`: gate dynamic property reads/deletes behind `Object.hasOwn` so the saveType/dependency keys can never resolve to `Object.prototype` (closes CodeQL alert 158). * `TestIpcServer`: stop embedding the listen path and message in `node -e` source. Helper scripts are now written to a per-server temp dir at `listen()` time and invoked with the path/message passed via argv, so no dynamic data flows through shell-quoted JavaScript source. A regex-anchored `quoteShellArg` rejects anything outside a known-safe character set, removing the cross-platform escaping risk Copilot called out (closes CodeQL alert 157).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__utils__/test-ipc-server/src/TestIpcServer.ts (1)
81-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up the helper temp dir on failure paths too.
helperDiris only removed on the happy-path dispose. If helper-file creation,server.listen(), orserver.close()fails, these temp directories are left behind under the system temp dir.♻️ Suggested cleanup pattern
public static async listen (handle?: string): Promise<TestIpcServer> { const listenPath = computeHandlePath(handle) const server = net.createServer() const helperDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'pnpm-test-ipc-')) const stdinHelperPath = path.join(helperDir, 'stdin.cjs') const lineHelperPath = path.join(helperDir, 'line.cjs') - await Promise.all([ - fs.promises.writeFile(stdinHelperPath, STDIN_HELPER_SOURCE), - fs.promises.writeFile(lineHelperPath, LINE_HELPER_SOURCE), - ]) - const testIpcServer = new TestIpcServer(server, listenPath, { - dir: helperDir, - stdinHelperPath, - lineHelperPath, - }) - - return new Promise((resolve, reject) => { - server.once('error', reject) - server.listen(listenPath, () => { - resolve(testIpcServer) - }) - }) + try { + await Promise.all([ + fs.promises.writeFile(stdinHelperPath, STDIN_HELPER_SOURCE), + fs.promises.writeFile(lineHelperPath, LINE_HELPER_SOURCE), + ]) + const testIpcServer = new TestIpcServer(server, listenPath, { + dir: helperDir, + stdinHelperPath, + lineHelperPath, + }) + await new Promise<void>((resolve, reject) => { + server.once('error', reject) + server.listen(listenPath, resolve) + }) + return testIpcServer + } catch (err) { + server.close() + await fs.promises.rm(helperDir, { recursive: true, force: true }) + throw err + } } ... public [Symbol.asyncDispose] = async (): Promise<void> => { const close = promisify(this.server.close).bind(this.server) - await close() - await fs.promises.rm(this.helperDir, { recursive: true, force: true }) + try { + await close() + } finally { + await fs.promises.rm(this.helperDir, { recursive: true, force: true }) + } }Also applies to: 143-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__utils__/test-ipc-server/src/TestIpcServer.ts` around lines 81 - 100, The temp helper directory (helperDir) and its files (stdinHelperPath, lineHelperPath) must be removed on all failure paths: wrap the async setup (mkdtemp + writeFile + new TestIpcServer + server.listen) so that any thrown error or the server.once('error') rejection triggers cleanup of helperDir (use fs.promises.rm or equivalent with recursive/force), and also ensure the same cleanup runs if server.close()/dispose in TestIpcServer fails (add a finally or error handler in the dispose flow). Update the Promise returned around server.listen and any code paths near TestIpcServer construction and the dispose logic referenced around the later block (lines ~143-146) to call the same removal routine for helperDir on rejection/failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__utils__/test-ipc-server/src/TestIpcServer.ts`:
- Around line 131-140: The current allowlist regex used by quoteShellArg is too
restrictive and also unsafe for POSIX double-quoted strings (it accepts a
trailing backslash and rejects valid tokens like `@scope/pkg` or JSON), so update
TestIpcServer to either document the restriction or make quoting robust: replace
the current quoteShellArg implementation with a safe encoder used by
sendLineScript and generateSendStdinScript (e.g., base64-encode the payload and
decode in the helper, or implement proper POSIX-safe escaping that
disallows/escapes a trailing backslash and supports characters such as @, :, and
braces), and ensure tests/helpers (lineHelperPath/stdinHelperPath) decode/handle
the chosen encoding; reference quoteShellArg, sendLineScript,
generateSendStdinScript and TestIpcServer when applying the change.
---
Outside diff comments:
In `@__utils__/test-ipc-server/src/TestIpcServer.ts`:
- Around line 81-100: The temp helper directory (helperDir) and its files
(stdinHelperPath, lineHelperPath) must be removed on all failure paths: wrap the
async setup (mkdtemp + writeFile + new TestIpcServer + server.listen) so that
any thrown error or the server.once('error') rejection triggers cleanup of
helperDir (use fs.promises.rm or equivalent with recursive/force), and also
ensure the same cleanup runs if server.close()/dispose in TestIpcServer fails
(add a finally or error handler in the dispose flow). Update the Promise
returned around server.listen and any code paths near TestIpcServer construction
and the dispose logic referenced around the later block (lines ~143-146) to call
the same removal routine for helperDir on rejection/failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c40b51ca-6038-4309-aa5a-7c7a21be52fd
📒 Files selected for processing (2)
__utils__/test-ipc-server/src/TestIpcServer.tsinstalling/deps-installer/src/uninstall/removeDeps.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- installing/deps-installer/src/uninstall/removeDeps.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx}: Use trailing commas in code following Standard Style with modifications
Prefer functions over classes
Declare functions after they are used, relying on hoisting
Functions should have no more than two or three arguments; use a single options object for functions needing more parameters
Organize imports in order: standard libraries, external dependencies (sorted alphabetically), then relative imports
Files:
__utils__/test-ipc-server/src/TestIpcServer.ts
…backslash Addresses CodeRabbit feedback: * Add `@` so scoped names like `@scope/pkg` aren't rejected. * Reject a trailing backslash explicitly — under both POSIX sh and the Windows command-line parser the trailing `\\"` consumes the closing quote and breaks the command line.
Addresses Copilot PR review: * Export `isProtoPollutionKey` from `@pnpm/types` and use it from both `updateProjectManifestObject` and `removeDeps`, so the allowlist of blocked keys lives in one place. * Add a `RegistryResponseError` hint test covering `foo@1.0.0`, `foo1.0.0`, scoped names, and an adversarial all-digits input that would have been slow under the old regex. * Add a test for `updateProjectManifestObject` that verifies the prototype-pollution guard skips `__proto__`/`constructor`/`prototype` aliases without mutating `Object.prototype`. * Document the `quoteShellArg` allowlist constraint and the resulting throw on `sendLineScript` / `generateSendStdinScript` in the methods' JSDoc.
Contributor
Benchmark Results
Run 25763601341 · 10 runs per scenario · triggered by @zkochan |
…endencies The literal-key switch worked but forced a parallel update every time a new runtime was added to RUNTIME_NAMES. Use the shared `isProtoPollutionKey` barrier instead — it's unreachable for the current set but keeps the dynamic assignment safe (and CodeQL quiet) without needing parallel updates.
…ncies CodeQL didn't recognise the shared `isProtoPollutionKey` helper as a barrier on this code path (alert 159 re-opened). Inline the literal equality checks against `__proto__`/`constructor`/`prototype` so CodeQL accepts them. A widening `const key: string = runtimeName` is needed so TypeScript allows the comparison — the loop variable is the narrow `'node' | 'deno' | 'bun'` literal type. The barrier is still extensible: adding a new runtime to `RUNTIME_NAMES` requires no change here unless someone tries to add one of the polluting keys, in which case the guard rejects it.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
utils/test-ipc-server/src/TestIpcServer.ts:100
TestIpcServer.listen()creates a temphelperDirand writes helper scripts before callingserver.listen(), but ifwriteFile()orserver.listen()fails (e.g. EADDRINUSE/permission errors), the Promise rejects and the temp directory is never cleaned up. Consider wrapping the setup + listen flow in a try/catch (and also cleaning up on theerrorpath) sohelperDiris removed whenlisten()fails.
public static async listen (handle?: string): Promise<TestIpcServer> {
const listenPath = computeHandlePath(handle)
const server = net.createServer()
const helperDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'pnpm-test-ipc-'))
const stdinHelperPath = path.join(helperDir, 'stdin.cjs')
const lineHelperPath = path.join(helperDir, 'line.cjs')
await Promise.all([
fs.promises.writeFile(stdinHelperPath, STDIN_HELPER_SOURCE),
fs.promises.writeFile(lineHelperPath, LINE_HELPER_SOURCE),
])
const testIpcServer = new TestIpcServer(server, listenPath, {
dir: helperDir,
stdinHelperPath,
lineHelperPath,
})
return new Promise((resolve, reject) => {
server.once('error', reject)
server.listen(listenPath, () => {
resolve(testIpcServer)
})
})
…oveDeps Addresses Copilot review feedback: silently skipping aliases like \`constructor\` and \`prototype\` would have broken \`pnpm add constructor\` and \`pnpm remove prototype\`, even though those are legitimate npm package names. * \`updateProjectManifestObject\`: instead of skipping, use a small \`defineDepEntry\` helper that goes through \`Object.defineProperty\` so pollution-key aliases become regular own data properties (and CodeQL recognises defineProperty as a sanitiser). \`findSpec\`/\`guessDependencyType\` now check ownership with \`Object.hasOwn\` so a missing pollution-key alias no longer reads through to the prototype chain. * \`removeDeps\`: drop the up-front filter. Every dynamic delete is already guarded by \`Object.hasOwn\`, so pollution-key names are safe to pass through and are removed only if they're real own properties. * Drop the now-unused \`isProtoPollutionKey\` export from \`@pnpm/types\` and its changeset entry — no published API churn.
zkochan
added a commit
that referenced
this pull request
May 12, 2026
CodeQL alert 159 (js/prototype-polluting-assignment) stayed open after #11609 was merged: the barrier I added in convertEnginesRuntimeToDependencies checked a re-aliased `key` variable, but the actual write went through the original loop variable `runtimeName`. CodeQL's local data-flow analysis followed `runtimeName` and didn't see the barrier. Replace the inline barrier + dynamic assignment with `Object.defineProperty`, which is the CodeQL-blessed sanitiser for this pattern. The dependency-map write now produces a regular own data property even if a future entry in `RUNTIME_NAMES` happens to match an inherited name like `__proto__`, so the analyser is happy without depending on any pre-write check.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves the 15 open alerts on https://github.com/pnpm/pnpm/security/code-scanning by addressing all four categories that CodeQL flagged.
Prototype-polluting assignment (3 alerts, product code)
pkg-manifest/utils/src/convertEnginesRuntimeToDependencies.ts: the inner write now dispatches over a literalswitchonruntimeName, so the assignment is always keyed by'node' | 'deno' | 'bun'.pkg-manifest/utils/src/updateProjectManifestObject.ts: added anisProtoPollutionKeybarrier at the top of the loop sopackageSpec.aliascan never reach the dynamic property write with__proto__/constructor/prototype.installing/deps-installer/src/uninstall/removeDeps.ts: the package list is filtered throughisProtoPollutionKeyonce up front, and the dependency record is captured into a local before the loop.Polynomial ReDoS (2 alerts)
deps/inspection/list/src/renderDependentsTree.ts:replace(/\n+$/, '')swapped for a constant-timecharCodeAttrim.resolving/npm-resolver/src/fetch.ts: removed the super-linear-backtrackingsemverRegexand replaced it with an O(n)stripTrailingSemverSuffixthat splits on the rightmost@andsemver.valids, with a digit-block fallback sofoo1.0.0-style names still produce the existing "Did you mean foo?" hint.Bad code sanitization (8 alerts, test infrastructure)
__utils__/test-ipc-server/src/TestIpcServer.ts: theJSON.stringify(...).slice(1, -1)smell at the source of all 8 test-file alerts is gone. BothsendLineScriptandgenerateSendStdinScriptnow build the JS source with plainJSON.stringifyand delegate shell wrapping to a newwrapNodeEvalhelper that escapes\\and"for the outer double-quoted shell argument.Incomplete sanitization (2 alerts, test file)
releasing/commands/test/publish/oidcProvenance.test.ts:.replace('/', '%2f')→.replaceAll(...)on both flagged lines.Test plan
compileclean across the five touched packageslintclean (only pre-existingjest/no-disabled-testswarnings)pkg-manifest/utilsunit tests: 14/14 passdeps/inspection/listunit tests: 21/21 that run pass (two suites fail with a pre-existingmodule is already linkedESM/Jest issue that reproduces onmain)TestIpcServer.generateSendStdinScript()/sendLineScript()output – escaping is correct on macOSWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests
Chores