Skip to content

Commit c70abac

Browse files
committed
ci: unlabel on edited body, cap ref count for pull_request_target safety
Two review findings: - On `edited` events the script now diffs `payload.changes.body.from` against the new body and removes 'in-progress' from any issue whose Closes/Fixes/Resolves reference was deleted. Previously, editing a PR to drop `Closes #42` left #42 shielded from the stale bot indefinitely. - Cap the number of references processed per event at 50 (`MAX_REFS`). `pull_request_target` runs on PRs from forks, so an accidental or malicious PR body with thousands of matches would burn the repo's REST budget on labeling calls. Also adds a one-line comment noting that cross-repo refs (owner/repo#N) are intentionally out of scope; this workflow only labels issues in the current repo.
1 parent 74edb67 commit c70abac

1 file changed

Lines changed: 52 additions & 20 deletions

File tree

.github/workflows/link-issues-to-prs.yml

Lines changed: 52 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -32,41 +32,73 @@ jobs:
3232
const pr = context.payload.pull_request;
3333
const body = pr.body || '';
3434
const re = /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+#(\d+)/gi;
35-
const nums = [...new Set([...body.matchAll(re)].map(m => Number(m[1])))];
36-
if (nums.length === 0) {
37-
core.info('No Closes/Fixes/Resolves references found; nothing to do.');
38-
return;
35+
// Cross-repo references (owner/repo#123) are intentionally skipped;
36+
// this workflow only labels issues in the current repo.
37+
const MAX_REFS = 50; // cap runaway PR bodies from forks (pull_request_target).
38+
const extract = (text) => [...new Set(
39+
[...(text || '').matchAll(re)].map(m => Number(m[1]))
40+
)].slice(0, MAX_REFS);
41+
42+
const current = extract(body);
43+
44+
// On `edited`, compute which references were REMOVED so their
45+
// labels come off. Without this, a PR that once said "Closes #42"
46+
// and no longer does would leave #42 shielded indefinitely.
47+
let removed = [];
48+
if (context.payload.action === 'edited') {
49+
const prevBody = context.payload.changes?.body?.from;
50+
if (prevBody !== undefined) {
51+
const previous = extract(prevBody);
52+
const now = new Set(current);
53+
removed = previous.filter(n => !now.has(n));
54+
}
3955
}
4056
4157
// Apply the label while the PR is open. Remove it when the PR
4258
// closes without merging (merged PRs also close, but the issue
4359
// will be auto-closed by GitHub once the merge lands, so the
44-
// in-progress label on it is harmless).
60+
// in-progress label on it is harmless). Also remove on `edited`
61+
// when a reference was deleted from the body.
4562
const shouldLabel = pr.state === 'open';
46-
const shouldUnlabel = pr.state === 'closed' && !pr.merged;
63+
const closeUnlabel = pr.state === 'closed' && !pr.merged ? current : [];
64+
const toUnlabel = [...new Set([...removed, ...closeUnlabel])];
4765
48-
for (const n of nums) {
49-
try {
50-
if (shouldLabel) {
66+
if (current.length === 0 && toUnlabel.length === 0) {
67+
core.info('No Closes/Fixes/Resolves references to process; nothing to do.');
68+
return;
69+
}
70+
71+
const removeLabelSafe = async (n) => {
72+
await github.rest.issues.removeLabel({
73+
...context.repo,
74+
issue_number: n,
75+
name: 'in-progress',
76+
}).catch(err => {
77+
// 404 just means the label was not present; not an error.
78+
if (err.status !== 404) throw err;
79+
});
80+
core.info(`#${n}: removed in-progress`);
81+
};
82+
83+
if (shouldLabel) {
84+
for (const n of current) {
85+
try {
5186
await github.rest.issues.addLabels({
5287
...context.repo,
5388
issue_number: n,
5489
labels: ['in-progress'],
5590
});
5691
core.info(`#${n}: added in-progress`);
57-
} else if (shouldUnlabel) {
58-
await github.rest.issues.removeLabel({
59-
...context.repo,
60-
issue_number: n,
61-
name: 'in-progress',
62-
}).catch(err => {
63-
// 404 just means the label was not present; not an error.
64-
if (err.status !== 404) throw err;
65-
});
66-
core.info(`#${n}: removed in-progress (PR closed unmerged)`);
92+
} catch (err) {
93+
core.warning(`#${n}: ${err.message}`);
6794
}
95+
}
96+
}
97+
98+
for (const n of toUnlabel) {
99+
try {
100+
await removeLabelSafe(n);
68101
} catch (err) {
69-
// Do not fail the whole run on one bad reference; log and continue.
70102
core.warning(`#${n}: ${err.message}`);
71103
}
72104
}

0 commit comments

Comments
 (0)