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: Securely allow fork repo PRs and unittest coverage #156

Merged
merged 36 commits into from
Feb 16, 2024

Conversation

alperenkose
Copy link
Collaborator

Description

This PR introduces a secure way of handling fork repo PRs with delegating the actual work to workflow_run workflows to get proper access for workflows as suggested in the below link:
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Motivation and Context

We didn't handle required action permissions for PRs from fork repos resulted in unittest coverage not being published on PR as well as Close PR workflow failing to close PRs on pan.dev.

With the updated workflows unittests are successful and coverage results are commented on the PRs from fork repos.

How Has This Been Tested?

Tested on a fork repo and by creating another fork repo based on it.
Closing of pan.dev preview PRs are also tested.

⚠️ Important Note
For coverage report to be commented on the PR from fork repos, the repository name in fork repo must match the original repository name. This happens from a limitation in coverage report action using Github API list PRs which doesn't allow filtering of fork repo PRs with a different repository name. See "head" filter on the Github API List pull requests call.

Types of changes

CI changes.

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

Copy link
Collaborator

@adambaumeister adambaumeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, the logic makes sense as far as I can tell.

@alperenkose alperenkose merged commit e180eb0 into PaloAltoNetworks:main Feb 16, 2024
8 checks passed
@horiagunica
Copy link
Collaborator

🎉 This PR is included in version 0.3.4 🎉

The release is available on PyPI and GitHub release

Posted by semantic-release bot

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

Successfully merging this pull request may close these issues.

3 participants