express
express copied to clipboard
expressjs
Improve pipelines
Main Changes
This change include the pinning for the GItHub Actions dependencies (ce959ac) and the permissions definition for the pipeline (ea604fb)
Impact in the OSSF Scorecard
Context
Changes related
- OSSF Scorecard Documentation | Tokens permissions
- OSSF Scorecard Documentation | Pinned dependencies
Team discussion related
- Ref: https://github.com/expressjs/security-wg/issues/2
- Report: https://kooltheba.github.io/openssf-scorecard-api-visualizer/#/projects/github.com/expressjs/express/commit/2a00da2067b7017f769c9100205a2a5f267a884b
Changelog
- ce959ac chore: pin dependencies in the pipeline by @UlisesGascon
- ea604fb chore: specify permissions in the pipeline by @UlisesGascon
What is the npmCommand warnings referring to in that report output?
I think the issue is with npm install --save-dev ${{ matrix.npm-i }}. I can ask the Scorecard Team for support on this, because I can see that the dependencies in the matrix.npm-i are pinned.
Take in account that until we don't merge https://github.com/expressjs/express/pull/5431 we are depending on the OSSF Scorecard CRON job to update the results. So the last scanned commit was 0e3ab6ec215fc297473323fb1e8d0df03033e774, here is the visual report
Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness? I know it doesn't normally (which is soooooo annoying and broken) but I thought there was a setting to disallow majors, I am not sure that would work with commit hashes.
Not blocking at all, but the more we lean into these the more general maintenance we need to do. IMO a better way is to move this kind of stuff to a shared workflow in .github and keep all the packages in the orgs pointing to one individual source we need to update. Does that description make sense?
@wesleytodd
Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?
Dependabot should ignore deps pinned to commits. It just doesn't know what to do with them, so it will skip them.
So the problem then is we don't get any updates. Which is maybe fine, but still this is a bad DX imo.
Pinning these deps is part of the scorecard score and guidance recommendations, hence why they have been proposed this way.
Yeah I get that, doesnt help make it any less of a UX problem though right? We still need to update them on a regular basis.
@inigomarquinez I think that updating the dependencies might break the pipelines, I will have a look into it.
Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?
Yes, it should support this approach. The comment is just to make it more human readable. The reason why we need to use commit hashes and not the tag reference directly is because the tags are not immutable so we can end up running something different that what we expect.
More context about the attack vector
dependabot does update pinned dependencies. At least in all the projects where I use pinned dependencies in GitHub actions along with dependabot, it opens a new pull request to update them to the latest pinned dependency. It even updates the comment saying to which version it belongs to.
@wesleytodd
Do we know if the dependabot updates (assuming we decide to use that) for these versions will take into account semverness?
Dependabot should ignore deps pinned to commits. It just doesn't know what to do with them, so it will skip them.
Frankly seeing this same change in many repos is going to not be something I am interested in dealing with. I am border line on this, but I think I am leaning toward: This shouldn't land until there is a way to centrally manage this across all the repos with a single update.
Which I think means that we should refactor many of the CICD setups to use shared worfklows from the .github repo.