hubcio commented on code in PR #4257:
URL: https://github.com/apache/iggy/pull/4257#discussion_r4070282801
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -159,32 +182,119 @@ jobs:
ls -la payload/ >&2 || true
exit 1
fi
- # Body is attacker-controlled. Route it via a file rather than
- # $GITHUB_ENV: heredoc terminators can be forged inside the body
- # to close the block early and inject arbitrary env vars (e.g.
- # NODE_OPTIONS, GITHUB_PATH) into the subsequent github-script
- # step, which holds a write GITHUB_TOKEN. The other fields below
- # are constrained single-line values (numeric ids, enum states,
- # GH login charset) and can stay in $GITHUB_ENV.
- jq -r '.comment.body // .review.body // ""' "$f" > payload/body.txt
+
+ # A forged newline in an artifact id must never reach a step output.
+ require_num() {
+ case "$1" in
+ ''|*[!0-9]*) echo "non-numeric ${2}: '${1}'" >&2; exit 1 ;;
+ esac
+ }
+
+ author=""; assoc=""; state=""; pr=""; id=""
Review Comment:
simplification: `id` only ever copies `cid`, and the review path leaves it
empty. declare `cid=""` on this line and drop `id` and line 206.
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -159,32 +182,119 @@ jobs:
ls -la payload/ >&2 || true
exit 1
fi
- # Body is attacker-controlled. Route it via a file rather than
- # $GITHUB_ENV: heredoc terminators can be forged inside the body
- # to close the block early and inject arbitrary env vars (e.g.
- # NODE_OPTIONS, GITHUB_PATH) into the subsequent github-script
- # step, which holds a write GITHUB_TOKEN. The other fields below
- # are constrained single-line values (numeric ids, enum states,
- # GH login charset) and can stay in $GITHUB_ENV.
- jq -r '.comment.body // .review.body // ""' "$f" > payload/body.txt
+
+ # A forged newline in an artifact id must never reach a step output.
+ require_num() {
+ case "$1" in
+ ''|*[!0-9]*) echo "non-numeric ${2}: '${1}'" >&2; exit 1 ;;
+ esac
+ }
+
+ author=""; assoc=""; state=""; pr=""; id=""
+ : > payload/body.txt
+
+ case "$UPSTREAM_EVENT" in
+ issue_comment)
+ cid="$(jq -r '.comment.id // ""' "$f")"
+ require_num "$cid" comment_id
+ c="$(gh api "repos/${REPO}/issues/comments/${cid}")"
Review Comment:
warning: one 502 or 429 on these new fetches aborts the step under `set -euo
pipefail`, so the command for that event is dropped. retry 5xx and 429 the way
`withRetry` does for the js path. also at lines 216, 223 and
`stale-prs-unmark-on-activity.yml:109`.
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -139,16 +143,35 @@ jobs:
github-token: ${{ secrets.GITHUB_TOKEN }}
path: payload/
- - name: Parse triage event payload
+ - name: Resolve the trigger from the API
+ id: resolve
if: github.event_name == 'workflow_run'
env:
UPSTREAM_EVENT: ${{ github.event.workflow_run.event }}
+ HEAD_SHA: ${{ github.event.workflow_run.head_sha }}
+ RUN_CREATED: ${{ github.event.workflow_run.created_at }}
+ REPO: ${{ github.repository }}
+ GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
set -euo pipefail
shopt -s nullglob
- # The artifact contains exactly one file: the JSON event payload
- # uploaded as `${{ github.event_path }}`. Its on-disk name is
- # whatever the runner used (e.g. event.json); resolve it dynamically.
+ # The collect workflow runs the PR head's own copy of itself on the
+ # pull_request_review path, so a fork can upload any payload it
likes.
+ # Nothing in the artifact is trusted for identity. It is read only
for
+ # the numeric trigger id, and every field the gate depends on -
author,
+ # association, PR number, body, review state - is re-fetched from the
+ # API below, which a fork cannot forge. A forged id resolves to a
real
+ # object (whose real properties are used) or 404s; it cannot invent a
+ # committer command. The collect run's commit must also belong to the
+ # claimed PR and the review must be as new as the run, so an old
review
+ # cannot be replayed onto another PR or onto the fork's own.
+ # The case below routes on UPSTREAM_EVENT, which GitHub sets from the
+ # collect run itself, not from the artifact. GitHub only runs
+ # issue_comment workflows from the default branch, so a fork's copy
+ # of collect can never fire on that event: a fork's run always
+ # arrives as pull_request_review and takes the checked path, while
+ # the issue_comment path only ever sees an artifact written by the
+ # default branch's collect. Any other event is rejected.
payload=( payload/*.json payload/event.json )
Review Comment:
simplification: `payload/*.json` already matches `payload/event.json`, so
the second pattern is dead and the loop always takes element 0.
`f="${payload[0]:-}"` keeps the same behavior with 3 fewer lines.
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -159,32 +182,119 @@ jobs:
ls -la payload/ >&2 || true
exit 1
fi
- # Body is attacker-controlled. Route it via a file rather than
- # $GITHUB_ENV: heredoc terminators can be forged inside the body
- # to close the block early and inject arbitrary env vars (e.g.
- # NODE_OPTIONS, GITHUB_PATH) into the subsequent github-script
- # step, which holds a write GITHUB_TOKEN. The other fields below
- # are constrained single-line values (numeric ids, enum states,
- # GH login charset) and can stay in $GITHUB_ENV.
- jq -r '.comment.body // .review.body // ""' "$f" > payload/body.txt
+
+ # A forged newline in an artifact id must never reach a step output.
+ require_num() {
+ case "$1" in
+ ''|*[!0-9]*) echo "non-numeric ${2}: '${1}'" >&2; exit 1 ;;
+ esac
+ }
+
+ author=""; assoc=""; state=""; pr=""; id=""
+ : > payload/body.txt
+
+ case "$UPSTREAM_EVENT" in
+ issue_comment)
+ cid="$(jq -r '.comment.id // ""' "$f")"
+ require_num "$cid" comment_id
+ c="$(gh api "repos/${REPO}/issues/comments/${cid}")"
+ author="$(jq -r '.user.login // ""' <<<"$c")"
+ assoc="$(jq -r '.author_association' <<<"$c")"
+ jq -r '.body // ""' <<<"$c" > payload/body.txt
+ # issue_url ends in the PR (issue) number.
+ pr="$(jq -r '.issue_url' <<<"$c" | grep -oE '[0-9]+$' || true)"
+ id="$cid"
+ ;;
+ pull_request_review)
+ rid="$(jq -r '.review.id // ""' "$f")"
+ cpr="$(jq -r '.pull_request.number // ""' "$f")"
+ require_num "$rid" review_id
+ require_num "$cpr" pr_number
+ # Fetching the review under the claimed PR verifies the pair: a
+ # review id that does not belong to that PR 404s here, so the
+ # claimed PR number cannot be steered to another PR.
+ r="$(gh api "repos/${REPO}/pulls/${cpr}/reviews/${rid}")"
+ # The pair is real but may be someone else's old review on any
+ # PR. HEAD_SHA is set by GitHub, not the fork, and is the head
+ # the collect run fired on. A forged run fires on the fork's own
Review Comment:
nit: the reason in this comment is wrong - a head-equality test is just as
loose, and line 224 is what confines a forged payload to its own PR. keep the
test, fix the wording.
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -159,32 +182,119 @@ jobs:
ls -la payload/ >&2 || true
exit 1
fi
- # Body is attacker-controlled. Route it via a file rather than
- # $GITHUB_ENV: heredoc terminators can be forged inside the body
- # to close the block early and inject arbitrary env vars (e.g.
- # NODE_OPTIONS, GITHUB_PATH) into the subsequent github-script
- # step, which holds a write GITHUB_TOKEN. The other fields below
- # are constrained single-line values (numeric ids, enum states,
- # GH login charset) and can stay in $GITHUB_ENV.
- jq -r '.comment.body // .review.body // ""' "$f" > payload/body.txt
+
+ # A forged newline in an artifact id must never reach a step output.
+ require_num() {
+ case "$1" in
+ ''|*[!0-9]*) echo "non-numeric ${2}: '${1}'" >&2; exit 1 ;;
+ esac
+ }
+
+ author=""; assoc=""; state=""; pr=""; id=""
+ : > payload/body.txt
+
+ case "$UPSTREAM_EVENT" in
+ issue_comment)
+ cid="$(jq -r '.comment.id // ""' "$f")"
+ require_num "$cid" comment_id
+ c="$(gh api "repos/${REPO}/issues/comments/${cid}")"
+ author="$(jq -r '.user.login // ""' <<<"$c")"
+ assoc="$(jq -r '.author_association' <<<"$c")"
+ jq -r '.body // ""' <<<"$c" > payload/body.txt
+ # issue_url ends in the PR (issue) number.
+ pr="$(jq -r '.issue_url' <<<"$c" | grep -oE '[0-9]+$' || true)"
+ id="$cid"
+ ;;
+ pull_request_review)
+ rid="$(jq -r '.review.id // ""' "$f")"
+ cpr="$(jq -r '.pull_request.number // ""' "$f")"
+ require_num "$rid" review_id
+ require_num "$cpr" pr_number
+ # Fetching the review under the claimed PR verifies the pair: a
+ # review id that does not belong to that PR 404s here, so the
+ # claimed PR number cannot be steered to another PR.
+ r="$(gh api "repos/${REPO}/pulls/${cpr}/reviews/${rid}")"
Review Comment:
nit: a forged review id from the artifact 404s this fetch, which fails the
step and reddens the run. skip it the way line 719 skips a missing `pulls.get`.
##########
.github/workflows/pr-triage-apply.yml:
##########
@@ -159,32 +182,119 @@ jobs:
ls -la payload/ >&2 || true
exit 1
fi
- # Body is attacker-controlled. Route it via a file rather than
- # $GITHUB_ENV: heredoc terminators can be forged inside the body
- # to close the block early and inject arbitrary env vars (e.g.
- # NODE_OPTIONS, GITHUB_PATH) into the subsequent github-script
- # step, which holds a write GITHUB_TOKEN. The other fields below
- # are constrained single-line values (numeric ids, enum states,
- # GH login charset) and can stay in $GITHUB_ENV.
- jq -r '.comment.body // .review.body // ""' "$f" > payload/body.txt
+
+ # A forged newline in an artifact id must never reach a step output.
+ require_num() {
+ case "$1" in
+ ''|*[!0-9]*) echo "non-numeric ${2}: '${1}'" >&2; exit 1 ;;
+ esac
+ }
+
+ author=""; assoc=""; state=""; pr=""; id=""
+ : > payload/body.txt
+
+ case "$UPSTREAM_EVENT" in
+ issue_comment)
+ cid="$(jq -r '.comment.id // ""' "$f")"
+ require_num "$cid" comment_id
+ c="$(gh api "repos/${REPO}/issues/comments/${cid}")"
+ author="$(jq -r '.user.login // ""' <<<"$c")"
+ assoc="$(jq -r '.author_association' <<<"$c")"
+ jq -r '.body // ""' <<<"$c" > payload/body.txt
+ # issue_url ends in the PR (issue) number.
+ pr="$(jq -r '.issue_url' <<<"$c" | grep -oE '[0-9]+$' || true)"
+ id="$cid"
+ ;;
+ pull_request_review)
+ rid="$(jq -r '.review.id // ""' "$f")"
+ cpr="$(jq -r '.pull_request.number // ""' "$f")"
+ require_num "$rid" review_id
+ require_num "$cpr" pr_number
+ # Fetching the review under the claimed PR verifies the pair: a
+ # review id that does not belong to that PR 404s here, so the
+ # claimed PR number cannot be steered to another PR.
+ r="$(gh api "repos/${REPO}/pulls/${cpr}/reviews/${rid}")"
+ # The pair is real but may be someone else's old review on any
+ # PR. HEAD_SHA is set by GitHub, not the fork, and is the head
+ # the collect run fired on. A forged run fires on the fork's own
+ # commit, which is in no other PR's commit list. The review's
+ # commit_id is not compared: GitHub moves it when the branch is
+ # updated, which often happens seconds after an approval.
+ shas="$(gh api --paginate "repos/${REPO}/pulls/${cpr}/commits"
--jq '.[].sha')"
+ if ! grep -qxF "$HEAD_SHA" <<<"$shas"; then
+ echo "collect ran on ${HEAD_SHA}, which is not a commit of PR
${cpr}" >&2
+ exit 1
+ fi
+ # That leaves a fork replaying an old review on its own PR, for
+ # example a committer's /pin. RUN_CREATED is set by GitHub, and a
+ # real run is created seconds after its review is submitted.
+ sub="$(jq -r '.submitted_at // ""' <<<"$r")"
+ if [[ -z "$sub" ]]; then
+ echo "review ${rid} has no submitted_at" >&2
+ exit 1
+ fi
+ age=$(( $(date -u -d "$RUN_CREATED" +%s) - $(date -u -d "$sub"
+%s) ))
+ if (( age < -60 || age > 300 )); then
+ echo "review ${rid} was submitted ${age}s before the collect
run" >&2
Review Comment:
nit: a review that arrives more than 60 s after the run prints a negative
age, so the message reads backwards. print both timestamps, or pick the wording
from the sign.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
