Skip to content

Commit acba854

Browse files
Juice10eoghanmurrayclaude
authored
Fix security vulnerability in workflows (rrweb-io#1804)
* Fix a security hole in rrweb-io#1787 found by Arun Murugesan: "The workflow .github/workflows/eslint-check.yml contained a critical "pwn request" vulnerability that allows any GitHub user to execute arbitrary code with access to repository secrets by opening a pull request." See preactjs/compressed-size-action#54 for why that action shouldn't be used with pull_request_target This change in this PR drops compressed-size-action in favour of executing the steps ourselves in two workflows, one which produces the size artifact, and the other which reads the artifact has the permissions to write the message back to the original PR (which is in a third party repo) * The annotate action also needed pull-requests: write permission (fixes failing run 'ESLint Annotation') * ci(bundle-size): extract bundle size scripts and simplify workflow - Add `.github/scripts/measure-bundle-sizes.js` and `render-bundle-size-comment.js` to replace inline node scripts embedded in workflow YAML, improving readability and reusability - Refactor `eslint-check.yml` to use the new script files and fix checkout steps to handle both PR and non-PR triggers correctly - Refactor `pr-checks-privileged.yml` to replace the large `github-script` block with `render-bundle-size-comment.js` and the `marocchino/sticky-pull-request-comment` action; remove the now-unnecessary `pr_number.txt` artifact by reading the PR number directly from the workflow_run event - Pin `ataylorme/eslint-annotate-action` to a specific commit SHA - Add `actions: read` permission where needed for artifact downloads * ci: add fork PR support and harden workflow - Look up PR number via API when workflow_run.pull_requests is empty (GitHub leaves it empty for fork PRs), falling back gracefully - Use head SHA instead of branch name for PR checkout to avoid TOCTOU - Fix formatSignedSize to produce +0 instead of -0 for zero values - Gate comment steps on successful PR number lookup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Eoghan Murray <eoghan@getthere.ie> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bcf93ca commit acba854

4 files changed

Lines changed: 344 additions & 36 deletions

File tree

‎.github/scripts/measure-bundle-sizes.js‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
const fs = require('fs');
2+
const path = require('path');
3+
4+
const outputPath = process.argv[2];
5+
6+
if (!outputPath) {
7+
console.error(
8+
'Usage: node .github/scripts/measure-bundle-sizes.js <output-path>',
9+
);
10+
process.exit(1);
11+
}
12+
13+
const rootDir = process.cwd();
14+
const sizes = {};
15+
16+
function normalizePath(filePath) {
17+
return path.relative(rootDir, filePath).replace(/\\/g, '/');
18+
}
19+
20+
function shouldTrack(filePath) {
21+
const normalizedPath = normalizePath(filePath);
22+
23+
if (normalizedPath.startsWith('packages/packer/dist/')) {
24+
return false;
25+
}
26+
27+
return /(^|\/)dist\/[^/]+\.(js|cjs|mjs|css)$/.test(normalizedPath);
28+
}
29+
30+
function walk(dirPath) {
31+
for (const entry of fs.readdirSync(dirPath, { withFileTypes: true })) {
32+
if (entry.name === 'node_modules') {
33+
continue;
34+
}
35+
36+
const entryPath = path.join(dirPath, entry.name);
37+
38+
if (entry.isDirectory()) {
39+
walk(entryPath);
40+
continue;
41+
}
42+
43+
if (!shouldTrack(entryPath)) {
44+
continue;
45+
}
46+
47+
sizes[normalizePath(entryPath)] = fs.statSync(entryPath).size;
48+
}
49+
}
50+
51+
walk(rootDir);
52+
fs.writeFileSync(outputPath, `${JSON.stringify(sizes, null, 2)}\n`);

‎.github/scripts/render-bundle-size-comment.js‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
const fs = require('fs');
2+
3+
const prPath = process.argv[2];
4+
const basePath = process.argv[3];
5+
6+
if (!prPath || !basePath) {
7+
console.error(
8+
'Usage: node .github/scripts/render-bundle-size-comment.js <pr-sizes.json> <base-sizes.json>',
9+
);
10+
process.exit(1);
11+
}
12+
13+
function readJson(filePath) {
14+
return JSON.parse(fs.readFileSync(filePath, 'utf8'));
15+
}
16+
17+
function formatSize(bytes) {
18+
if (bytes == null) {
19+
return '-';
20+
}
21+
22+
if (Math.abs(bytes) < 1024) {
23+
return `${bytes} B`;
24+
}
25+
26+
return `${(bytes / 1024).toFixed(2)} kB`;
27+
}
28+
29+
function formatSignedSize(bytes) {
30+
const absoluteBytes = Math.abs(bytes);
31+
32+
if (absoluteBytes < 1024) {
33+
return `${bytes >= 0 ? '+' : '-'}${absoluteBytes} B`;
34+
}
35+
36+
return `${bytes >= 0 ? '+' : '-'}${(absoluteBytes / 1024).toFixed(2)} kB`;
37+
}
38+
39+
function formatDiff(diff, baseValue) {
40+
if (diff === 0) {
41+
return '-';
42+
}
43+
44+
const percentage =
45+
baseValue > 0
46+
? ` (${diff > 0 ? '+' : ''}${((diff / baseValue) * 100).toFixed(2)}%)`
47+
: '';
48+
49+
return `${formatSignedSize(diff)}${percentage}`;
50+
}
51+
52+
function getPackageName(filePath) {
53+
const match = filePath.match(/^packages\/([^/]+)\//);
54+
return match ? match[1] : '(root)';
55+
}
56+
57+
function getFileLabel(filePath, packageName) {
58+
const packagePrefix = `packages/${packageName}/dist/`;
59+
60+
if (packageName !== '(root)' && filePath.startsWith(packagePrefix)) {
61+
return filePath.slice(packagePrefix.length);
62+
}
63+
64+
return filePath;
65+
}
66+
67+
const prSizes = readJson(prPath);
68+
const baseSizes = readJson(basePath);
69+
70+
const allFiles = [
71+
...new Set([...Object.keys(prSizes), ...Object.keys(baseSizes)]),
72+
].sort();
73+
const changedFiles = allFiles.filter(
74+
(filePath) => prSizes[filePath] !== baseSizes[filePath],
75+
);
76+
77+
if (changedFiles.length === 0) {
78+
process.stdout.write('## Bundle Size Changes\n\nNo bundle size changes.\n');
79+
process.exit(0);
80+
}
81+
82+
const totalPrSize = allFiles.reduce(
83+
(sum, filePath) => sum + (prSizes[filePath] ?? 0),
84+
0,
85+
);
86+
const totalBaseSize = allFiles.reduce(
87+
(sum, filePath) => sum + (baseSizes[filePath] ?? 0),
88+
0,
89+
);
90+
const totalDiff = totalPrSize - totalBaseSize;
91+
92+
const filesByPackage = new Map();
93+
94+
for (const filePath of changedFiles) {
95+
const packageName = getPackageName(filePath);
96+
const files = filesByPackage.get(packageName) ?? [];
97+
files.push(filePath);
98+
filesByPackage.set(packageName, files);
99+
}
100+
101+
const sections = [...filesByPackage.entries()]
102+
.sort(([left], [right]) => left.localeCompare(right))
103+
.map(([packageName, files]) => {
104+
const packagePrSize = files.reduce(
105+
(sum, filePath) => sum + (prSizes[filePath] ?? 0),
106+
0,
107+
);
108+
const packageBaseSize = files.reduce(
109+
(sum, filePath) => sum + (baseSizes[filePath] ?? 0),
110+
0,
111+
);
112+
const packageDiff = packagePrSize - packageBaseSize;
113+
114+
const rows = files
115+
.map((filePath) => {
116+
const fileDiff = (prSizes[filePath] ?? 0) - (baseSizes[filePath] ?? 0);
117+
return `| \`${getFileLabel(filePath, packageName)}\` | ${formatSize(
118+
baseSizes[filePath],
119+
)} | ${formatSize(prSizes[filePath])} | ${formatDiff(
120+
fileDiff,
121+
baseSizes[filePath] ?? 0,
122+
)} |`;
123+
})
124+
.join('\n');
125+
126+
return [
127+
'<details>',
128+
`<summary>\`${packageName}\` - ${formatSize(
129+
packageBaseSize,
130+
)} -> ${formatSize(packagePrSize)} (${formatDiff(
131+
packageDiff,
132+
packageBaseSize,
133+
)})</summary>`,
134+
'',
135+
'| File | Base | PR | Diff |',
136+
'|------|------|----|------|',
137+
rows,
138+
'',
139+
'</details>',
140+
].join('\n');
141+
});
142+
143+
const body = [
144+
'## Bundle Size Changes',
145+
'',
146+
`**Size change:** ${formatDiff(
147+
totalDiff,
148+
totalBaseSize,
149+
)} | **Total size:** ${formatSize(totalPrSize)}`,
150+
'',
151+
sections.join('\n\n'),
152+
'',
153+
].join('\n');
154+
155+
process.stdout.write(body);

‎.github/workflows/eslint-check.yml‎

Lines changed: 52 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ name: ESLint Check
22

33
on:
44
push:
5-
pull_request_target:
5+
pull_request:
66

77
jobs:
88
eslint_check_upload:
@@ -12,10 +12,15 @@ jobs:
1212
name: ESLint Check and Report Upload
1313

1414
steps:
15-
- uses: actions/checkout@v4
15+
- name: Checkout pull request head
16+
if: github.event_name == 'pull_request'
17+
uses: actions/checkout@v4
1618
with:
1719
repository: ${{ github.event.pull_request.head.repo.full_name }}
18-
ref: ${{ github.head_ref }}
20+
ref: ${{ github.event.pull_request.head.sha }}
21+
- name: Checkout current branch
22+
if: github.event_name != 'pull_request'
23+
uses: actions/checkout@v4
1924
- name: Setup Node
2025
uses: actions/setup-node@v3
2126
with:
@@ -38,49 +43,60 @@ jobs:
3843
with:
3944
name: eslint_report.json
4045
path: eslint_report.json
41-
42-
annotation:
43-
# Skip the annotation action in push events
44-
if: github.event_name == 'pull_request_target'
45-
permissions:
46-
checks: write
47-
needs: eslint_check_upload
48-
runs-on: ubuntu-latest
49-
name: ESLint Annotation
50-
steps:
51-
- uses: actions/download-artifact@v4
52-
with:
53-
name: eslint_report.json
54-
- name: Annotate Code Linting Results
55-
uses: ataylorme/eslint-annotate-action@v2
46+
- name: Measure PR bundle sizes
47+
if: github.event_name == 'pull_request'
48+
run: node .github/scripts/measure-bundle-sizes.js pr-sizes.json
49+
- name: Upload PR bundle sizes
50+
if: github.event_name == 'pull_request'
51+
uses: actions/upload-artifact@v4
5652
with:
57-
repo-token: '${{ secrets.GITHUB_TOKEN }}'
58-
report-json: 'eslint_report.json'
53+
name: pr-sizes
54+
path: pr-sizes.json
5955

60-
bundle_size:
61-
# Only runs on PRs (needs a base branch to compare against)
62-
if: github.event_name == 'pull_request_target'
56+
bundle_size_build:
57+
# Only runs on PRs. Reuses the PR build from eslint_check_upload (via the
58+
# pr-sizes artifact) and only builds the base branch itself. The privileged
59+
# bundle-size-comment workflow then posts the PR comment without ever
60+
# executing fork code.
61+
if: github.event_name == 'pull_request'
62+
needs: eslint_check_upload
6363
runs-on: ubuntu-latest
6464
permissions:
65+
actions: read
6566
contents: read
66-
pull-requests: write
67-
name: Check Bundle Sizes
67+
name: Build Base for Bundle Size Comparison
6868
steps:
69+
- name: Checkout workflow ref
70+
uses: actions/checkout@v4
71+
- name: Prepare bundle size helper
72+
run: |
73+
cp .github/scripts/measure-bundle-sizes.js /tmp/measure-bundle-sizes.js
74+
# --- Base branch ---
6975
- uses: actions/checkout@v4
7076
with:
71-
repository: ${{ github.event.pull_request.head.repo.full_name }}
72-
ref: ${{ github.head_ref }}
73-
- name: Setup Node
74-
uses: actions/setup-node@v3
77+
ref: ${{ github.base_ref }}
78+
- name: Download PR bundle sizes
79+
uses: actions/download-artifact@v4
80+
with:
81+
name: pr-sizes
82+
- uses: actions/setup-node@v3
7583
with:
7684
node-version: lts/*
7785
cache: 'yarn'
78-
- name: Check bundle sizes
79-
uses: preactjs/compressed-size-action@v2
80-
with:
81-
install-script: 'yarn install --frozen-lockfile'
82-
build-script: 'build:all'
83-
compression: 'none'
84-
pattern: '**/dist/*.{js,cjs,mjs,css}'
86+
- name: Install base dependencies
87+
run: yarn install --frozen-lockfile
8588
env:
8689
PUPPETEER_SKIP_DOWNLOAD: true
90+
- name: Build base branch
91+
run: NODE_OPTIONS='--max-old-space-size=4096' yarn build:all
92+
env:
93+
PUPPETEER_SKIP_DOWNLOAD: true
94+
- name: Measure base bundle sizes
95+
run: node /tmp/measure-bundle-sizes.js base-sizes.json
96+
97+
- uses: actions/upload-artifact@v4
98+
with:
99+
name: bundle-size-data
100+
path: |
101+
pr-sizes.json
102+
base-sizes.json

0 commit comments

Comments
 (0)