From de0f33c2623b057e80b7cafd53e19fac2f984961 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Wed, 18 Dec 2024 08:26:15 -0800 Subject: [PATCH] ci: Refactor screenshot workflow (#7773) 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. --- .github/workflows/update-screenshots.yaml | 87 ++++++++++++++++++++--- 1 file changed, 79 insertions(+), 8 deletions(-) diff --git a/.github/workflows/update-screenshots.yaml b/.github/workflows/update-screenshots.yaml index 82ffeb3452..d46e3351ff 100644 --- a/.github/workflows/update-screenshots.yaml +++ b/.github/workflows/update-screenshots.yaml @@ -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: @@ -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: @@ -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. @@ -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 @@ -73,6 +87,10 @@ 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 @@ -80,23 +98,71 @@ jobs: # 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" @@ -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() }}