Skip to content

fix(ci): eliminate script injection in pending-decision.yml#1395

Merged
thepagent merged 1 commit into
mainfrom
fix/pending-decision-script-injection
Jul 13, 2026
Merged

fix(ci): eliminate script injection in pending-decision.yml#1395
thepagent merged 1 commit into
mainfrom
fix/pending-decision-script-injection

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

What

Fixes a script-injection vulnerability in pending-decision.yml, found during a follow-up audit after #1393.

The github-script step interpolated both workflow_call inputs directly into the JS source string:

const prNumber = ${{ inputs.pr_number }};
const decision = '${{ inputs.decision }}';

inputs.decision is type: string with no choice constraint or runtime validation — only a comment documents the expected values ("maintainer" / "contributor"). A crafted decision value could break out of the single-quoted JS literal and execute arbitrary code inside the github-script sandbox, which runs with pull-requests: write + issues: write.

Blast radius

This workflow_call reusable workflow currently has no caller in the repo — searched .github/workflows/ for any uses: reference and found none. pending-maintainer.yml / issue-pending-maintainer.yml both implement the label-flip logic inline rather than calling this workflow. So there's no live exploit path today, but the footgun is real the moment something calls it with attacker-influenced input (e.g. a bot command parsed from a PR comment).

Fix

Same pattern as the GitHub Actions security hardening guide (and the one already applied in #1393): pass values through env: and read them via process.env inside the script, never string-interpolated into the JS source.

  • PR_NUMBERNumber(process.env.PR_NUMBER)
  • DECISIONprocess.env.DECISION

process.env values are never parsed as code, so this closes the injection regardless of the input's declared type — not just for the current string/number types but for any future schema change too.

No behavior change: same label-flip logic, just a safe value-delivery mechanism.

Verification

  • YAML parses cleanly
  • actionlint clean (exit=0)

The reusable workflow interpolated ${{ inputs.pr_number }} and
${{ inputs.decision }} directly into the github-script JS source:

    const prNumber = ${{ inputs.pr_number }};
    const decision = '${{ inputs.decision }}';

inputs.decision is a free-form string (only documented by a comment as
"maintainer" or "contributor", not enforced by a choice type or any
validation). A crafted decision value could break out of the single-quoted
JS string literal and execute arbitrary code inside the github-script
sandbox, which holds pull-requests:write + issues:write permissions.

inputs.pr_number is typed as `number`, which GitHub Actions coerces at
the workflow-call boundary, but relying on the caller's input schema
alone is fragile defense — the value is still substituted as raw text
before the JS parser ever sees it.

This workflow currently has no caller in the repo (workflow_call with no
invoking workflow found), so there's no live exploit path today. Fixing
it now since it's a footgun for whoever wires it up next.

Fix: pass both inputs through env: (the pattern used elsewhere in this
repo's workflows, and the one GitHub's own security hardening guide
recommends), and read them via process.env inside the script instead of
string-interpolating into the JS source. Number() replaces the implicit
number coercion; process.env values are never eval'd as code regardless
of content, closing the injection regardless of the input's declared
type.
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 13, 2026 23:14
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Correct, minimal, idiomatic script-injection fix. No blocking findings.

What This PR Does

pending-decision.yml was directly interpolating ${{ inputs.pr_number }} and ${{ inputs.decision }} into the JavaScript source string of a github-script step. A crafted decision value could escape the single-quoted literal and execute arbitrary JS inside the github-script sandbox (permissions: pull-requests: write + issues: write). This PR moves both values to env: and reads them via process.env, closing the injection regardless of input type or future schema changes.

How It Works

env:
  PR_NUMBER: ${{ inputs.pr_number }}
  DECISION: ${{ inputs.decision }}
const prNumber = Number(process.env.PR_NUMBER);
const decision = process.env.DECISION;

env: values are delivered as data — never parsed as code. Number(...) preserves the correct type for the type: number input. Behavior is identical: same label-flip ternary, same API calls.

Findings

# Severity Finding Location
1 🟢 env: + process.env is the canonical pattern (same as #1393); fix is consistent with established repo convention .github/workflows/pending-decision.yml
2 🟢 Honest blast-radius analysis in PR description — correctly identifies no live caller, no live exploit path today PR description
3 🟢 Minimal scope: only the two interpolated values changed; no accidental behavior drift diff
Finding Details

🟢 F1: Canonical fix pattern

The env:process.env pattern matches the fix already applied in #1393. Consistency across workflows is good defensive discipline — future reviewers and contributors will recognize the pattern immediately.

🟢 F2: Clear blast-radius documentation

The PR body accurately identifies that pending-decision.yml has no callers on main today, so there is no live exploit path. It also calls out the precise risk scenario (bot command parsed from a PR comment) that would activate the vulnerability. This kind of honest threat modeling is exactly what good security PRs should include.

🟢 F3: Zero behavior drift

Only the value-delivery mechanism changed. The label-flip logic, ternary conditions, API calls, and console output are untouched. Number(process.env.PR_NUMBER) correctly coerces the env string back to the number type that the rest of the script expects.

Baseline Check
  • PR opened against: main
  • Main already has: pending-decision.yml with the vulnerable inline interpolation (${{ inputs.pr_number }} / ${{ inputs.decision }} direct in JS source)
  • Net-new value: safe value-delivery via env: + process.env; closes injection vector proactively before any caller is wired up
5️⃣ Three Reasons We Might Not Need This PR
  1. No caller, no risk todaypending-decision.yml has zero uses: references in the repo. The vulnerability cannot currently be triggered. Merging adds complexity for a threat that does not exist yet.
  2. Delete instead of fix — If this workflow has no callers and no planned callers, the lower-maintenance option is to delete the file entirely rather than maintain a hardened but unused workflow.
  3. type: number input partially mitigates pr_numberinputs.pr_number is declared as type: number, which limits its injection surface to numeric values. The original code (const prNumber = ${{ inputs.pr_number }};) was arguably already safe for that specific field. The real risk was only inputs.decision. A narrower fix scoped to decision alone would have been equally correct.

These are not blockers — they are decision checkpoints. The fix is correct and the approach is sound.

@thepagent thepagent merged commit d85f454 into main Jul 13, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants