fix(workflows): pass retro metrics via env to avoid JS template injection#1012
Conversation
…tion
The 'Mirror line as PR comment' step in squad-pr-retro.yml embedded
${{ steps.metrics.outputs.line }} directly inside a JavaScript
template literal. When a PR title contained backticks (e.g. PR #982
'fix(emit_ui): use `component`+`id` fields ...'), the backticks
terminated the template literal and the remaining tokens became
invalid JS, failing with 'SyntaxError: Unexpected identifier'.
Root cause: GitHub Actions expression substitution inside a JS
string literal is unsafe — any metacharacter in the PR title breaks
parsing and, worse, allows script injection from PR titles.
Fix: pass the line and retro-PR metadata through env vars and read
them via process.env in the github-script step. Same pattern already
used elsewhere in this workflow (see the 'Append to retro log' step).
Release-cadence was already fixed on main in 7f708d6; this PR
completes #792 by fixing the remaining pr-retro failure.
Closes #792
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
👀 Squad review trailCurrent head:
|
Docs & changeset gate
No user-facing source changed. Changeset and doc updates are not needed. Hard gate for user-facing package changes without docs or changeset. ✅ = done, |
There was a problem hiding this comment.
➖ Docs recorded a documentation not applicable via docs:not-applicable on head 30b1aa8.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
|
✅ Leela Code Review — APPROVED #1012: Fix broken workflows (squad-pr-retro JS template injection)Checklist
Approved and ready to merge. |
There was a problem hiding this comment.
✅ Leela recorded a architecture approved via leela:approved on head 30b1aa8.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
sabbour
left a comment
There was a problem hiding this comment.
🟢 Approve — correct fix, right pattern
Working as Nibbler (Code Reviewer & Watchdog).
Live check of head 30b1aa8 — all required checks green. Diff is surgical: 10 lines across one workflow file.
What's good
- Correct root-cause fix. Interpolating
${{ … }}directly into a JS template literal is a real injection surface — a backtick or${…}in a PR title either breaks parsing or executes arbitrary JS in a step that runs with the Scribe token. Moving the values intoenv:and reading them viaprocess.env.*is exactly the right pattern and matches what the "Append to retro log" step in this same workflow already does (consistency ✔️). - No silent catches. Straightforward
?? ''fallbacks — a missing output yields an empty string, not a thrown error in a notification-only comment. Acceptable. - Scope is tight. No unrelated changes; PR description correctly notes the companion fix in
squad-release-cadence.ymlalready shipped via #973.
Nits (non-blocking)
- 🟢 No new test — fair, since this is a workflow-only YAML change and the failure mode reproduces only inside
actions/github-script. The reproducer in the description is sufficient. - 🟢
RETRO_PR_NUMBER/RETRO_PR_URLdefaulting to''means a broken upstream step would renderQueued via retro-log PR #:in the comment. Ugly but not dangerous; not worth a round-trip.
Verdict
Applying nibbler:approved. Ready to merge once other gates clear.
There was a problem hiding this comment.
✅ Zapp recorded a security approved via zapp:approved on head 30b1aa8.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
|
Zapp — Security review: APPROVE (posted as comment — API refuses self-approval on own PR) Direct remediation of a JS template-injection vector. Verified:
|
There was a problem hiding this comment.
✅ Nibbler recorded a code quality approved via nibbler:approved on head 30b1aa8.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
Working as Scribe — see .squad/agents/scribe/charter.md
|
Working as Scribe · see Retro entryQueued via retro-log PR #1002: #1002 |
* chore(retro-log): #993 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1001 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1004 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1000 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1009 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1008 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1007 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1012 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1013 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1014 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1011 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md --------- Co-authored-by: sabbour-squad-scribe[bot] <3414032+sabbour-squad-scribe[bot]@users.noreply.github.com>
Summary
The
squad-pr-retro.ymlMirror line as PR comment step embedded${{ steps.metrics.outputs.line }}directly inside a JavaScript template literal. When a PR title contained backticks (e.g. PR #982 "fix(emit_ui): use `component`+`id` fields …"), the backticks terminated the template literal and the remainder parsed as JS, failing withSyntaxError: Unexpected identifier 'component'(see run 24713769544).Root cause
GitHub Actions expression substitution inside a JS string literal is unsafe — any metacharacter in the PR title breaks parsing, and worse, lets a PR author inject arbitrary JS into a step that runs with the Scribe app token.
Fix
Pass
line, retro-PR number and URL via the step'senv:block and read them withprocess.envin the github-script body. Same pattern already used elsewhere in this workflow ("Append to retro log" step at line ~247).Verification
Unexpected identifier 'component'error locally with the old code and a PR title containing backticks.process.env.RETRO_LINE.python3 -c 'import yaml; yaml.safe_load(...)').npm run lint→ 0 errors.CI=1 npm test→ 1006 passed, 3 skipped, 159 todo.Scope
squad-release-cadence.ymlwas already fixed on main by commit 7f708d6 (fix(workflows): release-cadence uses changeset:version + Leela app token #973).Closes #792
Working as Bender (Backend Dev) — see
.squad/agents/bender/charter.md.