From 8b2b3011d68cdb06a144227ba5a023df2b70e888 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:04:37 +0000 Subject: [PATCH 1/4] Initial plan From 53b22290e0e2675953cf68233ae1912d040a03bf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:08:41 +0000 Subject: [PATCH 2/4] Fix validate-and-render: use pull_request_target for fork PR write access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job was failing with HTTP 403 when trying to post a PR comment because PR #50 was a fork PR (dsirianni/portal → act-cms/portal). With the `pull_request` trigger, GitHub's GITHUB_TOKEN is read-only for fork PRs regardless of the declared `issues: write` permission. Fix: switch to `pull_request_target`, which runs in the base-repo context and always honours the declared permissions. Security: after checking out the PR head content, all executable files (scripts/, templates/, template.yaml, paths.yml, index.html, theme.css) are restored from the trusted base branch SHA. Only the lesson YAML data files from the fork are used, and those are only parsed, not executed. --- .github/workflows/pr-check.yml | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 4bda74a..554281b 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -1,15 +1,24 @@ name: PR Lesson Check -# Validates lesson submissions on pull requests (branch-based, no fork required) +# Validates lesson submissions on pull requests (branch-based and fork PRs) # and renders the portal pages, then posts a friendly pass/fail summary as a PR # comment. This does NOT deploy — deployment stays in build-site.yml on main. on: - pull_request: + pull_request_target: branches: [ main ] # Run when a lesson YAML is submitted, or when the build infrastructure # that renders/validates them changes. A submission is almost always a # single lessons/*.yml file. + # + # pull_request_target (rather than pull_request) is required so that the + # GITHUB_TOKEN has write access when the PR comes from a fork — the + # pull_request event restricts fork tokens to read-only regardless of the + # declared permissions block, which causes the "Post PR comment" step to + # fail with HTTP 403. pull_request_target runs in the base-repo context + # and always honours the declared permissions. Untrusted code from the fork + # is handled safely: after checking out the PR head we restore all + # executable scripts from the trusted base branch (see below). paths: - '**/*.yml' - '**/*.yaml' @@ -33,11 +42,23 @@ jobs: runs-on: ubuntu-latest steps: - - name: Checkout repository + - name: Checkout PR content uses: actions/checkout@v4 with: + ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 # need history to diff the PR against its base + # Security: with pull_request_target the GITHUB_TOKEN has base-repo write + # access, so we must ensure untrusted fork code cannot run. Lesson + # submissions are YAML *data* (safe to parse), but scripts/templates are + # executable — restore them from the trusted base branch before running + # anything. + - name: Restore trusted scripts and config from base branch + run: | + git fetch origin ${{ github.event.pull_request.base.sha }} + git checkout ${{ github.event.pull_request.base.sha }} -- \ + scripts/ templates/ template.yaml paths.yml index.html theme.css + - name: Set up Python uses: actions/setup-python@v5 with: @@ -62,9 +83,9 @@ jobs: # template.yaml (the reference) are the only YAML files that legitimately # live at the repo root. # - # We diff the PR head SHA against base (base...head), NOT ...HEAD. On a - # pull_request event, actions/checkout resolves HEAD to the *merge* ref - # (head merged into current main), so ...HEAD would also surface every + # We diff the PR head SHA against base (base...head), NOT ...HEAD. + # actions/checkout with pull_request_target resolves HEAD to the *merge* + # ref (head merged into current main), so ...HEAD would also surface every # change main made since the branch forked — misattributing main's own # commits (e.g. edits to this workflow) to the PR. Diffing the pure head # SHA yields exactly the author's changes, matching GitHub's "Files From c78c774588dd1adab9ff9f5dfe14015e04d5aee4 Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Fri, 31 Jul 2026 17:30:05 -0400 Subject: [PATCH 3/4] Split PR check into untrusted validator + trusted commenter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the HTTP 403 when posting the check summary on fork PRs, without granting a write-scoped token to a job that runs the PR author's code. pr-check.yml goes back to `pull_request` with `contents: read` only. It still checks out and runs the fork's scripts/ and templates/, but now holds a token that cannot write anything. Instead of posting a comment it uploads the rendered body as the `pr-check-comment` artifact. pr-comment.yml is new: it runs on `workflow_run` after the check completes, so it executes in the base-repo context with a writable token. It never checks out PR content — it only downloads the artifact and posts its text. This replaces the `pull_request_target` approach, which was not safe. `git checkout -- scripts/ templates/` overwrites files tracked in base but never deletes files the fork *added*, so a fork could drop in scripts/yaml.py (shadows PyYAML — sys.path[0] is the script's own directory) or templates/platforms/.html (picked up by the platform glob and included by a non-sandboxed Jinja env) and run arbitrary code with repo-write credentials. Reverting scripts/, templates/, paths.yml, index.html and theme.css to base also meant PRs touching build infrastructure were validated against base — a green check that tested none of the submitted change — and a lesson rename plus paths.yml edit would fail on base's now-dangling lesson ID. The artifact is treated as untrusted throughout: the body is posted verbatim and truncated to 60000 chars, the PR number is resolved from event data first and the artifact's claim is honoured only when that PR's head SHA matches the run's, and only a Bot-authored comment is updated so a planted marker cannot hijack the summary. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-check.yml | 85 ++++++++------------- .github/workflows/pr-comment.yml | 125 +++++++++++++++++++++++++++++++ .gitignore | 1 + 3 files changed, 156 insertions(+), 55 deletions(-) create mode 100644 .github/workflows/pr-comment.yml diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 554281b..94c8dfe 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -1,24 +1,24 @@ name: PR Lesson Check -# Validates lesson submissions on pull requests (branch-based and fork PRs) -# and renders the portal pages, then posts a friendly pass/fail summary as a PR -# comment. This does NOT deploy — deployment stays in build-site.yml on main. +# Validates lesson submissions on pull requests (branch PRs and forks) and +# renders the portal pages. This does NOT deploy — deployment stays in +# build-site.yml on main. +# +# This job checks out and runs the PR author's code (scripts/, templates/), so +# it deliberately holds a read-only GITHUB_TOKEN and touches no secrets. That +# means it cannot post its own summary comment: on `pull_request`, GitHub +# forces the fork token to read-only regardless of the declared permissions +# block, so the comment API returns HTTP 403. Instead the rendered comment is +# uploaded as an artifact and posted by pr-comment.yml, which runs on +# `workflow_run` in the base-repo context with a writable token and never +# checks out PR code. on: - pull_request_target: + pull_request: branches: [ main ] # Run when a lesson YAML is submitted, or when the build infrastructure # that renders/validates them changes. A submission is almost always a # single lessons/*.yml file. - # - # pull_request_target (rather than pull_request) is required so that the - # GITHUB_TOKEN has write access when the PR comes from a fork — the - # pull_request event restricts fork tokens to read-only regardless of the - # declared permissions block, which causes the "Post PR comment" step to - # fail with HTTP 403. pull_request_target runs in the base-repo context - # and always honours the declared permissions. Untrusted code from the fork - # is handled safely: after checking out the PR head we restore all - # executable scripts from the trusted base branch (see below). paths: - '**/*.yml' - '**/*.yaml' @@ -28,9 +28,7 @@ on: - 'index.html' permissions: - contents: read - pull-requests: write # needed to post/update the summary comment - issues: write # needed to create/update issue comments via the Issues API + contents: read # deliberately read-only — this job runs untrusted PR code # If an author pushes several times quickly, only check the latest commit. concurrency: @@ -42,23 +40,11 @@ jobs: runs-on: ubuntu-latest steps: - - name: Checkout PR content + - name: Checkout repository uses: actions/checkout@v4 with: - ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0 # need history to diff the PR against its base - # Security: with pull_request_target the GITHUB_TOKEN has base-repo write - # access, so we must ensure untrusted fork code cannot run. Lesson - # submissions are YAML *data* (safe to parse), but scripts/templates are - # executable — restore them from the trusted base branch before running - # anything. - - name: Restore trusted scripts and config from base branch - run: | - git fetch origin ${{ github.event.pull_request.base.sha }} - git checkout ${{ github.event.pull_request.base.sha }} -- \ - scripts/ templates/ template.yaml paths.yml index.html theme.css - - name: Set up Python uses: actions/setup-python@v5 with: @@ -83,9 +69,9 @@ jobs: # template.yaml (the reference) are the only YAML files that legitimately # live at the repo root. # - # We diff the PR head SHA against base (base...head), NOT ...HEAD. - # actions/checkout with pull_request_target resolves HEAD to the *merge* - # ref (head merged into current main), so ...HEAD would also surface every + # We diff the PR head SHA against base (base...head), NOT ...HEAD. On a + # pull_request event, actions/checkout resolves HEAD to the *merge* ref + # (head merged into current main), so ...HEAD would also surface every # change main made since the branch forked — misattributing main's own # commits (e.g. edits to this workflow) to the PR. Diffing the pure head # SHA yields exactly the author's changes, matching GitHub's "Files @@ -144,36 +130,25 @@ jobs: if [ "${{ steps.location.outputs.location_failed }}" = "true" ]; then result=failure; fi echo "result=$result" >> "$GITHUB_OUTPUT" + # pr_number.txt is only a last-resort hint for pr-comment.yml. It is + # written by this untrusted job, so that workflow verifies it against the + # run's head SHA before trusting it. - name: Format PR comment if: always() run: | python scripts/format_pr_comment.py "${{ steps.outcome.outputs.result }}" build.log pr_comment.md + echo "${{ github.event.pull_request.number }}" > pr_number.txt - - name: Post or update PR comment + - name: Upload the comment for pr-comment.yml to post if: always() - uses: actions/github-script@v7 + uses: actions/upload-artifact@v4 with: - script: | - const fs = require('fs'); - const body = fs.readFileSync('pr_comment.md', 'utf8'); - const marker = ''; - const { owner, repo } = context.repo; - const issue_number = context.issue.number; - - const { data: comments } = await github.rest.issues.listComments({ - owner, repo, issue_number, per_page: 100, - }); - const existing = comments.find(c => c.body && c.body.includes(marker)); - - if (existing) { - await github.rest.issues.updateComment({ - owner, repo, comment_id: existing.id, body, - }); - } else { - await github.rest.issues.createComment({ - owner, repo, issue_number, body, - }); - } + name: pr-check-comment + path: | + pr_comment.md + pr_number.txt + retention-days: 1 + if-no-files-found: error - name: Fail the check if anything failed if: steps.outcome.outputs.result == 'failure' diff --git a/.github/workflows/pr-comment.yml b/.github/workflows/pr-comment.yml new file mode 100644 index 0000000..34716be --- /dev/null +++ b/.github/workflows/pr-comment.yml @@ -0,0 +1,125 @@ +name: PR Lesson Check Comment + +# Posts the pass/fail summary produced by pr-check.yml as a PR comment. +# +# Why this is a second workflow: on `pull_request`, GitHub forces the fork's +# GITHUB_TOKEN to read-only no matter what `permissions:` declares, so the +# check itself cannot comment on a fork PR (HTTP 403). A `workflow_run` +# workflow runs in the base-repo context and does get a writable token. +# +# The safety property that makes this work: `workflow_run` always uses the copy +# of this file on the default branch, and this job never checks out PR content +# — it only downloads an artifact and posts its text. So the fork's scripts and +# templates never execute in a job holding a write-scoped token. (Switching +# pr-check.yml itself to `pull_request_target` would hand that token to a job +# that runs the fork's scripts/ and templates/ — a repo-takeover vector. Don't.) +# +# Everything in the artifact is untrusted data: it is written by the job that +# ran the PR author's code. It is only ever posted as a comment body, and the +# PR number it claims is verified against the run's head SHA before use. + +on: + workflow_run: + workflows: ["PR Lesson Check"] + types: [completed] + +permissions: + contents: read + actions: read # to download the check run's artifact + pull-requests: write # to post/update the summary comment + +# Serialize per branch so two runs finishing at once can't both conclude that +# no comment exists yet and each create one. +concurrency: + group: pr-comment-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: false + +jobs: + comment: + runs-on: ubuntu-latest + # A cancelled run (superseded by a newer push) uploaded no artifact. + if: >- + github.event.workflow_run.event == 'pull_request' && + (github.event.workflow_run.conclusion == 'success' || + github.event.workflow_run.conclusion == 'failure') + + steps: + - name: Download the comment artifact + id: download + continue-on-error: true # a run that died before uploading is not an error here + uses: actions/download-artifact@v4 + with: + name: pr-check-comment + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Post or update the PR comment + if: steps.download.outcome == 'success' + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const { owner, repo } = context.repo; + const run = context.payload.workflow_run; + + // Untrusted content — post as-is, never interpret. Cap it well + // under GitHub's 65536-character comment limit so a pathological + // build log can't make every comment fail to post. + const LIMIT = 60000; + let body = fs.readFileSync('pr_comment.md', 'utf8'); + if (body.length > LIMIT) { + body = body.slice(0, LIMIT) + + '\n\n_…truncated. See the full log in the **Actions** tab._\n'; + } + + // Resolve the PR from trusted event data first. `pull_requests` is + // empty for fork PRs, so fall back to the commit→PR lookup, and + // only then to the number the check job recorded — accepted just + // when that PR's head really is the commit this run tested. + let prNumber = run.pull_requests?.[0]?.number; + + if (!prNumber) { + const { data: prs } = + await github.rest.repos.listPullRequestsAssociatedWithCommit({ + owner, repo, commit_sha: run.head_sha, + }); + prNumber = prs.find(p => p.head.sha === run.head_sha)?.number; + } + + if (!prNumber && fs.existsSync('pr_number.txt')) { + const claimed = Number(fs.readFileSync('pr_number.txt', 'utf8').trim()); + if (Number.isInteger(claimed) && claimed > 0) { + const { data: pr } = + await github.rest.pulls.get({ owner, repo, pull_number: claimed }); + if (pr.head.sha === run.head_sha) { + prNumber = claimed; + } else { + core.warning(`Ignoring claimed PR #${claimed}: its head is ` + + `${pr.head.sha}, not ${run.head_sha}.`); + } + } + } + + if (!prNumber) { + core.setFailed(`No pull request found for ${run.head_sha}; comment not posted.`); + return; + } + + const marker = ''; + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, per_page: 100, + }); + // Only ever update our own comment — a marker in someone else's + // comment must not redirect the check summary. + const existing = comments.find( + c => c.user?.type === 'Bot' && c.body?.includes(marker)); + + if (existing) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body, + }); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, body, + }); + } diff --git a/.gitignore b/.gitignore index a1b0d6c..94b5bbe 100644 --- a/.gitignore +++ b/.gitignore @@ -196,6 +196,7 @@ cython_debug/ # CI artifacts (generated by the pr-check workflow) pr_comment.md +pr_number.txt #Claude .playwright-mcp From 71c8cfb01b9153d39cb7940efd70c1c9f0d4b42c Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Fri, 31 Jul 2026 17:32:55 -0400 Subject: [PATCH 4/4] Exempt .github/ from the lesson location check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The paths filter matches '**/*.yml', so a PR that only edits a workflow triggers this check and then fails it with "move it into lessons/" — which is what happened to this PR. CI config is not a lesson submission. Misnamed lesson files (lessons/*.yaml, lessons/*.yml.yaml) and stray YAML elsewhere in the tree are still flagged. Co-Authored-By: Claude Opus 5 --- .github/workflows/pr-check.yml | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pr-check.yml b/.github/workflows/pr-check.yml index 94c8dfe..3f02dfc 100644 --- a/.github/workflows/pr-check.yml +++ b/.github/workflows/pr-check.yml @@ -67,7 +67,10 @@ jobs: # is silently skipped — it would never be validated or deployed yet the # build still passes. We reject those here. paths.yml (learning paths) and # template.yaml (the reference) are the only YAML files that legitimately - # live at the repo root. + # live at the repo root, and .github/ is CI config rather than lesson + # content — without that exemption every workflow edit fails this check + # with "move it into lessons/", because the paths filter above matches + # '**/*.yml'. # # We diff the PR head SHA against base (base...head), NOT ...HEAD. On a # pull_request event, actions/checkout resolves HEAD to the *merge* ref @@ -88,9 +91,10 @@ jobs: *.yml|*.yaml) ;; *) continue ;; esac - if [ "$f" = "paths.yml" ] || [ "$f" = "template.yaml" ]; then - continue - fi + # Repo infrastructure, not a lesson submission. + case "$f" in + .github/*|paths.yml|template.yaml) continue ;; + esac case "$f" in lessons/*.yml) printf '%s\n' "$f" >> changed_lessons.txt