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: