search

ImagingDataCommons / idc-index-data

Python package providing the index to query and download data hosted by the NCI Imaging Data Commons
MIT License
1 stars 4 forks source link

CI: update workflow to pull_request_target #7

Closed fedorov closed 5 months ago

fedorov commented 5 months ago

Current pull_request does not allow PRs from forks to have access to the repo secrets.

See details in https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

jcfr commented 5 months ago

Usually, access to secrets is specific to the deployment jobs and associated workflows, for this reason there is no reason to allow access to secrets from pull request coming from fork.

In this case, since the "regular" jobs are expected to access the secret required to run the query, it may indeed be helpful to allow pull request to access the corresponding secret.

Alternatively, we could require maintainer to create branches and publish them in this repo.

As outlined in the post^1, it will be critical to careful review pull request from external contributor we do not know before clicking the Approve and Run when their first pull request is submitted.

jcfr commented 5 months ago

After reading more carefully the documentation, this should be reverted as it prevents workflow triggered in pull request from effectively testing the code being proposed.

See https://stackoverflow.com/questions/74957218/what-is-the-difference-between-pull-request-and-pull-request-target-event-in-git

jcfr commented 5 months ago

Few paths forward here:

  1. If we are okay with having pull-request accessing the "secret" (aka SERVICE_ACCOUNT_KEY), it would be easier to simply add it as a regular variable. This would avoid applying workaround requiring to explicit checkout the actual branch associated with the pull request. See INSECURE. Provided as an example only. example in the post^1.

  2. Revert this pull request, skip the part of the workflow that do not have access to the secret reporting a meaningful message indicating that they need to be added as collaborator. Indeed pull request from collaborator (with "write" access) will have access^2 to secret.

  3. Implement a more sophisticated set of workflows with a first one able to access the secret to generate the index-data file and uploading it as an artifact to then ensure a second workflow effectively test the pull request code by using the file generated.

Moving forward with (2) seems like the most reasonable solution here.