Skip to content

fix(ci): harden PR→issue linking gate against shell injection and missed refs#35761

Open
nollymar wants to merge 12 commits into
mainfrom
fix/pr-issue-linking-gate-hardening
Open

fix(ci): harden PR→issue linking gate against shell injection and missed refs#35761
nollymar wants to merge 12 commits into
mainfrom
fix/pr-issue-linking-gate-hardening

Conversation

@nollymar
Copy link
Copy Markdown
Member

@nollymar nollymar commented May 19, 2026

Fixes #35794

Summary

Hardens the PR-to-issue linking gate (.github/workflows/issue_open-pr.ymlissue_comp_link-issue-to-pr.yml) and adds a second gate on the linked issue's metadata.

Linking-gate fixes (the original three)

  • Shell injection / template interpolation. inputs.pr_body, pr_title, pr_author, pr_url, pr_branch, pr_merged were template-substituted by GitHub Actions directly into the bash run: scripts. PRs whose body contained backticks, $var(...), or unbalanced parens (e.g. feat(tiptap): convert Story Block content to Markdown (#35727) #35728's $markdownTool.blockToMarkdown(json)) caused a bash syntax error on the first Debug workflow inputs step → entire job exited code 2 → Add failure comment to PR was skipped (its condition needs failure_detected=true, never set) → PR appeared unchecked. Hoisted all inputs into a job-level env: block and reference them as $ENV_VAR in shell, so user-supplied values are treated as data, not code.
  • Missed markdown-link refs. The regex (close[ds]?|fix(e[ds])?|resolve[ds]?)(:)?\s+#([0-9]+) requires a literal # immediately after the keyword and misses GitHub's other valid form fixes [#123](url) (as in feat(dotAI): Dot AI LangChain4J - Amazon Bedrock #35242). Added a GraphQL closingIssuesReferences lookup as the 4th and final fallback in Determine final issue number.
  • Stale-on-open. The workflow only triggered on pull_request: [opened], so editing the body or pushing new commits never re-evaluated the gate. A once-broken PR stayed broken even after the author fixed the link. Broadened triggers to [opened, edited, synchronize, reopened]. Existing idempotency (grep -q "#$issue_number" before body patch, sort -u on PR-list comments, Remove failure comment step) handles the extra runs; the failure-comment step is now dedup-guarded.

New second gate — linked issue must have a team label

After the link is resolved, the workflow validates that the linked issue carries a Team : * label and fails with a distinct ❌ Linked Issue Needs Team Label PR comment if it doesn't. The validation step runs before the side-effect steps (PR-list comment on the issue, PR body PATCH), so a PR whose linked issue is missing a team label does not mutate any remote state before failing.

Review-driven follow-ups (commit 312ced3e)

  • Step order moved so gates run before side effects (Link PR to issue and the issue's PR-list comment no longer run when validation fails).
  • gh issue view errors in the team-label step are surfaced distinctly instead of being collapsed into "no team label."
  • $GITHUB_OUTPUT heredocs use a random ghadelimiter_<random> instead of literal EOF to prevent delimiter collision with user-controlled content.
  • Markdown link characters (\, [, ]) in PR_TITLE are escaped before being embedded in the bot's linked-issue comment.
  • URL-presence check on the existing PR list switched from grep -q to grep -qF.

Operational notes

  • Fork PRs are skipped via if: github.event.pull_request.head.repo.full_name == github.repository because the pull_request GITHUB_TOKEN is read-only on forks. If this gate ever becomes a required check, fork contributors will be unchecked by design — flag for follow-up at that point.
  • Explicit permissions: { pull-requests: write, issues: write } declared on the reusable workflow.
  • The check remains advisory (not in branch-protection required checks on main).

Test plan

  • Confirm no shell error in Debug workflow inputs when the PR body contains backticks / $var(...) (this PR's own description is the test case).
  • GraphQL fallback runs only when neither body regex nor branch-name extraction succeeds.
  • synchronize re-fires the check on a new push (empty commit 2e6b0ed0 validated this).
  • Linked-issue gate fails with ❌ Linked Issue Needs Team Label when the linked issue has no Team : * label.
  • Linked-issue gate passes when Team : * is restored on the issue.
  • Side-effect steps (Create new comment, Link PR to issue) are skipped on a run where team-label validation fails.
    🤖 Generated with Claude Code

…sed refs

The "PR opened" check silently failed on PRs whose body contained shell
metacharacters (backticks, `$var(`, etc.), exiting before the linkage logic
ran. Move `inputs.*` into a job-level `env:` block so they are no longer
template-interpolated into bash sources. Add a GraphQL
`closingIssuesReferences` fallback for refs the body regex misses (e.g.
markdown-linked `fixes [#123](url)`). Re-evaluate the check on `edited`,
`synchronize`, and `reopened` so a once-broken PR can recover after a body
edit or new push instead of being stuck on the stale `opened` run.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nollymar nollymar requested a review from a team as a code owner May 19, 2026 22:45
@github-actions github-actions Bot added the Area : CI/CD PR changes GitHub Actions/workflows label May 19, 2026
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 19, 2026

Claude finished @nollymar's task in 4m 57s —— View job


PR Review

The hardening is solid — env hoisting is consistent, the heredoc-delimiter randomization closes the GH_OUTPUT injection class, markdown-link escaping in the bot comment looks right, and the gate/side-effect ordering is mostly correct. A few things worth a second look:

Findings

1. Priority inversion between body-regex and closingIssuesReferences.github/workflows/issue_comp_link-issue-to-pr.yml:90 vs :152
The PR description says GraphQL was added "as the 4th and final fallback," but the body-regex in check_existing_issues (line 90) actually outranks it (it sets has_linked_issues=true and short-circuits the determine_issue step). For a body containing both fixes #1234 (typo) and fixes [#35794](url), the regex wins on the typo. GitHub's own closingIssuesReferences is the authoritative source for "what this PR closes" — consider promoting GraphQL above the loose regex, or at least falling back to GraphQL when the regex hit doesn't appear in closingIssuesReferences.

2. gh issue view failure is silent on the PR.github/workflows/issue_comp_link-issue-to-pr.yml:230-235
When the labels lookup fails (transient 5xx, rate limit, deleted issue, scope issue) the step exit 1s without setting validation_failed=true. Both Add failure comment and Add issue-validation failure comment are then gated off, so the job fails red but the PR shows no comment explaining why. The PR description specifically calls this case out as a fix vs. "collapsed into no team label" — but the author still gets nothing on the PR. Either set a third output flag (fetch_failed=true) with its own comment branch, or fall through to the existing "needs team label" comment with a generic message.

3. Remove failure comment if issue is now resolved runs before the team-label gate.github/workflows/issue_comp_link-issue-to-pr.yml:188-216 and :218-247
The PR description states "gates run before side effects," but this comment-deletion side effect runs between gate 1 (issue determination) and gate 2 (team-label validation). It's semantically defensible — the linking-required failure IS resolved as soon as we found a link — but a stricter ordering would be: gate, gate, then both cleanup steps. Worth a comment in the workflow noting the deliberate interleaving.

4. sort -u reorders the issue's PR-list every run.github/workflows/issue_comp_link-issue-to-pr.yml:319, 340
Lines are sorted alphabetically by title, so adding a new PR can re-shuffle older entries. Combined with edit-mode: replace, every new PR causes the whole list to reorder. Cosmetic but visible in the issue thread. Consider stable-sorting (e.g., by PR number / URL suffix) so order remains predictable.

5. GraphQL -F typing on string args.github/workflows/issue_comp_link-issue-to-pr.yml:153-155
gh api graphql -F owner="$owner" -F name="$repo" uses -F (smart JSON coercion). For owner="dotCMS", repo="core" this works, but -f is the explicit string form and matches the String! schema. If someone ever creates an all-numeric repo or owner, -F would coerce to a number and the query would 400. Minor — switch to -f owner=... -f name=... -F num=....

6. Pre-existing: connected timeline event picks first, not most-recent.github/workflows/issue_comp_link-issue-to-pr.yml:76
Not introduced by this PR, but: jq '.[] | select(.event == "connected") | .source.issue.number' | head -1 returns the earliest connect event, ignoring any subsequent disconnect/reconnect. A PR that was linked to issue A, unlinked, then linked to B will still resolve to A. Flag for follow-up.

7. Unchecked test-plan item is the most important one to verify
The last item — "Side-effect steps are skipped on a run where team-label validation fails" — is the whole point of the gate-reordering fix. Reading the workflow it should work (default behavior is to skip subsequent steps on a prior exit 1), but worth an actual run with a label-less linked issue to confirm Create new comment, Update existing comment, and Link PR to issue are visibly skipped in the action log before merging.

Nits

  • ${{ env.GH_TOKEN }} is still template-interpolated in curl headers in multiple steps (e.g. :64, :71, :198). Token is system-generated so not an injection vector, but inconsistent with the hardening goal — $GH_TOKEN would match the rest of the file.
  • Branch-name pattern ^([0-9]+)- at :110 will happily match 2025-something or 404-not-found. Heuristic of last resort and probably acceptable, but worth keeping in mind.

Summary

  • Read changed workflow files
  • Check shell-injection hardening (env hoisting, quoting)
  • Check the Determine final issue number fallback chain
  • Check team-label gate, step ordering, and side-effect skipping
  • Check the synchronize/edited re-run paths for idempotency
  • Post review

The shell-injection hardening is the highest-value change and looks correct. The two findings worth addressing before merge are #1 (priority ordering of GraphQL vs body regex) and #2 (silent failure on gh issue view errors). The rest are smaller cleanups or follow-ups.
· Branch

The widened triggers (synchronize/edited) caused the "Add failure comment
to PR" step to repost the ❌ Issue Linking Required comment on every push
to an unlinked PR. Mirror the lookup used by the Remove step and skip the
POST if a matching bot comment already exists.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Skip the linking job on fork-sourced PRs. The pull_request token is
  read-only for forks, so the PATCH/POST/DELETE calls would 403 on every
  run — now amplified by synchronize/edited triggers.
- Declare explicit pull-requests/issues write permissions on the reusable
  workflow so the dependency is visible and survives future default
  permission tightening.
- Surface GraphQL fallback errors via ::warning:: instead of silently
  swallowing stderr with 2>/dev/null; users debugging a misleading "no
  issue found" failure can now see the underlying call's error.
- Guard the fallback against an empty pr_number (malformed PR_URL) so it
  skips the GraphQL call with a clear warning instead of issuing an
  invalid Int query.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empty commit to exercise the synchronize trigger after the linking-gate
hardening changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a second gate after the issue-linking check: if the resolved issue
has no 'Team : *' label, the workflow fails and posts a distinct
'❌ Linked Issue Needs Team Label' comment on the PR. The comment is
dedup-guarded and auto-removed on the next successful run.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When the linked-issue comment already lists the current PR, the "keeping
existing list" branch built the GITHUB_OUTPUT heredoc with printf and no
trailing newline, so the closing EOF was appended to the last PR line
instead of being on its own. The runner then errored with "Matching
delimiter not found 'EOF'" and skipped the rest of the job. Only exposed
now that synchronize/edited triggers cause the workflow to re-run against
issues whose PR-list comment already exists. Add the missing trailing \n.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empty commit to test the new "Linked Issue Needs Team Label" gate after
removing the Team : Scout label from issue #35794.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Move the issue-link-success cleanup, team-label validation, and the
  validation-cleanup steps to run before the side-effect steps (issue
  PR-list comment, PR body PATCH). A PR whose linked issue is missing a
  team label no longer mutates remote state before failing.
- Surface gh CLI errors in the team-label step distinctly from "no team
  label" so transient API failures, deleted issues, or scope problems do
  not masquerade as missing-label and start a fix cycle that never works.
- Replace the literal `EOF` heredoc delimiter on $GITHUB_OUTPUT multiline
  values with a random `ghadelimiter_<random>` so user-supplied PR titles
  containing a literal "EOF" line cannot terminate the output block early
  or leak subsequent shell content into the value.
- Escape `\`, `[`, and `]` in PR titles before embedding them in the
  markdown link of the linked-issue PR-list comment, so titles like
  'Fix bug](https://evil)' cannot redirect the bot-rendered link.
- Switch the URL-presence check on the existing PR list from `grep -q`
  to `grep -qF` so `.` and `/` in the URL are not interpreted as regex.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Hoist all step-output values into per-step env: blocks instead of
  inlining them as ${{ steps.X.outputs.Y }} into run: scripts. Today
  these values are all numeric or trusted-shape, but the env-hoist
  invariant means a future widening (a label name, a title) cannot
  silently re-introduce the bash injection the PR was created to close.
- Add concurrency: { group: link-issue-<pr_number>, cancel-in-progress:
  false } to issue_open-pr.yml so synchronize + the edited fired by the
  workflow's own body PATCH cannot race on the PATCH/POST/DELETE calls.
- Anchor the issue-reference dedup in Link PR to issue with a
  non-digit-boundary regex so the body containing "closes #100" is no
  longer considered to already reference #10.
- Tighten the PR-list URL dedup in Check if comment already exists to
  match against the literal markdown-link terminator "($pr_url)" so a
  URL ending in /pull/10 cannot substring-match an existing entry for
  /pull/100.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Empty commit to exercise the synchronize trigger against the env-hoisted,
concurrency-guarded, anchor-matched version of the linking workflow.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A PR body containing 'fixes [#123](url)' was losing to a branch named
'456-some-feature' because the GraphQL closingIssuesReferences lookup
ran only AFTER branch-name extraction. The body was effectively
unreadable for markdown-linked refs — the exact case the PR claimed to
fix. Reorder priority so author intent in the PR body (whether plain or
markdown-linked) outranks the branch-name heuristic:

  1) check_existing_issues: timeline / plain-keyword regex
  2) closingIssuesReferences GraphQL: catches markdown-linked refs
  3) Branch name extraction: last-resort heuristic

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
peter-evans/create-or-update-comment defaults to edit-mode: append. On
every re-run, the workflow was appending the full "PRs linked to this
issue" block to the existing bot comment instead of replacing it —
issue #35794's comment accumulated 10 stacked copies of the same block
after the synchronize/edited triggers started firing on every push.
Set edit-mode: replace so each run produces a fresh single block.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : CI/CD PR changes GitHub Actions/workflows

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

harden PR→issue linking gate against shell injection and missed refs

2 participants