[code-scanning-fix] Fix go/unsafe-quoting: safe YAML quoting for category filter env vars

[github-actions]

Summary

Fix YAML injection vulnerability in category-filter env-var generation by switching from single-quoted YAML scalars ('%s') to Go %q double-quoted scalars for GH_AW_FAILURE_CATEGORIES_FILTER and GH_AW_FAILURE_EXCLUDED_CATEGORIES_FILTER. A category name containing a single quote (e.g. don't retry) would have broken out of the scalar boundary and injected arbitrary YAML into the generated workflow step.

Motivation / Root Cause

notify_comment.go embedded user-supplied category strings directly into YAML using '%s' format strings. YAML single-quoted scalars treat a single quote inside the value as an escape sequence ('') but the code was not performing that escaping. An attacker or unexpected input containing ' could terminate the scalar early and inject YAML keys, values, or anchors into the rendered workflow definition.

Go's %q verb produces a properly double-quoted, backslash-escaped string literal that is safe to embed in YAML double-quoted scalars regardless of the string's content.

Changes

File	Type	Description
pkg/workflow/notify_comment.go	fix	Replace '%s' with %q for both category-filter env-var format strings
pkg/workflow/notify_comment_test.go	test	Add TestConclusionJobCategoriesFilterQuoting with sub-tests asserting double-quoted output and asserting single-quoted output is absent

Testing

• TestConclusionJobCategoriesFilterQuoting – two sub-tests:
  1. Asserts that the rendered env vars use safe double-quoted YAML scalars.
  2. Asserts that the old single-quoted form is not present (regression guard).

go test ./pkg/workflow/... -run TestConclusionJobCategoriesFilterQuoting -v

Security

Severity	Class	Status
High	YAML injection via unescaped single quotes in category filter env vars	✅ Fixed

• Not a breaking change – output is semantically equivalent for category names that contain no single quotes; the rendered YAML value is now double-quoted instead of single-quoted.
• Consumers that parse the raw YAML text (rather than deserialising it) may observe the quoting style change, but the decoded value is identical.

Commits

• 1771d2be5 fix(security): use safe YAML quoting for JSON category filter env vars
• 7f46b21b6 test: add regression test for safe YAML quoting of category filter env vars

Generated by PR Description Updater for issue #40545 · 64.9 AIC · ⌖ 7.27 AIC · ⊞ 4.5K · ◷

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #40545 only modifies pkg/workflow/notify_comment.go (2 additions, 2 deletions — a YAML quoting fix with no test changes).

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #40545 does not have the 'implementation' label and has only 2 new lines of code in business logic directories (≤100 threshold).

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

This PR addresses a CodeQL go/unsafe-quoting finding by ensuring category-filter environment variables are safely quoted when embedded into generated GitHub Actions YAML. It updates the workflow compiler’s YAML emission to use double-quoted, escaped scalars so category names containing ' cannot break YAML structure.

Changes:

• Switched YAML generation for GH_AW_FAILURE_CATEGORIES_FILTER and GH_AW_FAILURE_EXCLUDED_CATEGORIES_FILTER from single-quoted '%s' to %q-quoted strings.
• Ensured []byte JSON output from json.Marshal is converted to string before quoting, avoiding Go’s byte-slice formatting and producing valid YAML scalars.

Show a summary per file

File	Description
pkg/workflow/notify_comment.go	Uses %q for safe YAML quoting of JSON-encoded failure category filters in generated env: blocks.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 1/1 changed files
• Comments generated: 0

[github-actions]

Security fix is correct — LGTM

The %q + string() approach is the right fix. YAML single-quoted scalars have no escape mechanism, so a category name containing ' would break the generated YAML structure. Go's %q format produces a valid YAML double-quoted scalar that handles any embedded " via \", and the string() cast is essential to prevent %q from hex-encoding the []byte as a Go byte-slice literal.

The pattern is already in use at lines 166, 204, and 372 in the same file, so this is a consistent, well-precedented change.

One non-blocking observation

No test was added for the specific attack vector (category names containing a single quote). The existing buildConclusionJob test infrastructure in notify_comment_test.go could host a regression test. This is not blocking — the code is straightforward enough to verify by inspection — but a snapshot test would prevent regressions if this code path is touched again.

🔎 Code quality review by PR Code Quality Reviewer · 67.6 AIC · ⌖ 7.21 AIC · ⊞ 5.1K

[github-actions]

Skills-Based Review 🧠

Applied /tdd and /diagnose — approving with one suggestion to add a regression test.

📋 Key Themes & Highlights

Key Themes

• Correct fix, aligned with existing patterns: %q + string() matches how GH_AW_MISSING_TOOL_LABELS and GH_AW_REPORT_INCOMPLETE_LABELS are emitted in the same function. The string() conversion is necessary because json.Marshal returns []byte, and applying %q to a raw []byte without conversion produces Go byte-slice escaping rather than a clean JSON string.
• Root cause fully addressed: The single-quote YAML scalar ('%s') was the injection point; replacing it with a Go double-quoted scalar via %q closes the CWE-078/089/094 vector cleanly.
• No regression test: The fix ships without automated coverage for the exact exploit input (a category name containing '). TestReportFailureAsIssueWithCategoriesFilter tests config parsing, not YAML emission — the gap remains unguarded.

Positive Highlights

• ✅ Minimal, surgical change with clear intent
• ✅ Consistent with the established %q + string(jsonBytes) pattern already in use in the same file
• ✅ Both affected env vars (GH_AW_FAILURE_CATEGORIES_FILTER and GH_AW_FAILURE_EXCLUDED_CATEGORIES_FILTER) are fixed symmetrically
• ✅ PR description is thorough and cites the CWEs, alert numbers, and self-documents the best practice

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 84.4 AIC · ⌖ 7.66 AIC · ⊞ 6.9K

[github-actions]

[/tdd] No regression test covers this security fix — a category name containing a single-quote (e.g. "it's-a-category") was the exact exploit vector, so it should be the first thing a test covers.

💡 Suggested test outline

func TestConclusionJobCategoriesFilterQuoting(t *testing.T) {
    compiler := NewCompiler()
    workflowData := &WorkflowData{
        Name: "Test Workflow",
        SafeOutputs: &SafeOutputsConfig{
            NoOp: &NoOpConfig{},
            ReportFailureAsIssue: true,
            ReportFailureAsIssueCategories: []string{"it's-a-category"},
        },
    }
    job, err := compiler.buildConclusionJob(workflowData, string(constants.AgentJobName), []string{})
    require.NoError(t, err)
    require.NotNil(t, job)

    jobYAML := strings.Join(job.Steps, "")
    // Single-quote must not break YAML; value must be double-quoted
    assert.Contains(t, jobYAML, `GH_AW_FAILURE_CATEGORIES_FILTER: "[\"it's-a-category\"]"`,
        "category name with single-quote must produce valid double-quoted YAML scalar")
}

Add a parallel case for GH_AW_FAILURE_EXCLUDED_CATEGORIES_FILTER (line 414). The existing TestReportFailureAsIssueWithCategoriesFilter in compiler_safe_outputs_config_test.go covers config parsing but not YAML emission — this gap is what leaves the fix unguarded.

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Added TestConclusionJobCategoriesFilterQuoting in pkg/workflow/notify_comment_test.go (commit test: add regression test for safe YAML quoting of category filter env vars). It has two sub-tests — one for GH_AW_FAILURE_CATEGORIES_FILTER and one for GH_AW_FAILURE_EXCLUDED_CATEGORIES_FILTER — each using a category name containing a single quote (it's-a-category, it's-excluded) to guard the exact exploit vector. Both sub-tests assert the emitted YAML uses a double-quoted scalar and that no single-quoted form appears.