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

[CI] Run Pronto on GitHub Actions. #8429

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Conversation

denschub
Copy link
Member

I'm kinda tired of maintaining our own webhook receiver and pronto runner on the project server. :) Also, sorry, another long essay.

This is surprisingly hard to get right, because GitHub Action's permission system is nearly where I want it to be for this. The default on: pull_request trigger works fine for team-internal PRs, but the access token it gets is always read-only for everything if the PR is coming from an external contributor. So this won't fly, especially since Pronto hasn't a good internal fallback to "just silently pass". If we run Pronto with the github_status and github_pr formatters, then it wants to leave an approving review and set the status to green. If it can't, it explodes.

A possible workaround to this would be to simply run Pronto as you'd run on your local setup, and have it dump the linting errors/warnings to the log and fail with a non-zero status code. This wouldn't require any access token, and just like our test suite, the status would just turn red. However, this means that if the linting fails, you'd have to look into the job log to figure out why. Both me and @SuperTux88 agreed in a side-channel chat that we kinda really like the way that Pronto leaves inline-comments.

A possible solution is to run the action on: pull_request_target. What this does is a bit complicated, but essentially, this runs the job for a PR as if it was triggered from within the repo or from a team member. The good thing is that this allows us to run jobs for an external contributor's PR with elevated permissions, but the bad thing is that this allows us to run jobs for an external contributor's PR with elevated permissions. If you're not careful, a malicious contributor could open a PR that changes bin/bundle, for example, with a script that pushes the access token to a malicious webservice and uses that token to do evil stuff or something. In theory, the scope of this security issue is limited as the token is supposed to expire when the job ends, but in practice, this solves absolutely nothing (as you could just do malicious stuff within your custom patch, or just block the job for a while - timeouts are large).

To avoid running unchecked third-party code with a GitHub Token with write access in scope, this job implementation does a few things. I'm not going to explain them here, the steps this is taking are documented in the workflow definition. Additionally, this repo is set to not run Actions for first-time contributors without approval. This isn't much, but it helps a bit.

I'm not sure if there's a better way to achieve this.

To review this, you have essentially two options:

  1. Here's a screenshot of how a PR run looks. Trust me(tm).
  2. Create a private fork in your personal account, enable actions, be sure to match this repo's security settings, push the PR to main, open a PR and test it yourself.

I'm happy to PR the same change to the Federation repo, but I'll hold off on that until this one is merged, just in case there are revisions! Please take a look, @jhass and @SuperTux88 - both of you, just because more eyes are more better.

Copy link
Member

@SuperTux88 SuperTux88 left a comment

Choose a reason for hiding this comment

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

Thanks 🍪 and yes, I would like to also have that for the federation repo after this got merged 👍

.github/workflows/lint.yml Outdated Show resolved Hide resolved
.github/workflows/lint.yml Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
@denschub denschub added this to the 1.0.0 milestone Jun 15, 2023
@denschub denschub merged commit 6288fe3 into diaspora:develop Jun 15, 2023
8 checks passed
@denschub denschub deleted the prontohub branch June 15, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants