feat(security): prevent RCE in various workflows #17157
Conversation
Interface Check ReportThis pull request does not change any public interfaces ! |
|
@RedYetiDev thanks for the PR. |
| @@ -13,6 +13,7 @@ jobs: | |||
| if: | |||
| (! contains(github.event.pull_request.body, '[X] does not change any runtime related code or build configuration')) | |||
| runs-on: ubuntu-latest | |||
| environment: external | |||
There was a problem hiding this comment.
Hi @RedYetiDev , thanks for your contribution.
May I ask why do we need to set an environment variable external here ?
There was a problem hiding this comment.
Setting this value to external means that the workflow won't proceed without approval from a repository admin
There was a problem hiding this comment.
Does it mean the admin should manually to approve the action every time?
There was a problem hiding this comment.
Setting this value to external means that the workflow won't proceed without approval from a repository admin
@RedYetiDev , we need the web-interface-check.yml run every time when Pull Request is updated even when it isn't approved.
So I don't know why should it have to be run after approval. Any specific security reasons?
There was a problem hiding this comment.
There will be an issue if we run this workflow after approval.
The old strategy:
- Developer send a pull request
- web-interface-check.yml will be run automatically
- Repo maintainer review the
interface changecomment sent byweb-interface-checktask - Repo maintainer approved the Pull Request
- Pull Request was merged
If we mark the it as external, the flow will be
- Developer send a pull request
- web-interface-check.yml will not run.
- Repo maintainer approved the Pull Request
- Repo may forget to wait the
web-interface-check.ymlto finish and merge the Pull Request, the interface changes will not be reviewed.
So make the environment to external will let each maintainer notice that there is a task being run after approved, and it will be easily forgotten.
There was a problem hiding this comment.
Sorry that this change wouldn't be the best anti-RCE method.
This file runs without approval, and uses local scripts, leading to RCE.
I'd suggest reworking the workflow to run on pull_request rather than pull_request_target.
I'm out-of-town right now, so I'll be unable to implement such a change.
There was a problem hiding this comment.
@RedYetiDev ok,thanks for the advice, I will check that.
There was a problem hiding this comment.
There was a problem hiding this comment.
In the end, fix it here : #17264
@RedYetiDev Could you please help to review that PR? Thank you.
|
I found that this Pull Request was closed since the source repository was deleted. I will send another PR to resolve the RCE issue in web-interface-check.yml. @RedYetiDev thanks for this PR. |
CC @minggo