-
Notifications
You must be signed in to change notification settings - Fork 188
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
Update code-path-changes.yml #3713
Conversation
Turns out in order for this to work for forked repos the trigger needs to be 'pull_request_target' rather than 'pull_request'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a potential security issue because this way all of the secrets are exposed. I think the common practice for when you need this is to limit the permissions. This should be enough for our use case:
permissions:
contents: read
Ok, added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for now with the intention to see whether it will work in the first place and stricten the permissions later.
OZ convinced me that contents:read is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Turns out in order for this to work for forked repos the trigger needs to be 'pull_request_target' rather than 'pull_request'
🔧 Type of changes
✨ What's the context?
Code path notifications are not working for PRs from forked repos