Bump.sh diff adjustments #186909
Bump.sh diff adjustments #186909
Conversation
In order for the Bump.sh action to be able to post github comments on PRs opened from forks, the GH action needs to listen to the 'pull_request_target' event (instead of the 'pull_request' event). More details about the introduction of this event in this GH blog post: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/ Complete documentation about this event: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target
Since the latest release of the Bump.sh GH action (https://github.com/bump-sh/github-action/releases/tag/v1.1.11), a PR comment is published for all changes made to the OAS file. The extra preview command should thus not be needed as a preview link is available in the automatic diff comment.
paulRbr
force-pushed
the
bump-diff-ajustements
branch
from
a52a148 to
c6e9886
Compare
lcawl
added
Feature:OAS
|
buildkite test this |
💔 Build FailedFailed CI StepsTo update your PR or re-run it, just comment with: |
|
Hi @v1v,
To quote the blogpost you have shared :
So unlike CircleCi, a malicious actor can't just submit a PR with the *pull_request_target* Actions configuration file tampered with to do something nasty, as that code is never used during the *pull_request_target* workflow.
This sounds safe, and it is. The problem comes when people do something unsafe in their repo's *pull_request_target* configuration, which can basically be summarised as one crucial mistake: using *actions/checkout* to checkout the pull request's repo HEAD, and thus introducing potentially malicious code into the workflow.
In the current PR, we don't do that mistake of checkout-ing the PR's head. Instead we only use the target branch checkout, and thus can't run unsafe code provided from the PR.
There is no security issue in the current suggestion workflow.
Please let me know where you are doubting the change could execute unsafe code?
Thanks!
8 Jul 2024 23:19:40 Victor Martinez ***@***.***>:
…
***@***.**** requested changes on this pull request.
----------------------------------------
In .github/workflows/bump.yml[#186909 (comment)]:
> @@ -7,7 +7,7 @@ on:
paths:
- 'oas_docs/kibana.serverless.yaml'
- pull_request:
+ pull_request_target:
IMO, this is a no-go when using GitHub secrets, see https://nathandavison.com/blog/github-actions-and-the-threat-of-malicious-pull-requests
—
Reply to this email directly, view it on GitHub[#186909 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAG4YANHQVQMJW7NHEGWBZTZLL66XAVCNFSM6AAAAABJ4HGA76VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNRUGQ2DSOJYGU].
You are receiving this because you authored the thread.
|
|
I probably was too quick to type here without checking whether the checkout was from the head, but we, Observability, agreed to ban GitHub itself says if you use it, then use a label to control when:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ In addition, this repository could be configured with this in case it's not yet. Regardless, I suggest asking the infosec team to clarify if that's okay (cc @ismisepaul). |
Hi @v1v, thanks for the suggestion. Indeed controlling the run of the action based on a label (added by code owners) on the PR would be a good balance to make sure the security risk is mitigated 👍. I hope the discussions with the infosec team worked out 🤞 (the next PR from Lisa isn't really reassuring 😅) |
I didn't start any discussion with Infosec; I guess that's something the Repo Owners or GitHub workflow owners should do. I only reviewed this PR following some of the hardening practices we have in place. |
| @@ -33,21 +33,12 @@ jobs: | |||
| file: oas_docs/kibana.serverless.yaml | |||
|
|
|||
| api-diff: | |||
| if: ${{ github.event_name == 'pull_request' }} | |||
| if: ${{ github.event_name == 'pull_request_target' }} | |||
| name: Check API diff on Bump.sh | |||
| runs-on: ubuntu-latest | |||
| steps: | |||
| - name: Checkout | |||
| uses: actions/checkout@v3 | |||
There was a problem hiding this comment.
| uses: actions/checkout@v3 | |
| uses: actions/checkout@v4 |
Summary
Two adjustments on the Bump.sh workflow when creating a diff comment about the OAS changes:
pull_request_targetto allow comments from PRs of forks(see commit descriptions for details of each change)