Split ci.yml to close a pull_request_target pwn-request hole

pull_request_target checked out fork PR branches with the repo's
write-scoped GITHUB_TOKEN and ran autopep8/metadata scripts/tests
against that fork content, letting a malicious PR rewrite test/*.py
for arbitrary code execution with push access and secrets. It's also
been failing outright for weeks since actions/checkout now blocks
unsafe fork checkouts here without explicit opt-in.

Split into ci-check.yml (plain pull_request, GitHub's read-only
no-secrets token, safe to run fork code) which uploads a diff
artifact, and ci-apply.yml (workflow_run, privileged) which only
applies that diff via `git apply`, never executing fork content.
ci.yml keeps just the push-to-main job as the strict integrity check.

Because GitHub runs the PR's own copy of ci-check.yml for
pull_request events, that artifact is attacker-authored: ci-apply.yml
therefore resolves PR identity from the workflow_run payload plus the
API rather than the artifact, passes every dynamic value through env:
instead of ${{ }} in run: blocks (which the runner substitutes before
the shell parses, so quotes don't contain it), and validates branch,
repo, sha and PR-number shapes before use. The patch itself stays
untrusted input: allowlist-validated and applied only to the fork's
own branch.

test_checks.py adds an env-gated lenient mode so ci-check.yml's
preview run doesn't fail on a brand-new plugin's not-yet-existing
commit sha, while history and push-to-main stay strict.
This commit is contained in:
Loup-Garou911XD 2026-08-09 19:06:41 +05:30
parent d442c44ea8
commit bc5c5aafc8
No known key found for this signature in database
GPG key ID: 296405D97C924E54
4 changed files with 450 additions and 29 deletions

268
.github/workflows/ci-apply.yml vendored Normal file
View file

@ -0,0 +1,268 @@
name: PR Apply
# Trusted counterpart to ci-check.yml (PR Check). Runs with this repo's
# write-scoped GITHUB_TOKEN via `workflow_run`, AFTER PR Check has finished
# safely (no secrets, fork content fully executed there instead of here).
#
# TWO HARD RULES, both load-bearing. Breaking either one reintroduces the
# pwn-request hole this split exists to close:
#
# 1. NEVER execute, import, or `unittest discover` anything from the fork's
# checkout. This job only applies a text patch (`git apply`), commits,
# pushes, and runs auto_apply_version_metadata.py from a SEPARATE checkout
# of this repo's own main branch - never the fork's copy of it.
#
# 2. NEVER interpolate a `${{ }}` expression into a `run:` block unless the
# value is fixed and trusted. `${{ }}` is substituted textually BEFORE the
# shell parses the script, so surrounding quotes do NOT contain it - a PR
# branch named `a";id;"` becomes live shell. Pass values via `env:` and
# reference them as "$VAR", which the shell treats as data.
#
# On trust: a fork PR fully controls ci-check.yml itself (GitHub runs the
# workflow file from the PR's own merge ref for `pull_request` events - that
# is why that job gets a read-only, secret-less token). So EVERYTHING in the
# pr-fixups artifact is attacker-authored. PR identity is therefore resolved
# from the workflow_run payload + the API, never from the artifact; the
# artifact supplies only the patch, which is allowlist-validated below and
# only ever lands on the fork's own branch.
on:
workflow_run:
workflows: ["PR Check"]
types: [completed]
permissions:
contents: write
pull-requests: write
actions: read # required to download an artifact from another workflow run
concurrency:
group: pr-apply-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }}
cancel-in-progress: false
jobs:
apply:
if: >-
github.event.workflow_run.conclusion == 'success' &&
github.event.workflow_run.event == 'pull_request'
runs-on: ubuntu-latest
steps:
# Resolves which PR this run belongs to using ONLY trusted inputs:
# workflow_run.head_sha (set by GitHub, not forgeable by the PR author)
# and the REST API. workflow_run.pull_requests is empty for fork PRs,
# hence the commit -> PR association lookup.
- name: Resolve and validate PR (trusted sources only)
id: pr
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }}
run: |
set -euo pipefail
skip() { echo "$1"; echo "proceed=0" >> "$GITHUB_OUTPUT"; exit 0; }
[[ "$RUN_HEAD_SHA" =~ ^[0-9a-f]{40}$ ]] || {
echo "Unexpected workflow_run head sha shape; refusing." >&2
exit 1
}
gh api "repos/${REPO}/commits/${RUN_HEAD_SHA}/pulls" > pulls.json
[ "$(jq 'length' pulls.json)" = "1" ] \
|| skip "Not exactly one PR associated with ${RUN_HEAD_SHA}; skipping."
PR_NUMBER="$(jq -r '.[0].number' pulls.json)"
[[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || skip "Bad PR number; skipping."
gh pr view "$PR_NUMBER" --repo "$REPO" \
--json state,baseRefName,headRefName,headRefOid,maintainerCanModify,headRepository,headRepositoryOwner \
> pr.json
STATE="$(jq -r .state pr.json)"
BASE_REF="$(jq -r .baseRefName pr.json)"
HEAD_REF="$(jq -r .headRefName pr.json)"
HEAD_SHA="$(jq -r .headRefOid pr.json)"
CAN_MODIFY="$(jq -r .maintainerCanModify pr.json)"
FORK="$(jq -r '.headRepositoryOwner.login + "/" + .headRepository.name' pr.json)"
# Branch/repo names are attacker-chosen strings, and git happily
# accepts refnames containing ` $( ) ; | ' " - so anything outside
# this conservative set is refused rather than carried forward.
# Bash =~ anchors to the whole string, so embedded newlines (which
# would otherwise inject extra $GITHUB_OUTPUT keys) are rejected too.
[[ "$HEAD_REF" =~ ^[A-Za-z0-9._/-]{1,255}$ ]] || skip "Unsafe branch name; skipping."
[[ "$FORK" =~ ^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$ ]] || skip "Unsafe repo name; skipping."
[[ "$HEAD_SHA" =~ ^[0-9a-f]{40}$ ]] || skip "Unsafe head sha; skipping."
[ "$STATE" = "OPEN" ] || skip "PR not open; skipping."
[ "$BASE_REF" = "main" ] || skip "PR not targeting main; skipping."
[ "$HEAD_SHA" = "$RUN_HEAD_SHA" ] || skip "PR head moved since PR Check ran; skipping."
# maintainerCanModify only has meaning for cross-fork PRs; GitHub
# reports false for a PR opened from a branch in this same repo,
# where we can always push because we own the branch.
if [ "$FORK" != "$REPO" ] && [ "$CAN_MODIFY" != "true" ]; then
echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT"
echo "needs_comment=1" >> "$GITHUB_OUTPUT"
skip "Maintainer edits disabled; cannot push."
fi
{
echo "pr_number=$PR_NUMBER"
echo "head_ref=$HEAD_REF"
echo "head_sha=$HEAD_SHA"
echo "fork=$FORK"
echo "proceed=1"
} >> "$GITHUB_OUTPUT"
- name: Comment if maintainer edits are disabled (actionable, not self-resolving)
if: steps.pr.outputs.needs_comment == '1'
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
PR_NUMBER: ${{ steps.pr.outputs.pr_number }}
run: |
set -euo pipefail
gh pr comment "$PR_NUMBER" --repo "$REPO" \
--body "I can't push the automatic formatting/metadata commits to this PR because \"Allow edits from maintainers\" is disabled. Please enable it, or apply \`autopep8\` and \`test/auto_apply_*_metadata.py\` locally."
- name: Download PR fixups artifact (untrusted data)
if: steps.pr.outputs.proceed == '1'
uses: actions/download-artifact@v8
with:
name: pr-fixups
path: pr-data
github-token: ${{ secrets.GITHUB_TOKEN }}
run-id: ${{ github.event.workflow_run.id }}
# Checking out fork PR content is what actions/checkout >=v6 refuses by
# default; the opt-in is reviewed and intentional here because nothing
# below ever EXECUTES this checkout - only git plumbing touches it.
# persist-credentials:false keeps the write token out of the untrusted
# working tree's .git/config; the push below authenticates explicitly.
- name: Checkout PR branch (fork) - allow-unsafe-pr-checkout is reviewed & intentional
if: steps.pr.outputs.proceed == '1'
uses: actions/checkout@v7
with:
repository: ${{ steps.pr.outputs.fork }}
ref: ${{ steps.pr.outputs.head_sha }}
token: ${{ secrets.GITHUB_TOKEN }}
fetch-depth: 0
persist-credentials: false
allow-unsafe-pr-checkout: true
path: pr
- name: Checkout TRUSTED scripts from our own main branch
if: steps.pr.outputs.proceed == '1'
uses: actions/checkout@v7
with:
ref: main
sparse-checkout: |
test/auto_apply_version_metadata.py
sparse-checkout-cone-mode: false
persist-credentials: false
path: trusted
- name: Re-verify checked-out head matches the resolved head
if: steps.pr.outputs.proceed == '1'
working-directory: pr
env:
HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
run: |
set -euo pipefail
test "$(git rev-parse HEAD)" = "$HEAD_SHA"
- name: Validate patch (allowlisted paths only, no symlinks/binaries)
if: steps.pr.outputs.proceed == '1'
working-directory: pr
run: |
set -euo pipefail
PATCH="${GITHUB_WORKSPACE}/pr-data/fixups.patch"
if [ ! -s "$PATCH" ]; then
echo "Empty patch, nothing to validate"
exit 0
fi
if grep -qE '^(deleted file mode 120000|new mode 120000|new file mode 120000|Binary files)' "$PATCH"; then
echo "Patch contains symlinks or binary content - refusing" >&2
exit 1
fi
# Deliberately excludes .github/** and test/** - a "[ci]"-authored
# commit touching CI config or the test suite is exactly what a
# reviewer would wave through, so those fail closed and are left to
# the contributor to format locally.
ALLOW='^(plugins/(minigames|utilities|maps)/[^/]+\.py|plugin_manager\.py|index\.json|plugins/(minigames|utilities|maps)\.json|CHANGELOG\.md)$'
# Redirect from a file rather than piping into `while`: a pipeline
# would run the loop in a subshell, where `exit 1` would not reliably
# fail the step.
git apply --numstat "$PATCH" | cut -f3 > "${RUNNER_TEMP}/patch_paths.txt"
while IFS= read -r f; do
[ -n "$f" ] || continue
if [[ ! "$f" =~ $ALLOW ]]; then
echo "Patch touches disallowed path: $f" >&2
exit 1
fi
done < "${RUNNER_TEMP}/patch_paths.txt"
git apply --check "$PATCH"
- name: Apply fixups patch and commit
if: steps.pr.outputs.proceed == '1'
working-directory: pr
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
HEAD_REF: ${{ steps.pr.outputs.head_ref }}
FORK: ${{ steps.pr.outputs.fork }}
run: |
set -euo pipefail
PATCH="${GITHUB_WORKSPACE}/pr-data/fixups.patch"
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
if [ ! -s "$PATCH" ]; then
echo "Empty patch, nothing to apply"
exit 0
fi
git apply "$PATCH"
# Stage exactly the paths the validation step allow-listed, so the
# commit scope can never exceed the scope that was validated. A
# broader pathspec (e.g. all of plugins/) would silently widen the
# blast radius if the allowlist ever regressed.
PATHS="${RUNNER_TEMP}/patch_paths.txt"
test -s "$PATHS" || {
echo "Validated path list missing or empty - refusing to stage" >&2
exit 1
}
git add -A --pathspec-from-file="$PATHS"
if ! git diff --cached --quiet; then
git commit -m "[ci] apply-plugin-metadata-and-formatting"
git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}"
fi
- name: Apply Version Metadata using the TRUSTED script only (never the fork's copy)
if: steps.pr.outputs.proceed == '1'
working-directory: pr
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
HEAD_REF: ${{ steps.pr.outputs.head_ref }}
FORK: ${{ steps.pr.outputs.fork }}
run: |
set -euo pipefail
python "${GITHUB_WORKSPACE}/trusted/test/auto_apply_version_metadata.py" "$(git rev-parse HEAD)"
# That script only ever writes index.json and the category manifests
# (it opens every .py read-only), so stage precisely those rather
# than the whole plugins/ tree.
git add -A -- index.json \
plugins/minigames.json plugins/utilities.json plugins/maps.json
if ! git diff --cached --quiet; then
git commit -m "[ci] apply-version-metadata"
git push "https://x-access-token:${GH_TOKEN}@github.com/${FORK}.git" "HEAD:${HEAD_REF}"
fi
- name: On mechanical failure, notify the contributor
if: failure() && steps.pr.outputs.pr_number != ''
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
REPO: ${{ github.repository }}
PR_NUMBER: ${{ steps.pr.outputs.pr_number }}
run: |
set -euo pipefail
gh pr comment "$PR_NUMBER" --repo "$REPO" \
--body "Automatic formatting/metadata could not be applied to this PR (patch touched a disallowed path, or a conflict occurred). A maintainer will need to look at this manually."

112
.github/workflows/ci-check.yml vendored Normal file
View file

@ -0,0 +1,112 @@
name: PR Check
# Runs entirely inside GitHub's hardcoded read-only, no-secrets sandbox for
# `pull_request` events from forks. Fully executes fork-supplied code
# (autopep8, metadata scripts, unittest discover) - that's safe here ONLY
# because this token can't push anywhere and has no secrets.
#
# This workflow NEVER pushes anywhere. It only uploads a plain-text patch as
# a build artifact for ci-apply.yml (a separate, trusted workflow) to apply.
#
# Note that a fork PR can modify THIS FILE and have its version run (GitHub
# uses the workflow from the PR's own merge ref for `pull_request`). So
# nothing produced here is trustworthy, and ci-apply.yml is written on that
# assumption: it resolves PR identity from the workflow_run payload and the
# API, and treats the uploaded patch as untrusted input to be validated.
#
# DO NOT switch this to `pull_request_target` and do not add `permissions:
# contents: write` here "to save a round trip" - that reintroduces the
# pwn-request hole this split exists to close.
on:
pull_request:
permissions:
contents: read
concurrency:
group: pr-check-${{ github.event.pull_request.number }}
cancel-in-progress: true
jobs:
build:
runs-on: ubuntu-latest
steps:
# Credentials are deliberately persisted here (unlike ci-apply.yml): the
# `git fetch` below needs them on a private repo, and the workflow-level
# `permissions: contents: read` caps this token to read-only in every
# case - fork PRs get a read-only token from GitHub regardless.
- name: Checkout PR head
uses: actions/checkout@v7
with:
ref: refs/pull/${{ github.event.pull_request.number }}/head
fetch-depth: 0
- name: Fetch PR base commit (for diffing)
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
run: git fetch --no-tags --depth=1 origin "$BASE_SHA"
- name: Set up Python
uses: actions/setup-python@v7
with:
python-version: "3.12"
- name: Install Dependencies
run: |
python -m pip install -U pip
python -m pip install -U pycodestyle==2.12.1 autopep8
python -m pip install -U -r test/pip_reqs.txt
# Filenames are attacker-controlled, so they are kept in files and
# passed as shell variables - never interpolated into a script or into
# $GITHUB_OUTPUT (where a file named after the heredoc delimiter, or one
# containing a newline, could inject extra output keys).
- name: Compute changed files
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
run: |
set -euo pipefail
git diff --name-only "$BASE_SHA" HEAD > "${RUNNER_TEMP}/changed_files.txt"
git diff --name-only -z "$BASE_SHA" HEAD -- '*.py' > "${RUNNER_TEMP}/changed_py.z"
cat "${RUNNER_TEMP}/changed_files.txt"
- name: Apply AutoPEP8 (changed .py files only)
run: |
set -euo pipefail
if [ -s "${RUNNER_TEMP}/changed_py.z" ]; then
xargs -0 -r autopep8 --in-place --max-line-length=100 \
< "${RUNNER_TEMP}/changed_py.z"
fi
- name: Apply Plugin Metadata (writes null version placeholders)
run: |
set -euo pipefail
python test/auto_apply_plugin_metadata.py "$(cat "${RUNNER_TEMP}/changed_files.txt")"
# IMPORTANT: capture the patch BEFORE the version-metadata preview step
# below. commit_sha values computed there are fictitious (no real
# commit exists yet) and must never be part of what ci-apply.yml applies.
- name: Snapshot fixups patch
run: |
set -euo pipefail
mkdir -p "${RUNNER_TEMP}/pr-data"
git diff > "${RUNNER_TEMP}/pr-data/fixups.patch"
wc -l "${RUNNER_TEMP}/pr-data/fixups.patch"
- name: Apply Version Metadata (local preview only - not uploaded)
run: |
python test/auto_apply_version_metadata.py "$(git rev-parse HEAD)"
- name: "Execute Tests (lenient: new-entry history checks deferred to ci-apply/push-to-main)"
env:
PLUGMAN_CI_LENIENT_HISTORY: "1"
run: |
python -m unittest discover -v
- name: Upload patch for ci-apply.yml
uses: actions/upload-artifact@v7
with:
name: pr-fixups
path: ${{ runner.temp }}/pr-data
retention-days: 5

View file

@ -1,20 +1,20 @@
name: CI
# WORD OF CAUTION:
# TO anyone modifying this
# Things will break if you modify this
# without understanding how it works
# A simple flow of this file:
# Apply AutoPEP8 → Apply Plugin Metadata → CRITICAL COMMIT (format + plugin meta)
# ← ← ← ← ← ↵
# ↪ Apply Version Metadata → Commit (version meta) → Tests
# Runs only on pushes to main - i.e. after a PR has been merged, or on a
# direct maintainer push. This is fully trusted, same-repo content, so it's
# safe for it to execute the tree and push directly. This is also the
# AUTHORITATIVE integrity check: test/test_checks.py's test_versions runs
# here unmodified/strict against real, permanent git history (unlike
# ci-check.yml, which can't yet resolve a commit sha for a brand-new plugin
# version and runs leniently instead).
on:
push:
branches:
- main
pull_request_target:
permissions:
contents: write
jobs:
build:
@ -22,9 +22,6 @@ jobs:
steps:
- uses: actions/checkout@v6
with:
token: ${{ secrets.GITHUB_TOKEN }}
repository: ${{ github.event.pull_request.head.repo.full_name }}
ref: ${{ github.head_ref }}
fetch-depth: 0
- name: Set up Python
@ -42,21 +39,10 @@ jobs:
run: |
autopep8 --in-place --recursive --max-line-length=100 .
- name: Apply Plugin Metadata
if: github.event_name == 'pull_request_target'
env:
GH_TOKEN: ${{ github.token }}
run: |
CHANGED_FILES=$(gh api "repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files" --jq '.[].filename')
python test/auto_apply_plugin_metadata.py "$CHANGED_FILES"
# This is a CRITICAL COMMIT for the next step
# which bases this as the commit to get the sha to store in index.json or plugin.json
- name: Commit Plugin Metadata and AutoPEP8
- name: Commit AutoPEP8 formatting
uses: stefanzweifel/git-auto-commit-action@v7
with:
commit_message: "[ci] apply-plugin-metadata-and-formatting"
branch: ${{ github.head_ref }}
commit_message: "[ci] apply-formatting"
- name: Apply Version Metadata
run: |
@ -66,7 +52,6 @@ jobs:
uses: stefanzweifel/git-auto-commit-action@v7
with:
commit_message: "[ci] apply-version-metadata"
branch: ${{ github.head_ref }}
- name: Execute Tests
run: |

View file

@ -11,6 +11,39 @@ from packaging.version import Version
import unittest
# Raised by Repo.commit() when a revision cannot be resolved. Both entries are
# required because which one you get depends on how the sha is spelled: an
# unresolvable abbreviated name - the 7-8 char form this repo actually stores,
# see auto_apply_version_metadata.py - raises BadName, whereas a full-length
# but absent 40-hex sha raises plain ValueError. BadName/BadObject are NOT
# ValueError subclasses, so neither entry is redundant. Kept narrow so lenient
# mode can't mask unrelated repository errors.
#
# BadName/BadObject originate in gitdb but are re-exported by GitPython in
# git.exc.__all__, so git.exc.* is the supported public spelling and is used
# here deliberately - it avoids importing gitdb, a transitive dependency that
# test/pip_reqs.txt does not declare directly.
UNRESOLVED_COMMIT_ERRORS = (ValueError, git.exc.BadName, git.exc.BadObject)
def is_unpublished_version(repository, version_metadata):
"""True if this version entry is being introduced by the PR under test.
Such an entry can't satisfy the history checks yet: its metadata is either
still an unstamped ``null`` placeholder, or was stamped against the current
HEAD by ci-check.yml's preview - describing the reformatted working tree
rather than anything committed. Published entries always point at an
earlier commit whose tree really does contain the described file, so they
are unaffected. Only consulted in lenient mode (ci-check.yml); the
authoritative strict run on push-to-main still validates these.
"""
if not version_metadata or not version_metadata.get("commit_sha"):
return True
try:
return repository.commit(version_metadata["commit_sha"]) == repository.head.commit
except UNRESOLVED_COMMIT_ERRORS:
return False # unresolvable: let the caller's handler report it
class TestPluginManagerMetadata(unittest.TestCase):
def setUp(self):
@ -40,8 +73,19 @@ class TestPluginManagerMetadata(unittest.TestCase):
assert sorted_versions == versions
def test_versions(self):
lenient = os.environ.get("PLUGMAN_CI_LENIENT_HISTORY") == "1"
for version_name, version_metadata in self.content["versions"].items():
commit = self.repository.commit(version_metadata["commit_sha"])
if lenient and is_unpublished_version(self.repository, version_metadata):
print(f"[lenient] skipping {version_name}: not committed yet")
continue
try:
commit = self.repository.commit(version_metadata["commit_sha"])
except UNRESOLVED_COMMIT_ERRORS as err:
if lenient:
print(f"[lenient] skipping {version_name}: commit "
f"{version_metadata['commit_sha']} not found yet ({err})")
continue
raise
plugin_manager = commit.tree / self.plugin_manager
with io.BytesIO(plugin_manager.data_stream.read()) as fin:
content = fin.read()
@ -144,9 +188,21 @@ class BaseCategoryMetadataTestCases:
self.assertTrue(len(plugin_metadata["versions"]) > 0)
def test_versions(self):
lenient = os.environ.get("PLUGMAN_CI_LENIENT_HISTORY") == "1"
for plugin_name, plugin_metadata in self.content["plugins"].items():
for version_name, version_metadata in plugin_metadata["versions"].items():
commit = self.repository.commit(version_metadata["commit_sha"])
if lenient and is_unpublished_version(self.repository, version_metadata):
print(f"[lenient] skipping {plugin_name} {version_name}: "
"not committed yet")
continue
try:
commit = self.repository.commit(version_metadata["commit_sha"])
except UNRESOLVED_COMMIT_ERRORS as err:
if lenient:
print(f"[lenient] skipping {plugin_name} {version_name}: "
f"commit {version_metadata['commit_sha']} not found yet ({err})")
continue
raise
plugin = os.path.join(self.category, f"{plugin_name}.py")
plugin_commit_sha = commit.tree / plugin
with io.BytesIO(plugin_commit_sha.data_stream.read()) as fin: