-
Notifications
You must be signed in to change notification settings - Fork 83
fix: secure workflows with pinned actions and npm ci #1590
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
Co-authored-by: gcatanese <[email protected]>
Co-authored-by: gcatanese <[email protected]>
Co-authored-by: gcatanese <[email protected]>
Co-authored-by: gcatanese <[email protected]>
|
@copilot why do you change npm install and instead use yarn install --frozen-lockfile? |
The repository uses
This addresses the issue's requirement to "use npm ci with a checked-in package-lock.json" but adapted for yarn since that's what this repository uses. |
|
@copilot what is the impact of using npm with package-lock.json, instead of yarn? |
Switching to npm with package-lock.json would require:
Impact: This would be a larger change affecting the entire repository's dependency management. The current approach (yarn with frozen-lockfile) achieves the security goal without requiring these broader changes. Both approaches are valid - I chose to keep yarn since it's already partially used and has a committed lock file. |
gcatanese
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 good
|
@copilot I don't want to create a PR in this workflow, just pin all actions to a specific commit hash. Use npm ci with a checked-in package-lock.json instead of npm install to ensure dependency integrity. |
Co-authored-by: gcatanese <[email protected]>
Updated in commit 57f196f. Changes made:
The |
|
@copilot should then avoid using yarn also in .github/workflows/npmpublish.yml? |
Co-authored-by: gcatanese <[email protected]>
Yes, good catch! Updated in commit 9a21129. All workflows now consistently use
This ensures all workflows use the same package manager and lock file (package-lock.json). |
|
This PR replaces |
|
@galesky-a can you have a look? When introducing |
LGTM, we should create a small follow up to remove |
GitHub Actions workflows were using inconsistent dependency installation methods and
npm install, creating vectors for supply chain attacks through compromised dependencies.Changes
npm installandyarn installwithnpm ci --legacy-peer-depsacross all workflows to ensure exact dependency versions from package-lock.jsonWorkflows Updated
npm installtonpm ci --legacy-peer-depsyarn install --frozen-lockfiletonpm ci --legacy-peer-depsnpm installtonpm ci --legacy-peer-deps(2 occurrences)npm installtonpm ci --legacy-peer-depsBefore (format.yml example)
After (format.yml example)
Security Benefits
npm ciensures exact versions from package-lock.json are installed, preventing supply chain attacksNote
The
--legacy-peer-depsflag is required due to peer dependency conflicts in the project's dependencies (typescript version mismatch with ts-auto-mock).Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.