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

[Loup-Garou911XD]

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.

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.

[Loup-Garou911XD]

created by claude
@rikkolovescats @Dliwk pls review

[Loup-Garou911XD]

Thanks — addressed in 9034d1d.

Staging scope wider than the validation allowlist (both occurrences): fixed. This was a legitimate gap. git add -A -- plugins ... would stage anything dirty under plugins/, including paths the allowlist rejects (e.g. plugins/evil/bad.py, since ALLOW only permits plugins/(minigames|utilities|maps)/*.py).

• The fixups step now stages exactly the paths the validation step allow-listed, reusing the file it already produced: git add -A --pathspec-from-file="${RUNNER_TEMP}/patch_paths.txt" (plus a guard that refuses to stage if that list is missing/empty). Commit scope can no longer exceed validated scope, by construction.
• The version-metadata step now stages only index.json and the three category manifests. That script opens every .py read-only and writes only those files, so the narrower pathspec matches exactly what it can modify.

Verified with a scratch repo: with plugins/evil/bad.py and .github/workflows/ci.yml both dirty, only the validated path gets staged; previously the first would have been swept in.

Passing the changed-file list as one CLI argument: not changing, deliberately. Both stated risks are bounded here:

• ARG_MAX is ~2MB on Linux; a realistic changed-file list is a few KB.
• git diff --name-only C-quotes paths containing newlines, so they stay on one line and split("\n") does not mis-split. A quoted path simply fails to match plugins/<category>/*.py and is skipped — it fails safe.

More importantly this is in ci-check.yml, which runs with GitHub's read-only, secret-less token, and nothing it emits is trusted: ci-apply.yml independently allow-list-validates the patch. Fixing it "properly" means changing auto_apply_plugin_metadata.py's CLI contract — a script this PR does not otherwise touch and which has other callers — so it is out of scope here. Happy to do it as a follow-up if maintainers want.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/test_checks.py:17

• The comment describes unresolved commits as “absent 40-hex sha raises ValueError”, but this repo stores abbreviated commit SHAs (first 8 chars) in metadata (see test/auto_apply_version_metadata.py). The wording is misleading; make it SHA-length agnostic (rev/hex object name) so future readers don’t assume a 40-hex requirement.

# Raised by Repo.commit() when a sha cannot be resolved: a well-formed but
# absent 40-hex sha raises plain ValueError, while a malformed name raises
# BadName/BadObject (which are NOT ValueError subclasses). Kept narrow so
# lenient mode can't mask unrelated repository errors.

.github/workflows/ci-check.yml:85

• python test/auto_apply_plugin_metadata.py "$(cat changed_files.txt)" collapses newlines to spaces in bash command substitution, but the script splits on \n (sys.argv[1].split('\n')). This causes the metadata step to miss most changed files. Invoke the script per file (or adjust the script to read the file), so it receives one path per run.

        run: |
          set -euo pipefail
          python test/auto_apply_plugin_metadata.py "$(cat "${RUNNER_TEMP}/changed_files.txt")"

.github/workflows/ci-check.yml:71

• git diff ... -- '*.py' only matches Python files in the repo root; it won’t include changed plugin files under plugins/**, so autopep8 won’t run on most PR changes. Use a recursive pathspec so all .py files are captured.

          git diff --name-only -z "$BASE_SHA" HEAD -- '*.py' > "${RUNNER_TEMP}/changed_py.z"

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Dliwk]

copilot reviewing claude, dead internet theory

[Loup-Garou911XD]

@bombsquad-community/plugman-maintainers ill merge this tomorrow if no one wants to review it

[Loup-Garou911XD]

I think I need time to have another look at this

[brostosjoined]

you could have just allowed the unsafe method let things progress smoothly while you find time to fix it and test

[Loup-Garou911XD]

you could have just allowed the unsafe method let things progress smoothly while you find time to fix it and test

Well the unsafe method was the one we were using till now
Until GitHub decided that it needs another ENV var to allow it

[brostosjoined]

- 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
    allow-unsafe-pr-checkout: true <----------------------------

[brostosjoined]

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

on:
  push:
    branches:
      - main
  pull_request_target:
    types: [labeled] # Only triggers PR builds when a label is explicitly added

jobs:
  build:
    runs-on: ubuntu-latest
    # CRUCIAL: Allow it to run unconditionally on pushes to main, 
    # but require the "reviewed" label for pull_request_target events.
    if: >
      github.event_name == 'push' || 
      (github.event_name == 'pull_request_target' && github.event.label.name == 'reviewed')
    
    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
          allow-unsafe-pr-checkout: true # Now safe because a human approved the execution context

      - name: Set up Python
        uses: actions/setup-python@v6
        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

      - name: Apply AutoPEP8
        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
        uses: stefanzweifel/git-auto-commit-action@v7
        with:
          commit_message: "[ci] apply-plugin-metadata-and-formatting"
          branch: ${{ github.head_ref }}

      - name: Apply Version Metadata
        run: |
          python test/auto_apply_version_metadata.py $(git log --pretty=format:'%h' -n 1)

      - name: Commit Version Metadata
        uses: stefanzweifel/git-auto-commit-action@v7
        with:
          commit_message: "[ci] apply-version-metadata"
          branch: ${{ github.head_ref }}

      - name: Execute Tests
        run: |
          python -m unittest discover -v

[Loup-Garou911XD]

This would be a very asynchronous way, a contributor opens a pr in week 1, a review goes to merge it in week 2, approves the run, sees it's failing, asks the contributor to fix in, contributor fixes in week 3, and repeat if it happens again
Although contributors can test it on their fork, i would prefer it to work here aswell

And this relies on the reviewer to be able to catch a malicious piece of code, and that "approve workflow run" looks pretty harmless so might not always be true that a reviewer actually reviews the changes before approving a run

[brostosjoined]

run a ghost workflow that does not commit anything it only tells the pr what errors where found so that the workflow will run when pulled on the main branch