Repository navigation
Conversation
Harden generated maintenance action pins against multiline comments and Actions expressions, propagate validation failures, and cover package option rejection and subagent stdin isolation. Refs #69882 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Commit pushed:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
🏗️ Design Decision Gate — ADR RequiredThis PR adds 153 lines of core business logic under 📄 Draft ADR committed: Decision inferred from the diff: validate action pins and version comments before emitting YAML, and fail closed.
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and check that the implementation matches the decision. ❓ Why ADRs MatterADRs create a permanent, searchable record of why the codebase looks the way it does. This change makes action-pin emission fail closed and alters the maintenance builder signatures. That is a security policy future YAML emitters should follow, so the rationale belongs in the record. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Request changes. The new action-pin hardening is bypassable for unquoted uses: entries.
Details
pkg/workflow/action_pins.go:225: validation and safe quoting are gated on the source line having an opening quote; unquoted mapped references/comments are still emitted raw, allowing YAML or expression injection.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 2.22 AIC · ⌖ 0.621 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the fix patches the symptom in the quoted uses: pin path but leaves the same injection vector open for unquoted pins.
📋 Key Themes & Highlights
Key Themes
- Root cause not fully addressed: mapPinnedUsesLine in pkg/workflow/action_pins.go only calls validateYAMLActionPin when captures[2] != "" (the original line was already quoted). When the source uses: line is unquoted, the function returns
indentation + reference + comment, nilat line 234 without ever validating reference/comment. I reproduced this locally with the exact attack pattern used in the PR's own new regression test (TestMapPinnedUsesRejectsUnsafeQuotedPins), just starting from an unquoted input line — the malicious prefix mapping injects a run: key into the generated YAML with no error returned. - Positive: the quoted-pin fix itself (validateYAMLActionPin + quoteYAMLActionPin returning errors, propagated through buildMaintenanceLabelDisableJob/buildMaintenanceLabelApplySafeOutputsJob/buildMaintenanceLabelTriggeredJobs/buildMaintenanceWorkflowYAML) is solid and well-tested — control characters, Unicode line separators, and ${{ }} expressions are all correctly rejected, with both unit tests (TestQuoteYAMLActionPin*) and integration tests (TestMaintenanceLabelJobsRejectUnsafeActionPins) passing.
- Test coverage gap: there is no regression test exercising the unquoted path with an unsafe mapped/resolved reference, which is likely why this gap wasn't caught — all new tests start from an already-quoted source line.
Positive Highlights
- ✅ Good defense-in-depth reasoning in the PR description distinguishing dismissed vs. newly-fixed alerts.
- ✅ Pip/uv and Pi-subagent test additions are correct and verified passing (TestValidatePipCommandPackageArg, new stdin-only test in pi_subagent_extension.test.cjs).
- ✅ Error propagation plumbing through the maintenance workflow builders is clean and consistent with existing patterns.
Requesting changes to close the unquoted-pin gap in action_pins.go before merge, since it undermines the PR's stated goal of rejecting unsafe pins across the board. See inline comment for a concrete repro and fix.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 81.8 AIC · ⌖ 15 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
🔵 Needs a closer look
Validation gaps remain, and critical-alert dispositions require maintainer verification.
2 open issues
What changed in this PR
Hardens quoted action-pin handling in gh-aw’s YAML generators and adds regression coverage supporting security-alert triage.
Changes:
- Reject unsafe quoted pins and version comments.
- Propagate validation errors through maintenance generation and pin mappings.
- Expand package-argument and subagent-stdin safety tests.
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_job_identity_test.go | Handles the label builder’s new error return. |
| pkg/workflow/maintenance_workflow_yaml.go | Propagates label-job generation errors. |
| pkg/workflow/maintenance_workflow_yaml_jobs.go | Validates quoted pins and returns errors. |
| pkg/workflow/maintenance_workflow_labels_test.go | Tests quoting, rejection, and error propagation. |
| pkg/workflow/argument_injection_test.go | Covers additional option-like package names. |
| pkg/workflow/action_pins.go | Validates quoted mapped pins and comments. |
| pkg/workflow/action_pins_test.go | Tests unsafe quoted mapping rejection. |
| actions/setup/js/pi_subagent_extension.test.cjs | Verifies shell-looking tasks remain stdin data. |
Review effort: Max
Validate mapped pins regardless of quoting, check maintenance resolver results independent of label triggers, and finalize ADR-70616 with compatibility evidence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In reply to #70616 (comment): Accepted the review outcome. The unquoted mapped-pin validation bypass is fixed and covered alongside the quoted cases. |
|
In reply to #70616 (comment): No code change is needed for this no-action notification. The actionable validation gaps from the other reviews are handled in this batch. |
|
In reply to #70616 (comment): Accepted the generated ADR draft and completed its unresolved verification notes without treating generation as human approval. The finalized ADR is linked from the PR description. |
|
In reply to #70616 (comment): Accepted the underlying skills-based review finding. Mapped references and comments are now validated regardless of source quoting, with regression coverage for unquoted inputs. |
|
In reply to #70616 (comment): No code change is needed for this no-action test-analysis notification. This batch adds focused regressions for both reported validation bypasses. |
|
In reply to #70616 (comment): Accepted the underlying unquoted-pin finding and fixed the validation boundary. The managed comment-memory contents were left unchanged. |
|
In reply to #70616 (comment): Accepted: the ADR is finalized with verified compatibility limits, alternatives, and consequences, and linked from the PR description. It records quote-independent mapped-pin validation and label-independent maintenance validation. |
Reject malformed references as lint errors regardless of strict policy, including Unicode control and whitespace characters in local paths. Retain mutable-ref policy severity and cover both ASTs and CLI failures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Fixes #69882. Reviews source reports #68742, #69063, and #70442 against current code; the full alert-by-alert dispositions are recorded in artifact 1 on #69882.
ADR: ADR-70616: Reject Unsafe Action Pins with Explicit Errors Instead of Escaping
action-pinlint rule to enforce formatting: malformed references are errors even for native Actions and non-strict AW. Reject invalid UTF-8, control characters, Unicode whitespace/line separators, malformed paths/refs/digests, and expressions. Valid but mutable refs retain their existing policy severity. No duplicate lint rule is registered.Run the pin check directly on either source format with
gh aw lint --rules action-pin path/to/workflow.ymlor the corresponding.mdfile. Diagnostics identify the configuration path without exposing the pin value.Alert dispositions
shell: false, and task-only stdin.No live security-alert state was changed. Human-review requirements for critical dismissals remain with maintainers.
Validation
Latest commit
cb519e9a78:make build,make fmt, and the fullmake agent-report-progressgate passed, including Go lint, change-scoped custom Go lint, JavaScript lint/typecheck, and impacted tests. The shared-lint-lock limitation from the prior batch is cleared by this successful gate.Earlier focused validation also passed the mapped/maintenance boundary regressions, all 246 tests across the reported JavaScript suites, and all 13 lint-monster inventory tests.
The existing branch name is retained because the shared repository's
packed-refs.lockblocked the requested rename; no shared lock was removed.