/milestone 0.11.0
/hold

Mmh i cannot get the trigger to work :/ i tried pushing a commit that touches driver/event_table.c but nothing happened.

/cc @Molter73 WDYT? You are super expert in the CI area :D
I don't get why the workflows are not triggered!

Was able to test it in FedeDP/plugins#1.
It worked fine; i think we need to merge this in master before seeing it in action.

I don't get why the workflows are not triggered!

Sorry I'm late for this, but my guess is that the problem was with setting the ready_for_review type as the only trigger, which would make it so the workflow only triggers when a PR is opened or goes from draft to ready. Using the default events of opened, reopened and synchronize looks like what we actually want.

Sorry I'm late for this, but my guess is that the problem was with setting the ready_for_review type as the only trigger, which would make it so the workflow only triggers when a PR is opened or goes from draft to ready. Using the default events of opened, reopened and synchronize looks like what we actually want.

Yep that is what i thought too! Thanks for reviewing :)

This is still on hold because i am not sure about the security impact of this change:

Warning: For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch. To help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered. For more information, see "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" on the GitHub Security Lab website.

@LucaGuerra

This is still on hold because i am not sure about the security impact of this change:

yep this is my only concern here :/

Another solution would be to just make the job fail; i don't like it that much though :/

I have reviewed the security properties of this.

The PR, as is, is relatively safe because nothing in the workflow actually runs anything that is supplied as code (while the GHA content itself won't run until it's merged because it's picked from the base of the PR not the head). In fact, the workflow only checks if certain files are modified and does not run or otherwise inspect the content.

HOWEVER:
The moment someone (such as @FedeDP) decides to add something smarter to this check to make the message more user-friendly, say, a custom Bash script in the repo that inspects the file contents ( 😱 @FedeDP ) the thing would become extra vulnerable because someone can change the bash script and run arbitrary code with the repository's full permission once the CI is allowed to run. That said, it is possible to implement that securely but the approach is heavier as described in the GH blog post.

I propose the following: please @FedeDP add a comment that scares anyone away from adding anything that remotely resembles custom code execution to this workflow and swear you won't do it (cc @Andreagit97 for good measure). If this is needed we'll build a more complex workflow, such as the one that is presented in the linked article https://securitylab.github.com/research/github-actions-preventing-pwn-requests .

I promise i won't add any magic to these workflows 🤣
I will add the comment, thank you very much for this in-depth research @LucaGuerra!

@LucaGuerra should be ok now :) Thank you again!

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, LucaGuerra, Molter73

The full list of commands accepted by this bot can be found here.

The pull request process is described here

/unhold