Skip to content

chore(ci): take fork-reachable jobs off the self-hosted runners (ENG-2017) - #5

Open
lucas-koontz wants to merge 3 commits into
mainfrom
fix/eng-2017-fork-jobs-off-self-hosted-runners
Open

chore(ci): take fork-reachable jobs off the self-hosted runners (ENG-2017)#5
lucas-koontz wants to merge 3 commits into
mainfrom
fix/eng-2017-fork-jobs-off-self-hosted-runners

Conversation

@lucas-koontz

@lucas-koontz lucas-koontz commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

User story

As an engineer who reviews external contributions to this repository
I want approving CI on a stranger's pull request to be a harmless act
So that a plausible-looking patch cannot execute its own code on a pod inside our clusters

Why this matters

Five jobs in build_deploy_dev.yml already carry if: !github.event.pull_request.head.repo.fork, so the threat was understood when it was written. Four did not, and they sat on mdb-dev, which is a pod inside our newdev EKS cluster holding a Kubernetes identity, an AWS role and a filesystem other repositories' credentialed jobs write into.

The one that matters is code_checking inside tests_unit.yml. It checks out the pull request's head, runs uv run tests/scripts/check_print_statements.py from that tree, and then runs pre-commit against the config in that tree. A fork owns all three, so an approved fork pull request got arbitrary command execution on the pod without editing a workflow file at all.

A fork could also pick its own runner label without touching a workflow file. check_install and unit_tests use runs-on: ${{ matrix.runs_on }}, and matrix.runs_on comes from .github/workflows/matrix_includes.json in the pull request's own checkout. Moving code_checking off the runner does not close that on its own.

Separately, cla.yml ran on mdb-dev too, and its triggers run in base-repo context, so no approval gate applied at all.

What happens today

flowchart TD
    F["Fork pull request"] --> APP{"Someone with push clicks Approve and run"}
    APP --> C["changes, tests_completed on mdb-dev"]
    APP --> U["code_checking on mdb-dev"]
    U --> X["Runs the fork's script and pre-commit config"]
    APP --> M["matrix_prep reads the fork's matrix_includes.json"]
    M --> R["runs-on takes whatever label that file names"]
Loading

What should happen

flowchart TD
    F["Fork pull request"] --> APP{"Someone with push clicks Approve and run"}
    APP --> C["changes, tests_completed, code_checking on ubuntu-latest"]
    APP --> M["matrix_prep on ubuntu-latest"]
    M --> V{"Every runs_on a GitHub-hosted label?"}
    V -->|"no"| FAIL["Job fails and names the label"]
    V -->|"yes"| OK["Matrix jobs run on hosted runners"]
Loading

Acceptance criteria

  • No job reachable from a fork pull request requests mdb-dev, and every job that still does carries an explicit fork guard.
  • A fork editing matrix_includes.json to name a self-hosted label fails matrix_prep rather than obtaining a runner.
  • The CLA job no longer requests mdb-dev and skips outright for a comment that is not a CLA comment.
  • Negative: a first-party pull request still filters, builds, deploys, tests and reports exactly as before.
  • Negative: an external contribution still gets CI, rather than a wall of skipped jobs.

How to test

  1. Open a first-party pull request. changes, code_checking, matrix_prep and tests_completed run on ubuntu-latest; get-deploy-labels, build, scan-keycloak and build-cache behave exactly as before.
  2. Open a pull request from a fork that edits .github/workflows/matrix_includes.json to name mdb-dev, approve the run, and confirm matrix_prep fails with the label named in the error and no downstream job starts.
  3. Open a pull request from a fork using an account not in the CLA allowlist and confirm the CLA job runs on a hosted runner and asks for a signature.
  4. Merge to main and confirm the staging build and deploy pipeline, which calls the same tests_unit.yml, is unaffected.

Notes for the reviewer

The green CLAssistant row on this pull request does not exercise this change. pull_request_target loads the workflow from the base branch, so the check that passed here is the copy this PR replaces, running on mdb-dev. The new job cannot test itself on its own pull request; it is exercised on the first pull request opened after this merges.

Moved rather than skipped, and that is the judgement call here. changes gates every other job, so guarding it would skip the whole pipeline for a fork. tests_completed is the single collection job a branch rule points at, so skipping it leaves a fork pull request with an ambiguous check. Both are trivial and need nothing from the cluster, so a hosted runner is the answer and external contributions keep getting CI.

code_checking swaps the FSx cache path for the hosted cache. cache-local-path: /home/runner/_work/_tool/uv-local-cache is the volume the self-hosted runners mount; on a hosted runner it is an empty directory that does not survive the job. enable-cache: true is what check_install and unit_tests in the same file already use. I confirmed from a real job log that setup-uv creates and activates the venv itself, so uv pip install behaves identically off the cluster.

The runner allowlist says why it is an allowlist. The jq subtracts a fixed set of GitHub-hosted labels rather than matching our own self-hosted ones, so a label nobody has thought of yet fails rather than passes. That is the right direction and it is invisible from the code, so the next person adding a hosted image to matrix_includes.json now finds a comment saying the allowlist is where to add it.

The matrix validator fails loudly rather than dropping the entry. A fork that names a self-hosted label has written something invalid, and a silent drop reads as "your tests passed". It handles the list form of runs-on and a missing runs_on on purpose: my first version crashed jq on a list and reported nothing, which would have passed a hostile input.

scan-keycloak gains the guard its four neighbours carry. It was already unreachable for a fork, but only through a needs edge on a guarded job. One reordering of needs would have made it reachable with nothing in the file saying so.

The CLA change is the same one landing in nine sibling repositories, replacing a hand-rolled job with the reusable anton, cowork and cowork-server already call.

All Tests Succeeded is red on this PR and it is red on base. Its step fails when run_integration_tests is not success on a first-party pull request, and run_integration_tests is skipped whenever deploy is skipped, which is whenever the PR carries no deploy-to-* label. So every first-party PR without a deploy label fails this check. Proof it predates this change: run 27372947958, the previous pull request on this repo, shows the same job as failure on mdb-dev-p2snb-runner-xnpgw. All three unit-test matrix jobs are green on this PR. Worth its own fix, and not this one.

The agreement link names the canonical repository rather than a redirect. Every URL the org used for this document was a rename redirect. They all resolve to the same file and serve byte-identical content, so nothing changes for a reader today. A redirect stops being harmless the moment somebody creates a repository at the old name, and this is the page a contributor reads before agreeing to it.

Verified locally

Check What I observed
actionlint findings, base vs branch 34 on base, 29 on this branch, v1.7.12. The five that went away are the mdb-dev unknown-label warnings on the jobs I moved: that class drops from 21 to 16. No new finding. The two tests_unit.yml shellcheck notes are pre-existing and outside the changed lines.
Matrix validator, real inputs The pull request matrix (3 hosted entries) and the full main matrix (12 hosted entries) both pass.
Matrix validator, hostile inputs mdb-dev fails, bare self-hosted fails, the list form ["self-hosted","linux"] fails naming both, and an entry with no runs_on fails naming null. Empty and absent include pass, which is correct: there is no job to run.
Every remaining mdb-dev job get-deploy-labels, build, scan-keycloak, build-cache, all four now carry if: ${{ !github.event.pull_request.head.repo.fork }}. Parsed from the file rather than read by eye.
tests_unit.yml self-hosted jobs None left. code_checking and matrix_prep are ubuntu-latest; the two matrix jobs are validated.
uv behaviour off the cluster From run 27372947958's log: setup-uv reports "Activating python venv" and sets VIRTUAL_ENV itself, so that is not something the self-hosted image provides.

Ships with

One behaviour across eleven repositories, one pull request each. The anchor is
mindsdb/github-actions, which hardens the shared CLA reusable this repo now
calls. No Deploys: line and no pull request environment: nothing here brings
one up.

Merge order: the anchor first, and that is a requirement rather than a
preference.
This wrapper grants actions: read, and until
mindsdb/github-actions#55 merges the reusable still declares actions: write.
A called workflow's permissions are enforced when its file is loaded, so a
callee naming a scope its caller has not granted rejects the whole run as a
startup_failure with zero jobs, and there is no job left to report it. The
run is red, the CLA gate is down, and nothing announces either.

Refs: ENG-2017

…2017)

Four jobs reached mdb-dev from a pull request with no fork guard: changes,
tests_completed, and code_checking and matrix_prep inside tests_unit. Of those
code_checking is the one that matters, because it checks out the fork's tree and
then runs a script and a pre-commit config the fork controls. Move all four to
GitHub-hosted runners rather than skipping them, so an external contribution
still gets CI and the collection job a branch rule points at still reports.

code_checking swaps the FSx cache path for the hosted cache the matrix jobs in
the same file already use; that path only exists on the self-hosted image.

matrix_prep now rejects a runner label that is not GitHub-hosted.
matrix_includes.json is read from the pull request's own checkout and feeds
runs-on directly, so a fork could have named mdb-dev there without touching a
workflow file.

scan-keycloak gains the explicit fork guard its four neighbours carry. It was
already unreachable for a fork, but only through a needs edge on a guarded job.

The CLA workflow moves to the shared reusable in the same change, which takes it
off mdb-dev and pins the third-party action it runs.

Refs: ENG-2017
…017)

Both URLs the org uses for this document are rename redirects that resolve to
the same repository and serve byte-identical content. A redirect is fine until
somebody creates a repository at the old name, and this is the page a
contributor reads before agreeing to it. Name the repository that actually
holds the file.

Refs: ENG-2017
The jq subtracts a fixed set of GitHub-hosted labels rather than matching our
own self-hosted ones, so a label nobody has thought of yet fails instead of
passing. That is the right direction and it is not obvious from the code: the
next person adding a hosted image to matrix_includes.json will hit the error
and needs to know the allowlist is where to add it.

No behaviour change. jq reads `#` to end of line inside the program, and the
validator still passes the real matrix and still rejects mdb-dev, a bare
self-hosted, the list form and a missing runs_on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant