-
-
Notifications
You must be signed in to change notification settings - Fork 10
Use ignore-scripts to guard against supply-chain attacks #4045
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.
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4045 +/- ##
=======================================
Coverage 74.55% 74.55%
=======================================
Files 294 294
Lines 10897 10897
Branches 1366 1366
=======================================
Hits 8124 8124
Misses 2378 2378
Partials 395 395
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements supply-chain attack mitigation by preventing npm packages from automatically executing install/postinstall scripts during installation. The changes add multiple layers of protection through command-line flags, configuration files, and environment variables.
Key Changes:
- Added
--ignore-scriptsflag to allnpm ci,npm i, andnpxcommands - Created
.npmrcconfiguration file withignore-scripts=true - Replaced
license-checker-rseidelsohndevDependency withnpxusage to reduce attack surface
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added --ignore-scripts flags to npm/npx commands; switched license generation to npx; removed license-checker-rseidelsohn dependency |
| package-lock.json | Removed license-checker-rseidelsohn and its transitive dependencies |
| Dockerfile | Added .npmrc copy and --ignore-scripts flag to npm ci command |
| .npmrc | Created new configuration file with ignore-scripts setting |
| .github/workflows/frontend.yml | Added --ignore-scripts flags and npm_config_ignore_scripts environment variable to all npm operations |
| .dockerignore | Allowed .npmrc file to be included in Docker builds |
Comments suppressed due to low confidence (1)
Dockerfile:37
- For defense-in-depth consistency with the PR's goal, the
npm run buildcommand should explicitly setnpm_config_ignore_scripts=truein the environment. While the.npmrcfile is copied and should prevent script execution, adding the environment variable makes the intent explicit and provides an additional layer of protection. Consider:RUN npm_config_ignore_scripts=true npm run build
RUN npm run build
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
myieye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you got everything.
I've never really thought this stuff through.
pnpm has an onlyBuiltDependencies field that restricts which packages can run scripts (the name feels unintuitive to me). Someone (not me) set that up for Lexbox, but I should definitely add it to FieldWorks Lite.
Looks like the npm version of that is a separate package: @lavamoat/allow-scripts
jasonleenaylor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonleenaylor reviewed 5 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @myieye)
imnasnainaec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imnasnainaec dismissed @myieye from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @myieye)
This is to reduce our vulnerability to supply-chain attacks.
--ignore-scriptsto every execution ofnpm ci,npm iandnpx..npmrcwithignore-scripts=true.npm_config_ignore_scripts: trueinenv:to everynpmworkflow call.The redundancies would make it more obvious if a bad pr tries to bypass this.
Also switch our frontend license generator to
npx, since we don't otherwise need that package and its dependencies.This change is