Drop the transitional pull_request trigger #86

Merged

branderson merged 2 commits from trigger-followup into main 2026-08-05 21:33:11 -07:00

Conversation 1 Commits 2 Files Changed 1

+20 -16

dejima-agent commented 2026-08-05 20:49:28 -07:00

Collaborator

Copy Link

Copy Source

Summary generated by LLM agent

Merge immediately after #84. Stacked on it, and until #84 lands this will
show that PR's commits too.

#84 carries both triggers on purpose: pull_request_target only takes effect
once it is on the base branch, so the change introducing it cannot be covered
by it. Keeping pull_request alongside meant #84 was still tested by its own
copy from the head.

That is now true, so pull_request comes out. Leaving it in place would keep
taking an untrusted run from the head on every pull request, with runs-on
chosen by the branch — the exact thing pull_request_target exists to
prevent. The window between the two merges is the only time both run, which is
why this wants merging promptly rather than eventually.

This change is itself covered by the copy already on main, which is the
point of the two-step: the first merge makes the trigger real, the second
removes the one it replaced, and neither lands without checks.

After this merges, the static.bats guard goes back to refusing
pull_request outright — it currently permits the pair only while the
transitional note is present, so removing the note without removing the
trigger fails rather than letting a temporary state become permanent.

**Summary generated by LLM agent** **Merge immediately after #84.** Stacked on it, and until #84 lands this will show that PR's commits too. #84 carries both triggers on purpose: `pull_request_target` only takes effect once it is on the base branch, so the change introducing it cannot be covered by it. Keeping `pull_request` alongside meant #84 was still tested by its own copy from the head. That is now true, so `pull_request` comes out. Leaving it in place would keep taking an untrusted run from the head on every pull request, with `runs-on` chosen by the branch — the exact thing `pull_request_target` exists to prevent. The window between the two merges is the only time both run, which is why this wants merging promptly rather than eventually. This change is itself covered by the copy already on `main`, which is the point of the two-step: the first merge makes the trigger real, the second removes the one it replaced, and neither lands without checks. After this merges, the `static.bats` guard goes back to refusing `pull_request` outright — it currently permits the pair only while the transitional note is present, so removing the note without removing the trigger fails rather than letting a temporary state become permanent.

dejima-agent added 9 commits 2026-08-05 20:49:29 -07:00

ci: disclose the risk, verify our own runners, and point at dejima-ci-hardener ... aa7fe3c2d3

Replaces the earlier attempt to ship a CI hardening bundle from this
repository. That approach required copying files out of a dejima checkout onto
a CI box, which is backwards: a runner is a machine whose purpose is executing
untrusted code, and nothing about dejima belongs on it. dejima-ci-hardener is
self-contained and owns the how; this repository keeps the disclosure and
links out.

What stays here is what only this repository can say or check.

docs/ci.md keeps the threat model: what a run adds over a sandbox - execution
on a host you own, egress from a network position you own, credentials the run
is handed - and why no approval gate is offered, since every candidate is
reachable from the branch, covered by a credential the session already holds,
or not a gate at all. It keeps the reasoning behind the controls, including
that the allowlist has to come from the branch the operator controls rather
than the checkout. It no longer carries the configuration: two copies of a
firewall rule drift, and one of them had already needed the same fix twice.

test/ci-runner.bats stays because it is this repository's CI asserting its own
runners, which is nobody else's business to run. Both failures it covers look
exactly like success from inside a job: nothing in a job's log mentions
whether the Docker socket was mounted into it, and a job that can reach an
off-list resolver still passes every check that only inspects DOCKER-USER. Two
jobs, one per runner, with the tier declared by the workflow rather than
sniffed, and every negative check behind a positive control - an allowlisted
destination must resolve and connect first, or a runner with no network at all
passes the lot.

The sandbox matrix moves to the dejima-ci label so those jobs cannot land on a
runner that grants everything, and host-docker keeps dejima-runtime.

runtime.bats stops polling a container that has already exited, so an aborted
entrypoint surfaces at once rather than after the full two-minute wait.

With ci/ gone, static.bats loses the twelve guards over its files - they move
with what they guarded - and the leak scanner returns to its strict form,
since the private-subnet example that needed a carve-out went with them.

docs: use the real dejima-ci-hardener URL ... ca6d9805b2

The placeholder was in place while that repository was private.

docs: rewrap a line the link substitution ran together 1c4dfc39e4

docs: a runner label routes jobs, it does not contain them ... b474acac0a

This document said that naming a dedicated label meant jobs "cannot land
anywhere else". That is true of a branch that is not trying, and false of one
that is - which is the distinction the rest of the document is built on, so it
was the wrong claim to make here of all places.

runs-on is in the workflow file and the workflow file is in the branch. A
session can change runs-on: dejima-ci to runs-on: ubuntu-latest, push, and
open a pull request; if any runner answering that label will take the job, it
has stepped outside every control described here - no allowlist, no socket
restriction, no firewall - without touching anything else.

Whether one will is a property of where runners are registered, which is the
actual boundary. Gitea's own FAQ is explicit that there is no per-repository
runner ACL and that registration scope is the control instead: an
instance-level runner serves every repository on the instance, and gitea.com
registers its own for a single organization so they will not run other
repositories' jobs.

So the property worth holding is that a repository's CI posture is that of the
weakest runner it can reach, not the one its workflow names. The runner-host
section framed registration scope as keeping other repositories off your
hardened runner; the converse - keeping your repository off nobody's hardened
runner - is the direction that bites, and is now stated first.

Also recorded: the runner-environment tests do not close this either, since
they are jobs in the same file and a branch redirecting runs-on can drop them
in the same commit. They catch a misconfigured runner, not a redirected job.
What survives the branch is the run log, which records which runner took each
job, and a required status check, which stops such a pull request being
merged. Both are after the fact, which is this document's whole point.

static.bats pins the correction, including that the old phrasing does not
return.

ci: read this workflow from main, not from the branch under test ... 10a52c2dde

Under `on: pull_request` the workflow is read from the head of the pull
request, so the branch decides what runs and - the part that matters - where.
A branch can point its jobs at any runner its repository can reach, which
makes every control the runner provides opt-out by whoever wrote the branch.
The dedicated label was routing, not containment.

`pull_request_target` reads it from the base branch instead. Gitea does this
explicitly: it resolves the base commit, detects workflows there, and takes
only the pull_request_target ones - while discarding any pull_request_target
workflow found on the head commit. So a branch can neither edit this file's
copy nor introduce its own, and runs-on is whatever main says.

The conversion has a trap, and it fails green. The run's ref becomes the base
branch, so a bare checkout fetches main and every suite passes without seeing
the change. Both suites that test the branch now name the head sha explicitly.

The runner-environment jobs keep the bare checkout, deliberately and now with
a comment saying so: they assert on the runner rather than on the change, so
their assertions should come from the base branch - which also means a branch
can neither weaken nor delete them.

This does not make the repository safe by itself, and docs/ci.md says so where
it explains why: a branch can still ADD a workflow with on: pull_request, and
that runs from the head with any runs-on it likes. Only which runners the
repository can reach stops that. What this buys is that the intended CI is
trustworthy, and that escaping it requires adding a file rather than editing
one - a far louder diff.

Recorded alongside it, because it changes what that remaining fix costs: a
runner can belong to an OWNER, a user as well as an organization, and then
serves all of that owner's repositories. Gitea's runner model names the three
scopes directly. So an instance with sixty personal repositories and two
general-purpose runners needs two re-registrations at user scope rather than
sixty at repository scope, with the repositories that receive agent-authored
branches living under a different owner whose reachable runners are all
hardened.

static.bats pins the trigger, that pull_request does not return, and the
checkout split - two jobs on the head sha, four checkouts in total, so a
bare checkout added to a branch-testing job fails here rather than passing
green in CI.

docs: make the CI advisory answerable without moving a repository ... 49d904bed7

The advisory had drifted into being about hardening a runner you already
operate, with "move the repositories somewhere with only hardened runners" as
the way to close what hardening cannot. For a public project that means
breaking its name, its links and its ownership, which is not a thing to ask
of anyone - and it left users whose projects have no CI believing none of this
applied to them.

Both are now addressed by asking a different question first: not "how do I
harden my runner" but "what runners can this repository's branches reach". The
rule is stated once - every reachable runner must be one you would accept an
attacker running code on - and everything else follows from it.

That framing makes the no-CI case visible, which is the one nobody expects. A
repository with no workflows is not safe by virtue of having none, because a
branch adds the first one; the exposure follows from reachability, which is a
property of the forge rather than of the project. Said in the README too,
since that is as far as many readers get.

It also separates the mild case from the sharp one instead of alarming
everybody equally. On hosted runners the machine is disposable and is not
yours, so the "execution on a host you own" item largely evaporates and what
remains is credentials - worth saying plainly rather than leaving a reader to
assume the worst applies to them.

For the sharp case there are now three answers that move nothing:

Disable the Actions unit where a project does not use CI. No workflow runs at
all, so nothing is reachable and a branch cannot add the first one. Re-enabling
needs admin - Gitea puts repository settings behind reqAdmin() - so a bot
account with Write cannot undo it. Free where CI was not being used, and the
most commonly overlooked.

Scope the dangerous runner down instead of moving the repository out. A runner
can belong to an owner, a user as well as an organization; make the hardened
one the owner-level default and register the permissive one at repository
scope for the few repositories that genuinely need a socket. Most repositories
need to run tests, not build images.

Or decline the push grant, which closes the vector rather than bounding it and
is the only answer that does not depend on getting a forge's configuration
right.

static.bats pins the no-CI claim in both files and that the three
move-nothing answers are present, since losing any of them returns the
document to advice a public project cannot take.

ci: carry both triggers through the first merge ... 76f61e77c0

pull_request_target is read from the base branch, and Gitea explicitly ignores
a pull_request_target workflow found on a pull request's head. So the change
that introduces it cannot be tested by it: with only pull_request_target here
and only pull_request on main, the head's copy is skipped, the base has
nothing to contribute, and no checks run at all - which reads as a broken
workflow rather than as the bootstrap problem it is.

Keeping pull_request alongside it means this change is tested by its own copy
from the head, the way every pull request is today. Once merged, main carries
pull_request_target and a one-line follow-up removes pull_request - and that
follow-up is covered by the copy on main, so CI is continuous across both.

Between the two merges pull requests get both runs: untrusted from the head,
trusted from the base. That is noisy, and for the untrusted half it is the
status quo rather than a regression, so the window wants to be short.

The static guard now allows the pair only while the transitional note is
present, so dropping the note without dropping the trigger fails rather than
letting a temporary state become permanent. docs/ci.md records the two-merge
adoption, since anyone following this hits the same silence.

ci: say why a job cannot resolve, before apt says something else ... 14fde86f68

A job on a runner behind the egress allowlist fails at `apt-get update` with
"Temporary failure resolving" and then "Unable to locate package bats". None
of that is about apt, and none of it names the resolver or the allowlist -
which is where the fault actually is.

The suite that would say so precisely, ci-runner.bats, cannot run: installing
it is the step that just failed. So the diagnosis has to come before the
install, from what the image already has - getent is libc, and the resolver's
address is in resolv.conf.

The step now names the resolver the job was given and lists what to check in
order, including the case that produces exactly this symptom and looks like
nothing else: an empty allowlist. With no-resolv and no server= lines there is
no upstream for any name, so every lookup is refused and every job dies in apt
regardless of what it was going to install.

ci: drop the transitional pull_request trigger ... b663d51678

The pair existed so the change introducing pull_request_target could be tested
by its own copy from the head - pull_request_target only takes effect once it
is on the base branch, so that change could not be covered by it.

That is now true, so pull_request comes out. Leaving it would keep taking an
untrusted run from the head on every pull request, with runs-on chosen by the
branch, which is the thing pull_request_target exists to prevent.

This change is itself covered by the copy already on main, which is the point
of the two-step: the first merge makes the trigger real, the second removes
the one it replaced, and neither lands without checks.

branderson added 1 commit 2026-08-05 21:18:49 -07:00

Merge branch 'main' into trigger-followup 0bc660b8d1

branderson approved these changes 2026-08-05 21:32:47 -07:00

branderson merged commit 56c1ce90c3 into main 2026-08-05 21:33:11 -07:00

branderson referenced this issue from a commit 2026-08-05 21:33:12 -07:00

Merge pull request 'Drop the transitional pull_request trigger' (#86) from trigger-followup into main

dejima-agent referenced this pull request 2026-08-05 22:07:06 -07:00

Keep one copy of the pull_request_target rationale #87

Sign in to join this conversation.

Reviewers

No Reviewers

branderson

Labels

Clear labels

Compat/Breaking

Breaking change that won't be backward compatible

Kind/Bug

Something is not working

Kind/Documentation

Documentation changes

Kind/Enhancement

Improve existing functionality

Kind/Feature

New functionality

Kind/Security

This is security issue

Kind/Testing

Issue or pull request related to testing

Priority

Critical

1

The priority is critical

Priority

High

2

The priority is high

Priority

Low

4

The priority is low

Priority

Medium

3

The priority is medium

Reviewed

Confirmed

1

Issue has been confirmed

Reviewed

Duplicate

2

This issue or pull request already exists

Reviewed

Invalid

3

Invalid issue

Reviewed

Won't Fix

3

This issue won't be fixed

Status

Abandoned

3

Somebody has started to work on this but abandoned work

Status

Blocked

1

Something is blocking this issue or pull request

Status

Need More Info

2

Feedback is required to reproduce issue or to continue work

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

branderson dejima-agent

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dejima/dejima#86