chore: hardening the GA workflows#2092
Conversation
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request pins GitHub Actions to commit SHAs, scopes workflow permissions, adjusts checkout credentials and push commands, updates testing and release workflows, and adds Netlify docs-preview build and deployment workflows. ChangesGitHub Actions workflow updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@derberg @jonaslagoni @magicmatatjahu Please take a look at this PR. Thanks! 👋 |
…cope the write permissions and prevent pwn requests when using pull_request_target
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.github/workflows/update-docs-in-website.yml (1)
38-40: 💤 Low valuegit config still uses
--globaldespite PR description.The PR description states "Scope git config to the repository instead of --global", but lines 39-40 still use
--global. While this isn't harmful in an ephemeral CI environment, consider using local config for consistency with the stated hardening objective:Suggested change
run: | - git config --global user.name asyncapi-bot - git config --global user.email [email protected] + git config user.name asyncapi-bot + git config user.email [email protected]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/update-docs-in-website.yml around lines 38 - 40, The git config calls currently use --global; change them to set repository-local config instead by removing --global (or using --local) for the two commands that set user.name and user.email so that git config user.name asyncapi-bot and git config user.email [email protected] are applied only to the repository in the update-docs-in-website.yml workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/manual_netlify_preview.yml:
- Around line 67-68: The deployment directory precedence is wrong: change the
logic that sets DEPLOY_DIR so a static build (site/out) is preferred over
site/.next; instead of defaulting DEPLOY_DIR to site/.next when that directory
exists, first check for site/out and set DEPLOY_DIR="site/out" if present,
otherwise fall back to site/.next. Update the lines that currently set
DEPLOY_DIR and the conditional [ -d "site/.next" ] check so they reference the
functions/variables DEPLOY_DIR and the directories "site/out" and "site/.next"
in that order.
In @.github/workflows/manual-netlify-build.yml:
- Around line 33-34: Replace the fragile rm commands in the workflow that remove
README.md and the jsdoc2md-handlebars directory with idempotent variants: use rm
-f for the file removal and rm -rf (or rm -rf for the directory) so the steps
succeed even if the targets are missing; update the two commands shown ("rm
website/markdown/docs/tools/generator/README.md" and "rm -r
website/markdown/docs/tools/generator/jsdoc2md-handlebars") to their
force/recursive-force equivalents to make the cleanup resilient.
In @.github/workflows/pr-review-checklist.yml:
- Line 4: The workflow currently triggers on pull_request and uses
secrets.GH_TOKEN to post the checklist, which fails for forked PRs; change the
trigger to pull_request_target or split the posting into a separate job that
runs only for trusted contexts, and ensure any steps that run untrusted PR code
(e.g., actions/checkout of head ref or test/build steps) are not executed under
pull_request_target; specifically, replace the trigger 'pull_request' with
'pull_request_target' or add logic to run the checklist-posting step (the step
that uses secrets.GH_TOKEN) only when the event originates from the same repo or
use a dedicated job that posts comments while avoiding checking out PR code with
elevated secrets.
In @.github/workflows/pr-testing-with-test-project.yml:
- Around line 47-49: The condition currently compares github.actor to a numeric
ID which will never match; update the conditional expression in the workflow to
either compare the login string (github.actor == 'allcontributors[bot]') or use
the numeric id field (github.actor_id == 46447321) so the clause with
startsWith(github.event.pull_request.title, 'docs: add') can evaluate correctly;
modify the expression that references github.actor to use github.actor_id or
change the literal to 'allcontributors[bot]'.
In @.github/workflows/welcome-first-time-contrib.yml:
- Around line 19-21: The workflow permissions use an invalid scope name
'pull_requests'; update the permissions mapping to replace the 'pull_requests'
key with the correct GitHub Actions scope 'pull-requests' so the permissions
block (the permissions: mapping containing 'issues: write' and the erroneous
'pull_requests: write') uses 'pull-requests: write' to allow PR review actions.
---
Nitpick comments:
In @.github/workflows/update-docs-in-website.yml:
- Around line 38-40: The git config calls currently use --global; change them to
set repository-local config instead by removing --global (or using --local) for
the two commands that set user.name and user.email so that git config user.name
asyncapi-bot and git config user.email [email protected] are applied only to the
repository in the update-docs-in-website.yml workflow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c5de0b44-964a-4fec-9017-634835142729
📒 Files selected for processing (27)
.github/workflows/add-good-first-issue-labels.yml.github/workflows/automerge-for-humans-add-ready-to-merge-or-do-not-merge-label.yml.github/workflows/automerge-for-humans-merging.yml.github/workflows/automerge-for-humans-remove-ready-to-merge-label-on-edit.yml.github/workflows/automerge-orphans.yml.github/workflows/automerge.yml.github/workflows/bounty-program-commands.yml.github/workflows/bump.yml.github/workflows/help-command.yml.github/workflows/if-nodejs-pr-testing.yml.github/workflows/local-generate-files.yml.github/workflows/manual-netlify-build.yml.github/workflows/manual-netlify-preview.yml.github/workflows/manual_netlify_preview.yml.github/workflows/microgrant-program-commands.yml.github/workflows/notify-tsc-members-mention.yml.github/workflows/please-take-a-look-command.yml.github/workflows/pr-review-checklist.yml.github/workflows/pr-testing-with-test-project.yml.github/workflows/release-announcements.yml.github/workflows/release-with-changesets.yml.github/workflows/transfer-issue.yml.github/workflows/update-docs-in-website.yml.github/workflows/update-docs-on-docs-commits.yml.github/workflows/update-maintainers.yml.github/workflows/update-pr.yml.github/workflows/welcome-first-time-contrib.yml
💤 Files with no reviewable changes (1)
- .github/workflows/manual-netlify-preview.yml
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/update-pr.yml
- .github/workflows/please-take-a-look-command.yml
- .github/workflows/transfer-issue.yml
|
There was a problem hiding this comment.
Pull request overview
This PR hardens GitHub Actions workflows by pinning many actions to commit SHAs, reducing default token permissions, and replacing the previous Netlify preview workflow with a split build/deploy workflow.
Changes:
- Pins many workflow actions to specific SHAs and adjusts checkout credential persistence.
- Adds deny-by-default workflow permissions with job-level scopes in several workflows.
- Replaces the Netlify docs preview workflow with separate pull request build and workflow-run deploy workflows.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/add-good-first-issue-labels.yml |
Pins github-script. |
.github/workflows/automerge-for-humans-add-ready-to-merge-or-do-not-merge-label.yml |
Pins github-script steps. |
.github/workflows/automerge-for-humans-merging.yml |
Pins github-script steps. |
.github/workflows/automerge-for-humans-remove-ready-to-merge-label-on-edit.yml |
Pins github-script. |
.github/workflows/automerge-orphans.yml |
Pins checkout and github-script. |
.github/workflows/automerge.yml |
Pins github-script. |
.github/workflows/bounty-program-commands.yml |
Pins github-script steps. |
.github/workflows/bump.yml |
Adds default-deny permissions and pins checkout. |
.github/workflows/help-command.yml |
Pins github-script steps. |
.github/workflows/if-nodejs-pr-testing.yml |
Pins checkout, setup-node, and shared node-version action. |
.github/workflows/local-generate-files.yml |
Adds default-deny permissions, pins actions, and changes Slack notifier. |
.github/workflows/manual-netlify-preview.yml |
Removes the previous combined Netlify preview workflow. |
.github/workflows/manual-netlify-build.yml |
Adds pull request build workflow for docs preview artifacts. |
.github/workflows/manual_netlify_preview.yml |
Adds workflow-run deploy workflow for Netlify preview comments. |
.github/workflows/microgrant-program-commands.yml |
Pins github-script steps. |
.github/workflows/notify-tsc-members-mention.yml |
Pins checkout, setup-node, and github-script steps. |
.github/workflows/please-take-a-look-command.yml |
Pins github-script. |
.github/workflows/pr-review-checklist.yml |
Changes trigger and permissions while pinning checkout. |
.github/workflows/pr-testing-with-test-project.yml |
Adds reduced permissions and pins checkout/shared actions. |
.github/workflows/release-announcements.yml |
Pins checkout and github-script. |
.github/workflows/release-with-changesets.yml |
Adds reduced permissions, pins actions, and changes Slack notifier. |
.github/workflows/transfer-issue.yml |
Pins checkout and github-script. |
.github/workflows/update-docs-in-website.yml |
Hardens permissions, checkout credentials, and push behavior. |
.github/workflows/update-docs-on-docs-commits.yml |
Pins checkout, setup-node, and shared node-version action. |
.github/workflows/update-maintainers.yml |
Adds default-deny permissions, pins actions, updates Node, and hardens push flow. |
.github/workflows/update-pr.yml |
Pins github-script steps. |
.github/workflows/welcome-first-time-contrib.yml |
Adds job permissions and pins github-script. |
Comments suppressed due to low confidence (3)
.github/workflows/release-with-changesets.yml:85
- This switches to
rtCamp/action-slack-notifybut keeps the old8398a7/action-slackinputs andSLACK_WEBHOOK_URLvariable. The rtCamp action expects values such asSLACK_WEBHOOKandSLACK_MESSAGEinenv, so this failure notification will not be sent until the configuration is migrated to that action's interface.
uses: rtCamp/action-slack-notify@e31e87e03dd19038e411e38ae27cbad084a90661 # v2.3.3
with:
status: ${{ job.status }}
fields: repo,action,workflow
text: "Release workflow failed in testing job"
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_CI_FAIL_NOTIFY }}
.github/workflows/release-with-changesets.yml:153
- This failure notification was changed to
rtCamp/action-slack-notifybut still uses the old action'swithinputs andSLACK_WEBHOOK_URLenv name. Migrate these values to the rtCamp action's expected env variables, for exampleSLACK_WEBHOOKandSLACK_MESSAGE, otherwise release-job failures will not notify Slack.
uses: rtCamp/action-slack-notify@e31e87e03dd19038e411e38ae27cbad084a90661 # v2.3.3
with:
status: ${{ job.status }}
fields: repo,action,workflow
text: "Release workflow failed in release job"
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_CI_FAIL_NOTIFY }}
.github/workflows/local-generate-files.yml:58
- This action was migrated to
rtCamp/action-slack-notify, but the configuration is still for8398a7/action-slack(with.status,with.fields,with.text, andSLACK_WEBHOOK_URL). Use the rtCamp action's env-based configuration such asSLACK_WEBHOOKandSLACK_MESSAGE; otherwise release asset-generation failures will not produce Slack notifications.
uses: rtCamp/action-slack-notify@e31e87e03dd19038e411e38ae27cbad084a90661 # v2.3.3
with:
status: ${{ job.status }}
fields: repo,action,workflow
text: 'Unable to bump the version in package.json after the release'
env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_CI_FAIL_NOTIFY }}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| startsWith(github.event.pull_request.title, 'chore(release):') | ||
| )) || | ||
| (github.actor == 'allcontributors[bot]' && | ||
| (github.actor == '46447321' && |
|
|
||
| on: | ||
| pull_request_target: | ||
| pull_request: |
There was a problem hiding this comment.
- please address comments from bots
-
uses: asyncapi/.github/.github/actions/get-node-version-from-package-lock@mastermust stay, it is safe as it comes from our own repo. Changing to SHA will not work, as when we make a change to these actions and push them, we do not know SHA, so we would not be able to set it in workflows at the same time - in few workflows where you switched to SHA, you also upgraded versions used, did you double check if there are no breaking changes? also for changes where you switch from one action to another?
- I do not understand changes to netflify files, for one you created new file instead of editing old, so now hard to track what was changed - also why we need separate build workflow
Hey @derberg I will address the first two comments and about testing the workfloas, I did not :( I am going to revert pinning to latest and only pin the actions to the sha of the version they are pinned to where possible
For the netlify file, the issue is that there is a pwn request (which allows for remote code execution of untrusted forks)This is cause because the workflow satisfies all the pwn request conditions:
The workflow does need the elevated priviledge to create the docs preview so the solutioin top this was to separate the code into two files. One file to pull the pull_request, checks out the PR, syncs docs, and builds the static site, then uploads the built output + PR number as an artifact. |
|
@Shurtu-gal any ideas how to test the workflows still work after pînning actions SHA to the latest hashes? |
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
…age-lock back to master
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/release-with-changesets.yml (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the rationale comment for consistency.
Based on learnings, workflows referencing the shared org repo via
@mastershould include an inline comment explaining that the usage is intentional. Although this rationale is provided earlier in the file, adding the same comment above this step will maintain consistency and clarify the usage for future readers.📝 Proposed fix
- if: steps.packagejson.outputs.exists == 'true' name: Check package-lock version + # This workflow is from our own org repo and safe to reference by 'master'. uses: asyncapi/.github/.github/actions/get-node-version-from-package-lock@master # pinned to latest master with:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-with-changesets.yml at line 110, Add the same rationale comment used by other shared-org workflow references immediately above the step using asyncapi/.github/.github/actions/get-node-version-from-package-lock@master, clarifying that the `@master` reference is intentional and pinned to the latest master.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/release-with-changesets.yml:
- Line 110: Add the same rationale comment used by other shared-org workflow
references immediately above the step using
asyncapi/.github/.github/actions/get-node-version-from-package-lock@master,
clarifying that the `@master` reference is intentional and pinned to the latest
master.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb4db310-b6f1-49e2-988c-5c39e0d9d1ae
📒 Files selected for processing (5)
.github/workflows/if-nodejs-pr-testing.yml.github/workflows/local-generate-files.yml.github/workflows/pr-testing-with-test-project.yml.github/workflows/release-with-changesets.yml.github/workflows/update-docs-on-docs-commits.yml
@derberg I changed the |
… checking out fork PR code for pull_request_target/workflow_run events
|
Description
Pinned all unpinned actions to a commit SHA. The goal is to move from tag pinned to SHA pinned to avoid tag move compromised from affecting our org
The
manual_netlify_priview.ymlworkflow satisfied most of the requirements of apwn_requestso I split it into two which is Github best practices. chore: update build config #2091 (comment)Add a zizmor workflow which scans for common pipeline security issues such as:
harden
update-docs-in-website.ymlworkflowAdd
permissions: {}at workflow level with minimal job-level scopes(contents: write, pull-requests: write) to fix excessive-permissions (without this block, the workflow inherits the write-all permissions of the parent)
Set persist-credentials: false on the generator checkout since it is
read-only; keep it true on the website checkout for git push
Replace embedded-token push URL with git push origin HEAD to prevent
token exposure in logs and process listings (template-injection)
Move github.sha expansions into env vars to keep shell steps
injection-safe
Scope git config to the repository instead of --global
Generated-by: Claude Code Opus 4.7 and Sonnet 5
Related issue(s)
TBC
Summary by CodeRabbit
Summary by CodeRabbit