Skip to content

Partially Revert "fix(action): pin nested action references to full c… - #990

Open
wu21-web wants to merge 3 commits into
alibaba:mainfrom
wu21-web:pins
Open

Partially Revert "fix(action): pin nested action references to full c…#990
wu21-web wants to merge 3 commits into
alibaba:mainfrom
wu21-web:pins

Conversation

@wu21-web

@wu21-web wu21-web commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

…ommit SHAs (#836)"

Description

This partially reverts commit 7e52a4f.

Why: Maintainers can simply enable "Require actions to be pinned to a full-length commit SHA" under Actions/General in the repo settings.
This is more secure because Github Runners reject to start those jobs when SHAs are not pinned. While a script check fails the job after ci.yml has already done checks (verify license headers)

A manually written verify-actions-pins.sh might cause problems, and it is not as neat as a single switch.

What's not reverted:

  1. Existing pinned actions SHAs in action.yml
  2. The README in examples/github_action
Screenshot 2026-08-18 at 4 33 58 PM

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have signed the CLA

Related Issues

None

@wu21-web
wu21-web marked this pull request as ready for review August 18, 2026 07:40
@github-actions

github-actions Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment thread .github/workflows/ci.yml
Comment on lines 40 to 41
- name: Verify sources contain no unapproved non-English text
run: go run scripts/verify-english-only.go

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

security · medium
Removal of action pin verification reduces CI security posture.

The verify-action-pins.sh script was an automated guardrail that ensured GitHub Actions are pinned to full commit SHAs, protecting against tag-hijacking attacks. Removing this step means:

  1. If any third-party actions are added in the future, there will be no automated check to enforce SHA pinning.
  2. First-party actions (actions/*) like actions/checkout@v7 and actions/setup-go@v7 currently used are acceptable with tag-based pinning per best practices, but this removal eliminates the enforcement mechanism for the entire repository.

Consider either:

  • Keeping this verification step to maintain the security guardrail.
  • Replacing it with an alternative tool (e.g., Dependabot, StepSecurity, or a GitHub Action like step-security/action-pinned-versions).
  • At minimum, documenting why this check is being removed and what compensating controls exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removal is the purpose of the PR. Action SHAs are not pinned now. After all actions have been pinned, maintainers toggle the switch.

@lizhengfeng101

Copy link
Copy Markdown
Collaborator

Thanks for digging into this — agree the shell script is clunky, and leaning on a platform switch instead of hand-rolled regex is the right instinct. A few things I'd want sorted before this goes in though.

The setting doesn't look like it's actually enforcing. Every workflow in this repo still uses floating tags (actions/checkout@v7, softprops/action-gh-release@v3, actions/cache@v6, codeql-action@v4, …) and all 13 checks on this PR passed. If runners were rejecting unpinned jobs, release.yml and codeql.yml should be dead in the water. So either the toggle isn't on, or it doesn't cover these cases — either way, merging this leaves zero enforcement. Could you pin the workflows first and show one deliberately-unpinned ref getting blocked?

The switch protects this repo; the script protected consumers. That was the whole point of #816. The nested uses: in action.yml resolve on the consumer's runner under their org policy — our setting has no reach there. If their policy is transitive, one stray floating tag in action.yml breaks every consumer's run and we'd never see it. If it isn't, the "outer SHA freezes everything" promise quietly breaks. These two mechanisms don't overlap; they're complementary.

Dangling doc reference. examples/github_actions/README.md:41 says pins are "enforced by scripts/verify-action-pins.sh in CI". That line needs updating in this PR — it's the only remaining reference to the file.

We also lose the # vX.Y.Z convention. The GitHub setting only checks the SHA shape, not the trailing comment — and that comment is what README:41 tells people lets Dependabot/Renovate track versions. All five uses: in action.yml follow it today; nothing will keep that true.

Smaller stuff: a repo setting is invisible in the tree and can be flipped off silently, so worth noting as a required invariant in AGENTS.md if we go this route. And github_workflows.md:5 in our own rule docs tells users to pin to full SHAs, so dropping our own check reads a bit odd.

If the goal is mainly "stop maintaining fragile bash," swapping the script for zizmor / ratchet / actionlint gets you that without giving up the action.yml guarantee — and it composes fine with the repo setting.

@wu21-web

wu21-web commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

@lizhengfeng101 Thank you for reviewing this. Here are answers to your questions:

  1. This pull request itself does not have action pins, nor is the repo's "enforce SHA pinning for actions" on (I guess). So CI runs wont be blocked. I originally thought this is a job for another author @Fanzzzd (PR fix(ci): pin workflow action references to full commit SHAs #856 ), which is the follow-up of this pull request (he will pin the SHAs for me). But since you mentioned that task to me, I with co-author with him and get the job done.
  2. I assume consumers will probably use that switch too. I think I will update the documentation for this mainly. But if you oppose, react with 😕 and I will get it done in no-time.
  3. Will fix shorty.
  4. Good point, but the comment shouldn't be enforcing. Comments are meant to be optional.

@wu21-web

Copy link
Copy Markdown
Contributor Author

P.S I will update the docs accordingly.

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.

2 participants