Skip to content

Commit

Permalink
ci: Refactor screenshot workflow (shaka-project#7773)
Browse files Browse the repository at this point in the history
This workflow, triggerable only by maintainers, had some potential
security issues. This is a big refactor, and makes several changes:

 - Clean up description text (non-critical)
- Add granular permissions to set status (without this, the workflow was
broken since we changed default permissions)
- Split the update-pr job into commit-new-screenshots (unprivileged) and
update-pr (privileged as @shaka-bot)

The commit-new-screenshots job runs code that the PR author controls,
such as "npm ci" (controlled through package.json and
package-lock.json), and "./build/updateScreenshots.py" (easily edited to
do whatever). These steps could be used to do literally anything,
including modify tools in /usr/bin on the workflow VM that are needed by
the privileged steps.

By moving the privileged steps into a completely separate job, we can
ensure a clean slate without worrying about the VM's state. We only
transfer the .git/ folder between the two jobs. So the
commit-new-screenshots job will create the commit, and the update-pr job
will actually push that commit from a clean VM.

The job is once again functional, and for the first time, actually safe.
  • Loading branch information
joeyparrish authored Dec 18, 2024
1 parent 3d742fe commit de0f33c
Showing 1 changed file with 79 additions and 8 deletions.
87 changes: 79 additions & 8 deletions .github/workflows/update-screenshots.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
# avoid malicious code executing in the lab.
inputs:
pr:
description: "A PR number to build and test in the lab. Required even if \"pr\" is given."
description: "A PR number to build and test in the lab."
required: true

jobs:
Expand All @@ -29,6 +29,11 @@ jobs:
name: Set Pending Status
needs: compute-sha
runs-on: ubuntu-latest

permissions:
# "Write" to statuses to update commit status
statuses: write

steps:
- uses: actions/checkout@v4
with:
Expand All @@ -46,6 +51,11 @@ jobs:
name: Get Selenium Lab Screenshots
needs: [set-pending-status]
uses: ./.github/workflows/selenium-lab-tests.yaml

permissions:
# "Write" to statuses to update commit status, needed by nested jobs.
statuses: write

with:
# Pass the pre-computed SHA directly to the nested workflow.
# Do NOT pass "pr" and reinterpret it into a SHA in the nested workflow.
Expand All @@ -54,14 +64,18 @@ jobs:
ignore_test_status: true
job_name_prefix: "Get Selenium Lab Screenshots / "

update-pr:
name: Update PR
commit-new-screenshots:
name: Commit New Screenshots
runs-on: ubuntu-latest
needs: [compute-sha, run-lab-tests]
# NOTE: NO PERMISSIONS ON THIS JOB. It runs PR-author-controlled code from
# the PR, and so must be untrusted!

steps:
- uses: actions/checkout@v4
with:
ref: ${{ needs.compute-sha.outputs.SHA }}
fetch-depth: 0
persist-credentials: false

- name: Get artifacts
Expand All @@ -73,30 +87,82 @@ jobs:

- name: Update screenshots
run: |
# NOTE: Steps of this could be influenced by the PR author, which is
# why we run this job without any accessible tokens or special
# permissions.
# Install prerequisites.
npm ci
# Update the official screenshots for any that have visibly changed.
# This is not a byte-for-byte comparison, but based on pixel diffs.
./build/updateScreenshots.py
# Emulate the actions bot.
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
# Act as Shaka Bot.
git config user.name "shaka-bot"
git config user.email "shaka-bot@users.noreply.github.com"
# Commit the changes. Ignore failure, in case there are no changes.
# Commit the changes to the screenshots only. Ignore failure, in
# case there are no changes.
git add test/test/assets/screenshots/*/*.png || true
git commit -m ':robot: Update all screenshots' || true
- name: Cache Commits
# Here we cache commits, made above in an untrusted job, to pull into a
# separate, trusted job with permission to push to the repo. The
# untrusted job can't pollute the environment of the trusted job by,
# say, modifying /usr/bin/gh.
uses: actions/cache/save@v4
with:
path: .git/
key: screenshot-commits-${{ needs.compute-sha.outputs.SHA }}

- name: Debug
uses: mxschmitt/action-tmate@v3.17
with:
limit-access-to-actor: true
if: failure()

update-pr:
name: Update PR
runs-on: ubuntu-latest
needs: [compute-sha, commit-new-screenshots]

# NOTE: No granular permissions here, because we use SHAKA_BOT_TOKEN
# instead of the default token. The action to push to the PR must be done
# by an actor with permission, and the default GitHub token doesn't work.

steps:
- uses: actions/checkout@v4
with:
ref: ${{ needs.compute-sha.outputs.SHA }}
fetch-depth: 0
persist-credentials: false

- name: Restore Commits
# Here we restore commits, made above in the above untrusted job, to
# pull into this trusted job. See comments above on "Cache Commits".
uses: actions/cache/restore@v4
with:
path: .git/
key: screenshot-commits-${{ needs.compute-sha.outputs.SHA }}

- name: Update PR
env:
GH_TOKEN: ${{ github.token }}
GH_TOKEN: ${{ secrets.SHAKA_BOT_TOKEN }}
run: |
# Update the PR.
# Compute the destination for the push. This uses the GitHub API
# because this workflow is not triggered directly by a PR, so there
# is no context variable that supplies these details.
PR_API_URL="/repos/${{ github.repository }}/pulls/${{ inputs.pr }}"
REMOTE=$(gh api $PR_API_URL | jq -r .head.repo.html_url)
BRANCH=$(gh api $PR_API_URL | jq -r .head.ref)
# Lean on $GH_TOKEN to authenticate the push.
gh auth setup-git
# If there were no changes, this will do nothing, but succeed.
git push "$REMOTE" HEAD:"$BRANCH"
Expand All @@ -109,6 +175,11 @@ jobs:
set-final-status:
name: Set Final Status
runs-on: ubuntu-latest

permissions:
# "Write" to statuses to update commit status
statuses: write

needs: [compute-sha, run-lab-tests, update-pr]
# Will run on success or failure, but not if the workflow is cancelled.
if: ${{ success() || failure() }}
Expand Down

0 comments on commit de0f33c

Please sign in to comment.