From 3b49c1801e39097854c8aa7e4919541e1d35f0da Mon Sep 17 00:00:00 2001 From: Derek Lewis Date: Sat, 15 Aug 2026 04:51:53 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=8F=97=EF=B8=8F=F0=9F=94=A7=EF=BC=9Akeep?= =?UTF-8?q?=20the=20branch=20under=20review=20off=20the=20runner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A semgrep audit rule flagged this workflow for checking out code from the incoming pull request. It does not: the ref was `github.event.pull_request.base.ref`, the base branch. The rule matches any ref beginning `github.event.pull_request`, so it cannot tell the base from the head. The finding was still worth acting on. The branch under review was fetched so its commit messages could be read, which put a stranger's objects on a runner holding credentials that can write here, with nothing but a comment to stop a later step executing them. Those messages now come from the API, so nothing of the branch arrives at all, and the checkout takes the default rather than naming a ref that reads like the wrong one. The token is no longer left in .git/config either. Nothing here pushes. Signed-off-by: Derek Lewis Assisted-by: Claude-Code:claude-opus-5 --- .github/workflows/commit-queue.yml | 31 +++++++++++++++--------------- build/tasks/land-pull-request.mts | 31 ++++++++++++++++-------------- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/.github/workflows/commit-queue.yml b/.github/workflows/commit-queue.yml index 89628fd47..f9db4b0be 100644 --- a/.github/workflows/commit-queue.yml +++ b/.github/workflows/commit-queue.yml @@ -1,11 +1,13 @@ # Landing a pull request when it is labelled `commit-queue`. # # `pull_request_target` runs in the context of the base branch and can reach -# secrets, which `pull_request` cannot do for a fork. That is only safe -# because nothing here checks out or executes the pull request's code: the -# checkout below is the base branch, and the branch under review is fetched -# only so that its commit messages can be read. Never add a build, an install -# or a test step to this workflow -- those belong in the checks it waits for, +# secrets, which `pull_request` cannot do for a fork. That is only safe while +# nothing from the pull request reaches this runner, and nothing does: the +# checkout is this repository at the base branch, and the commit messages +# being read come from the API rather than from a fetch. +# +# Never add a build, an install or a test step here, and never check out the +# branch under review. Those belong in the checks this workflow waits for, # which run without a token that can write anything. # # Actions are pinned by commit, never by tag. @@ -37,24 +39,23 @@ jobs: app-id: ${{ secrets.LAND_APP_ID }} private-key: ${{ secrets.LAND_APP_PRIVATE_KEY }} - - name: Check out the base branch + # No `ref:`. For this event the default is already the base branch, and + # naming it explicitly, even as `base.ref`, is indistinguishable to a + # reader -- and to a scanner -- from naming the branch under review. + # Nothing from that branch is fetched at all: its commit messages are + # asked of the API, so a stranger's code never reaches this runner. + - name: Check out this repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - # The base, deliberately, not the pull request. Full history so that - # the range between the two can be read. - ref: ${{ github.event.pull_request.base.ref }} - fetch-depth: 0 + # Nothing here pushes, and a token left in .git/config is one more + # thing that could be picked up by something that should not have it. + persist-credentials: false - name: Set up Node.js runtime uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version-file: 'package.json' - - name: Fetch the commits under review - env: - NUMBER: ${{ github.event.pull_request.number }} - run: git fetch --quiet origin "pull/${NUMBER}/head" - - name: Land it id: land env: diff --git a/build/tasks/land-pull-request.mts b/build/tasks/land-pull-request.mts index 23702ea0c..5e2f8819e 100644 --- a/build/tasks/land-pull-request.mts +++ b/build/tasks/land-pull-request.mts @@ -225,19 +225,22 @@ try { console.error(`#${number} cannot be landed: ${reason}.`); process.exitCode = 1; } else { - // Oldest first, so the landed message reads in the order the work was - // done rather than the order git lists it. - const shas = git( - 'rev-list', - '--reverse', - '--no-merges', - `origin/${defaultBranch()}..${pull.head}` - ) - .split('\n') - .filter(Boolean); - const parts = shas.map((sha) => - partsOfMessage(git('log', '-1', '--format=%B', sha)) + // Asked of the API rather than of git, so that the branch under review + // never has to be fetched. Running as `pull_request_target`, this holds + // credentials that can write to the repository, and the safest thing to + // do with a stranger's commits is to not have them on disk at all. + // GitHub returns them oldest first, which is the order to read them in, + // and merges are dropped since their messages say nothing. + const messages: string[] = JSON.parse( + gh( + 'api', + '--paginate', + `repos/${repository()}/pulls/${number}/commits`, + '--jq', + '[.[] | select(.parents | length < 2) | .commit.message]' + ) ); + const parts = messages.map((message) => partsOfMessage(message)); const message = composeLandingMessage( parts, `https://github.com/${repository()}/pull/${number}` @@ -247,10 +250,10 @@ try { console.log(`${rule}\n${pull.title}\n\n${message}\n${rule}`); console.log( - `#${number}: ${shas.length} commit${shas.length === 1 ? '' : 's'}, ${pull.mergeableState}` + `#${number}: ${messages.length} commit${messages.length === 1 ? '' : 's'}, ${pull.mergeableState}` ); - if (shas.length === 0) { + if (messages.length === 0) { console.error(`\n#${number} has no commits over ${defaultBranch()}.`); process.exitCode = 1; } else if (problems.length > 0) {