From ccf499a9e6f2c492e83e252e07cc865ffd2a70b5 Mon Sep 17 00:00:00 2001 From: mjnong Date: Thu, 27 Aug 2026 17:00:34 +0200 Subject: [PATCH 1/2] fix(ci): harden preview cleanup (CEL-1328) --- .../deploy-static-job-boundaries.test.rb | 69 ++++++++++++++++++- .github/workflows/deploy-static-website.yaml | 31 +++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/.github/tests/deploy-static-job-boundaries.test.rb b/.github/tests/deploy-static-job-boundaries.test.rb index 7f48fa1..b1f8ce1 100644 --- a/.github/tests/deploy-static-job-boundaries.test.rb +++ b/.github/tests/deploy-static-job-boundaries.test.rb @@ -177,9 +177,76 @@ abort "#{step_name} must serialize gcloud storage workers" unless storage_parallelism == expected_serial_execution end -cleanup_guard = jobs.fetch("cleanup").fetch("if") +cleanup = jobs.fetch("cleanup") +cleanup_guard = cleanup.fetch("if") abort "Preview cleanup must survive author offboarding" if cleanup_guard.include?("author_association") +cleanup_steps = cleanup.fetch("steps") +cleanup_authorization_index = cleanup_steps.index { |step| step.fetch("name", "") == "Revalidate cleanup authorization" } +cleanup_oidc_index = cleanup_steps.index { |step| step.fetch("name", "") == "Authenticate to Google Cloud" } +abort "Cleanup authorization must be revalidated immediately before OIDC" unless cleanup_authorization_index && cleanup_authorization_index + 1 == cleanup_oidc_index +cleanup_authorization = cleanup_steps.fetch(cleanup_authorization_index) +abort "Cleanup authorization recheck must use pinned GitHub Script" unless cleanup_authorization.fetch("uses", "").match?(/\Aactions\/github-script@[0-9a-f]{40}\z/) +abort "Cleanup authorization recheck must use the job token" unless cleanup_authorization.fetch("with", {}).fetch("github-token", nil) == "${{ github.token }}" +cleanup_script = cleanup_authorization.fetch("with").fetch("script") +abort "Cleanup authorization must require a closed pull request" unless cleanup_script.include?("pullRequest.state === 'closed'") +abort "Cleanup authorization must require actor ownership" unless cleanup_script.include?("pullRequest.user?.login === actor") +abort "Cleanup authorization must require current write permission" unless cleanup_script.include?("['admin', 'maintain', 'write'].includes(access.permission)") + +cleanup_base_pull_request = { + "state" => "closed", + "user" => { "login" => "maintainer" }, + "head" => { "repo" => { "full_name" => "CellarNode/site" } }, +} +cleanup_cases = { + "current trusted author" => [cleanup_base_pull_request, "write", true], + "open pull request" => [cleanup_base_pull_request.merge("state" => "open"), "write", false], + "forked head" => [cleanup_base_pull_request.merge("head" => { "repo" => { "full_name" => "attacker/site" } }), "write", false], + "different author" => [cleanup_base_pull_request.merge("user" => { "login" => "other-user" }), "write", false], + "revoked permission" => [cleanup_base_pull_request, "read", false], +} +cleanup_cases.each do |name, (pull_request, permission, expected)| + _stdout, _stderr, status = Open3.capture3( + { + "ACTOR" => "maintainer", + "AUTHORIZATION_SCRIPT" => cleanup_script, + "CURRENT_PERMISSION" => permission, + "PR_JSON" => JSON.generate(pull_request), + "PR_NUMBER" => "42", + "REPOSITORY" => "CellarNode/site", + }, + "node", "-e", <<~'JAVASCRIPT', + const AsyncFunction = Object.getPrototypeOf(async () => null).constructor; + const pullRequest = JSON.parse(process.env.PR_JSON); + const github = { + rest: { + pulls: { get: async () => ({ data: pullRequest }) }, + repos: { + getCollaboratorPermissionLevel: async () => ({ + data: { permission: process.env.CURRENT_PERMISSION }, + }), + }, + }, + }; + const context = { repo: { owner: 'CellarNode', repo: 'site' } }; + const core = { setFailed: message => { throw new Error(message); } }; + new AsyncFunction('github', 'context', 'core', process.env.AUTHORIZATION_SCRIPT)(github, context, core) + .catch(error => { console.error(error.message); process.exitCode = 1; }); + JAVASCRIPT + ) + abort "cleanup #{name}: expected accepted=#{expected}, got accepted=#{status.success?}" unless status.success? == expected +end + +delete_preview = cleanup_steps.find { |step| step.fetch("name", "") == "Delete preview" } +cleanup_parallelism = delete_preview&.fetch("env", {})&.slice( + "CLOUDSDK_STORAGE_PROCESS_COUNT", + "CLOUDSDK_STORAGE_THREAD_COUNT", +) +abort "Preview cleanup must serialize gcloud storage workers" unless cleanup_parallelism == { + "CLOUDSDK_STORAGE_PROCESS_COUNT" => "1", + "CLOUDSDK_STORAGE_THREAD_COUNT" => "1", +} + validation_path = File.expand_path("../workflows/validate-static-deploy.yaml", __dir__) validation = YAML.safe_load(File.read(validation_path), aliases: true) validation_checkout = validation.fetch("jobs").fetch("lock-validator").fetch("steps").find { |step| step.fetch("uses", "").start_with?("actions/checkout@") } diff --git a/.github/workflows/deploy-static-website.yaml b/.github/workflows/deploy-static-website.yaml index d07b02c..9a1fa32 100644 --- a/.github/workflows/deploy-static-website.yaml +++ b/.github/workflows/deploy-static-website.yaml @@ -731,8 +731,37 @@ jobs: permissions: contents: read id-token: write + pull-requests: read steps: + - name: Revalidate cleanup authorization + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b + env: + ACTOR: ${{ github.actor }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPOSITORY: ${{ github.repository }} + with: + github-token: ${{ github.token }} + script: | + const actor = process.env.ACTOR; + const prNumber = Number(process.env.PR_NUMBER); + const repository = process.env.REPOSITORY; + const { data: pullRequest } = await github.rest.pulls.get({ + ...context.repo, + pull_number: prNumber, + }); + const { data: access } = await github.rest.repos.getCollaboratorPermissionLevel({ + ...context.repo, + username: actor, + }); + const authorized = pullRequest.state === 'closed' && + pullRequest.user?.login === actor && + pullRequest.head.repo?.full_name === repository && + ['admin', 'maintain', 'write'].includes(access.permission); + if (!authorized) { + core.setFailed('Cleanup authorization is no longer valid'); + } + - name: Authenticate to Google Cloud uses: google-github-actions/auth@7c6bc770dae815cd3e89ee6cdf493a5fab2cc093 with: @@ -750,6 +779,8 @@ jobs: - name: Delete preview env: CLOUDSDK_CONFIG: ${{ runner.temp }}/gcloud + CLOUDSDK_STORAGE_PROCESS_COUNT: '1' + CLOUDSDK_STORAGE_THREAD_COUNT: '1' WEBSITE_SLUG: ${{ inputs.website_slug }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | From 0a03a445ac329dcdc7b4689de59b1dbb674b4413 Mon Sep 17 00:00:00 2001 From: mjnong Date: Thu, 27 Aug 2026 17:22:07 +0200 Subject: [PATCH 2/2] fix(ci): allow maintainer preview cleanup (CEL-1328) --- .../tests/deploy-static-job-boundaries.test.rb | 15 ++++++++------- .github/workflows/deploy-static-website.yaml | 1 - 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/tests/deploy-static-job-boundaries.test.rb b/.github/tests/deploy-static-job-boundaries.test.rb index b1f8ce1..878ce1c 100644 --- a/.github/tests/deploy-static-job-boundaries.test.rb +++ b/.github/tests/deploy-static-job-boundaries.test.rb @@ -190,19 +190,19 @@ abort "Cleanup authorization recheck must use the job token" unless cleanup_authorization.fetch("with", {}).fetch("github-token", nil) == "${{ github.token }}" cleanup_script = cleanup_authorization.fetch("with").fetch("script") abort "Cleanup authorization must require a closed pull request" unless cleanup_script.include?("pullRequest.state === 'closed'") -abort "Cleanup authorization must require actor ownership" unless cleanup_script.include?("pullRequest.user?.login === actor") +abort "Cleanup authorization must allow a write-capable closer who is not the pull-request author" if cleanup_script.include?("pullRequest.user?.login === actor") abort "Cleanup authorization must require current write permission" unless cleanup_script.include?("['admin', 'maintain', 'write'].includes(access.permission)") cleanup_base_pull_request = { "state" => "closed", - "user" => { "login" => "maintainer" }, + "user" => { "login" => "author" }, "head" => { "repo" => { "full_name" => "CellarNode/site" } }, } cleanup_cases = { - "current trusted author" => [cleanup_base_pull_request, "write", true], + "write-capable maintainer closes another author's pull request" => [cleanup_base_pull_request, "write", true], + "write-capable author closes own pull request" => [cleanup_base_pull_request.merge("user" => { "login" => "maintainer" }), "write", true], "open pull request" => [cleanup_base_pull_request.merge("state" => "open"), "write", false], "forked head" => [cleanup_base_pull_request.merge("head" => { "repo" => { "full_name" => "attacker/site" } }), "write", false], - "different author" => [cleanup_base_pull_request.merge("user" => { "login" => "other-user" }), "write", false], "revoked permission" => [cleanup_base_pull_request, "read", false], } cleanup_cases.each do |name, (pull_request, permission, expected)| @@ -222,9 +222,10 @@ rest: { pulls: { get: async () => ({ data: pullRequest }) }, repos: { - getCollaboratorPermissionLevel: async () => ({ - data: { permission: process.env.CURRENT_PERMISSION }, - }), + getCollaboratorPermissionLevel: async ({ username }) => { + if (username !== process.env.ACTOR) throw new Error(`unexpected permission subject: ${username}`); + return { data: { permission: process.env.CURRENT_PERMISSION } }; + }, }, }, }; diff --git a/.github/workflows/deploy-static-website.yaml b/.github/workflows/deploy-static-website.yaml index 9a1fa32..2d5b692 100644 --- a/.github/workflows/deploy-static-website.yaml +++ b/.github/workflows/deploy-static-website.yaml @@ -755,7 +755,6 @@ jobs: username: actor, }); const authorized = pullRequest.state === 'closed' && - pullRequest.user?.login === actor && pullRequest.head.repo?.full_name === repository && ['admin', 'maintain', 'write'].includes(access.permission); if (!authorized) {