ci: remove the !fix autofix workflow, enforce formatting in lint instead - #27
Merged
Conversation
…ranches Autofix Linting triggers on issue_comment, which runs in the base repository context with contents: write. It gated only on the comment body, so any GitHub user could start it, and it resolved the pull request head branch by name against this repository. On 2026-05-22 an outside account opened a fork pull request whose head branch was named main, commented !fix, and the job checked out this repository's main, ran forge fmt and pushed two commits to it (49e1a78, 4cbaa91). - gate on comment author_association so only OWNER, MEMBER or COLLABORATOR can trigger the job - refuse fork pull requests and refuse the default branch - write the branch through $GITHUB_OUTPUT. ::set-output still works on hosted runners, so this is hygiene rather than the bug - state the whole if condition as one expression. The old form mixed a bare operand with ${{ ... }}, which compiles to a format() call whose non-empty string result is always truthy - quote every expansion, pass event data through env, validate the branch name - drop checks: write, default the workflow to no permissions, and scope contents: write plus pull-requests: read to the job - upgrade and pin actions/checkout and foundry-rs/foundry-toolchain by SHA - use printf so .git-blame-ignore-revs entries get a real newline Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r comment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The autofix workflow was reachable by anyone who could comment. On 2026-05-22 an external user with author_association NONE commented "!fix" on a fork pull request whose head branch was named "main"; checkout defaults `repository` to the base repo, so `ref: main` resolved against shipyard and the bot committed and pushed 49e1a78 and 4cbaa91 to this repository's default branch. Hardening it means keeping an issue_comment trigger with contents: write that checks out a pull request's code. Deleting it removes that surface entirely and costs a convenience that "forge fmt" already provides locally. Formatting is still enforced, by the lint workflow, which now fails the build instead of pointing at the bot. That workflow also stops overriding the checkout ref with github.head_ref, which is the same bare-branch-name resolution that made the May incident possible, and now declares permissions: contents: read.
ryanio
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the
!fixautofix workflow and enforces formatting in CI instead.Why remove rather than harden
It fired. On 2026-05-22 an external user with
author_association: NONEcommented!fixon a fork pull request whose head branch was namedmain.actions/checkoutdefaultsrepositorytogithub.repository, soref: mainresolved against this repo rather than the fork, and the bot committed and pushed to the default branch:49e1a784cbaa91Both are on
main. Run26276212320, eventissue_comment, conclusion success. Not code execution, but an unauthenticated write to the default branch.Hardening keeps the shape that made it possible: an
issue_commenttrigger, which runs in the base repo context with the repo's secrets, holdingcontents: write, checking out a pull request's code. An earlier revision of this PR added an author-association gate and a fork refusal, and that would have stopped the May incident. Deleting the workflow removes the surface instead of narrowing it, and the convenience it bought is oneforge fmtaway locally.What replaces it
Nothing new.
lint.ymlalready ranforge fmt --check; it now fails the build and says to runforge fmt, rather than advertising a bot. Two fixes went in alongside:ref: ${{ github.head_ref }}. On apull_requestevent, checkout defaults to the merge commit, which is what CI should judge. Naminghead_refresolves a bare branch name against this repository, which is the same resolution that made the May incident possible.permissions: contents: read, and pinnedactions/checkoutandfoundry-rs/foundry-toolchainby SHA.Verified, and one thing I did not
The two bot commits, the run id, and the event type are read from the API, not inferred.
lint.ymlparses as valid YAML and the run block is four lines.Not verified: whether any comment other than the two probes ever triggered the workflow. Run retention has expired for anything older, so how often it was legitimately used is unknown. If maintainers relied on it, the tradeoff here is worth saying out loud rather than assuming nobody will miss it.
Unrelated failure
Forge Testsfails on this branch and onmain.foundry.tomlremapsopenzeppelin-contracts/tolib/openzeppelin-contracts/contracts/whilelib/shipyard-coreimportsopenzeppelin-contracts/contracts/utils/..., producing a doubledcontracts/contracts/path. This PR changes only workflow YAML and cannot affect Solidity compilation. Being fixed separately.Still open for you
Whether to close the two probe PRs, #25 and #26. Untouched here.