From 4bac9d71d4b7208155ad9efb7799ad3768070aa3 Mon Sep 17 00:00:00 2001 From: Harvey Tseng Date: Sat, 5 Sep 2026 21:07:15 -0400 Subject: [PATCH 1/2] feat: soothfast-bot identity for comments, pages, releases - Every write soothfast makes now carries the bot identity, and a pull request job still never holds a token: it sends the gate comment to the broker's /comment, which posts as the bot and revokes its own token. - Tags are judged by the commit that ran (the OIDC sha), accepted only when it is already on the default branch, so a moved tag changes nothing and a release equals a dispatch from master. - Release creation moves to its own job: a job carries one environment and publish already needs the release approval gate. --- .github/workflows/ci.yml | 17 +- .github/workflows/release.yml | 48 ++++- .github/workflows/soothfast-gate.yml | 39 ++-- CLAUDE.md | 17 +- action.yml | 3 +- action/bot-token.sh | 23 +-- action/comment.sh | 36 +++- action/gate.sh | 10 +- action/oidc.sh | 13 ++ bot/src/github.ts | 65 ++++++- bot/src/index.ts | 179 ++++++++++++++++-- bot/src/oidc.ts | 1 + bot/src/policy.ts | 64 +++++-- bot/test/github.test.ts | 78 +++++++- bot/test/index.test.ts | 269 ++++++++++++++++++++++----- bot/test/keys.ts | 1 + bot/test/policy.test.ts | 89 +++++++-- docs/ci.md | 37 ++-- 18 files changed, 817 insertions(+), 172 deletions(-) create mode 100644 action/oidc.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78a27ef..de499c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -210,7 +210,8 @@ jobs: ] permissions: contents: read - pull-requests: write # gate comment + pull-requests: write # gate comment fallback + id-token: write # soothfast-bot comment token via the broker uses: ./.github/workflows/soothfast-gate.yml with: package: ${{ matrix.package }} @@ -239,8 +240,10 @@ jobs: if: github.event_name == 'push' && github.ref == 'refs/heads/master' needs: [check, docs, build-cli] runs-on: ubuntu-latest + environment: soothfast-bot permissions: - contents: write # push the built site to gh-pages + contents: read + id-token: write # soothfast-bot token pushes the built site to gh-pages env: SOOTHFAST: ./bin/cargo-soothfast steps: @@ -267,7 +270,15 @@ jobs: - run: make baselines - run: make docs-pages - run: bin/cargo-soothfast docs build --baseline self + - name: Mint a soothfast-bot token + id: bot + run: action/bot-token.sh - uses: peaceiris/actions-gh-pages@84c30a85c19949d7eee79c4ff27748b70285e453 # v4.1.0 with: - github_token: ${{ github.token }} + github_token: ${{ steps.bot.outputs.token }} publish_dir: ./site + - name: Revoke the soothfast-bot token + if: always() && steps.bot.outputs.token != '' + env: + GH_TOKEN: ${{ steps.bot.outputs.token }} + run: gh api -X DELETE /installation/token diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 52e1a2c..def874e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -149,11 +149,6 @@ jobs: - uses: dtolnay/rust-toolchain@6bed0761d98439e5a578e2877258200ad565ba87 # stable with: toolchain: stable - - name: Pin the rustdoc toolchain - run: | - TC=$(make -s print-SOOTHFAST_RUSTDOC_TOOLCHAIN) - echo "SOOTHFAST_RUSTDOC_TOOLCHAIN=$TC" >> "$GITHUB_ENV" - rustup toolchain install "$TC" - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: lookup-only: true @@ -175,6 +170,37 @@ jobs: sleep 30 done + # The release itself is authored by soothfast-bot; the broker accepts the + # tag because its commit is already on master. + release: + name: Create GitHub Release + runs-on: ubuntu-latest + needs: [resolve, publish] + environment: soothfast-bot + permissions: + contents: read + id-token: write # soothfast-bot token creates the release + outputs: + tag: ${{ needs.resolve.outputs.tag }} + steps: + - uses: step-security/harden-runner@05e31511f85b41b11d1cf0ef85d0992719546e2c # v2.21.0 + with: + egress-policy: audit + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + fetch-depth: 0 # previous-tag lookup + surface diff need history + - uses: dtolnay/rust-toolchain@6bed0761d98439e5a578e2877258200ad565ba87 # stable + with: + toolchain: stable + - name: Pin the rustdoc toolchain + run: | + TC=$(make -s print-SOOTHFAST_RUSTDOC_TOOLCHAIN) + echo "SOOTHFAST_RUSTDOC_TOOLCHAIN=$TC" >> "$GITHUB_ENV" + rustup toolchain install "$TC" + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 + with: + lookup-only: true # Dogfood: release assets are produced from this tag's own measurements. - name: Produce release report from recorded measurements run: | @@ -217,9 +243,12 @@ jobs: printf '%s\n' "$NOTES" > /tmp/release_notes.md fi + - name: Mint a soothfast-bot token + id: bot + run: action/bot-token.sh - name: Create GitHub Release env: - GH_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ steps.bot.outputs.token }} TAG: ${{ needs.resolve.outputs.tag }} USE_AUTO: ${{ steps.changelog.outputs.use_auto }} run: | @@ -231,11 +260,16 @@ jobs: else gh release create "$TAG" --verify-tag --title "$TAG" --notes-file /tmp/release_notes.md $ASSETS fi + - name: Revoke the soothfast-bot token + if: always() && steps.bot.outputs.token != '' + env: + GH_TOKEN: ${{ steps.bot.outputs.token }} + run: gh api -X DELETE /installation/token binaries: name: Prebuilt cargo-soothfast (${{ matrix.target }}) runs-on: ${{ matrix.runner }} - needs: [resolve, publish] + needs: [resolve, release] permissions: contents: write # upload the release asset strategy: diff --git a/.github/workflows/soothfast-gate.yml b/.github/workflows/soothfast-gate.yml index fe1ad53..bb6d069 100644 --- a/.github/workflows/soothfast-gate.yml +++ b/.github/workflows/soothfast-gate.yml @@ -29,9 +29,11 @@ permissions: {} jobs: gate: runs-on: ubuntu-latest + environment: soothfast-bot permissions: contents: read - pull-requests: write # gate comment + pull-requests: write # fallback for the comment when no bot token + id-token: write # the broker posts the gate comment as soothfast-bot steps: - uses: step-security/harden-runner@05e31511f85b41b11d1cf0ef85d0992719546e2c # v2.21.0 with: @@ -40,6 +42,16 @@ jobs: with: persist-credentials: false fetch-depth: 0 # merge-base needs history + # The action scripts at this workflow's own commit, whoever calls it. + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + repository: Verdenroz/soothfast + ref: ${{ github.job_workflow_sha }} + path: .soothfast-action + sparse-checkout: | + action + action.yml + persist-credentials: false - uses: dtolnay/rust-toolchain@6bed0761d98439e5a578e2877258200ad565ba87 # stable with: toolchain: stable @@ -54,7 +66,10 @@ jobs: - if: inputs.cli-artifact != '' run: chmod +x bin/cargo-soothfast - if: inputs.cli-artifact == '' - uses: Verdenroz/soothfast@ead9d50c79606edcf80a438f8fac73229c01120c # v0.2.0 + uses: ./.soothfast-action + with: + gate: "false" + changelog: "false" - name: Run gate id: gate run: | @@ -72,27 +87,21 @@ jobs: name: soothfast-triage path: .soothfast/triage/ if-no-files-found: ignore - # Fork PRs get a read-only token here; the comment is best effort. The - # marker finds our own comment: github.token's author is shared with - # every other action in the repo, so --edit-last would hit theirs. + # The broker posts the comment as soothfast-bot; a fork pull request has + # no OIDC token and falls back to github.token. Best effort either way. - name: PR comment with gate results if: always() && github.event_name == 'pull_request' continue-on-error: true env: GH_TOKEN: ${{ github.token }} PR_NUMBER: ${{ github.event.pull_request.number }} + MARKER: "" + BODY_FILE: ${{ runner.temp }}/soothfast-gate-comment.md run: | { - echo '' echo '## soothfast gate' echo '```' - tail -n 60 gate-output.txt + tail -n 60 gate-output.txt | sed 's/```/` ` `/g' echo '```' - } > comment.md - id=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \ - --jq '[.[] | select(.body | startswith("")) | .id][0] // empty') - if [ -n "$id" ]; then - gh api -X PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${id}" -F body=@comment.md >/dev/null - else - gh api -X POST "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -F body=@comment.md >/dev/null - fi + } > "$BODY_FILE" + .soothfast-action/action/comment.sh diff --git a/CLAUDE.md b/CLAUDE.md index 87f4b94..d3c5980 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -331,9 +331,20 @@ workflow holds the App's private key. The job runs `action/bot-token.sh`, which trades the job's GitHub Actions OIDC token for a one-hour installation token minted by the broker under `bot/` (a Cloudflare Worker, deployed by `bot.yml`). The broker mints only for a job in the `soothfast-bot` -environment, on a `push`/`workflow_dispatch`/`schedule` event, on the -repository's default branch, for a repository the App is installed on, and -scopes the token to that repository. `action/land.sh` then commits, pushes, +environment, for a repository the App is installed on, scoped to that +repository: a landing token (contents + pull requests write) on a +`push`/`workflow_dispatch`/`schedule` event on the default branch or on a +tag whose commit (the OIDC `sha`, never the tag name) is already on it. A +`pull_request` run gets no token; it POSTs the gate comment text to the +broker's `/comment`, which posts it as the bot with its own token and +revokes it. Gate comments, `deploy-docs`' gh-pages push, and the GitHub +Release all carry the bot identity this way; a fork pull request has no OIDC +token and its gate comment falls back to `github.token`. This repo's +`soothfast-bot` environment must have no deployment branch policy: the gate +runs on `refs/pull/*` and the release on `refs/tags/*`. Anything a bot comment +quotes from a pull request's build output is untrusted text under a +write-access author: it stays inside a code fence and fence sequences in it +are neutralised first. `action/land.sh` then commits, pushes, opens or refreshes the bot PR, merges it (queued behind required checks when the default branch has any, immediately otherwise), and revokes the token. Minting happens after the build step on purpose: no step that compiles the diff --git a/action.yml b/action.yml index f4c5def..5b0babd 100644 --- a/action.yml +++ b/action.yml @@ -70,7 +70,7 @@ inputs: required: false default: soothfast-bot broker: - description: Token broker URL. Empty uses the default in action/bot-token.sh. + description: Token broker URL. Empty uses the default in action/oidc.sh. required: false default: "" @@ -160,6 +160,7 @@ runs: BASE_REF: ${{ github.base_ref }} GH_TOKEN: ${{ inputs.token }} PR_NUMBER: ${{ github.event.pull_request.number }} + BROKER: ${{ inputs.broker }} run: "$GITHUB_ACTION_PATH/action/gate.sh" - name: Upload triage artifacts if: steps.gate.outputs.failed == 'true' diff --git a/action/bot-token.sh b/action/bot-token.sh index 7fb7de9..93f6b61 100755 --- a/action/bot-token.sh +++ b/action/bot-token.sh @@ -4,29 +4,26 @@ # Inputs: BROKER (URL, optional). Outputs: token, app_slug, expires_at. set -euo pipefail -BROKER=${BROKER:-https://soothfast-bot.verdenroz.workers.dev} +# shellcheck source=action/oidc.sh +source "$(dirname "$0")/oidc.sh" -if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ]; then - echo "::error::soothfast-bot needs 'id-token: write' in the job's permissions" +fail() { + echo "::error::$1" exit 1 -fi +} -oidc=$(curl -sSf --max-time 30 -H "Authorization: bearer $ACTIONS_ID_TOKEN_REQUEST_TOKEN" \ - "${ACTIONS_ID_TOKEN_REQUEST_URL}&audience=soothfast-bot" | jq -r .value) +oidc=$(oidc_token) || fail "soothfast-bot needs 'id-token: write' in the job's permissions" -response=$(curl -sS --max-time 30 -w '\n%{http_code}' -X POST "$BROKER/token" -H "Authorization: Bearer $oidc") +response=$(curl -sS --max-time 30 -w '\n%{http_code}' -X POST "$BROKER/token" -H "Authorization: Bearer $oidc") || + fail "could not reach the broker at $BROKER" status=${response##*$'\n'} body=${response%$'\n'*} if [ "$status" != 200 ]; then - echo "::error::soothfast-bot refused (HTTP $status): $(jq -r '.reason // .' <<<"$body")" - exit 1 + fail "soothfast-bot refused (HTTP $status): $(jq -r '.reason // .' <<<"$body")" fi -token=$(jq -er .token <<<"$body") || { - echo "::error::soothfast-bot returned no token" - exit 1 -} +token=$(jq -er .token <<<"$body") || fail "soothfast-bot returned no token" echo "::add-mask::$token" { echo "token=$token" diff --git a/action/comment.sh b/action/comment.sh index 1729ba4..0f3bd59 100755 --- a/action/comment.sh +++ b/action/comment.sh @@ -1,16 +1,36 @@ #!/usr/bin/env bash -# Create or update this action's one comment on a pull request. A marker -# line identifies it: github.token's author is shared with every other -# action in the repository, so "edit the last comment by me" would hit theirs. -# Inputs: GH_TOKEN PR_NUMBER MARKER BODY_FILE. +# Create or update this action's one comment on a pull request, identified +# by a marker line. The broker posts it as soothfast-bot; a job with no OIDC +# identity (a fork pull request) falls back to GH_TOKEN, whose author is +# shared with every other action in the repository, hence the marker rather +# than "edit my last comment". +# Inputs: GH_TOKEN PR_NUMBER MARKER BODY_FILE, BROKER (optional). set -euo pipefail -body="${RUNNER_TEMP:-/tmp}/soothfast-comment.md" -{ echo "$MARKER"; cat "$BODY_FILE"; } >"$body" +# shellcheck source=action/oidc.sh +source "$(dirname "$0")/oidc.sh" + +if oidc=$(oidc_token); then + payload=$(jq -n --argjson pr "$PR_NUMBER" --arg marker "$MARKER" --rawfile body "$BODY_FILE" \ + '{pull_request: $pr, marker: $marker, body: $body}') + response=$(curl -sS --max-time 30 -w '\n%{http_code}' -X POST "$BROKER/comment" \ + -H "Authorization: Bearer $oidc" -H "content-type: application/json" --data-binary "$payload") || response=$'\n000' + status=${response##*$'\n'} + body=${response%$'\n'*} + if [ "$status" = 200 ]; then + echo "commented as $(jq -r .app_slug <<<"$body")" + exit 0 + fi + reason=$(jq -r '.reason // empty' <<<"$body" 2>/dev/null || true) + echo "::notice::soothfast-bot did not post the comment (HTTP $status${reason:+: $reason}); posting with the job token instead" +fi + +with_marker="${RUNNER_TEMP:-/tmp}/soothfast-comment.md" +{ echo "$MARKER"; cat "$BODY_FILE"; } >"$with_marker" id=$(gh api "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" --paginate \ --jq "[.[] | select(.body | startswith(\"$MARKER\")) | .id][0] // empty") if [ -n "$id" ]; then - gh api -X PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${id}" -F "body=@${body}" >/dev/null + gh api -X PATCH "repos/${GITHUB_REPOSITORY}/issues/comments/${id}" -F "body=@${with_marker}" >/dev/null else - gh api -X POST "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -F "body=@${body}" >/dev/null + gh api -X POST "repos/${GITHUB_REPOSITORY}/issues/${PR_NUMBER}/comments" -F "body=@${with_marker}" >/dev/null fi diff --git a/action/gate.sh b/action/gate.sh index 3237e2a..482661a 100755 --- a/action/gate.sh +++ b/action/gate.sh @@ -2,8 +2,11 @@ # Gate every package against the pull request's base branch and post the # tail of each package's output as one PR comment. Never exits non-zero on a # regression: the caller reads the `failed` output so the comment and triage -# upload still happen first. -# Inputs: CLI PACKAGES BASE_REF GH_TOKEN PR_NUMBER. Output: failed (true|false). +# upload still happen first. Output is untrusted (the PR's own binaries wrote +# it) and the comment is authored by a write-access identity, so nothing in +# it may escape the code fence. +# Inputs: CLI PACKAGES BASE_REF GH_TOKEN PR_NUMBER, BROKER (optional). +# Output: failed (true|false). set -euo pipefail read -ra pkgs <<<"$PACKAGES" @@ -17,7 +20,8 @@ mkdir -p "$out_dir" "$CLI" gate -p "$pkg" --against-ref "origin/${BASE_REF}" 2>&1 | tee "$out" >&2 || failed=true echo "### ${pkg}" echo '```' - tail -n 40 "$out" + # shellcheck disable=SC2016 # literal backticks, nothing to expand + tail -n 40 "$out" | sed 's/```/` ` `/g' echo '```' done } >"${out_dir}/comment.md" diff --git a/action/oidc.sh b/action/oidc.sh new file mode 100644 index 0000000..a96c961 --- /dev/null +++ b/action/oidc.sh @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +# Shared by bot-token.sh and comment.sh: the broker location and the job's +# OIDC token. Source it; do not run it. + +BROKER=${BROKER:-https://soothfast-bot.verdenroz.workers.dev} + +# Prints the job's OIDC token for the soothfast-bot audience, or nothing when +# the job has no `id-token: write` (a fork pull request, for one). +oidc_token() { + [ -n "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || return 1 + curl -sSf --max-time 30 -H "Authorization: bearer $ACTIONS_ID_TOKEN_REQUEST_TOKEN" \ + "${ACTIONS_ID_TOKEN_REQUEST_URL}&audience=soothfast-bot" | jq -r .value +} diff --git a/bot/src/github.ts b/bot/src/github.ts index 8fc75c9..75e1cf3 100644 --- a/bot/src/github.ts +++ b/bot/src/github.ts @@ -7,8 +7,6 @@ import { const API = "https://api.github.com"; const RS256 = { name: "RSASSA-PKCS1-v1_5", hash: "SHA-256" }; -const TOKEN_PERMISSIONS = { contents: "write", pull_requests: "write" }; - export interface Installation { id: number; } @@ -22,6 +20,16 @@ export interface Repository { default_branch: string; } +export interface Comparison { + status: string; +} + +export interface IssueComment { + id: number; + body: string; + html_url: string; +} + export class GitHubError extends Error { status: number; constructor(status: number, message: string) { @@ -38,9 +46,33 @@ export interface GitHubApi { mintToken( installationId: number, repositoryId: number, + permissions: Record, appJwt: string, ): Promise; repository(repository: string, token: string): Promise; + compare( + repository: string, + base: string, + head: string, + token: string, + ): Promise; + listComments( + repository: string, + issue: number, + token: string, + ): Promise; + createComment( + repository: string, + issue: number, + body: string, + token: string, + ): Promise; + updateComment( + repository: string, + id: number, + body: string, + token: string, + ): Promise; revoke(token: string): Promise; appSlug(appJwt: string): Promise; } @@ -171,11 +203,8 @@ export function githubApi(fetchFn: typeof fetch = fetch): GitHubApi { ); return result.status === 404 ? undefined : expectOk(result); }, - async mintToken(installationId, repositoryId, jwt) { - const body = { - repository_ids: [repositoryId], - permissions: TOKEN_PERMISSIONS, - }; + async mintToken(installationId, repositoryId, permissions, jwt) { + const body = { repository_ids: [repositoryId], permissions }; const path = `/app/installations/${installationId}/access_tokens`; return expectOk(await call("POST", path, jwt, body)); }, @@ -184,6 +213,28 @@ export function githubApi(fetchFn: typeof fetch = fetch): GitHubApi { await call("GET", `/repos/${repository}`, token), ); }, + async compare(repository, base, head, token) { + const path = `/repos/${repository}/compare/${encodeURIComponent(base)}...${encodeURIComponent(head)}`; + return expectOk(await call("GET", path, token)); + }, + async listComments(repository, issue, token) { + const pages: IssueComment[][] = []; + for (let page = 1; ; page++) { + const path = `/repos/${repository}/issues/${issue}/comments?per_page=100&page=${page}`; + const batch = expectOk(await call("GET", path, token)); + pages.push(batch); + if (batch.length < 100) break; + } + return pages.flat(); + }, + async createComment(repository, issue, body, token) { + const path = `/repos/${repository}/issues/${issue}/comments`; + return expectOk(await call("POST", path, token, { body })); + }, + async updateComment(repository, id, body, token) { + const path = `/repos/${repository}/issues/comments/${id}`; + return expectOk(await call("PATCH", path, token, { body })); + }, async revoke(token) { expectOk(await call("DELETE", "/installation/token", token)); }, diff --git a/bot/src/index.ts b/bot/src/index.ts index 7799666..5730efb 100644 --- a/bot/src/index.ts +++ b/bot/src/index.ts @@ -8,8 +8,13 @@ import { import { githubJwks, OidcError, verifyOidc, type JwksLookup } from "./oidc.ts"; import { AUDIENCE, + COMMENT_MAX_BYTES, + PERMISSIONS, decideBranch, decideClaims, + decideTag, + pullRequestNumber, + type Decision, type OidcClaims, } from "./policy.ts"; @@ -24,6 +29,18 @@ export interface Deps { now?: () => number; } +interface CommentRequest { + pull_request: number; + marker: string; + body: string; +} + +interface Minted { + token: string; + expires_at: string; + slug: string; +} + const json = (status: number, body: unknown) => new Response(JSON.stringify(body), { status, @@ -48,13 +65,13 @@ export async function handle( env: Env, deps: Deps, ): Promise { - if (request.method !== "POST" || new URL(request.url).pathname !== "/token") { + const route = request.method === "POST" ? new URL(request.url).pathname : ""; + if (route !== "/token" && route !== "/comment") return json(404, { reason: "not found" }); - } + const authorization = request.headers.get("authorization") ?? ""; if (!authorization.startsWith("Bearer ")) return denied(401, "missing bearer token"); - const verify = verifyOidc(authorization.slice("Bearer ".length), { audience: AUDIENCE, jwks: deps.jwks, @@ -67,7 +84,26 @@ export async function handle( const policy = decideClaims(claims); if (!policy.ok) return denied(403, policy.reason, claims); - return mint(claims, env, deps); + if (route === "/token" && policy.mode !== "land") { + return denied( + 403, + `a ${claims.event_name} run cannot hold a token; use /comment`, + claims, + ); + } + if (route === "/comment" && policy.mode !== "comment") { + return denied(403, "/comment serves pull_request runs only", claims); + } + + const body = + route === "/comment" + ? parseComment(await request.text(), claims) + : undefined; + if (body instanceof Response) return body; + + const auth = await authenticate(claims, policy.mode, env, deps); + if (auth instanceof Response) return auth; + return body ? comment(claims, body, auth, deps) : land(claims, auth, deps); } let cachedKey: { pem: string; key: CryptoKey } | undefined; @@ -89,11 +125,12 @@ async function appSlug( return cachedSlug.slug; } -async function mint( +async function authenticate( claims: OidcClaims, + mode: keyof typeof PERMISSIONS, env: Env, deps: Deps, -): Promise { +): Promise { const jwt = await appJwt( env.GITHUB_APP_CLIENT_ID, await privateKey(env.GITHUB_APP_PRIVATE_KEY), @@ -113,27 +150,133 @@ async function mint( const minted = await deps.github.mintToken( installation.id, Number(claims.repository_id), + PERMISSIONS[mode], jwt, ); - const repository = await deps.github.repository( - claims.repository, - minted.token, - ); - const branch = decideBranch(claims.ref, repository.default_branch); - if (!branch.ok) { - await deps.github - .revoke(minted.token) - .catch((e: unknown) => console.error("revoke failed", e)); - return denied(403, branch.reason, claims); - } const slug = await appSlug(deps.github, env.GITHUB_APP_CLIENT_ID, jwt); + return { ...minted, slug }; +} + +async function revoke(token: string, deps: Deps): Promise { + await deps.github + .revoke(token) + .catch((e: unknown) => console.error("revoke failed", e)); +} + +async function land( + claims: OidcClaims, + minted: Minted, + deps: Deps, +): Promise { + const ref = await decideRef(claims, minted.token, deps); + if (!ref.ok) { + await revoke(minted.token, deps); + return denied(403, ref.reason, claims); + } return json(200, { token: minted.token, expires_at: minted.expires_at, - app_slug: slug, + app_slug: minted.slug, }); } +async function decideRef( + claims: OidcClaims, + token: string, + deps: Deps, +): Promise { + const repository = await deps.github.repository(claims.repository, token); + if (claims.ref.startsWith("refs/tags/")) { + const comparison = await deps.github.compare( + claims.repository, + repository.default_branch, + claims.sha, + token, + ); + return decideTag(claims.ref, comparison.status); + } + return decideBranch(claims.ref, repository.default_branch); +} + +// The job never sees this token: the broker posts on its behalf and revokes. +async function comment( + claims: OidcClaims, + body: CommentRequest, + minted: Minted, + deps: Deps, +): Promise { + try { + const text = `${body.marker}\n${body.body}`; + const existing = ( + await deps.github.listComments( + claims.repository, + body.pull_request, + minted.token, + ) + ).find((c) => c.body.startsWith(body.marker)); + const posted = existing + ? await deps.github.updateComment( + claims.repository, + existing.id, + text, + minted.token, + ) + : await deps.github.createComment( + claims.repository, + body.pull_request, + text, + minted.token, + ); + return json(200, { comment_url: posted.html_url, app_slug: minted.slug }); + } finally { + await revoke(minted.token, deps); + } +} + +function parseComment( + raw: string, + claims: OidcClaims, +): CommentRequest | Response { + if (raw.length > COMMENT_MAX_BYTES * 2) + return json(400, { reason: "request body too large" }); + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return json(400, { reason: "body is not JSON" }); + } + if (typeof parsed !== "object" || parsed === null) + return json(400, { reason: "body is not an object" }); + const { pull_request, marker, body } = parsed as Partial; + if ( + typeof pull_request !== "number" || + typeof marker !== "string" || + typeof body !== "string" + ) { + return json(400, { + reason: "expected {pull_request: number, marker: string, body: string}", + }); + } + if (pull_request !== pullRequestNumber(claims.ref)) { + return denied( + 403, + `pull request ${pull_request} is not the one this run belongs to`, + claims, + ); + } + if (!/^$/.test(marker)) { + return json(400, { + reason: "marker must be an HTML comment like ", + }); + } + if ( + new TextEncoder().encode(`${marker}\n${body}`).length > COMMENT_MAX_BYTES + ) { + return json(400, { reason: `comment exceeds ${COMMENT_MAX_BYTES} bytes` }); + } + return { pull_request, marker, body }; +} + const deps: Deps = { jwks: githubJwks(), github: githubApi() }; export default { diff --git a/bot/src/oidc.ts b/bot/src/oidc.ts index 0a741a9..488be05 100644 --- a/bot/src/oidc.ts +++ b/bot/src/oidc.ts @@ -48,6 +48,7 @@ function asClaims(payload: Record): OidcClaims { "repository", "repository_id", "ref", + "sha", "event_name", ] as const; const wellFormed = diff --git a/bot/src/policy.ts b/bot/src/policy.ts index 51b0f5e..29a3c96 100644 --- a/bot/src/policy.ts +++ b/bot/src/policy.ts @@ -7,29 +7,42 @@ export interface OidcClaims { repository: string; repository_id: string; ref: string; + sha: string; event_name: string; environment?: string; } -export type Decision = { ok: true } | { ok: false; reason: string }; +export type Mode = "land" | "comment"; + +export type Decision = { ok: true; mode: Mode } | { ok: false; reason: string }; export const ISSUER = "https://token.actions.githubusercontent.com"; export const AUDIENCE = "soothfast-bot"; export const ENVIRONMENT = "soothfast-bot"; -export const ALLOWED_EVENTS: readonly string[] = [ +export const LAND_EVENTS: readonly string[] = [ "push", "workflow_dispatch", "schedule", ]; +export const COMMENT_EVENTS: readonly string[] = ["pull_request"]; + +// A landing token is handed to the job. A pull request runs code nobody has +// merged yet, so it never receives a token: the broker posts the comment +// itself with a short-lived token of its own. +export const PERMISSIONS: Record> = { + land: { contents: "write", pull_requests: "write" }, + comment: { pull_requests: "write" }, +}; + +export const COMMENT_MAX_BYTES = 65536; -const allow: Decision = { ok: true }; const deny = (reason: string): Decision => ({ ok: false, reason }); export function decideClaims(claims: OidcClaims): Decision { - if (!ALLOWED_EVENTS.includes(claims.event_name)) { - return deny( - `event ${claims.event_name} cannot mint; allowed: ${ALLOWED_EVENTS.join(", ")}`, - ); + const mode = modeFor(claims.event_name); + if (mode === undefined) { + const allowed = [...LAND_EVENTS, ...COMMENT_EVENTS].join(", "); + return deny(`event ${claims.event_name} cannot mint; allowed: ${allowed}`); } if (claims.environment !== ENVIRONMENT) { return deny(`job must run in the "${ENVIRONMENT}" environment`); @@ -40,14 +53,43 @@ export function decideClaims(claims: OidcClaims): Decision { if (!/^\d+$/.test(claims.repository_id)) { return deny("repository_id claim is malformed"); } - if (!claims.ref.startsWith("refs/heads/")) { - return deny(`ref ${claims.ref} is not a branch`); + if (mode === "comment" && pullRequestNumber(claims.ref) === undefined) { + return deny(`ref ${claims.ref} is not a pull request merge ref`); } - return allow; + if ( + mode === "land" && + !claims.ref.startsWith("refs/heads/") && + !claims.ref.startsWith("refs/tags/") + ) { + return deny(`ref ${claims.ref} is neither a branch nor a tag`); + } + return { ok: true, mode }; +} + +function modeFor(event: string): Mode | undefined { + if (LAND_EVENTS.includes(event)) return "land"; + if (COMMENT_EVENTS.includes(event)) return "comment"; + return undefined; } export function decideBranch(ref: string, defaultBranch: string): Decision { return ref === `refs/heads/${defaultBranch}` - ? allow + ? { ok: true, mode: "land" } : deny(`ref ${ref} is not the default branch (${defaultBranch})`); } + +// Compare status of default...sha for the commit the job actually ran: +// "behind" or "identical" means it is already on the default branch. The +// tag name is never consulted, since a tag can be moved after the run starts. +export function decideTag(ref: string, compareStatus: string): Decision { + return compareStatus === "behind" || compareStatus === "identical" + ? { ok: true, mode: "land" } + : deny( + `tag ${ref} points at a commit that is not on the default branch (compare status ${compareStatus})`, + ); +} + +export function pullRequestNumber(ref: string): number | undefined { + const match = /^refs\/pull\/(\d+)\/merge$/.exec(ref); + return match ? Number(match[1]) : undefined; +} diff --git a/bot/test/github.test.ts b/bot/test/github.test.ts index c49d704..c1472ea 100644 --- a/bot/test/github.test.ts +++ b/bot/test/github.test.ts @@ -138,7 +138,15 @@ test("mintToken scopes by repository id and permissions", async () => { const { api, calls } = fakeGitHub({ "POST /app/installations/7/access_tokens": { status: 201, body: minted }, }); - assert.deepEqual(await api.mintToken(7, 12345, "jwt"), minted); + assert.deepEqual( + await api.mintToken( + 7, + 12345, + { contents: "write", pull_requests: "write" }, + "jwt", + ), + minted, + ); assert.deepEqual(calls[0].body, { repository_ids: [12345], permissions: { contents: "write", pull_requests: "write" }, @@ -177,3 +185,71 @@ test("a non-JSON error body becomes the GitHubError message", async () => { /Bad gateway/.test(e.message), ); }); + +test("compare encodes both sides and returns the status", async () => { + const { api, calls } = fakeGitHub({ + "GET /repos/acme/mylib/compare/main...v1.0.0": { + status: 200, + body: { status: "behind" }, + }, + }); + assert.deepEqual(await api.compare("acme/mylib", "main", "v1.0.0", "ghs_x"), { + status: "behind", + }); + assert.equal(calls[0].auth, "Bearer ghs_x"); +}); + +test("listComments paginates until a short page", async () => { + const full = Array.from({ length: 100 }, (_, i) => ({ + id: i, + body: "x", + html_url: "", + })); + const { api, calls } = fakeGitHub({ + "GET /repos/acme/mylib/issues/7/comments": { status: 200, body: full }, + }); + let page = 0; + const paged = githubApi((async ( + url: string | URL | Request, + init?: RequestInit, + ) => { + page++; + const body = + page === 1 + ? full + : [{ id: 100, body: "\nhi", html_url: "u" }]; + void init; + void url; + return new Response(JSON.stringify(body), { status: 200 }); + }) as typeof fetch); + const all = await paged.listComments("acme/mylib", 7, "ghs_x"); + assert.equal(all.length, 101); + assert.equal(page, 2); + void api; + void calls; +}); + +test("createComment and updateComment send the body", async () => { + const { api, calls } = fakeGitHub({ + "POST /repos/acme/mylib/issues/7/comments": { + status: 201, + body: { id: 1, body: "b", html_url: "u1" }, + }, + "PATCH /repos/acme/mylib/issues/comments/1": { + status: 200, + body: { id: 1, body: "c", html_url: "u1" }, + }, + }); + assert.equal( + (await api.createComment("acme/mylib", 7, "b", "ghs_x")).html_url, + "u1", + ); + assert.equal( + (await api.updateComment("acme/mylib", 1, "c", "ghs_x")).body, + "c", + ); + assert.deepEqual( + calls.map((c) => c.body), + [{ body: "b" }, { body: "c" }], + ); +}); diff --git a/bot/test/index.test.ts b/bot/test/index.test.ts index 3245bc9..d59850a 100644 --- a/bot/test/index.test.ts +++ b/bot/test/index.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import type { GitHubApi } from "../src/github.ts"; +import type { GitHubApi, IssueComment } from "../src/github.ts"; import { handle, type Env } from "../src/index.ts"; import { KID, @@ -29,49 +29,101 @@ test.before(async () => { interface Fake { github: GitHubApi; revoked: string[]; + minted: Record[]; + compared: string[]; + comments: IssueComment[]; + posted: { kind: "create" | "update"; body: string }[]; } function fakeGitHub( - opts: { installed?: boolean; defaultBranch?: string } = {}, + opts: { + installed?: boolean; + defaultBranch?: string; + compareStatus?: string; + comments?: IssueComment[]; + } = {}, ): Fake { - const revoked: string[] = []; - const github: GitHubApi = { - async installationFor() { - return opts.installed === false ? undefined : { id: 7 }; - }, - async mintToken(installationId, repositoryId) { - assert.equal(installationId, 7); - assert.equal(repositoryId, 12345); - return { token: "ghs_minted", expires_at: "2026-01-01T01:00:00Z" }; - }, - async repository() { - return { default_branch: opts.defaultBranch ?? "main" }; - }, - async revoke(token) { - revoked.push(token); - }, - async appSlug() { - return "soothfast-bot"; + const fake: Fake = { + revoked: [], + minted: [], + compared: [], + comments: opts.comments ?? [], + posted: [], + github: { + async installationFor() { + return opts.installed === false ? undefined : { id: 7 }; + }, + async mintToken(installationId, repositoryId, permissions) { + assert.equal(installationId, 7); + assert.equal(repositoryId, 12345); + fake.minted.push(permissions); + return { token: "ghs_minted", expires_at: "2026-01-01T01:00:00Z" }; + }, + async repository() { + return { default_branch: opts.defaultBranch ?? "main" }; + }, + async compare(_repository, _base, head) { + fake.compared.push(head); + return { status: opts.compareStatus ?? "diverged" }; + }, + async listComments() { + return fake.comments; + }, + async createComment(_repository, _issue, body) { + fake.posted.push({ kind: "create", body }); + return { + id: 99, + body, + html_url: "https://github.com/acme/mylib/pull/7#issuecomment-99", + }; + }, + async updateComment(_repository, id, body) { + fake.posted.push({ kind: "update", body }); + return { + id, + body, + html_url: `https://github.com/acme/mylib/pull/7#issuecomment-${id}`, + }; + }, + async revoke(token) { + fake.revoked.push(token); + }, + async appSlug() { + return "soothfast-bot"; + }, }, }; - return { github, revoked }; + return fake; +} + +interface Body { + token?: string; + app_slug?: string; + comment_url?: string; + reason?: string; } async function request( overrides: Parameters[0], fake: Fake, - token?: string, + opts: { route?: string; token?: string; body?: unknown } = {}, ) { const jwt = - token ?? + opts.token ?? (await signJwt( keys.privateKey, { alg: "RS256", kid: KID }, { ...claims(overrides) }, )); - const req = new Request("https://bot.example/token", { + const req = new Request(`https://bot.example${opts.route ?? "/token"}`, { method: "POST", headers: { authorization: `Bearer ${jwt}` }, + body: + opts.body === undefined + ? undefined + : typeof opts.body === "string" + ? opts.body + : JSON.stringify(opts.body), }); const deps = { jwks: async (kid: string) => (kid === KID ? keys.jwk : undefined), @@ -79,24 +131,28 @@ async function request( now: () => NOW, }; const res = await handle(req, env, deps); - return { - status: res.status, - body: (await res.json()) as { - token?: string; - app_slug?: string; - reason?: string; - }, - }; + return { status: res.status, body: (await res.json()) as Body }; } -test("allowed claims mint a scoped token", async () => { - const { status, body } = await request({}, fakeGitHub()); +const pr = { event_name: "pull_request", ref: "refs/pull/7/merge" }; +const commentBody = { + pull_request: 7, + marker: "", + body: "## soothfast gate\nok", +}; + +test("allowed claims mint a scoped landing token", async () => { + const fake = fakeGitHub(); + const { status, body } = await request({}, fake); assert.equal(status, 200); assert.deepEqual(body, { token: "ghs_minted", expires_at: "2026-01-01T01:00:00Z", app_slug: "soothfast-bot", }); + assert.deepEqual(fake.minted, [ + { contents: "write", pull_requests: "write" }, + ]); }); test("missing environment is refused before any GitHub call", async () => { @@ -108,9 +164,104 @@ test("missing environment is refused before any GitHub call", async () => { assert.match(body.reason ?? "", /environment/); }); -test("pull_request is refused", async () => { +test("a pull request run cannot obtain a token", async () => { + const fake = fakeGitHub(); + const { status, body } = await request(pr, fake); + assert.equal(status, 403); + assert.match(body.reason ?? "", /use \/comment/); + assert.deepEqual(fake.minted, []); +}); + +test("a push run cannot use /comment", async () => { + const { status, body } = await request({}, fakeGitHub(), { + route: "/comment", + body: commentBody, + }); + assert.equal(status, 403); + assert.match(body.reason ?? "", /pull_request runs only/); +}); + +test("/comment creates the marked comment with a pull_requests-only token and revokes it", async () => { + const fake = fakeGitHub(); + const { status, body } = await request(pr, fake, { + route: "/comment", + body: commentBody, + }); + assert.equal(status, 200); + assert.match(body.comment_url ?? "", /issuecomment-99/); + assert.equal(body.app_slug, "soothfast-bot"); + assert.deepEqual(fake.minted, [{ pull_requests: "write" }]); + assert.deepEqual(fake.posted, [ + { kind: "create", body: "\n## soothfast gate\nok" }, + ]); + assert.deepEqual(fake.revoked, ["ghs_minted"]); +}); + +test("/comment updates an existing marked comment and ignores others", async () => { + const fake = fakeGitHub({ + comments: [ + { id: 1, body: "\nother bot", html_url: "" }, + { id: 2, body: "\nold", html_url: "" }, + ], + }); + const { status } = await request(pr, fake, { + route: "/comment", + body: commentBody, + }); + assert.equal(status, 200); + assert.deepEqual(fake.posted, [ + { kind: "update", body: "\n## soothfast gate\nok" }, + ]); +}); + +test("/comment refuses another pull request's number", async () => { + const fake = fakeGitHub(); + const { status, body } = await request(pr, fake, { + route: "/comment", + body: { ...commentBody, pull_request: 8 }, + }); + assert.equal(status, 403); + assert.match(body.reason ?? "", /not the one this run belongs to/); + assert.deepEqual(fake.posted, []); + assert.deepEqual(fake.minted, [], "refused before any mint"); +}); + +test("/comment validates its body", async () => { + const fake = fakeGitHub(); + assert.equal( + (await request(pr, fake, { route: "/comment", body: "nope" })).status, + 400, + ); + assert.equal( + ( + await request(pr, fake, { + route: "/comment", + body: { pull_request: "7" }, + }) + ).status, + 400, + ); + const badMarker = { ...commentBody, marker: "no marker" }; + assert.equal( + (await request(pr, fake, { route: "/comment", body: badMarker })).status, + 400, + ); + const huge = { ...commentBody, body: "x".repeat(70000) }; + assert.equal( + (await request(pr, fake, { route: "/comment", body: huge })).status, + 400, + ); + assert.equal( + (await request(pr, fake, { route: "/comment", body: "null" })).status, + 400, + ); + assert.deepEqual(fake.posted, []); + assert.deepEqual(fake.minted, [], "a rejected body must not cost a mint"); +}); + +test("pull_request_target is refused", async () => { const { status } = await request( - { event_name: "pull_request" }, + { event_name: "pull_request_target", ref: "refs/pull/7/merge" }, fakeGitHub(), ); assert.equal(status, 403); @@ -130,6 +281,32 @@ test("a non-default branch is refused and the minted token revoked", async () => assert.deepEqual(fake.revoked, ["ghs_minted"]); }); +test("a tag is judged by the commit that ran, not the tag name", async () => { + const fake = fakeGitHub({ compareStatus: "behind" }); + const { status, body } = await request({ ref: "refs/tags/v1.0.0" }, fake); + assert.equal(status, 200); + assert.equal(body.token, "ghs_minted"); + assert.deepEqual(fake.compared, [claims().sha]); +}); + +test("a tag whose commit is off the default branch is refused and revoked", async () => { + const fake = fakeGitHub({ compareStatus: "diverged" }); + const { status, body } = await request({ ref: "refs/tags/v1.0.0" }, fake); + assert.equal(status, 403); + assert.match(body.reason ?? "", /not on the default branch/); + assert.deepEqual(fake.revoked, ["ghs_minted"]); +}); + +test("a failing revoke on the wrong branch still refuses with 403", async () => { + const fake = fakeGitHub({ defaultBranch: "master" }); + fake.github.revoke = async () => { + throw new Error("network down"); + }; + const { status, body } = await request({}, fake); + assert.equal(status, 403); + assert.match(body.reason ?? "", /not the default branch/); +}); + test("an invalid signature is 401", async () => { const other = await generateKeys(); const forged = await signJwt( @@ -137,7 +314,7 @@ test("an invalid signature is 401", async () => { { alg: "RS256", kid: KID }, { ...claims() }, ); - const { status, body } = await request({}, fakeGitHub(), forged); + const { status, body } = await request({}, fakeGitHub(), { token: forged }); assert.equal(status, 401); assert.match(body.reason ?? "", /signature/); }); @@ -147,20 +324,16 @@ test("other routes are 404 and missing bearer is 401", async () => { const deps = { jwks: async () => undefined, github: fake.github }; const get = await handle(new Request("https://bot.example/token"), env, deps); assert.equal(get.status, 404); + const other = await handle( + new Request("https://bot.example/other", { method: "POST" }), + env, + deps, + ); + assert.equal(other.status, 404); const noAuth = await handle( - new Request("https://bot.example/token", { method: "POST" }), + new Request("https://bot.example/comment", { method: "POST" }), env, deps, ); assert.equal(noAuth.status, 401); }); - -test("a failing revoke on the wrong branch still refuses with 403", async () => { - const fake = fakeGitHub({ defaultBranch: "master" }); - fake.github.revoke = async () => { - throw new Error("network down"); - }; - const { status, body } = await request({}, fake); - assert.equal(status, 403); - assert.match(body.reason ?? "", /not the default branch/); -}); diff --git a/bot/test/keys.ts b/bot/test/keys.ts index 9155322..29a405f 100644 --- a/bot/test/keys.ts +++ b/bot/test/keys.ts @@ -51,6 +51,7 @@ export function claims(overrides: Partial = {}): OidcClaims { repository: "acme/mylib", repository_id: "12345", ref: "refs/heads/main", + sha: "0123456789abcdef0123456789abcdef01234567", event_name: "push", environment: ENVIRONMENT, ...overrides, diff --git a/bot/test/policy.test.ts b/bot/test/policy.test.ts index 641ce89..94e4cbf 100644 --- a/bot/test/policy.test.ts +++ b/bot/test/policy.test.ts @@ -1,34 +1,59 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { decideBranch, decideClaims } from "../src/policy.ts"; +import { + decideBranch, + decideClaims, + decideTag, + pullRequestNumber, +} from "../src/policy.ts"; import { claims } from "./keys.ts"; const cases = [ { - name: "push to a branch in the environment is allowed", + name: "push to a branch in the environment lands", overrides: {}, - reason: undefined, + mode: "land", }, { - name: "workflow_dispatch is allowed", + name: "workflow_dispatch lands", overrides: { event_name: "workflow_dispatch" }, - reason: undefined, + mode: "land", }, { - name: "schedule is allowed", + name: "schedule lands", overrides: { event_name: "schedule" }, - reason: undefined, + mode: "land", }, { - name: "pull_request is refused", - overrides: { event_name: "pull_request" }, - reason: /event pull_request cannot mint/, + name: "push of a tag is a landing candidate", + overrides: { ref: "refs/tags/v1.0.0" }, + mode: "land", + }, + { + name: "pull_request on a merge ref comments", + overrides: { event_name: "pull_request", ref: "refs/pull/7/merge" }, + mode: "comment", }, { name: "pull_request_target is refused", - overrides: { event_name: "pull_request_target" }, + overrides: { event_name: "pull_request_target", ref: "refs/pull/7/merge" }, + reason: /cannot mint/, + }, + { + name: "issue_comment is refused", + overrides: { event_name: "issue_comment" }, reason: /cannot mint/, }, + { + name: "pull_request on a branch ref is refused", + overrides: { event_name: "pull_request" }, + reason: /not a pull request/, + }, + { + name: "push on a pull ref is refused", + overrides: { ref: "refs/pull/7/merge" }, + reason: /neither a branch nor a tag/, + }, { name: "missing environment is refused", overrides: { environment: undefined }, @@ -39,11 +64,6 @@ const cases = [ overrides: { environment: "production" }, reason: /"soothfast-bot" environment/, }, - { - name: "tag ref is refused", - overrides: { ref: "refs/tags/v1.0.0" }, - reason: /not a branch/, - }, { name: "malformed repository is refused", overrides: { repository: "acme" }, @@ -59,8 +79,8 @@ const cases = [ for (const c of cases) { test(c.name, () => { const decision = decideClaims(claims(c.overrides)); - if (c.reason === undefined) { - assert.deepEqual(decision, { ok: true }); + if ("mode" in c) { + assert.deepEqual(decision, { ok: true, mode: c.mode }); } else { assert.equal(decision.ok, false); assert.match(decision.ok ? "" : decision.reason, c.reason); @@ -69,7 +89,10 @@ for (const c of cases) { } test("default branch matches", () => { - assert.deepEqual(decideBranch("refs/heads/main", "main"), { ok: true }); + assert.deepEqual(decideBranch("refs/heads/main", "main"), { + ok: true, + mode: "land", + }); }); test("feature branch is not the default branch", () => { @@ -84,3 +107,31 @@ test("feature branch is not the default branch", () => { test("default branch name is not a prefix match", () => { assert.equal(decideBranch("refs/heads/main-old", "main").ok, false); }); + +test("a tag whose commit is on the default branch lands", () => { + assert.deepEqual(decideTag("refs/tags/v1.0.0", "behind"), { + ok: true, + mode: "land", + }); + assert.deepEqual(decideTag("refs/tags/v1.0.0", "identical"), { + ok: true, + mode: "land", + }); +}); + +test("a tag ahead of or diverged from the default branch is refused", () => { + for (const status of ["ahead", "diverged"]) { + const decision = decideTag("refs/tags/v1.0.0", status); + assert.equal(decision.ok, false); + assert.match( + decision.ok ? "" : decision.reason, + /not on the default branch/, + ); + } +}); + +test("pullRequestNumber reads the merge ref only", () => { + assert.equal(pullRequestNumber("refs/pull/7/merge"), 7); + assert.equal(pullRequestNumber("refs/pull/7/head"), undefined); + assert.equal(pullRequestNumber("refs/heads/main"), undefined); +}); diff --git a/docs/ci.md b/docs/ci.md index 03d0fa7..caaae74 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -39,12 +39,13 @@ Two one-time settings, no secrets: Because the job names an environment, every run of it, pull request or push, appears under Environments and Deployments and on the pull request as -"deployed to soothfast-bot". If you would rather keep pull requests out of -the environment, split the step into two jobs with the same `uses:` line: -the gate job on `pull_request` without an environment and with `changelog: -false`, and a regeneration job on `push` with the environment and `gate: -false`. That split also lets you put a default-branch policy on the -environment. +"deployed to soothfast-bot". If you would rather keep pull requests out of the environment, split the +step into two jobs with the same `uses:` line: the gate job on +`pull_request` without an environment and with `changelog: false`, and a +regeneration job on `push` with the environment and `gate: false`. That split +lets you put a default-branch policy on the environment; its cost is that +gate comments are then posted by github-actions rather than soothfast-bot, +since only a job in the environment can obtain a bot token. Two repository settings decide what happens to the bot's pull request: @@ -72,10 +73,13 @@ soothfast gate -p PKG --against-ref origin/` and appends the output to one comment on the pull request, updated in place on later pushes. The comment shows the last forty lines per package. On a regression it uploads `.soothfast/triage/` as the `soothfast-triage` artifact and fails the step. -The comment is posted with `github.token`, so on a pull request from a fork, -where that token is read-only, the comment is skipped with a warning and the -gate result still decides the step. A fork pull request never receives a bot -token. +The comment is posted as soothfast-bot by the broker itself: the job sends +the text over its OIDC identity and never holds a token, so a pull request +branch, which runs code nobody has merged, cannot borrow the bot for +anything else. A pull request from a fork has no OIDC identity; there the +comment falls back to `github.token` (github-actions), or is skipped with a +warning where that token is read-only, and the gate result still decides the +step. **On a push to the default branch.** It measures each package into the `baseline` baseline, regenerates `CHANGELOG.md` against the latest tag (or @@ -97,11 +101,14 @@ The step needs `id-token: write` to prove its identity to the broker. That permission is also common on jobs that publish to crates.io, PyPI, or a cloud provider over OIDC. Requiring the `soothfast-bot` environment means only a job that opts in can obtain a bot token; a compromised action in one of those -other jobs cannot. The broker also refuses any event other than `push`, -`workflow_dispatch`, or `schedule`, any ref other than your default branch, -and any repository the App is not installed on, and it scopes the token it -mints to your repository for one hour. The token is revoked when the step -finishes. +other jobs cannot. The broker hands out one kind of token, scoped to your repository for one +hour and revoked when the step finishes: a landing token (contents and pull +requests write) for `push`, `workflow_dispatch`, or `schedule` runs on your +default branch, or on a tag whose commit is already on it. A `pull_request` +run from the repository itself gets no token at all; it asks the broker to +post the gate comment, and the broker does so with a token it holds and +revokes itself. Every other event, ref, or repository the App is not +installed on is refused. ## Inputs From f2ddd0cc2090ef1d00808b11dc8110e1177bb407 Mon Sep 17 00:00:00 2001 From: Harvey Tseng Date: Sat, 5 Sep 2026 22:08:17 -0400 Subject: [PATCH 2/2] fix: name the missing broker secrets in the 500 --- bot/src/index.ts | 7 +++++++ bot/test/index.test.ts | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/bot/src/index.ts b/bot/src/index.ts index 5730efb..4321b25 100644 --- a/bot/src/index.ts +++ b/bot/src/index.ts @@ -65,6 +65,13 @@ export async function handle( env: Env, deps: Deps, ): Promise { + if (!env.GITHUB_APP_CLIENT_ID || !env.GITHUB_APP_PRIVATE_KEY) { + console.error("GITHUB_APP_CLIENT_ID or GITHUB_APP_PRIVATE_KEY is not set"); + return json(500, { + reason: + "broker is not configured: set GITHUB_APP_CLIENT_ID and GITHUB_APP_PRIVATE_KEY", + }); + } const route = request.method === "POST" ? new URL(request.url).pathname : ""; if (route !== "/token" && route !== "/comment") return json(404, { reason: "not found" }); diff --git a/bot/test/index.test.ts b/bot/test/index.test.ts index d59850a..b9d971f 100644 --- a/bot/test/index.test.ts +++ b/bot/test/index.test.ts @@ -337,3 +337,18 @@ test("other routes are 404 and missing bearer is 401", async () => { ); assert.equal(noAuth.status, 401); }); + +test("a broker without its secrets says so", async () => { + const fake = fakeGitHub(); + const deps = { jwks: async () => undefined, github: fake.github }; + const res = await handle( + new Request("https://bot.example/token", { method: "POST" }), + { GITHUB_APP_CLIENT_ID: "", GITHUB_APP_PRIVATE_KEY: "" }, + deps, + ); + assert.equal(res.status, 500); + assert.match( + ((await res.json()) as { reason: string }).reason, + /not configured/, + ); +});