fix(ci): 🚑 use pull_request_target instead of pull_request

[okineadev]

Description

This will fix the falling workflows
pull_request_target unlike pull_request grants write access to GITHUB_TOKEN¹ pull_request_target is recommended to be used only where tasks such as commenting and labeling² are very necessary, which is what we need

You can read more about my discovery of the problem starting from this comment in #2365: #2365 (comment)

2: https://stackoverflow.com/questions/74957218/what-is-the-difference-between-pull-request-and-pull-request-target-event-in-git#:~:text=The%20pull_request_tar get%20event%20grants%20workflows%20triggered%20by%20pull%20requests%20from%20forks%20access%20to%20repository%20secrets%20and%20a%20read/write%20GITHUB_TOKEN
2: https://stackoverflow.com/questions/74957218/what-is-the-difference-between-pull-request-and-pull-request-target-event-in-git#:~:text=only%20use%20pull_request_target %20for%20workflow ws%20where%20access%20to%20secrets%20or%20write%20permissions%20is%20strictly%20necessary%2C%20such%20as%20commenting%2C%20labeling%2C%20or%20status%20updates%20on%20pull%20requests.

Contribution Guidelines

• By creating this pull request, I acknowledge that I have read the Contribution Guidelines for this project
• I have read the Code Of Conduct and promise to abide by these rules

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

[PKief]

@okineadev is that really necessary? According to the answers on Stack overflow and @lishaduck comments I could make out that "pull_request_target" is more risky:

The pull_request_target event grants workflows triggered by pull requests from forks access to repository secrets and a read/write GITHUB_TOKEN. That is inherently risky if the workflow inadvertently exposes these secrets or allows for unauthorized modifications to the repository.

Short question: why don't we just revert the changes of the labeling and live without it? I don't see that much of a benefit. I don't want to fix more pipeline issues only because we want to have some labels in PRs 😕

[okineadev]

@okineadev is that really necessary? According to the answers on Stack overflow and @lishaduck comments I could make out that "pull_request_target" is more risky:

The pull_request_target event grants workflows triggered by pull requests from forks access to repository secrets and a read/write GITHUB_TOKEN. That is inherently risky if the workflow inadvertently exposes these secrets or allows for unauthorized modifications to the repository.

Short question: why don't we just revert the changes of the labeling and live without it? I don't see that much of a benefit. I don't want to fix more pipeline issues only because we want to have some labels in PRs 😕

Please do not roll back the changes

[okineadev]

I tried really hard to implement this feature

[PKief]

I appreciate that you've tried hard, but I'm asking if it's necessary to continue if it means that we're introducing more security risks and more head aches in maintaining our pipelines. We're talking about some simple labels here...

[okineadev]

I appreciate that you've tried hard, but I'm asking if it's necessary to continue if it means that we're introducing more security risks and more head aches in maintaining our pipelines. We're talking about some simple labels here...

If it was possible to allow privileged access only for the step that adds the label, that would be very great

But let's discuss it tomorrow

[PKief]

ok, I marked it as draft until a solution is found.

[okineadev]

Now this PR is almost normal, you can even merge the changes in the pr-closed.yml file because I am sure of its safety and functionality

pull_request_target in all other workflows is required because they all have a step that adds a label or comment
But you're right, it's not very safe to use, so we need to figure out how to make only one step privileged

I found such an article somewhere, I will attach the link later

UPD: Link

[okineadev]

We can implement a privileged step by creating a separate workflow called add-labels.yml, which is triggered upon completion of the build and color check workflows, according to the names of the completed workflows, a label will be added

Maybe it's even + for the DRY principle, because we won't have to add the same step in different workflows

[PKief]

We can implement a privileged step by creating a separate workflow called add-labels.yml, which is triggered upon completion of the build and color check workflows, according to the names of the completed workflows, a label will be added

Maybe it's even + for the DRY principle, because we won't have to add the same step in different workflows

I don't have experience with that and no time to figure it out because I'm quite busy these days.

Shall we revert the changes until we have implemented that? I can see that none of the new PRs have successful pipelines and it's impossible to review it without our checks.

[PKief]

@okineadev FYI: I've rolled back the changes. It's not good when all pipelines are failing and we don't have a fix ready yet. As I said, I've limited time capacity these days, I can't figure out a solution. And adding and removing labels to PRs is not critical to me.

[lishaduck]

pull_request_target is inherently vulnerable. As I'd suggest a while ago, I highly suggest running Zizmor in CI before messing with workflows more.
The add-labels idea would work, and is generally the suggested approach. I have no experience with it though, so please don't pester me. I'm happy to review it, but I'm not a genie.

My 2¢ is that life was fine before labeling, so rolling it back is the right decision, and further tampering should only be merged after it's been ensured it works.

[okineadev]

@okineadev FYI: I've rolled back the changes. It's not good when all pipelines are failing and we don't have a fix ready yet. As I said, I've limited time capacity these days, I can't figure out a solution. And adding and removing labels to PRs is not critical to me.

Okay, so tomorrow I'll re-implement it according to my idea, after thoroughly testing it first, then when you have the opportunity, take a look.

[okineadev]

I highly suggest running Zizmor in CI

How is CodeQL worse?

[okineadev]

Move to #2829

[lishaduck]

I highly suggest running Zizmor in CI

How is CodeQL worse?

I didn't know you'd enabled CodeQL. I personally like a) being able to run Zizmor locally, whereas CodeQL can't be (easily), and b) they'll both have different edge cases, and I believe CodeQL is primarily focused on taint checking.

[okineadev]

I highly suggest running Zizmor in CI

How is CodeQL worse?

I didn't know you'd enabled CodeQL. I personally like a) being able to run Zizmor locally, whereas CodeQL can't be (easily), and b) they'll both have different edge cases, and I believe CodeQL is primarily focused on taint checking.

Do you see these CodeQL workflows?

[lishaduck]

Do you see these CodeQL workflows?

I do now. I'm not exactly in the habit of checking which status checks are passing on any given PR.