Skip to content
This repository was archived by the owner on Jul 10, 2026. It is now read-only.

Commit 4231b4b

Browse files
alejoamirasalejoamiras
authored
ci: fix commenting on prs from forks (#18)
The `peter-evans/create-or-update-comment` action fails on fork PRs with "Resource not accessible by integration" due to GitHub Actions token restrictions. ### Solution Implemented secure `workflow_run` approach following [GitHub Security Lab best practices](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/). ### Security Architecture Split into two isolated workflows: - **`benchmark.yml`**: Uses `pull_request` trigger (limited permissions, safe for untrusted fork code) - **`comment-benchmark.yml`**: Uses `workflow_run` trigger (elevated permissions, **never checks out PR code**) ### Execution Context - **`benchmark.yml`**: Runs in fork context with limited permissions, can safely build untrusted code - **`comment-benchmark.yml`**: Runs in target repo context with write permissions, only processes trusted artifacts - **Data transfer**: Secure artifact-based communication prevents secret exposure ### Technical Implementation 1. **Benchmark workflow**: `pull_request` trigger, builds both base and PR code, uploads results as artifacts 2. **Comment workflow**: `workflow_run` trigger, downloads artifacts, posts comments on PRs 3. **Change detection**: GitHub API-based file change detection (works with fork PRs) 4. **Conditional execution**: Only runs benchmarks when Noir contracts, config, or Aztec version changes ### Result - ✅ **Fork PRs**: Now receive automated benchmark comments - ✅ **Same-repo PRs**: Continue working as before - ✅ **Security**: No PWN request vulnerabilities - ✅ **Compliance**: Follows GitHub Security Lab guidelines
1 parent e0afda4 commit 4231b4b

2 files changed

Lines changed: 173 additions & 33 deletions

File tree

.github/workflows/compare-benchmark.yml renamed to .github/workflows/benchmark.yml

Lines changed: 58 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,51 +1,61 @@
1-
name: Aztec Benchmark Diff
1+
name: Aztec Benchmark
22

33
on:
44
pull_request:
5-
6-
permissions:
7-
contents: read
8-
pull-requests: write
5+
types: [opened, synchronize, reopened]
96

107
env:
118
BENCH_DIR: ./benchmarks
129

1310
jobs:
1411
check-changes:
15-
name: Check for benchmark-relevant changes
12+
name: Check for relevant changes
1613
runs-on: ubuntu-latest
1714
outputs:
1815
should-benchmark: ${{ steps.changes.outputs.should-benchmark }}
1916
steps:
2017
- name: Checkout repository
2118
uses: actions/checkout@v4
22-
with:
23-
fetch-depth: 0
2419

2520
- name: Check for relevant changes
2621
id: changes
27-
run: |
28-
echo "Checking for changes that would affect benchmarks..."
29-
30-
# Check if Noir contracts, config, or benchmarks changed
31-
if git diff --name-only ${{ github.event.pull_request.base.sha }} HEAD | grep -E '^(src/nr/|Nargo\.toml|benchmarks/)'; then
32-
echo "📝 Noir contracts, config, or benchmark files changed"
33-
echo "should-benchmark=true" >> $GITHUB_OUTPUT
34-
else
35-
echo "📁 No Noir contract, config, or benchmark changes detected"
22+
uses: actions/github-script@v7
23+
with:
24+
script: |
25+
console.log('Checking for changes that would affect benchmarks...');
3626
37-
# Check for AZTEC_VERSION change in package.json
38-
if git diff ${{ github.event.pull_request.base.sha }} HEAD package.json | grep -q 'aztecVersion'; then
39-
echo "🔄 AZTEC_VERSION changed in package.json"
40-
echo "should-benchmark=true" >> $GITHUB_OUTPUT
41-
else
42-
echo "⏭️ No benchmark-relevant changes detected, skipping benchmarks"
43-
echo "should-benchmark=false" >> $GITHUB_OUTPUT
44-
fi
45-
fi
27+
const { data: files } = await github.rest.pulls.listFiles({
28+
owner: context.repo.owner,
29+
repo: context.repo.repo,
30+
pull_number: context.issue.number,
31+
});
32+
33+
console.log('Changed files:');
34+
files.forEach(file => console.log(`- ${file.filename}`));
35+
36+
// Check for Noir contracts, config, or benchmark changes
37+
const relevantPatterns = /^(src\/nr\/|Nargo\.toml|benchmarks\/)/;
38+
const hasRelevantChanges = files.some(file => relevantPatterns.test(file.filename));
39+
40+
// Check for AZTEC_VERSION change in package.json
41+
const hasAztecVersionChange = files.some(file =>
42+
file.filename === 'package.json' && file.patch?.includes('aztecVersion')
43+
);
44+
45+
const shouldBenchmark = hasRelevantChanges || hasAztecVersionChange;
46+
47+
if (hasRelevantChanges) {
48+
console.log('📝 Noir contracts, config, or benchmark files changed');
49+
} else if (hasAztecVersionChange) {
50+
console.log('🔄 AZTEC_VERSION changed in package.json');
51+
} else {
52+
console.log('⏭️ No benchmark-relevant changes detected, skipping benchmarks');
53+
}
54+
55+
core.setOutput('should-benchmark', shouldBenchmark);
4656
4757
benchmark:
48-
name: Run benchmark comparison
58+
name: Run comparison
4959
needs: check-changes
5060
if: needs.check-changes.outputs.should-benchmark == 'true'
5161
runs-on: ubuntu-latest
@@ -218,16 +228,31 @@ jobs:
218228
run: script -e -c "aztec codegen target --outdir src/artifacts --force"
219229

220230
# ──────────────────────────────────────────────────────────────
221-
# 3️⃣ DIFF & COMMENT
231+
# 3️⃣ DIFF & UPLOAD ARTIFACTS
222232
# ──────────────────────────────────────────────────────────────
223-
- name: Generate Markdown diff
233+
234+
- name: Generate benchmark comparison
224235
uses: defi-wonderland/aztec-benchmark/action@main
225236
with:
226237
base_suffix: '_base'
227238
current_suffix: '_pr'
228239

229-
- name: Comment diff
230-
uses: peter-evans/create-or-update-comment@v4
240+
# SECURITY: Upload results for secure comment workflow
241+
- name: Upload benchmark results and metadata
242+
uses: actions/upload-artifact@v4
243+
with:
244+
name: benchmark-results
245+
path: |
246+
benchmark-comparison.md
247+
retention-days: 1
248+
249+
- name: Save PR number for comment workflow
250+
run: |
251+
echo "${{ github.event.number }}" > pr-number.txt
252+
253+
- name: Upload PR metadata
254+
uses: actions/upload-artifact@v4
231255
with:
232-
issue-number: ${{ github.event.pull_request.number }}
233-
body-file: benchmark-comparison.md
256+
name: pr-metadata
257+
path: pr-number.txt
258+
retention-days: 1

‎.github/workflows/comment-benchmark.yml‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
name: Comment Results
2+
3+
# SECURITY: workflow_run trigger = elevated permissions, no fork code access
4+
on:
5+
workflow_run:
6+
workflows: ["Aztec Benchmark"]
7+
types:
8+
- completed
9+
10+
permissions:
11+
contents: read
12+
pull-requests: write # Elevated permissions for commenting
13+
14+
jobs:
15+
comment:
16+
name: Comment results
17+
runs-on: ubuntu-latest
18+
if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success'
19+
20+
steps:
21+
# SECURITY: Never checkout PR code in this workflow
22+
# This job has elevated permissions but only handles trusted artifacts
23+
24+
- name: Download benchmark results
25+
id: download-results
26+
uses: actions/github-script@v7
27+
with:
28+
script: |
29+
// Download benchmark results artifact
30+
const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
31+
owner: context.repo.owner,
32+
repo: context.repo.repo,
33+
run_id: ${{ github.event.workflow_run.id }},
34+
});
35+
36+
const benchmarkArtifact = artifacts.data.artifacts.find(artifact =>
37+
artifact.name === 'benchmark-results'
38+
);
39+
40+
if (benchmarkArtifact) {
41+
const download = await github.rest.actions.downloadArtifact({
42+
owner: context.repo.owner,
43+
repo: context.repo.repo,
44+
artifact_id: benchmarkArtifact.id,
45+
archive_format: 'zip',
46+
});
47+
48+
const fs = require('fs');
49+
fs.writeFileSync('benchmark-results.zip', Buffer.from(download.data));
50+
console.log('Downloaded benchmark results artifact');
51+
} else {
52+
console.log('No benchmark results found - likely skipped due to no relevant changes');
53+
core.setOutput('has-results', 'false');
54+
return;
55+
}
56+
57+
core.setOutput('has-results', 'true');
58+
59+
- name: Extract benchmark results
60+
if: steps.download-results.outputs.has-results == 'true'
61+
id: extract-results
62+
run: |
63+
unzip -q benchmark-results.zip
64+
if [[ -f benchmark-comparison.md ]]; then
65+
echo "results-available=true" >> $GITHUB_OUTPUT
66+
echo "Found benchmark comparison results"
67+
else
68+
echo "results-available=false" >> $GITHUB_OUTPUT
69+
echo "No benchmark comparison file found"
70+
fi
71+
72+
- name: Download PR metadata
73+
uses: actions/github-script@v7
74+
with:
75+
script: |
76+
// Download PR metadata artifact
77+
const artifacts = await github.rest.actions.listWorkflowRunArtifacts({
78+
owner: context.repo.owner,
79+
repo: context.repo.repo,
80+
run_id: ${{ github.event.workflow_run.id }},
81+
});
82+
83+
const metadataArtifact = artifacts.data.artifacts.find(artifact =>
84+
artifact.name === 'pr-metadata'
85+
);
86+
87+
if (metadataArtifact) {
88+
const download = await github.rest.actions.downloadArtifact({
89+
owner: context.repo.owner,
90+
repo: context.repo.repo,
91+
artifact_id: metadataArtifact.id,
92+
archive_format: 'zip',
93+
});
94+
95+
const fs = require('fs');
96+
fs.writeFileSync('pr-metadata.zip', Buffer.from(download.data));
97+
console.log('Downloaded PR metadata artifact');
98+
} else {
99+
throw new Error('PR metadata artifact not found');
100+
}
101+
102+
- name: Extract PR number
103+
id: extract-pr
104+
run: |
105+
unzip -q pr-metadata.zip
106+
PR_NUMBER=$(cat pr-number.txt)
107+
echo "pr-number=$PR_NUMBER" >> $GITHUB_OUTPUT
108+
echo "Found PR number: $PR_NUMBER"
109+
110+
- name: Comment results
111+
if: steps.extract-results.outputs.results-available == 'true'
112+
uses: peter-evans/create-or-update-comment@v4
113+
with:
114+
issue-number: ${{ steps.extract-pr.outputs.pr-number }}
115+
body-file: benchmark-comparison.md

0 commit comments

Comments
 (0)