[code-scanning-fix] Fix go/unsafe-quoting: escape single quotes in GH_AW_UPGRADE_OPTIONS - #58495
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
🟡 Changes recommended
The critical quoting fix lacks a generator-level regression test using an apostrophe-containing value.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Escapes apostrophes in JSON embedded in generated YAML, preventing unsafe scalar termination.
Changes:
- Applies the existing YAML single-quote escaping helper to upgrade options.
File summaries
| File | Description |
|---|---|
pkg/workflow/auto_update_workflow.go |
Safely embeds upgrade options in YAML. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| return "", fmt.Errorf("failed to encode auto-upgrade options: %w", err) | ||
| } | ||
| upgradeOptionsEnv = "\n GH_AW_UPGRADE_OPTIONS: '" + string(encodedOptions) + "'" | ||
| upgradeOptionsEnv = "\n GH_AW_UPGRADE_OPTIONS: '" + escapeYAMLSingleQuoted(string(encodedOptions)) + "'" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs. The fix is a correct, minimal, surgical one-liner that reuses the existing escapeYAMLSingleQuoted helper already used elsewhere in the package — good practice, no new escaping logic introduced.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: no test covers an upgrade option containing a single quote to verify the vulnerability stays fixed.
Positive Highlights
- ✅ Root cause addressed directly (unescaped single quote in YAML single-quoted scalar), not just a symptom patch.
- ✅ Reuses an existing, already-tested helper rather than inventing new escaping logic.
- ✅ Change is scoped to exactly the vulnerable line.
Approving since the fix itself is correct and low-risk; the missing regression test is a minor follow-up, not a blocker.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 18.3 AIC · ⌖ 14.6 AIC · ⊞ 10.3K
Comment /matt to run again
| return "", fmt.Errorf("failed to encode auto-upgrade options: %w", err) | ||
| } | ||
| upgradeOptionsEnv = "\n GH_AW_UPGRADE_OPTIONS: '" + string(encodedOptions) + "'" | ||
| upgradeOptionsEnv = "\n GH_AW_UPGRADE_OPTIONS: '" + escapeYAMLSingleQuoted(string(encodedOptions)) + "'" |
There was a problem hiding this comment.
[/diagnosing-bugs] Correct minimal fix reusing the existing escapeYAMLSingleQuoted helper. However, no regression test was added exercising an upgrade option value containing a single quote to confirm the generated YAML stays valid and GH_AW_UPGRADE_OPTIONS round-trips correctly.
💡 Suggested test
func TestGenerateAutoUpdateWorkflow_UpgradeOptionsWithQuote(t *testing.T) {
opts := AutoUpdateOptions{UpgradeOptions: []string{"--flag=it's"}}
yamlStr, err := generateAutoUpdateWorkflowYAML(..., opts)
require.NoError(t, err)
// parse yamlStr and assert GH_AW_UPGRADE_OPTIONS decodes back to the original JSON
}Without this test, a future refactor could silently reintroduce the go/unsafe-quoting vulnerability since nothing currently asserts the quote-escaping behavior for this specific code path.
@copilot please address this.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Verdict
I dont see a merge-blocking regression in the changed line. The patch correctly routes the JSON blob through the existing YAML single-quote escaper, which is the right fix for the reported injection risk.
Why Im not requesting changes
The only changed line now matches the escaping pattern already used for other JSON-valued YAML env vars in this package. I also checked the nearby tests and generated workflow expectation; nothing in the diff introduces a new correctness, performance, or maintainability issue on the changed line.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 18.8 AIC · ⌖ 7.31 AIC · ⊞ 21.8K
Comment /review to run again
There was a problem hiding this comment.
Minimal, correct security fix: reuses the existing tested escapeYAMLSingleQuoted helper to properly escape embedded single quotes in the GH_AW_UPGRADE_OPTIONS YAML value, matching the pattern used elsewhere in the package. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 15.7 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
Security Fix: Unsafe YAML single-quote embedding in auto-upgrade workflow generator
Alert Number: #673
Severity: critical
Rule: go/unsafe-quoting
CWE: CWE-78, CWE-89, CWE-94
Vulnerability Description
generateAutoUpdateWorkflowYAML(inpkg/workflow/auto_update_workflow.go) builds theGH_AW_UPGRADE_OPTIONSenvironment value by JSON-encodingupgradeOptionsand embedding the raw JSON string directly inside a single-quoted YAML scalar:JSON encoding does not escape single quotes. If any upgrade option string contains a
'character, it would prematurely close the YAML single-quoted scalar, corrupting the generated workflow YAML structure and potentially allowing injection of unintended YAML/shell content into the generated GitHub Actions workflow file.Location
pkg/workflow/auto_update_workflow.goFix Applied
Wrapped the encoded JSON value with the existing
escapeYAMLSingleQuotedhelper (already used elsewhere in this package, e.g.observability_otlp.go,evals_steps.go,central_slash_command_workflow.go) which doubles embedded single quotes per YAML single-quoted scalar escaping rules before embedding the value.Changes Made:
string(encodedOptions)toescapeYAMLSingleQuoted(string(encodedOptions))when constructing theGH_AW_UPGRADE_OPTIONSenv line.Security Best Practices
Testing Considerations
go build ./pkg/workflow/...— succeeds.go test ./pkg/workflow/ -run TestGenerateAutoUpdateWorkflow— all 12 tests pass, includingTestGenerateAutoUpdateWorkflow_UpgradeOptions.Automated by: Code Scanning Fixer Workflow
Run ID: 33875303219
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.