Skip to content

Commit 7279a6f

Browse files
committed
ci: correct the security claim in the windows routing comment
The comment landed in aeb7423 claimed fork PRs "therefore stay on GitHub-hosted runners, unconditionally." That does not follow, and the same comment supplies the reason: on `pull_request` this workflow is loaded from the PR head. The `case "$EVENT_NAME"` block is owned by the proposed patch exactly like the `if:` guard the comment correctly rejects. A hostile PR deletes the branch, hardcodes the self-hosted labels into `$GITHUB_OUTPUT`, and `runs-on` honours it. That `select-windows-runner` itself runs on ubuntu-latest is irrelevant — the untrusted part is its output, not its host. The path is reachable rather than theoretical: `.github/workflows/ci.yml` is in this workflow's own `pull_request.paths`, so the edit triggers the run it needs. This repository is public with 506 forks. An independent audit of the routing design caught this. Leaving the claim in place would have been worse than the original wiring error: a maintainer reading ci.yml to decide whether flipping OCX_SELF_HOSTED_WINDOWS is safe would have found a guarantee that does not exist, in the most authoritative place to look for one. What actually holds the boundary lives outside this file, where a PR cannot reach it — the fork-PR approval policy, runner-group restrictions, and the judgement of whoever approves the run. GitHub recommends against self-hosted runners on public repositories for exactly this reason, and the comment now says so rather than implying the wiring solved it. The routing is unchanged and still worth having: it keeps honest pull requests on GitHub-hosted runners. It is a cost control, and the comment now describes it as one. The kill-switch comment gets the same treatment, and the `push`/`workflow_dispatch` note now records that a trusted author is not audited code, since merging a contributor PR to `dev` fires `push` and runs its dependencies and postinstall hooks on the self-hosted box. No behaviour change: comments only.
1 parent 58be730 commit 7279a6f

1 file changed

Lines changed: 27 additions & 16 deletions

File tree

.github/workflows/ci.yml

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -47,20 +47,29 @@ concurrency:
4747
jobs:
4848
# Which Windows runner this run is allowed to use.
4949
#
50-
# The self-hosted Windows box is a maintainer's PERSONAL machine on a home
51-
# network, reachable over Tailscale. Anything that runs on it runs as a local
52-
# user with that user's filesystem and LAN in reach, so the only question that
53-
# matters is whether the commit is trusted BEFORE a runner ever sees it.
50+
# READ THIS BEFORE TREATING IT AS A SECURITY BOUNDARY: it is not one.
5451
#
55-
# `push` on dev/main/preview is trusted: reaching those branches requires the
56-
# push permission, which only the maintainers in MAINTAINERS.md hold.
57-
# `pull_request` is NOT trusted, and no author check can make it so — the
58-
# workflow file itself comes from the PR head, so any `if:` guard written here
59-
# is deleted by the same patch it is meant to stop. Fork PRs therefore stay on
60-
# GitHub-hosted runners, unconditionally.
52+
# On `pull_request` this workflow is loaded from the PR head, so the `case`
53+
# below is owned by the proposed patch exactly like an `if:` guard would be.
54+
# A hostile PR can delete the branch and hardcode the self-hosted labels into
55+
# `$GITHUB_OUTPUT`, and `runs-on` will honour it. That this job runs on
56+
# `ubuntu-latest` changes nothing — the untrusted part is its OUTPUT, not its
57+
# host. `.github/workflows/ci.yml` is in this workflow's `pull_request.paths`,
58+
# so such an edit triggers its own run.
6159
#
62-
# `workflow_dispatch` is the maintainer escape hatch: dispatch requires write
63-
# access, so a maintainer can deliberately aim a branch at the home box.
60+
# What actually keeps untrusted code off a self-hosted runner lives OUTSIDE
61+
# this file, where a PR cannot reach it: the fork-PR approval policy
62+
# (`all_external_contributors`), runner-group restrictions, and the judgement
63+
# of whoever clicks approve. GitHub's own guidance is to avoid self-hosted
64+
# runners on public repositories for this reason.
65+
#
66+
# So read the routing below as a COST control that keeps honest pull requests
67+
# on GitHub-hosted runners, not as a guarantee about hostile ones.
68+
#
69+
# `push` on dev/main/preview requires the push permission, and
70+
# `workflow_dispatch` requires write access, so both carry a trusted author.
71+
# A trusted author is not audited code: merging a contributor PR into `dev`
72+
# fires `push`, and its dependencies and postinstall hooks then run here.
6473
select-windows-runner:
6574
name: select windows runner
6675
runs-on: ubuntu-latest
@@ -85,10 +94,12 @@ jobs:
8594
push|workflow_dispatch) trusted=yes ;;
8695
esac
8796
88-
# Repository variable OCX_SELF_HOSTED_WINDOWS acts as a kill switch. When
89-
# it is anything other than `1` — including unset, which is the state
90-
# before the runner exists — every job falls back to windows-latest and
91-
# CI keeps working with the box powered off.
97+
# Repository variable OCX_SELF_HOSTED_WINDOWS is an OPERATIONAL switch,
98+
# not a security control: a PR that rewrites this script ignores it for
99+
# the same reason it ignores the event check above. Its job is to keep CI
100+
# working when the box is off or busy. Anything other than `1` —
101+
# including unset, the state before a runner exists — falls back to
102+
# windows-latest.
92103
if [ "$trusted" = "yes" ] && [ "${USE_SELF_HOSTED:-}" = "1" ]; then
93104
echo 'runner=["self-hosted","Windows","X64","ocx-home"]' >> "$GITHUB_OUTPUT"
94105
echo 'label=self-hosted (ocx-home)' >> "$GITHUB_OUTPUT"

0 commit comments

Comments
 (0)