Skip to content

Review 5517

Jump to bottom
Cindy Zhang edited this page Aug 26, 2026 · 2 revisions

#5517 — ci: derive PR-comment inputs from the workflow_run context

bhamodi · open, request changes drafted · reviewed at b4124d48505 · view on GitHub

Verdict: request changes — the shell hole is genuinely closed, and the replacement lookup resolves nothing on a fork

The review asks for one thing: resolve the PR number a way that works for a fork head, because GET /commits/{sha}/pulls matches only head branches in this repository and returns [] for every fork PR, so the analysis comment disappears from the PRs the workflow was written to serve. It did not merge because that is a visible regression for the majority of open PRs, and nothing on the PR page would say it had happened.

Problem

The PR Analysis Report job runs on workflow_run with pull-requests: write and built a shell command line out of strings it read back from the pr-analysis artifact. JSON.stringify quotes for JSON, not for sh, so $(…) inside a value survives into the command. Verified in a real shell: execSync on https://evil.example/$(echo PWNED)/ delivers https://evil.example/PWNED/, while execFileSync delivers the literal string.

The person is a fork contributor. A pull_request run executes the PR's own workflow definition, so a fork PR that edits ci.yml can write anything into pr-meta.json and the privileged job then runs it — and for anyone with a merged PR, no maintainer approval is needed. That is command execution in the base repo's job, and the ability to rewrite the bot comment on any PR in the repo. The PR body describes the change and never says the old behaviour was exploitable; the header comment the diff adds does state the rule — "derived from the workflow_run context, never read from the artifact… invoked without a shell" — so the rationale is recorded in the file rather than the body.

Solution

(2 decisions · +34 −13, one file, all of it one embedded script)

  1. Resolve the PR number and the three preview URLs from workflow_run rather than from pr-meta.json — the trust boundary.
  2. execSync(<string>) → execFileSync('node', args) — the shell.

The job used to ask the report artifact two questions: which PR is this, and where do its previews live. It now asks the event that woke it, and hands the generator its arguments directly rather than writing out a command line for a shell to re-parse. The artifact goes back to being report data only. Both decisions trace to the stated problem and they ship together — dropping the shell is only sufficient once the values stop coming from an untrusted place.

The derivation lives with its owner, the privileged reporting job in .github/workflows/pr-comment.yml. The defect is that the resolver it chose cannot see the one seam this workflow exists for; the workflow's own header says it was written because "the report never appeared on external-contributor PRs". Driven against the live API at this head rather than read off the source: a same-repo PR resolves (/commits/<sha>/pulls → [5538], [5268,5536], [5268,5535,5536]); a fork PR fails, with workflow_run.pull_requests empty and /commits/<sha>/pulls returning [] on #5517, #5518 and #5521; a docsite-only PR is unchanged, returning before the lookup; and a force-push mid-run is new behaviour — the stale run posts nothing rather than an outdated comment.

Decision 2 is the fix rather than a fix: pr-comment.yml:138 was the only shell interpolation in any workflow's inline script (git grep execSync -- .github/workflows returns that one line), and deploy-preview.yml already validates its two artifact fields with a regex, so the class is closed for the privileged jobs. The residue — analysis.json content reaching the comment body unescaped — is #5521's subject and already open.

Impact

Every fork PR loses its PR Analysis Report: no Storybook link, no Sandbox link, no bundle table, no a11y summary. An outside contributor opens a PR and the thing that tells them whether their change is fine never appears, with no signal that anything is missing, because the job exits on a warning nobody reads.

98 of the roughly 180 open PRs today are from forks — 34 from one account, 18, 17 and 17 from three more, and eight other accounts behind them. This PR is one of them, so on its own next push it would silence its own report. Same-repo PRs are unaffected, and nothing user-facing ships: no package, no runtime, no npm consumer.

What landing it newly exposes: pr-meta.json keeps storybookUrl and sandboxUrl fields that nothing reads any more, and the gh-pages URL layout is spelled in a second file.

API

No API change — a CI workflow, no package, no export, no prop, no type.

Theme targets

n/a — structural only, and not source at all. The style grep over the changed workflow returns 0, no token is touched, and no astryx-* class is added or renamed.

Ossification

Nothing public is added, so nothing ossifies. Internal to the repo, the change writes the gh-pages preview URL layout into a second file (pr-comment.yml:110-112 alongside ci.yml:247-248), so the layout is now spelled twice with nothing holding the two together. The cheapest close is deleting storybookUrl and sandboxUrl from ci.yml's pr-meta.json, since after this PR nothing reads them.

Breaking

  • API — no. Nothing importable changes.
  • Visual — no. Nothing renders, and the comment's text is byte-identical.
  • Theme — no. No target, token or override is touched.
  • Behaviour — yes, and it is the review. A same-repo PR gets an identical comment. A fork PR goes from a posted comment to no comment at all — the job warns and returns. A docsite-only PR or one with no artifact keeps its quiet return. An artifact present without pr-meta.json now proceeds, since that file is no longer needed. And a force-push between CI ending and the job starting drops the stale comment instead of posting it, with the newer run posting instead.

Empty, loading, error, disabled and boundary states are not reachable — this is a workflow, it has no component states.

Performance & resources

No React, so no effects. One network call is added — listPullRequestsAssociatedWithCommit, once per CI run, and only when workflow_run.pull_requests is empty, which is every fork run — in a job that already makes at least two REST calls. No listeners, no observers, no layout reads, no bundle change, no new dependency. Against that, one sh process per run is removed.

Visual evidence

No frames, and the reason is that nothing here is renderable: a CI workflow, no geometry, no colour, no DOM.

The equivalent evidence is that the derived strings match what ships today, checked against the PR Analysis Report already posted on this PR — which main's copy of this workflow generated, so it is the real before:

live comment today new code derives
storybook https://facebook.github.io/astryx/pr/5517/ https://${owner}.github.io/${repo}/pr/${prNumber}/ identical
sandbox https://facebook.github.io/astryx/pr/5517/sandbox/ `${storybookUrl}sandbox/` identical
run the CI run's own URL run.html_url identical

The fork-resolution failure was found by reading and confirmed by running, which is the second independent confirmation a block owes: /commits/<sha>/pulls returns [] for the heads of #5517, #5518 and #5521, and [5538] for a same-repo control that passes; the runs API reports pull_requests: 0; and ?head=<owner>:<branch> resolves the PR, which is the working form. The commit itself is present in the base repo — commits/<sha> resolves for all three — so this is not a missing object. The endpoint matches head branches in this repository, and a fork's branch is not one.

A11y & i18n

Nothing here is touched, and this is what was checked: aria-|role=|useTranslator|t('@astryx over the workflow → 0. No user-visible or AT-facing string is added, the comment's rendered text is unchanged, and nothing renders, so no direction question exists.

Adjacent and not this PR's: generate-pr-comment.js interpolates artifact fields raw into HTML — extLink at :57 drops a value straight into an href — which is fork-controlled content in a bot comment. That is #5521, by the same author, already open.

Judgement

request changes. The stated goal is half met: the shell hole is genuinely closed, measured both ways, and the three derived URLs are byte-identical to what is on the PR right now. The other half — resolving the PR from the payload — does not work on the case this workflow exists for.

1. /commits/{sha}/pulls does not match a fork PR's head, and
   workflow_run.pull_requests is empty there
   → every external contributor's PR stops getting its analysis comment,
     and nothing tells anyone it is gone            · pr-comment.yml:94

2. [not blocking] the gh-pages URL layout is now written in two files
   → whoever moves the preview path next leaves every PR comment
     linking a 404                                 · pr-comment.yml:110

The worst sentence anywhere in the slots — "every fork PR loses its PR Analysis Report; an outside contributor opens a PR and the thing that tells them whether their change is fine never appears" — is not one a note can sit beside. No design call is involved: this is a defect in a CI change, measured against a contract the workflow's own header comment states.

Two things found and not spent on the author. The force-push behaviour change is accepted — the newer run posts within the minute, and #5518 makes the same trade deliberately for previews, so it is not worth a round trip. And #5518 carries the identical lookup, which is named in the comment as the PR that follows this one rather than promised as work.

Not verified: whether the Actions GITHUB_TOKEN sees the same empty result the review's user token saw on /commits/{sha}/pulls. The endpoint's behaviour is about branch ownership rather than visibility, and the same-repo control passed on the same token, but it could not be run as the Actions token.

The review, as drafted

Thanks for this — the shell hole was real, and execFileSync is the right fix.

The PR lookup doesn't reach fork PRs. GET /commits/{sha}/pulls only matches head branches in this repo, so a fork head returns [] — I ran it against three open fork PRs including this one. workflow_run.pull_requests is empty on forks too, so the job warns and returns: no analysis comment on any fork PR. That's 98 of the open ones today.

Two ways out. deploy-preview.yml:70 keeps reading prNumber from the artifact and validates it with ^[0-9]+$ — digits can't reach a shell, and it stays exact. Or resolve it from the run's own fork and branch:

head: `${run.head_repository.owner.login}:${run.head_branch}`

#5518 has the same lookup and can follow this one. Which would you rather?

If you'd rather talk it through with someone, we're in Discord.

Inline: .github/workflows/pr-comment.yml:94 — Hmm, this returns [] for a fork head, and workflow_run.pull_requests is empty there too. ?head=owner:branch matches.

Rounds

One review, two gate passes.

  • Gate 1 — failed on seven counts, every one of them the private brief leaking into the posted text. The inline was 24 words against a 20-word cap and read like the verdict rather than a pointer — a copy of the finding's consequence chain with a line number on it. The review offered "the safe version" without naming it, when the thing meant is a concrete regex on the artifact's prNumber. A clause about the workflow's history was doing work the 98 number already carried. The comment named #5518 without saying which PR lands first, leaving the author to guess whether he was blocked on someone else's work. The force-push behaviour row never reached the judgement, so it was neither a finding nor explicitly not one. And the time slot was empty, which is a fail on its own.
  • Gate 2 — clean. All seven answered: the inline is 15 words and in her register, the remedy is named rather than gestured at, the background clause is cut, the ordering with #5518 is stated, the force-push row is disposed of explicitly, and the time slot is filled. Both anchors were opened at the PR head, and the gate recorded one thing worth keeping for the loop: the stop-list hit on .github/workflows/ meant no install and no Storybook, and the evidence this review actually needed was live API calls and a four-line shell reproduction — three minutes of measurement against the thirty-five a worktree-and-build run costs.

Status

Drafted, held for Cindy's judgement, not posted. Nothing is on the PR: no review, no comment, no inline. The presentation's merge trailer is a hold, and the hold is the finding itself rather than a repo gate — the fork lookup resolves nothing, so the review asks for a change before it can be an approve. If it is posted as drafted, a [Full review](https://github.com/cixzhang/astryx/wiki/Review-5517) line goes in before the attribution.

Clone this wiki locally