Skip to content
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

Refactoring - A consolidated workflow to handle/process Internal and External PR #17

Closed
AndyBoWu opened this issue Jan 11, 2024 · 3 comments
Assignees

Comments

@AndyBoWu
Copy link
Contributor

AndyBoWu commented Jan 11, 2024

I created a dummy PR from my forked repo: #16

Some issues I discovered and I think they can be improved:

  1. security issue

    pull_request_target:

    This trigger is very powerful, i.e., pull_request_target can have access write permission to the repo, also access to the secrets. Although the these secrets are stored in runner memory, but there are some findings that the secrets could be leaked with the following factors:
    1.1 we checkout the code from the forked repo, meaning, the actor, if they are willing to, can embed malicious code / package. (
    sha: ${{ github.event.pull_request.head.sha }}
    )
    1.2 npm install seems innocent, but the bad package can be slipped into the PR if we (code reviewer) didn't pay 200% attention

  2. the dummy PR triggered the internal workflow too which is not ideal, https://github.com/storyprotocol/sdk/actions/runs/7494857454/job/20403690431#step:7:38

  3. the above job failed anyway since it does not have access to the secret (a PR from a forked repo)

I am thinking to do a bit optimization, and the following goals are expected reached:

  1. a more consolidated workflow instead of dedicated workflow for internal and external PR
  2. make event triggering work as expected

REF
[1] https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
[2] https://securitylab.github.com/research/github-actions-untrusted-input/
[3] https://securitylab.github.com/research/github-actions-building-blocks/

@AndyBoWu AndyBoWu self-assigned this Jan 11, 2024
@AndyBoWu
Copy link
Contributor Author

PR: #18

More testing are needed once the PR merged to main.

@AndyBoWu
Copy link
Contributor Author

AndyBoWu commented Jan 12, 2024

Testing Plan (per my discussion with Ze):

  1. workflow is located in the branch: feat/secure_workflow
  2. an identical branch protection rule like main
  3. a PR created from within the org
  4. a PR created from a forked repo

Expected Results:

  1. PR from any forked repo needs to be manually approved first (every single time)
  2. PR from within org can trigger the workflow
  3. no sensitive info leaked in the log

@AndyBoWu
Copy link
Contributor Author

After discussion with @edisonz0718 , we deleted the insure GHA workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant