From 45b86659dfe6ee17b2bbce62002acc05a38912c5 Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Tue, 18 Aug 2026 14:17:15 +0530 Subject: [PATCH 1/2] fix: report PR test/build results from a workflow_run job Every PR check has been failing at "Report Build Size" with POST /repos/StabilityNexus/Chainvoice/issues/{n}/comments 403 Resource not accessible by integration while lint, tests, and the build all pass. A `pull_request` run triggered from a fork gets a read-only GITHUB_TOKEN regardless of what the workflow's `permissions:` block requests, so every label and comment call in that job is refused. Contributors work from forks, so this hits all PRs. The sibling "Remove test failure label" step made the same call and only looked healthy because it swallowed the error in a try/catch. Split the two concerns. test.yml drops to `contents: read`, makes no API calls, and hands its results to an artifact; a new pr-report.yml runs on `workflow_run`, which executes in the base-repo context with a real write token, and does the labelling and commenting there. Notes on the split: - github.event.workflow_run.pull_requests is empty for forked PRs, so the PR number travels in the artifact rather than the event payload. - The reporting job never checks out the PR's code. It holds a write token, so running fork code there would be a privilege-escalation path. It only reads text out of the artifact and quotes it, with backticks neutered so a crafted log cannot break out of the fence. - `test case failing` and `build failed` do not exist in this repo, so the script creates them on first use with a colour and description instead of letting addLabels auto-generate one. - The comment is now upserted against a hidden marker. The old version posted a fresh comment per run, which stacked up on PRs with many pushes. --- .github/workflows/pr-report.yml | 145 ++++++++++++++++++++++++++++++++ .github/workflows/test.yml | 113 ++++++------------------- 2 files changed, 172 insertions(+), 86 deletions(-) create mode 100644 .github/workflows/pr-report.yml diff --git a/.github/workflows/pr-report.yml b/.github/workflows/pr-report.yml new file mode 100644 index 00000000..c33fd60b --- /dev/null +++ b/.github/workflows/pr-report.yml @@ -0,0 +1,145 @@ +name: PR Report + +# Companion to test.yml. A `pull_request` run from a fork gets a read-only +# GITHUB_TOKEN no matter what its `permissions:` block asks for, so labelling and +# commenting from inside that run always fails with +# "Resource not accessible by integration". `workflow_run` runs in the base-repo +# context with a real write token, so the reporting lives here instead. +on: + workflow_run: + workflows: ['Test and Build'] + types: [completed] + +permissions: + issues: write + pull-requests: write + actions: read + +jobs: + report: + if: github.event.workflow_run.event == 'pull_request' + runs-on: ubuntu-latest + steps: + # This job holds a write token, so it must never check out or execute the + # PR's code. It only reads text out of the artifact and quotes it. + - name: Download report data + id: download + continue-on-error: true + uses: actions/download-artifact@v4 + with: + name: pr-report + path: pr-report + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Label and comment + if: steps.download.outcome == 'success' + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const { owner, repo } = context.repo; + + const read = (name) => { + try { + return fs.readFileSync(`pr-report/${name}`, 'utf8'); + } catch { + return ''; + } + }; + + const prNumber = Number(read('pr_number').trim()); + if (!prNumber) { + core.info('No PR number in artifact; nothing to report.'); + return; + } + + const testsFailed = read('tests_failed').trim() === 'true'; + const buildFailed = read('build_failed').trim() === 'true'; + + // Artifact contents come from a fork's build. Fence-breaking is the + // only thing quoting it can do, but neuter it anyway. + const fence = (text, limit = 60000) => { + let out = text.replace(/`/g, 'ˋ'); + if (out.length > limit) out = out.slice(-limit); + return out.trim() || '(no output)'; + }; + + const LABELS = { + 'test case failing': { color: 'd93f0b', description: 'Automated tests are failing on this PR' }, + 'build failed': { color: 'b60205', description: 'The production build is failing on this PR' }, + }; + + const setLabel = async (name, shouldHave) => { + if (shouldHave) { + try { + await github.rest.issues.getLabel({ owner, repo, name }); + } catch { + await github.rest.issues.createLabel({ owner, repo, name, ...LABELS[name] }); + } + await github.rest.issues.addLabels({ + owner, repo, issue_number: prNumber, labels: [name], + }); + } else { + try { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: prNumber, name, + }); + } catch { + // Label was not applied; nothing to remove. + } + } + }; + + await setLabel('test case failing', testsFailed); + await setLabel('build failed', buildFailed); + + const sections = []; + + if (testsFailed) { + sections.push( + `⚠️ **Tests failed!**\n\n
Test Output\n\n\`\`\`\n` + + `${fence(read('test_output.log'))}\n\`\`\`\n
` + ); + } + + if (buildFailed) { + sections.push( + `❌ **Build failed!**\n\n
Build Output\n\n\`\`\`\n` + + `${fence(read('build_output.log'))}\n\`\`\`\n
` + ); + } else { + const metrics = read('build_output.log') + .split('\n') + .filter((line) => line.includes('dist/assets/')) + .join('\n'); + sections.push( + `✅ **Build successful!**\n\n
Build Size Metrics\n\n\`\`\`\n` + + `${fence(metrics || 'Metrics not found')}\n\`\`\`\n
` + ); + } + + // One comment per PR, edited in place, so a PR with many pushes does + // not accumulate a wall of near-identical reports. + const MARKER = ''; + const runUrl = `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.payload.workflow_run.id}`; + const body = [ + MARKER, + sections.join('\n\n'), + `[Workflow run](${runUrl}) · commit ${context.payload.workflow_run.head_sha.slice(0, 7)}`, + ].join('\n\n'); + + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: prNumber, per_page: 100, + }); + const existing = comments.find((c) => 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/.github/workflows/test.yml b/.github/workflows/test.yml index a69fd4ca..75bddb29 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,19 +10,20 @@ on: paths: - 'frontend/**' +# Read-only on purpose. This workflow runs untrusted fork code, so it must not +# hold a token that can write to the repo. Labels and the build-size comment are +# posted by pr-report.yml, which runs after this one in the base-repo context. permissions: - issues: write - pull-requests: write contents: read jobs: test-and-build: runs-on: ubuntu-latest - + defaults: run: working-directory: ./frontend - + steps: - name: Checkout repository uses: actions/checkout@v4 @@ -48,56 +49,6 @@ jobs: npm run test:ci > test_output.log 2>&1 || echo "TESTS_FAILED=true" >> $GITHUB_ENV cat test_output.log - - name: Handle Test Failure - if: env.TESTS_FAILED == 'true' && github.event_name == 'pull_request' - uses: actions/github-script@v7 - with: - script: | - const fs = require('fs'); - let testOutput = ''; - try { - testOutput = fs.readFileSync('./frontend/test_output.log', 'utf8'); - if (testOutput.length > 60000) { - testOutput = testOutput.substring(testOutput.length - 60000); - } - } catch (e) { - testOutput = 'Could not read test output.'; - } - - await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - labels: ['test case failing'] - }); - - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: `⚠️ **Tests failed!**\n\n
Test Output\n\n\`\`\`\n${testOutput}\n\`\`\`\n
` - }); - - - name: Fail if tests failed - if: env.TESTS_FAILED == 'true' - run: exit 1 - - - name: Remove test failure label (if passed) - if: env.TESTS_FAILED != 'true' && github.event_name == 'pull_request' - uses: actions/github-script@v7 - with: - script: | - try { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - name: 'test case failing' - }); - } catch (e) { - // Ignore if label doesn't exist - } - - name: Run build id: build continue-on-error: true @@ -105,40 +56,30 @@ jobs: npm run build > build_output.log 2>&1 || echo "BUILD_FAILED=true" >> $GITHUB_ENV cat build_output.log - - name: Handle Build Failure - if: env.BUILD_FAILED == 'true' && github.event_name == 'pull_request' - uses: actions/github-script@v7 - with: - script: | - await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - labels: ['build failed'] - }); + # github.event.workflow_run.pull_requests is empty for forked PRs, so the + # reporting workflow cannot recover the PR number from the event payload. + # Hand it over through the artifact instead. + - name: Collect report data + if: always() && github.event_name == 'pull_request' + run: | + mkdir -p pr-report + echo "${{ github.event.pull_request.number }}" > pr-report/pr_number + echo "${TESTS_FAILED:-false}" > pr-report/tests_failed + echo "${BUILD_FAILED:-false}" > pr-report/build_failed + cp test_output.log pr-report/ 2>/dev/null || true + cp build_output.log pr-report/ 2>/dev/null || true - - name: Report Build Size - if: env.BUILD_FAILED != 'true' && github.event_name == 'pull_request' - uses: actions/github-script@v7 + - name: Upload report data + if: always() && github.event_name == 'pull_request' + uses: actions/upload-artifact@v4 with: - script: | - const fs = require('fs'); - let buildOutput = ''; - try { - buildOutput = fs.readFileSync('./frontend/build_output.log', 'utf8'); - const lines = buildOutput.split('\n'); - const metrics = lines.filter(line => line.includes('dist/assets/')).join('\n'); - buildOutput = metrics || 'Metrics not found'; - } catch (e) { - buildOutput = 'Could not read build output.'; - } - - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body: `✅ **Build successful!**\n\n
Build Size Metrics\n\n\`\`\`\n${buildOutput}\n\`\`\`\n
` - }); + name: pr-report + path: frontend/pr-report + retention-days: 1 + + - name: Fail if tests failed + if: env.TESTS_FAILED == 'true' + run: exit 1 - name: Fail if build failed if: env.BUILD_FAILED == 'true' From 045f566866a0a1e8473cecf3392d7ecaa7a531ad Mon Sep 17 00:00:00 2001 From: Atharva0506 Date: Tue, 18 Aug 2026 14:35:10 +0530 Subject: [PATCH 2/2] fix: harden the PR reporting job against a hostile artifact Addresses CodeRabbit's review on #200. The first cut took the target PR number out of the artifact, which was a privilege escalation. A `pull_request` run executes the fork's own copy of test.yml, so a PR could have written any number into pr_number and had the privileged job add labels and post bot comments on an unrelated issue. pr-report.yml now resolves the PR from the workflow_run payload only: `pull_requests[0]` for same-repo PRs, otherwise a pulls.list lookup keyed on the run's own head_repository and head_branch, preferring an exact head_sha match. pr_number is gone from the artifact. The remaining artifact fields are documented as advisory. A fork can claim its tests passed, but only on its own PR, and the required check's conclusion is the signal that actually gates merge. Also from the review: - Add workflow-level concurrency keyed on head repository and branch with cancel-in-progress. Two pushes in quick succession would otherwise both list comments, both see no report, and both create one. - Share one 60000-character budget across the report's sections. Tests and build both failing produced a ~120000-character body and a 422 from the comments API, which caps bodies at 65536. - Require github-actions[bot] as the comment author and the marker at the start of the body. `find` on the marker alone let a PR author plant it in their own comment and have the report overwrite it. - Pin every action in both files to a commit SHA. This matters most for the reporting job, which runs with issues: write. Named the report job so it is not an anonymous definition. --- .github/workflows/pr-report.yml | 105 ++++++++++++++++++++++++-------- .github/workflows/test.yml | 13 ++-- 2 files changed, 85 insertions(+), 33 deletions(-) diff --git a/.github/workflows/pr-report.yml b/.github/workflows/pr-report.yml index c33fd60b..c5d07683 100644 --- a/.github/workflows/pr-report.yml +++ b/.github/workflows/pr-report.yml @@ -15,8 +15,17 @@ permissions: pull-requests: write actions: read +# One report in flight per head branch. Without this, two pushes in quick +# succession race: both runs list the comments, both see no existing report, and +# both create one. The newest run always has the freshest results, so superseding +# an in-flight one loses nothing. +concurrency: + group: pr-report-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} + cancel-in-progress: true + jobs: report: + name: Label and comment if: github.event.workflow_run.event == 'pull_request' runs-on: ubuntu-latest steps: @@ -25,7 +34,7 @@ jobs: - name: Download report data id: download continue-on-error: true - uses: actions/download-artifact@v4 + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: name: pr-report path: pr-report @@ -34,11 +43,40 @@ jobs: - name: Label and comment if: steps.download.outcome == 'success' - uses: actions/github-script@v7 + uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7 with: script: | const fs = require('fs'); const { owner, repo } = context.repo; + const run = context.payload.workflow_run; + + // Which PR to write to is decided from the event payload alone, never + // from the artifact. A `pull_request` run executes the fork's own copy + // of test.yml, so every byte of that artifact is attacker-controlled: a + // PR that wrote someone else's number into it could drive labels and + // bot comments onto any issue in this repo. + const findPullRequest = async () => { + // Populated only when head and base are the same repo. + if (run.pull_requests?.length) return run.pull_requests[0].number; + const prs = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + head: `${run.head_repository.owner.login}:${run.head_branch}`, + per_page: 100, + }); + // Prefer the exact commit; a newer push leaves this run's sha behind, + // and the branch match is still the right PR. + return (prs.find((pr) => pr.head.sha === run.head_sha) ?? prs[0])?.number; + }; + + const prNumber = await findPullRequest(); + if (!prNumber) { + core.info( + `No open PR for ${run.head_repository.full_name}:${run.head_branch}; nothing to report.`, + ); + return; + } const read = (name) => { try { @@ -48,20 +86,17 @@ jobs: } }; - const prNumber = Number(read('pr_number').trim()); - if (!prNumber) { - core.info('No PR number in artifact; nothing to report.'); - return; - } - + // Advisory only. A fork controls its own copy of test.yml and could + // claim anything here; the authoritative pass/fail signal is the + // required check's own conclusion, which this cannot influence. const testsFailed = read('tests_failed').trim() === 'true'; const buildFailed = read('build_failed').trim() === 'true'; // Artifact contents come from a fork's build. Fence-breaking is the // only thing quoting it can do, but neuter it anyway. - const fence = (text, limit = 60000) => { + const fence = (text, limit) => { let out = text.replace(/`/g, 'ˋ'); - if (out.length > limit) out = out.slice(-limit); + if (out.length > limit) out = `…(truncated)\n${out.slice(-limit)}`; return out.trim() || '(no output)'; }; @@ -97,42 +132,60 @@ jobs: const sections = []; if (testsFailed) { - sections.push( - `⚠️ **Tests failed!**\n\n
Test Output\n\n\`\`\`\n` + - `${fence(read('test_output.log'))}\n\`\`\`\n
` - ); + sections.push({ + heading: '⚠️ **Tests failed!**', + summary: 'Test Output', + text: read('test_output.log'), + }); } if (buildFailed) { - sections.push( - `❌ **Build failed!**\n\n
Build Output\n\n\`\`\`\n` + - `${fence(read('build_output.log'))}\n\`\`\`\n
` - ); + sections.push({ + heading: '❌ **Build failed!**', + summary: 'Build Output', + text: read('build_output.log'), + }); } else { const metrics = read('build_output.log') .split('\n') .filter((line) => line.includes('dist/assets/')) .join('\n'); - sections.push( - `✅ **Build successful!**\n\n
Build Size Metrics\n\n\`\`\`\n` + - `${fence(metrics || 'Metrics not found')}\n\`\`\`\n
` - ); + sections.push({ + heading: '✅ **Build successful!**', + summary: 'Build Size Metrics', + text: metrics || 'Metrics not found', + }); } + // GitHub rejects comment bodies over 65536 characters. Budget is shared + // across sections rather than applied per section, so tests and build + // both failing cannot add up past the cap. + const BODY_LIMIT = 60000; + const OVERHEAD = 600; + const perSection = Math.floor((BODY_LIMIT - OVERHEAD) / sections.length); + // One comment per PR, edited in place, so a PR with many pushes does // not accumulate a wall of near-identical reports. const MARKER = ''; - const runUrl = `${context.serverUrl}/${owner}/${repo}/actions/runs/${context.payload.workflow_run.id}`; + const runUrl = `${context.serverUrl}/${owner}/${repo}/actions/runs/${run.id}`; const body = [ MARKER, - sections.join('\n\n'), - `[Workflow run](${runUrl}) · commit ${context.payload.workflow_run.head_sha.slice(0, 7)}`, + ...sections.map( + (s) => + `${s.heading}\n\n
${s.summary}\n\n\`\`\`\n` + + `${fence(s.text, perSection)}\n\`\`\`\n
`, + ), + `[Workflow run](${runUrl}) · commit ${run.head_sha.slice(0, 7)}`, ].join('\n\n'); const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100, }); - const existing = comments.find((c) => c.body?.includes(MARKER)); + // Must be our own comment: matching the marker anywhere in any comment + // would let a PR author plant it and have their comment overwritten. + const existing = comments.find( + (c) => c.user?.login === 'github-actions[bot]' && c.body?.startsWith(MARKER), + ); if (existing) { await github.rest.issues.updateComment({ diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 75bddb29..6659f65e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,10 +26,10 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 - name: Setup Node.js - uses: actions/setup-node@v4 + uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 with: node-version: 20 cache: 'npm' @@ -56,14 +56,13 @@ jobs: npm run build > build_output.log 2>&1 || echo "BUILD_FAILED=true" >> $GITHUB_ENV cat build_output.log - # github.event.workflow_run.pull_requests is empty for forked PRs, so the - # reporting workflow cannot recover the PR number from the event payload. - # Hand it over through the artifact instead. + # Results only. This job runs the fork's own copy of this file, so nothing + # written here is trustworthy; pr-report.yml resolves the target PR from the + # workflow_run payload rather than from anything in this artifact. - name: Collect report data if: always() && github.event_name == 'pull_request' run: | mkdir -p pr-report - echo "${{ github.event.pull_request.number }}" > pr-report/pr_number echo "${TESTS_FAILED:-false}" > pr-report/tests_failed echo "${BUILD_FAILED:-false}" > pr-report/build_failed cp test_output.log pr-report/ 2>/dev/null || true @@ -71,7 +70,7 @@ jobs: - name: Upload report data if: always() && github.event_name == 'pull_request' - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 with: name: pr-report path: frontend/pr-report