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

chore: fix: event trigger label conflict #7936

Merged
merged 7 commits into from
Sep 15, 2023

Conversation

ddevsr
Copy link
Collaborator

@ddevsr ddevsr commented Sep 13, 2023

Description
Based on https://github.com/actions/labeler#permissions and https://github.com/prince-chrismc/label-merge-conflicts-action#faq---how-do-i-fix-resource-not-accessible-by-integration must using event pull_request_target

In order to add labels to pull requests, the GitHub labeler action requires write permissions on the pull-request. However, when the action runs on a pull request from a forked repository, GitHub only grants read access tokens for pull_request events, at most. If you encounter an Error: HttpError: Resource not accessible by integration, it's likely due to these permission constraints. To resolve this issue, you can modify the on: section of your workflow to use pull_request_target instead of pull_request (see example above). This change allows the action to have write access, because pull_request_target alters the context of the action and safely grants additional permissions.

Follow-up #7928

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@ddevsr ddevsr added the github_actions Pull requests that update Github_actions code label Sep 13, 2023
@kenjis
Copy link
Member

kenjis commented Sep 13, 2023

using the (potentially dangerous) event pull_request_target
https://github.com/prince-chrismc/label-merge-conflicts-action#faq---how-do-i-fix-resource-not-accessible-by-integration

GitHub workflows can be triggered through a wide variety of repository events. This includes events related to incoming pull requests (PR). There exists a potentially dangerous misuse of the pull_request_target workflow trigger that may lead to malicious PR authors (i.e. attackers) being able to obtain repository write permissions or stealing repository secrets.
TL;DR: Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Are you sure that this PR is safe?
pull_request_target is dangerous.

@paulbalandan
Copy link
Member

I would not advocate to use pull_request_target as it makes the PR vulnerable.

@kenjis
Copy link
Member

kenjis commented Sep 13, 2023

If we use pull_request_target, and If the secrets of this repository were to be obtained by an attacker through PR, it would be a major security incident.

@datamweb
Copy link
Contributor

Anyway, the Member adds the label stale. I think similar to smart commenting safely.

@ddevsr
Copy link
Collaborator Author

ddevsr commented Sep 13, 2023

I admit that pull_request_target is dangerous. I will use sample from @datamweb, labeling manually but commenting with automation

@paulbalandan
Copy link
Member

@ddevsr can you try putting the permissions inside the specific job instead of outside? also change the token to ${{ secrets.GITHUB_TOKEN }}. although they're the same

@ddevsr
Copy link
Collaborator Author

ddevsr commented Sep 13, 2023

@kenjis @paulbalandan @datamweb I already change to implement smart commenting codeigniter4/shield#812

@kenjis kenjis changed the title fix: event trigger label conflict chore: fix: event trigger label conflict Sep 14, 2023
@ddevsr ddevsr added the stale Pull requests with conflicts label Sep 15, 2023
@ddevsr
Copy link
Collaborator Author

ddevsr commented Sep 15, 2023

@paulbalandan @kenjis I tested with result Resource not accessible by integration again

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6193689315/job/16815562654?pr=7936

@paulbalandan
Copy link
Member

try add contents: write permission

@ddevsr ddevsr added stale Pull requests with conflicts and removed stale Pull requests with conflicts labels Sep 15, 2023
@ddevsr ddevsr added stale Pull requests with conflicts and removed stale Pull requests with conflicts labels Sep 15, 2023
@ddevsr
Copy link
Collaborator Author

ddevsr commented Sep 15, 2023

@ddevsr ddevsr removed the stale Pull requests with conflicts label Sep 15, 2023
@ddevsr ddevsr added the stale Pull requests with conflicts label Sep 15, 2023
@kenjis
Copy link
Member

kenjis commented Sep 15, 2023

Because you sent the PR from your repository.
This workflow does not work if it is merged.

@ddevsr
Copy link
Collaborator Author

ddevsr commented Sep 15, 2023

https://github.com/orgs/community/discussions/63736

Give permissions above if conditions.

@kenjis Oh, okay

@ddevsr ddevsr removed the stale Pull requests with conflicts label Sep 15, 2023
@kenjis
Copy link
Member

kenjis commented Sep 15, 2023

Now let's merge and test.

@kenjis kenjis merged commit df03d10 into codeigniter4:develop Sep 15, 2023
1 of 2 checks passed
@ddevsr ddevsr deleted the event-trigger branch September 15, 2023 06:24
Comment on lines 2 to +8
on:
pull_request:
branches:
- 'develop'
- '4.*'
types:
- labeled
Copy link
Member

Choose a reason for hiding this comment

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

Should be?

on:
  pull_request:
    types:
      - labeled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants