Skip to content

Commit 836e053

Browse files
committed
Harden Explore triage writer trust boundaries
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 512eb347-ec89-4250-8bf1-87048974b01d
1 parent 1cb527c commit 836e053

1 file changed

Lines changed: 171 additions & 14 deletions

File tree

.github/workflows/explore-triage-commenter-writer.yml

Lines changed: 171 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,85 @@ jobs:
2727
GH_TOKEN: ${{ github.token }}
2828
REPOSITORY: ${{ github.repository }}
2929
RUN_ID: ${{ github.event.workflow_run.id }}
30+
MAX_ARTIFACT_BYTES: 1048576
31+
MAX_JSON_BYTES: 131072
3032
run: |
3133
set -euo pipefail
3234
mkdir -p "$RUNNER_TEMP/explore-triage"
33-
artifact_id="$(gh api "repos/$REPOSITORY/actions/runs/$RUN_ID/artifacts" \
34-
--jq '.artifacts[] | select(.name == "explore-triage-comment" and .expired == false) | .id' | head -n 1)"
35-
if [ -z "$artifact_id" ]; then
35+
artifact_rows="$(
36+
gh api --paginate "repos/$REPOSITORY/actions/runs/$RUN_ID/artifacts" \
37+
--jq '.artifacts[] | select(.name == "explore-triage-comment" and .expired == false) | [.id, .size_in_bytes] | @tsv'
38+
)"
39+
artifacts=()
40+
if [ -n "$artifact_rows" ]; then
41+
mapfile -t artifacts <<< "$artifact_rows"
42+
fi
43+
if (( ${#artifacts[@]} == 0 )); then
3644
echo "No explore triage artifact found for run $RUN_ID"
3745
echo "found=false" >> "$GITHUB_OUTPUT"
3846
exit 0
3947
fi
40-
gh api "repos/$REPOSITORY/actions/artifacts/$artifact_id/zip" > "$RUNNER_TEMP/explore-triage/artifact.zip"
41-
unzip -p "$RUNNER_TEMP/explore-triage/artifact.zip" explore-triage-comment.json > "$RUNNER_TEMP/explore-triage/comment.json"
48+
if (( ${#artifacts[@]} != 1 )); then
49+
echo "Expected exactly one explore triage artifact, found ${#artifacts[@]}" >&2
50+
exit 1
51+
fi
52+
53+
IFS=$'\t' read -r artifact_id artifact_size <<< "${artifacts[0]}"
54+
if ! [[ "$artifact_id" =~ ^[0-9]+$ && "$artifact_size" =~ ^[0-9]+$ ]]; then
55+
echo "Artifact metadata is invalid" >&2
56+
exit 1
57+
fi
58+
if (( artifact_size == 0 || artifact_size > MAX_ARTIFACT_BYTES )); then
59+
echo "Artifact archive size $artifact_size is outside the allowed range" >&2
60+
exit 1
61+
fi
62+
63+
artifact_zip="$RUNNER_TEMP/explore-triage/artifact.zip"
64+
comment_json="$RUNNER_TEMP/explore-triage/comment.json"
65+
gh api "repos/$REPOSITORY/actions/artifacts/$artifact_id/zip" > "$artifact_zip"
66+
67+
actual_artifact_size="$(stat -c '%s' "$artifact_zip")"
68+
if ! [[ "$actual_artifact_size" =~ ^[0-9]+$ ]] ||
69+
(( actual_artifact_size == 0 || actual_artifact_size > MAX_ARTIFACT_BYTES )); then
70+
echo "Downloaded artifact archive size $actual_artifact_size is outside the allowed range" >&2
71+
exit 1
72+
fi
73+
74+
ARTIFACT_ZIP="$artifact_zip" COMMENT_JSON="$comment_json" python3 - <<'PY'
75+
import os
76+
import zipfile
77+
78+
archive_path = os.environ["ARTIFACT_ZIP"]
79+
output_path = os.environ["COMMENT_JSON"]
80+
max_json_bytes = int(os.environ["MAX_JSON_BYTES"])
81+
expected_name = "explore-triage-comment.json"
82+
83+
with zipfile.ZipFile(archive_path) as archive:
84+
matches = [entry for entry in archive.infolist() if entry.filename == expected_name]
85+
if len(matches) != 1:
86+
raise ValueError(f"Expected exactly one {expected_name} entry, found {len(matches)}")
87+
88+
entry = matches[0]
89+
if entry.is_dir() or entry.flag_bits & 0x1:
90+
raise ValueError("Artifact JSON entry must be an unencrypted regular file")
91+
if entry.file_size == 0 or entry.file_size > max_json_bytes:
92+
raise ValueError(
93+
f"Artifact JSON entry size {entry.file_size} is outside the allowed range"
94+
)
95+
96+
total = 0
97+
with archive.open(entry) as source, open(output_path, "xb") as destination:
98+
while chunk := source.read(65536):
99+
total += len(chunk)
100+
if total > max_json_bytes:
101+
raise ValueError("Artifact JSON exceeded the allowed size while extracting")
102+
destination.write(chunk)
103+
104+
if total != entry.file_size:
105+
raise ValueError(
106+
f"Extracted JSON size {total} did not match declared size {entry.file_size}"
107+
)
108+
PY
42109
echo "found=true" >> "$GITHUB_OUTPUT"
43110
44111
- name: Upsert sticky comment
@@ -59,22 +126,38 @@ jobs:
59126
60127
const data = JSON.parse(fs.readFileSync(process.env.COMMENT_DATA_PATH, 'utf8'));
61128
validateIdentity(data);
129+
validatePayload(data);
62130
63131
const runHeadSha = await getWorkflowRunHeadSha(run);
64132
if (!/^[0-9a-f]{40}$/i.test(runHeadSha)) {
65133
throw new Error(`Workflow run head SHA is invalid: ${runHeadSha}`);
66134
}
67135
136+
const associatedPrNumber = await getAssociatedPullRequestNumber(run, runHeadSha);
137+
if (associatedPrNumber === null) return;
138+
if (data.prNumber !== associatedPrNumber) {
139+
core.info(`Artifact PR #${data.prNumber} is not the PR associated with workflow run ${run.id}; skipping.`);
140+
return;
141+
}
142+
68143
const { data: pr } = await github.rest.pulls.get({
69144
owner,
70145
repo,
71-
pull_number: data.prNumber,
146+
pull_number: associatedPrNumber,
72147
});
73148
74149
if (pr.head.sha !== runHeadSha) {
75150
core.info(`PR #${pr.number} head ${pr.head.sha} does not match workflow run head ${runHeadSha}; skipping.`);
76151
return;
77152
}
153+
if (run.head_repository && pr.head.repo && pr.head.repo.full_name !== run.head_repository.full_name) {
154+
core.info(`PR #${pr.number} head repository does not match the workflow run; skipping.`);
155+
return;
156+
}
157+
if (run.head_branch && pr.head.ref !== run.head_branch) {
158+
core.info(`PR #${pr.number} head branch does not match the workflow run; skipping.`);
159+
return;
160+
}
78161
if (pr.base.repo.full_name !== expectedRepo) {
79162
throw new Error(`Unexpected base repo: ${pr.base.repo.full_name}`);
80163
}
@@ -91,8 +174,10 @@ jobs:
91174
return;
92175
}
93176
94-
validatePayload(data);
95177
const body = renderComment(data);
178+
if (Buffer.byteLength(body, 'utf8') > 60000) {
179+
throw new Error('Rendered triage comment exceeds the allowed size.');
180+
}
96181
97182
const comments = await github.paginate(github.rest.issues.listComments, {
98183
owner,
@@ -135,14 +220,57 @@ jobs:
135220
return workflowRun.head_sha;
136221
}
137222
223+
async function getAssociatedPullRequestNumber(run, runHeadSha) {
224+
const runPullRequests = Array.isArray(run.pull_requests) ? run.pull_requests : [];
225+
if (runPullRequests.length > 0) {
226+
const associatedNumbers = runPullRequests
227+
.map(pull => pull && pull.number)
228+
.filter(number => Number.isSafeInteger(number) && number > 0);
229+
if (!associatedNumbers.includes(data.prNumber)) {
230+
core.info(`Artifact PR #${data.prNumber} is not in the workflow run pull request association; skipping.`);
231+
return null;
232+
}
233+
return data.prNumber;
234+
}
235+
236+
const headRepo = run.head_repository;
237+
const headBranch = run.head_branch;
238+
const headOwner = headRepo && headRepo.owner && headRepo.owner.login;
239+
if (!headRepo || typeof headRepo.full_name !== 'string' ||
240+
typeof headOwner !== 'string' || typeof headBranch !== 'string' ||
241+
headOwner.length === 0 || headBranch.length === 0) {
242+
throw new Error('Workflow run is missing the trusted head repository or branch association.');
243+
}
244+
245+
const candidates = await github.paginate(github.rest.pulls.list, {
246+
owner,
247+
repo,
248+
state: 'all',
249+
head: `${headOwner}:${headBranch}`,
250+
per_page: 100,
251+
});
252+
const matches = candidates.filter(pull =>
253+
Number.isSafeInteger(pull.number) &&
254+
pull.head && pull.head.sha === runHeadSha &&
255+
pull.head.ref === headBranch &&
256+
pull.head.repo && pull.head.repo.full_name === headRepo.full_name &&
257+
pull.base && pull.base.repo && pull.base.repo.full_name === expectedRepo
258+
);
259+
if (matches.length !== 1) {
260+
core.warning(`Could not uniquely associate workflow run ${run.id} with a pull request; found ${matches.length} matches.`);
261+
return null;
262+
}
263+
return matches[0].number;
264+
}
265+
138266
function validateIdentity(data) {
139267
if (!data || data.schema !== 'explore-triage-comment/v1') {
140268
throw new Error('Unexpected artifact schema.');
141269
}
142270
if (data.owner !== owner || data.repo !== repo) {
143271
throw new Error(`Artifact repo mismatch: ${data.owner}/${data.repo}`);
144272
}
145-
if (!Number.isInteger(data.prNumber) || data.prNumber <= 0) {
273+
if (!Number.isSafeInteger(data.prNumber) || data.prNumber <= 0) {
146274
throw new Error(`Artifact PR number is invalid: ${data.prNumber}`);
147275
}
148276
if (data.baseRepoFullName !== `${owner}/${repo}`) {
@@ -151,6 +279,9 @@ jobs:
151279
if (!/^[0-9a-f]{40}$/i.test(data.headSha)) {
152280
throw new Error('Artifact head SHA is invalid.');
153281
}
282+
if (typeof data.hasChanges !== 'boolean') {
283+
throw new Error('Artifact hasChanges flag is invalid.');
284+
}
154285
}
155286
156287
function validatePayload(data) {
@@ -161,16 +292,27 @@ jobs:
161292
throw new Error('Artifact contains too many topic or collection entries.');
162293
}
163294
for (const topic of data.topics) {
295+
if (!topic || typeof topic !== 'object') {
296+
throw new Error('Invalid topic entry.');
297+
}
164298
validateSlug(topic.slug);
165-
if (topic.count !== null && (!Number.isInteger(topic.count) || topic.count < 0)) {
299+
if (topic.count !== null && (!Number.isSafeInteger(topic.count) || topic.count < 0)) {
166300
throw new Error(`Invalid topic count for ${topic.slug}.`);
167301
}
168302
}
169303
for (const collection of data.collections) {
304+
if (!collection || typeof collection !== 'object') {
305+
throw new Error('Invalid collection entry.');
306+
}
170307
validateSlug(collection.slug);
171308
if (!['ok', 'not-found', 'error'].includes(collection.readStatus)) {
172309
throw new Error(`Invalid read status for ${collection.slug}.`);
173310
}
311+
validateOptionalStatusToken(
312+
collection.errorStatus,
313+
`collection ${collection.slug}`,
314+
collection.readStatus !== 'ok'
315+
);
174316
if (!Array.isArray(collection.items) || collection.items.length > 500) {
175317
throw new Error(`Invalid item list for ${collection.slug}.`);
176318
}
@@ -185,11 +327,11 @@ jobs:
185327
}
186328
187329
function validateItem(item) {
188-
if (!item || typeof item.name !== 'string' || item.name.length > 140) {
330+
if (!item || typeof item.name !== 'string' || item.name.length === 0 || item.name.length > 140) {
189331
throw new Error('Invalid item name.');
190332
}
191333
if (item.valid === false) {
192-
if (!/^[A-Za-z0-9._/-]+$/.test(item.name)) {
334+
if (/[\u0000-\u001f\u007f-\u009f\u2028\u2029]/u.test(item.name)) {
193335
throw new Error(`Unsafe invalid item token: ${item.name}`);
194336
}
195337
return;
@@ -200,11 +342,16 @@ jobs:
200342
if (!['ok', 'not-found', 'error'].includes(item.lookupStatus)) {
201343
throw new Error(`Invalid lookup status for ${item.name}.`);
202344
}
345+
validateOptionalStatusToken(
346+
item.errorStatus,
347+
`item ${item.name}`,
348+
item.lookupStatus !== 'ok'
349+
);
203350
if (item.lookupStatus === 'ok') {
204-
if (!Number.isInteger(item.stars) || item.stars < 0) throw new Error(`Invalid stars for ${item.name}.`);
351+
if (!Number.isSafeInteger(item.stars) || item.stars < 0) throw new Error(`Invalid stars for ${item.name}.`);
205352
if (item.pushed !== null && !/^\d{4}-\d{2}-\d{2}$/.test(item.pushed)) throw new Error(`Invalid pushed date for ${item.name}.`);
206-
if (typeof item.ownerType !== 'string' || !/^[A-Za-z]+$/.test(item.ownerType)) throw new Error(`Invalid owner type for ${item.name}.`);
207-
if (!Array.isArray(item.notes)) throw new Error(`Invalid notes for ${item.name}.`);
353+
if (typeof item.ownerType !== 'string' || !/^[A-Za-z]{1,32}$/.test(item.ownerType)) throw new Error(`Invalid owner type for ${item.name}.`);
354+
if (!Array.isArray(item.notes) || item.notes.length > 3) throw new Error(`Invalid notes for ${item.name}.`);
208355
for (const note of item.notes) {
209356
if (!['possible-self-submission', 'archived', 'disabled'].includes(note)) {
210357
throw new Error(`Invalid note for ${item.name}: ${note}`);
@@ -213,6 +360,16 @@ jobs:
213360
}
214361
}
215362
363+
function validateOptionalStatusToken(value, label, required) {
364+
if (value === null || value === undefined) {
365+
if (required) throw new Error(`Missing error status for ${label}.`);
366+
return;
367+
}
368+
if (typeof value !== 'string' || !/^[A-Za-z0-9_-]{1,32}$/.test(value)) {
369+
throw new Error(`Invalid error status for ${label}.`);
370+
}
371+
}
372+
216373
function renderComment(data) {
217374
const sections = [];
218375

0 commit comments

Comments
 (0)