Fix workflow run on PR's

[Tony763]

Description

Workflows runs triggered by pull_request event does not have write access to repository so status updates and upload to GitHub Pages does not work.

Write access can be gain when event for triggering workflow is set to pull_request_target, but it could be potentially unsafe as anybody could implement malicious code into code but not into workflow as it is run from repository not from commit. Any change to workflow is not use, when this event trigger it.

To make PRs working and secure I added these: Workflow can be triggered by pull or pull_request_target.

• Workflow triggered by event pull are run immediately.
• Workflow triggered by event pull_request_target are run only when label: secure is added.

Workflow is divided to two jobs:

safeguard:

• Check if event is push or pull request contain label secure
• If pull request, bot will remove secure label from PR

build:

• Runs only when safeguard passes with ok state
• do all tests and uploads as before

With those changes, when new PR is issued, no test runs. Only when maintainer check changes and place label secure to it, workflow start. If new commit is added to PR, no run is triggered until label secure is added again.

When a change to workflow is proposed, its a better to check it's outcome in forked repository.

For contributors there is still possibility to run workflow on their forks, so they can do changes and test them on them.

Type of PR

• [x] Bugfix
• [ ] Feature implementation
• [ ] Refactor of code (without functional changes)
• [ ] Documentation improvements
• [ ] Test improvements

Testing

Big thanks to @pfefferle for help with testing.

[Tony763]

@krisgesling could You check?

[pfefferle]

what about removing the travis config?

[Tony763]

I am not sure if marketplace still use it.

[stratus-ss]

@krisgesling should probably bump this up the priority list as several PRs are affected by this bug

[Tony763]

I will add also workflow to mark PR as untested, when new is added or new commit to it. Probably tonight.

[krisgesling]

Damn, that makes sense but its not great...

More details here: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ The "apply a label" option is one thing they suggest, however there are further ways this could be abused.

As nice as the github pages idea is, I think it would be best if we publish these to our main reports.mycroft.ai server and don't use pull_request_target if we don't absolutely have to. You can see how this is done in mycroft-core and mycroft-skills: https://github.com/MycroftAI/mycroft-core/blob/3495acf9abb183a7b36e5ebc0a8f5c25159993f8/Jenkinsfile#L109

We should be able to do the same via Github Actions from this repo with an ssh key in the repo secrets.

I've created a subdirectory: /var/www/voight-kampff/skill-homeassistant/ which means we'll end up with url's in the format: https://reports.mycroft.ai/skill-homeassistant/PR-87/

[Tony763]

Hi @krisgesling if we can use github secret, maybe I could use it to deploy github pages and also status of linters. I would just limit permisions what could be done with it.

On other hand when pull_request_target is used, you always use workflow from your repository, with pull_request, you run workflow from PR, which could abuse Secrets.

[Tony763]

Hi @krisgesling, i played a little with this.

When we change to to pull_request:

• workflow run from PR
• no labels (add/remove)
• no tests outcome status (linters)
• no allure (no github-pages and also no repository secrets as they are not accessible by PR)

When we use pull_request_target:

• workflow run from repository, not from PR
• we can trigger workflow by adding label (label is removed at begging of run)
• tests status
• allure can by deployed to github pages or with secret to any other place
• Maintainer have to check code if it's safe and execute workflow run by adding label secure

What I can do to make this safer: I could add special job that would add label Workflow change! to PR when it find changes to files in .github/workflows so you can see at first glance.

Any toughs?

[Tony763]

@krisgesling With @pfefferle we tested new workflow to add label workflow when a change is made in folder .github and it's sub folders. Works good. You can check in my repository.

Another security improvement would be to add Need for approval to run workflow for first time contributors.

[Tony763]

Cool, I did not know that there is a way to enable execution only from repo. Yes, linting errors were only for test. Latest commit is clean.

PR is ready to merge, You only have to create two new labels workflow (ideally red) and secure (ideally green).