Skip to content

Conversation

@gcanlin
Copy link
Contributor

@gcanlin gcanlin commented Jun 9, 2025

Hi. I'd like to suggest some security enhancements. In this PR, I change the configurations of actions:

  1. set up minimal permissions of actions
  2. pinned the dependencies of actions
  3. add dependabot for actions.

Here are the two corresponding security risks for each of them:

  1. Permissions: Lack of explicit permission settings may allow a malicious PR to inject malicious code through write permissions in actions. Adding read-only permissions at the top level will help you avoid forgetting permission configurations when adding other jobs next time.
  2. Pinned dependencies: There have been a related vulnerability recently. It indicates that if the version of a dependent action is pinned to a malicious commit, it may lead to the execution of malicious scripts, potentially exposing secrets such as the GITHUB_TOKEN. Dependency bots can help you avoid manually handling the hash values of actions and promptly notify you of upstream bug fixes.

If you’re interested, I’d be happy to discuss these security risks with you.

@gcanlin
Copy link
Contributor Author

gcanlin commented Jun 9, 2025

@ljharb Please take a look for this PR.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dependabot is one of the worst updating tools; if anything I'd do renovate.

Separately, pinning actions imo is a harmful idea, especially since there can be precisely zero collateral damage from all of these workflows being compromised - which means there's precisely zero benefit from pinning them, and multiple downsides.

I'm happy to take the explicit permission changes, though.

@ljharb ljharb marked this pull request as draft June 9, 2025 03:50
@gcanlin
Copy link
Contributor Author

gcanlin commented Jun 9, 2025

Thanks for reply!
As for pinning, I used to think it was purely a burden—until the vulnerability in tj-actions/changed-files:issue/2464 (CVE-2025-30066) was disclosed. The changed-files tag had been pointed to a malicious commit, which led to token leaks in hundreds of downstream repositories. That incident made me genuinely concerned about this issue. So regarding this issue, I still hope you can reconsider.

For the sake of a clean commit log, I’ll open a new PR that focuses solely on the permissions changes later if you'd prefer to unpin the dependencies.


steps:
- uses: actions/checkout@v2
- uses: actions/checkout@v2.7.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this as well, there’s no benefit from pinning to a minor version (ever)


steps:
- uses: ljharb/require-allow-edits@main
- uses: ljharb/require-allow-edits@13f90bc8cc5de000f2b44a0e2c3a11d108e8cd9f # main
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

@ljharb
Copy link
Collaborator

ljharb commented Jun 9, 2025

That vulnerability only matters when there’s a privileged token present - and most CI jobs don’t have any, including these.

@ljharb ljharb changed the title [actions] restrict permissions and pin actions with dependabot config [actions] restrict permissions Jun 10, 2025
@ljharb ljharb marked this pull request as ready for review June 10, 2025 01:26
@gcanlin
Copy link
Contributor Author

gcanlin commented Jun 10, 2025

OK. Thanks for your patience. I have reset the redundancy log and committed force only including restricting permissions.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.16%. Comparing base (77bde06) to head (371a097).
Report is 3 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (77bde06) and HEAD (371a097). Click for more details.

HEAD has 12 uploads less than BASE
Flag BASE (77bde06) HEAD (371a097)
15 3
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #479       +/-   ##
===========================================
- Coverage   85.84%   30.16%   -55.68%     
===========================================
  Files           2        2               
  Lines        2360     2360               
  Branches      614      614               
===========================================
- Hits         2026      712     -1314     
- Misses        334     1648     +1314     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gcanlin
Copy link
Contributor Author

gcanlin commented Jun 12, 2025

I accidentally pushed the test commits for Renovate to the upstream repository. Sorry about that — I'll figure out a way to remove them later.

@gcanlin
Copy link
Contributor Author

gcanlin commented Jun 16, 2025

@ljharb Hi! The CI workflow has completed successfully. Could you please merge this branch? Thanks!

@ljharb ljharb merged commit 0265dc4 into paulmillr:master Oct 9, 2025
383 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants