fix(plugin): pass --ignore-scripts to npm install for cloned plugin repos#1754
Open
Benjamin-eecs wants to merge 1 commit into
Open
fix(plugin): pass --ignore-scripts to npm install for cloned plugin repos#1754Benjamin-eecs wants to merge 1 commit into
Benjamin-eecs wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens plugin dependency installation by preventing npm lifecycle scripts from running when installing dependencies in a freshly cloned third-party plugin directory.
Changes:
- Add
--ignore-scriptstonpm installduring plugin dependency installation. - Add a unit test verifying the
--ignore-scriptsflag is passed toexecFileSync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/plugin.ts | Adds --ignore-scripts to the npm install invocation and documents the security rationale. |
| src/plugin.test.ts | Adds an assertion that installDependencies passes --ignore-scripts to npm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7288f09 to
762846c
Compare
…epos `installDependencies` runs `npm install` against a directory just cloned from a third-party Git URL via `opencli adapter install`. Without `--ignore-scripts`, npm executes preinstall / install / postinstall hooks declared in the plugin and every transitive dep, so a malicious plugin can run arbitrary code with the user's privileges at install time. Add `--ignore-scripts` so plugin code only executes via the expected discovery path. Fixes jackwener#1753
762846c to
2b4bf17
Compare
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.
Description
installDependenciesinsrc/plugin.tsrunsexecFileSync('npm', ['install', '--omit=dev'], ...)against a directory just cloned from a third-party Git URL viaopencli adapter install. Without--ignore-scripts, npm executes preinstall / install / postinstall hooks declared in the plugin'spackage.json(and in every transitive dep), so a malicious plugin or a compromised dep can run arbitrary code with the user's privileges at install time. Adapter code is loaded later by the plugin discovery path; the install-time script execution is an extra vector that adapter plugins do not need.This is Finding 2 in the #847 security audit. @Astro-Han ranked it the top-priority unfixed item in his triage: "
plugin installis a real issue, and one we should treat seriously … Using--ignore-scriptsplus a clear warning would be a good first step." This PR is the--ignore-scriptshalf; a follow-up could add a one-time advisory if useful.Plugins that depend on install scripts to compile native modules will silently skip those hooks. If a real case appears, an opt-in such as a manifest field or env var can be added in a follow-up; opencli adapter plugins observed in the wild depend on small pure-JS packages and do not exercise lifecycle scripts.
Related issue: fixes #1753.
Type of Change
Checklist
Screenshots / Output
Existing test
throws when npm install failsstill passes (mock-based regression check at the install error path). New testpasses --ignore-scripts to npm installasserts the flag reaches theexecFileSynccall via the existingmockExecFileSyncinfrastructure. Fullsrc/plugin*.test.tssuite: 105/105 pass on this branch.