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

ci: change event triggers for benchmark CI to include forks #840

Merged
merged 3 commits into from
Jan 9, 2024
Merged

Conversation

YamenMerhi
Copy link
Member

What does this PR introduce?

🤖 CI

This PR updates the GitHub Actions workflow event trigger from pull_request to pull_request_target. Below are the key differences and reasons for this change:

Differences between pull_request and pull_request_target

  • Context of Execution:

    • pull_request: The workflow runs in the context of the pull request itself, meaning it uses the code from the feature branch of the fork.
    • pull_request_target: The workflow runs in the context of the base repository, using the code from the base branch to which the PR is targeted.
  • Access to Secrets:

    • pull_request: Has limited access to secrets, particularly when the PR is from a forked repository. This is a security measure to prevent exposing secrets to potentially unsafe code in forks.
    • pull_request_target: Has access to all secrets of the base repository, even for PRs from forks.

Why the Change Enables Comment Posting from Forks

  • Permission to Comment:

    • By using pull_request_target, the workflow gains the necessary write permissions to post comments on PRs. This is essential for PRs originating from forks, as pull_request does not provide sufficient permissions for this action.
  • Access to Repository Secrets:

    • The workflow can now utilize repository secrets if needed for commenting or other operations, which is not possible with pull_request for forked PRs.
  • Security Considerations:

    • While this change provides more access, it's important to ensure that the workflow does not execute code from the PR itself to maintain security. This PR's workflow is specifically designed to post comments without executing PR code.

PR Checklist

  • Wrote Tests
  • Wrote & Generated Documentation (readme/natspec/dodoc)
  • Ran npm run lint && npm run lint:solidity (solhint)
  • Ran npm run format (prettier)
  • Ran npm run build
  • Ran npm run test

@Hugoo
Copy link
Contributor

Hugoo commented Jan 9, 2024

Task linked: DEV-9462 Fix CI to run benchmark from Fork

@CJ42
Copy link
Member

CJ42 commented Jan 9, 2024

Great find @YamenMerhi ! 🎉

@CJ42 CJ42 merged commit afe4fd6 into develop Jan 9, 2024
24 checks passed
@CJ42 CJ42 deleted the DEV-9462 branch January 9, 2024 15:25
@YamenMerhi YamenMerhi mentioned this pull request Jan 9, 2024
@Hugoo
Copy link
Contributor

Hugoo commented Jan 10, 2024

Wait are you seriously opening the repo secrets to ANY fork? including malicious people doing fork of this repo?

pull_request_target: Has access to all secrets of the base repository, even for PRs from forks.

Has access to all secrets of the base repository, even for PRs from forks.

@Hugoo
Copy link
Contributor

Hugoo commented Jan 10, 2024

Either I'm missing something huge here or this is extremely dangerous.

@Hugoo
Copy link
Contributor

Hugoo commented Jan 10, 2024

richtera pushed a commit that referenced this pull request Mar 6, 2024
Co-authored-by: Jean Cvllr <31145285+CJ42@users.noreply.github.com>
@richtera richtera mentioned this pull request Mar 6, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants