fix: argument injection in git archive fallback (CWE-88)

[Copilot]

The downloadWorkflowContentViaGit in pkg/cli/download_workflow.go passed path directly to git archive without validation or an end-of-options separator. An attacker-controlled source:/redirect: frontmatter value like --output=/home/dev/.ssh/authorized_keys would be parsed by git as a flag, enabling arbitrary local file writes via the auth-error fallback path.

Changes

• pkg/cli/download_workflow.go: Mirror the hardening already applied in pkg/parser/remote_download_file.go:
  • Call gitutil.ValidateGitRef(ref) and gitutil.ValidateGitPath(path) before constructing the command, rejecting values that start with -, contain .., or are empty
  • Insert "--" end-of-options separator before path

// Beforecmd:=exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, path)
// Afteriferr:=gitutil.ValidateGitRef(ref); err!=nil {
returnnil, fmt.Errorf("refusing git fallback: %w", err)
}
iferr:=gitutil.ValidateGitPath(path); err!=nil {
returnnil, fmt.Errorf("refusing git fallback: %w", err)
}
cmd:=exec.CommandContext(ctx, "git", "archive", "--remote="+repoURL, ref, "--", path)

• pkg/cli/download_workflow_test.go: Add TestDownloadWorkflowContentViaGitValidation covering path injection (--output=), ref injection, path traversal (../), .. in ref, empty ref, and empty path — all verified to return early with "refusing git fallback" before any subprocess is spawned.

• Fixes[vulnhunter] VulnHunter findings in github/gh-aw #49498

[github-actions]

PR Triage

• Category:bug | Risk:high | Priority:low
• Score: 32/100 (impact 15 + urgency 15 + quality 2)
• Recommended action:defer
• Batch:wip-drafts

Note: Draft, security-relevant (argument injection) but no diff yet - monitor closely once ready

Generated by 🔧 PR Triage Agent · auto · 48.9 AIC · ⌖ 6.24 AIC · ⊞ 7.9K · ◷

[github-actions]

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

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

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

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

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[Copilot]

Pull request overview

Hardens the Git archive fallback against CWE-88 argument injection, addressing #49498.

Changes:

• Validates Git refs and paths and adds an end-of-options separator.
• Adds regression tests for malicious inputs.
• Includes an unrelated generated workflow update requiring removal or explanation.

Show a summary per file

File	Description
pkg/cli/download_workflow.go	Secures Git archive arguments.
pkg/cli/download_workflow_test.go	Tests rejected refs and paths.
.github/workflows/daily-byok-ollama-test.lock.yml	Updates unrelated threat-detection configuration.

Review details

Tip

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

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Balanced

[github-actions]

The fix correctly addresses CWE-88 argument injection in downloadWorkflowContentViaGit. ValidateGitRef and ValidateGitPath reject empty values, leading dash, NUL bytes, and dotdot traversal before any subprocess is spawned. The -- end-of-options separator adds defence-in-depth. Tests cover all six injection/traversal cases. LGTM.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 25.5 AIC · ⌖ 11.8 AIC · ⊞ 5.3K

[github-actions]

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on two small but important gaps.

📋 Key Themes & Highlights

Key Themes

• Clone fallback not fully hardened: downloadWorkflowContentViaGitClone accepts ref without validation and passes it to git checkout without a -- separator. It is only reachable through the validated archive path today, but standalone callers would be vulnerable.
• #nosec comment inaccuracy: The suppression comment says ref/path are developer-authored, but the threat model in the PR description correctly identifies them as attacker-controlled. The mismatch undermines the value of future security audits.
• Missing happy-path test: All 6 test cases assert rejection; none assert that valid inputs actually pass validation and proceed (even to a subprocess failure). This leaves a logic inversion undetected.

Positive Highlights

• ✅ Core fix is correct and minimal — mirrors the pattern from pkg/parser/remote_download_file.go exactly
• ✅ -- end-of-options separator is defence-in-depth even after validation — correct approach
• ✅ Test names are clear specifications; table-driven structure is clean
• ✅ gitutil.ValidateGitRef / ValidateGitPath reuse existing, tested helpers rather than reinventing validation

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.9 AIC · ⌖ 11.8 AIC · ⊞ 7K
_{Comment /matt to run again}

Comments that could not be inline-anchored

pkg/cli/download_workflow.go:150

[/diagnosing-bugs]downloadWorkflowContentViaGitClone passes ref to git checkout without a -- end-of-options separator. Currently reachable only via downloadWorkflowContentViaGit (which validates first), but if ever called directly a leading-dash ref could be parsed as a git flag, bypassing the fix.

<details>
<summary>💡 Suggested fix</summary>

Add ValidateGitRef(ref) at the top of downloadWorkflowContentViaGitClone and use -- in checkout:

// top of downloadWorkflowC…</details><details><summary>pkg/cli/download_workflow.go:47</summary>**[/diagnosing-bugs]**The`#nosec G204`commentsaysref/pathare&quot;authoredbythedeveloper&quot;, butthePR&#39;sownthreatmodelshowstheycomefromattacker-controlled`source:`/`redirect:` frontmatter. Thecommentshouldbeupdatedtoaccuratelyreflectthetrustboundary.
&lt;details&gt;
&lt;summary&gt;💡 Suggestedupdate&lt;/summary&gt;
```go// #nosec G204 -- repoURL is derived from the developer&#39;s workflow import configuration.// ref and path are user-supplied values validated above by ValidateGitRef/Vali…</details><details><summary>pkg/cli/download_workflow_test.go:140</summary>**[/tdd]** The test suite covers all 6 rejection cases but has no happy-path test. Without one, a future refactor that breaks the early-return logic (e.g. validation always returns an error) would go undetected.&lt;details&gt;&lt;summary&gt;💡 Suggested addition&lt;/summary&gt;Add a table entry with a valid repo/ref/path that should *not* error on validation. Since the subprocess will fail in a test environment, assert the error does **not** contain `&quot;refusinggitfallback&quot;`:```go
{
name: &quot;valid input…
</details>

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 85/100 — Excellent

Analyzed 1 test function with 6 subtests: 1 design test, 0 implementation tests, 0 violations.

📊 Metrics (1 test, 6 subtests)

Metric	Value
Analyzed	1 behavioral test (6 table-driven rows)
✅ Design	1 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	6/6 (100%) — all rows verify error handling
Duplicate clusters	0
Inflation	4.6:1 (65 test lines : 14 prod lines)
🚨 Violations	0

Test Details:

Test	File	Classification	Coverage
TestDownloadWorkflowContentViaGitValidation	pkg/cli/download_workflow_test.go:80–142	Design test (security)	6 edge cases (CWE-88 injection vectors)

✅ Test Quality Strengths

• Security-focused design test: All 6 subtests verify argument injection (CWE-88) boundary conditions before git subprocess invocation
• Comprehensive attack vector coverage: path dash-prefix, ref dash-prefix, path traversal (../), ref dotdot (..), empty ref, empty path
• Proper error handling: Both require.Error() (mandatory error) and assert.Contains() (specific error message) assertions present
• Table-driven structure: Each scenario is independent and clearly named; easy to add new cases
• No mock libraries: Direct function testing (appropriate for unit test)
• Build tag compliance: //go:build !integration present on line 1
• Justified inflation: 4.6:1 ratio reflects comprehensive security validation with table-driven rows; each row is necessary

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). 1 design test covering 6 security-critical edge cases with proper error assertions. No violations.

🧪 Test quality analysis by Test Quality Sentinel · haiku45 · 19.3 AIC · ⌖ 7.15 AIC · ⊞ 8.4K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 85/100 — Excellent. 1 design test with 6 security-critical edge cases (CWE-88 argument injection), 0% implementation tests (threshold: 30%), no violations.

[github-actions]

Verdict: Approve

Clean, well-tested CWE-88 fix. Reuses existing gitutil.ValidateGitRef/ValidateGitPath validators consistently with pkg/parser/remote_download_file.go, adds the -- end-of-options separator as defence-in-depth, and includes solid negative-path test coverage (dash-prefixed ref/path, traversal, empty values) verifying the subprocess is never spawned.

💡 Notes

No blocking issues found in the diff. The lock.yml change appears to be an unrelated auto-generated regeneration artifact bundled into the PR — worth double-checking it wasn't accidentally included, but it's not a security or correctness concern for this fix.

🔎 Code quality review by PR Code Quality Reviewer · auto · 17.2 AIC · ⌖ 11.2 AIC · ⊞ 7.8K
_{Comment /review to run again}

[pelikhan]

@copilot run pr-finisher skill

[Copilot]

@copilot run pr-finisher skill

Investigated the lock file diff. The .github/workflows/daily-byok-ollama-test.lock.yml change is the correct compiler-generated output from the unchanged .md source — the compiler itself was updated (adding defaultAiCreditsPricing and the models routing table to the firewall config), so make recompile now produces a different lock file from the same markdown source. Reverting to the old lock file causes a drift error in the pre-commit gate (make check-workflow-drift). The current state is in sync and all pre-PR checks pass.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.84.2