From bc5c5aafc83e5560cb8992fc2e91a3d268233c07 Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Sun, 9 Aug 2026 19:06:41 +0530 Subject: [PATCH 1/3] 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. --- .github/workflows/ci-apply.yml | 268 +++++++++++++++++++++++++++++++++ .github/workflows/ci-check.yml | 112 ++++++++++++++ .github/workflows/ci.yml | 39 ++--- test/test_checks.py | 60 +++++++- 4 files changed, 450 insertions(+), 29 deletions(-) create mode 100644 .github/workflows/ci-apply.yml create mode 100644 .github/workflows/ci-check.yml diff --git a/.github/workflows/ci-apply.yml b/.github/workflows/ci-apply.yml new file mode 100644 index 0000000..aa56423 --- /dev/null +++ b/.github/workflows/ci-apply.yml @@ -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." diff --git a/.github/workflows/ci-check.yml b/.github/workflows/ci-check.yml new file mode 100644 index 0000000..5eb96e3 --- /dev/null +++ b/.github/workflows/ci-check.yml @@ -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 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 15fe835..7f7144a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -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: | diff --git a/test/test_checks.py b/test/test_checks.py index c5bc6df..5de8010 100644 --- a/test/test_checks.py +++ b/test/test_checks.py @@ -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: From 44cffc5cb57b637c9dd37d58579692860d308eeb Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Mon, 10 Aug 2026 13:03:07 +0530 Subject: [PATCH 2/3] Make plugin metadata idempotent by comparing against the base branch auto_apply_plugin_metadata.py asked "what version is already released?" by reading the category manifest out of the PR's own working tree. Once ci-apply.yml has pushed its "[ci] apply-plugin-metadata-and-formatting" commit back to the PR branch, that tree already lists the version being added - so the check compared the new version against itself and raised "Version cant be lower or equal than the previous version." That fires on every re-run of PR Check: the one ci-apply.yml's own push triggers, and any run caused by a contributor pushing a follow-up commit. Published state now comes from the base branch instead, read with `git show $PLUGMAN_BASE_REF:` (ci-check.yml supplies the PR base sha; local runs fall back to origin/main, then to the working tree). The writer is idempotent to match: a version entry that is already stamped and still matches the plugin's md5sum is left alone rather than reset to null. An entry whose md5sum has drifted is still reset, which implements the TODO this replaces - a contributor can keep iterating on an unpublished version during review without bumping it every round. Bump enforcement against published versions is unchanged. Version comparison and ordering switch to the existing get_comparable_version_tuple_from_string; versioning_tools.semantic_to_str ordered 1.0.10 below 1.0.9 and silently truncated 3-digit components. --- .github/workflows/ci-check.yml | 7 ++ test/auto_apply_plugin_metadata.py | 142 ++++++++++++++++++++++------- 2 files changed, 115 insertions(+), 34 deletions(-) diff --git a/.github/workflows/ci-check.yml b/.github/workflows/ci-check.yml index 5eb96e3..ea4b8d8 100644 --- a/.github/workflows/ci-check.yml +++ b/.github/workflows/ci-check.yml @@ -79,7 +79,14 @@ jobs: < "${RUNNER_TEMP}/changed_py.z" fi + # PLUGMAN_BASE_REF tells the script which tree counts as "already + # published". Without it the script compares against the PR's own + # manifest, which ci-apply.yml has already written the new version into - + # so every re-run of this workflow (including the one ci-apply.yml's push + # triggers) would fail with "Version cant be lower or equal". - name: Apply Plugin Metadata (writes null version placeholders) + env: + PLUGMAN_BASE_REF: ${{ github.event.pull_request.base.sha }} run: | set -euo pipefail python test/auto_apply_plugin_metadata.py "$(cat "${RUNNER_TEMP}/changed_files.txt")" diff --git a/test/auto_apply_plugin_metadata.py b/test/auto_apply_plugin_metadata.py index 228e810..088e3cc 100644 --- a/test/auto_apply_plugin_metadata.py +++ b/test/auto_apply_plugin_metadata.py @@ -2,11 +2,20 @@ import sys import json import ast import os +import hashlib +import subprocess import get_latest -import versioning_tools +from auto_apply_version_metadata import get_comparable_version_tuple_from_string DEBUG = True +MANIFEST_PATHS = { + "minigames": "plugins/minigames.json", + "utilities": "plugins/utilities.json", + "maps": "plugins/maps.json", + "plugman": "index.json", +} + print("DOES THIS RUN AUTO APPLY PLUGIN METADATA?") @@ -15,23 +24,69 @@ def debug_print(*args, **kwargs): print(*args, **kwargs) -def get_latest_version(plugin_name, category) -> str: - filepaths = { - "minigames": "plugins/minigames.json", - "utilities": "plugins/utilities.json", - "maps": "plugins/maps.json", - "plugman": "index.json", - } +def version_key(version): + """Comparison/sort key for a version string, so 1.0.10 sorts above 1.0.9.""" + try: + return get_comparable_version_tuple_from_string(version) + except ValueError: + raise ValueError(f"Version {version!r} is not in x.y.z form.") + +def md5sum_of(path): + with open(path, "rb") as fin: + return hashlib.md5(fin.read()).hexdigest() + + +def get_latest_version(plugin_name, category) -> str: try: if category != "plugman": - return get_latest.get_latest_plugin_version(plugin_name, filepaths[category]) + return get_latest.get_latest_plugin_version(plugin_name, MANIFEST_PATHS[category]) return get_latest.get_latest_plugman_version() except Exception as e: raise e +def read_manifest_at(path, ref): + """Load a manifest as it exists at `ref`, or None if it can't be read there.""" + try: + blob = subprocess.run( + ["git", "show", f"{ref}:{path}"], + capture_output=True, + check=True, + ).stdout + return json.loads(blob) + except (OSError, subprocess.CalledProcessError, json.JSONDecodeError): + return None + + +def get_published_versions(plugin_name, category): + """Versions of `plugin_name` that are already published on the PR's base branch. + + The working tree cannot answer this. Once ci-apply.yml has pushed its + "[ci] apply-plugin-metadata-and-formatting" commit back to the PR branch, + the PR's own copy of the manifest already lists the version being added - + so comparing against the working tree rejects every re-run of PR Check, + including the one ci-apply.yml's own push triggers, and every re-run caused + by a contributor pushing a follow-up commit. + + PLUGMAN_BASE_REF is set by ci-check.yml to the PR's base sha. Local runs + fall back to origin/main, then to the working tree when there is no history + to consult at all. + """ + path = MANIFEST_PATHS[category] + manifest = None + for ref in (os.environ.get("PLUGMAN_BASE_REF"), "origin/main", "main"): + if ref: + manifest = read_manifest_at(path, ref) + if manifest is not None: + break + if manifest is None: + with open(path, "r") as file: + manifest = json.load(file) + return manifest["plugins"].get(plugin_name, {}).get("versions", {}) + + def update_plugman_json(version): with open("index.json", "r+") as file: data = json.load(file) @@ -45,39 +100,58 @@ def update_plugman_json(version): data["versions"] = dict(sorted(data["versions"].items(), reverse=True)) -def update_plugin_json(plugin_info, category): +def update_plugin_json(plugin_info, category, plugin_path): name = plugin_info["plugin_name"] + version = plugin_info["version"] + + # Ensure the version is always greater than the already PUBLISHED version - + # what is on the base branch, not what this PR's own tree happens to say. + published = get_published_versions(name, category) + if published: + latest_published = max(published, key=version_key) + if version_key(version) <= version_key(latest_published): + raise Exception( + "Version cant be lower or equal than the previous version. " + f"{name} {latest_published} is already published; bump the version " + f"in its plugman dict (currently {version})." + ) with open(f"plugins/{category}.json", "r+") as file: data = json.load(file) - try: - # Check if plugin is already in the json - plugin = data["plugins"][name] - plugman_version = int( - versioning_tools.semantic_to_str(get_latest_version(name, category)) - ) - current_version = int(versioning_tools.semantic_to_str(plugin_info["version"])) - # Ensure the version is always greater from the already released version - if current_version > plugman_version: - plugin["versions"][plugin_info["version"]] = None - # Ensure latest version appears first - plugin["versions"] = dict(sorted(plugin["versions"].items(), reverse=True)) - plugin["description"] = plugin_info["description"] - plugin["external_url"] = plugin_info["external_url"] - plugin["authors"] = plugin_info["authors"] - # In future - # We can clear the version metadata for if current_version = plugman_version for PRs - # So that its easier to update the plugin after review without changing version - elif current_version <= plugman_version: - raise Exception("Version cant be lower or equal than the previous version.") - except KeyError: - data["plugins"][name] = { + plugin = data["plugins"].get(name) + if plugin is None: + # New plugin. Key order here is the shape every other entry has. + plugin = data["plugins"][name] = { "description": plugin_info["description"], "external_url": plugin_info["external_url"], "authors": plugin_info["authors"], - "versions": {plugin_info["version"]: None}, + "versions": {}, } + versions = plugin["versions"] + stamped = versions.get(version) + # A null placeholder is only (re)written when there is something for + # auto_apply_version_metadata.py to stamp, which keeps re-runs over an + # already-processed tree a no-op - otherwise ci-apply.yml's push would + # trigger a PR Check that undoes the stamp, forever. + # + # A stamped entry whose md5sum no longer matches the file means the + # contributor pushed further edits under the same UNPUBLISHED version + # (the published case raised above). Reset it so the stamp is + # recomputed, rather than demanding a bump for every review iteration. + if version not in versions or ( + isinstance(stamped, dict) and stamped.get("md5sum") != md5sum_of(plugin_path) + ): + versions[version] = None + + # Ensure latest version appears first + plugin["versions"] = dict( + sorted(versions.items(), key=lambda item: version_key(item[0]), reverse=True) + ) + plugin["description"] = plugin_info["description"] + plugin["external_url"] = plugin_info["external_url"] + plugin["authors"] = plugin_info["authors"] + file.seek(0) json.dump(data, file, indent=2, ensure_ascii=False) # Ensure old content is removed @@ -132,7 +206,7 @@ def extract_plugman(plugins): ) result[kw.arg] = ast.literal_eval(kw.value) if category: - update_plugin_json(result, category=category) + update_plugin_json(result, category=category, plugin_path=plugin) else: update_plugman_json(result) # raise ValueError("Variable plugman not found in the file or has unsupported format.") From 3e0dfccea64e78a76e13f9b78315f058e907fd6d Mon Sep 17 00:00:00 2001 From: Loup-Garou911XD <90267658+Loup-Garou911XD@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:36:27 +0530 Subject: [PATCH 3/3] fix a loophole --- .github/workflows/ci-apply.yml | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci-apply.yml b/.github/workflows/ci-apply.yml index aa56423..046e97c 100644 --- a/.github/workflows/ci-apply.yml +++ b/.github/workflows/ci-apply.yml @@ -18,6 +18,14 @@ name: PR Apply # branch named `a";id;"` becomes live shell. Pass values via `env:` and # reference them as "$VAR", which the shell treats as data. # +# 3. NEVER identify the target PR by commit sha alone. A sha is a value, not an +# identity: forks share object storage, so anyone can push ANOTHER PR's head +# commit onto a branch of their own, open a PR at it, and then close that PR +# mid-run so the commit -> PR lookup below resolves to the victim's PR - at +# which point this job would push the attacker's artifact to the victim's +# branch. The resolved PR must be pinned to workflow_run.head_repository AND +# head_branch AND head_sha, so a run can only ever write to its own branch. +# # 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 @@ -47,16 +55,21 @@ jobs: 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) + # Resolves which PR this run belongs to using ONLY trusted inputs: the + # workflow_run payload (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. + # hence the commit -> PR association lookup. That lookup ANSWERS with a + # PR but does not PROVE it is this run's PR - a sha can be adopted by any + # fork even though it cannot be forged - so the result is pinned to the + # payload's head repo/branch/sha below. See HARD RULE 3 in the header. - 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_HEAD_REPO: ${{ github.event.workflow_run.head_repository.full_name }} + RUN_HEAD_BRANCH: ${{ github.event.workflow_run.head_branch }} run: | set -euo pipefail @@ -96,7 +109,20 @@ jobs: [ "$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." + + # HARD RULE 3: pin the resolved PR to the run that produced the + # artifact. The sha check alone is not enough - a sha is adoptable by + # any fork, so on its own it would let a run push to somebody else's + # PR branch. These are compared only against FORK/HEAD_REF, which the + # regexes above already validated, so no shape check is needed here: + # a null head_repository yields "" and simply fails to match, which is + # the fail-closed direction. Do not "simplify" these away. + [ "$FORK" = "$RUN_HEAD_REPO" ] \ + || skip "Resolved PR head repo != the run's head repo; skipping." + [ "$HEAD_REF" = "$RUN_HEAD_BRANCH" ] \ + || skip "Resolved PR head branch != the run's head branch; 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,