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

Add theme-check action #8170

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Add theme-check action #8170

merged 3 commits into from
Sep 17, 2024

Conversation

matiasbenedetto
Copy link
Member

@matiasbenedetto matiasbenedetto commented Sep 17, 2024

Changes proposed in this Pull Request:

Add a theme-check github action that runs theme-check plugin checks on the themes that were modified in a particular PR. The action post their reports as a comment.

It's using the theme-check wp-cli from this PR until it's merged.
I created a demo repo to test it.

Screenshot

The action should post a message like this:

image

Copy link
Contributor

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

Works as advertised!

For other iterations, could we consider only posting a message if there are errors in order to avoid extra comments?

name: WordPress Theme Check

on:
pull_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

Using pull_request means that the action won't be able to post comments on PRs created from a fork. A workaround for this is using pull_request_target instead, but we would have to make sure that the audit is free from exploit opportunities before merging, as that event runs with higher permissions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what's the way to go around this. I think you researched this before for the playground preview action, right? Could you provide more context, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this action looks safe because we are not loading code from outside the YML file. The risk we faced when using pull_request_target earlier came from the fact that the YML file was checking out a file from the same repo, which could potentially be modified by an attacker.

Here's a more in-depth and general explanation: https://github.com/CycodeLabs/raven/blob/main/docs/Pull%20Request%20Injections/README.md#2-pull_request_target--checkout

The real danger arises when such a workflow is combined with checkout action, which checks out code from an incoming, potentially untrusted pull request and then executes scripts or runs commands based on that code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the context! I think we can merge this PR and research more in case we need forked repos run this action.

Comment on lines 39 to 41
docker exec $(docker ps -qf "ancestor=wordpress:php8.2") curl -O https://raw.githubusercontent.com/wp-cli/builds/gh-pages/phar/wp-cli.phar
docker exec $(docker ps -qf "ancestor=wordpress:php8.2") chmod +x wp-cli.phar
docker exec $(docker ps -qf "ancestor=wordpress:php8.2") mv wp-cli.phar /usr/local/bin/wp
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use @wordpress/env instead? I tested the theme-check cli integration using npx @wordpress/env run cli

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is good suggestion; I replaced the plain docker definition by the use the @wordpress/env. The code looks a bit simpler now. 2840740

@matiasbenedetto
Copy link
Member Author

matiasbenedetto commented Sep 17, 2024

For other iterations, could we consider only posting a message if there are errors in order to avoid extra comments?

👍 Yes, implemented that here: 6f61618

@matiasbenedetto matiasbenedetto merged commit a244279 into trunk Sep 17, 2024
1 of 2 checks passed
@matiasbenedetto matiasbenedetto deleted the add/theme-check-action branch September 17, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants