-
Notifications
You must be signed in to change notification settings - Fork 887
chore: auto merge dependabot PRs for patch updates #16222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- name: Harden Runner | ||
uses: step-security/harden-runner@cb605e52c26070c328afc4562f0b4ada7618a84e # v2.10.4 | ||
with: | ||
egress-policy: audit | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these as they do not provide any value in this workflow.
Also, this does not affect the OpenSSF score.
runs-on: ubuntu-latest | ||
if: github.event_name == 'pull_request_target' | ||
if: github.event.pull_request.user.login == 'dependabot[bot]' && github.repository == 'coder/coder' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems far too flimsy; is there no way we can reference a user by ID?
This workflow has to be crazy hardened if we're going to enable it.
See also: https://www.synacktiv.com/en/publications/github-actions-exploitation-dependabot
I have not read it fully; I'm focused on other tasks today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been doing it at other places in ci.yaml
too.
Also, they suggest we follow the GitHub docs. See: https://www.synacktiv.com/en/publications/github-actions-exploitation-dependabot#footnote4_i8j88po
And we are exactly following that.
As far as I understand, the exploit only works when using pull_request_target,
and I have changed that to pull_request
as per GitHub's recommendation.
See: https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also trpc fixed it by updating github.actor
--> github.event.pull_request.user.login
See: trpc/trpc@8187594
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://api.github.com/users/dependabot[bot]
Can we not reference the ID instead? I wonder why they suggest login
instead; feels flimsy.
This will ensure we run on label chnages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'd feel a lot more comfortable if we had some oversight of this in Slack
For example, every automerge for these PRs notifies a specific channel which we can check from time to time.
We can do that. I am holding to merge and see if I can squeeze within the current PR. |
This is a bit tricky, as we are only enabling auto merge but would still need to wait for the CI to pass. We need to wait for the PR to merge before we send the notification. I will come to this in a follow up. |
Based on the instructions from https://docs.github.com/en/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions