fix: git argument injection via unvalidated ref/path in remote import fallbacks (VULN-001)#47957
Conversation
…parators (VULN-001) - Add ValidateGitRef and ValidateGitPath helpers to pkg/gitutil that reject refs/paths starting with '-' (argument injection CWE-88) and refs containing '..' (git traversal expressions) - Validate ref at parse time in parseWorkflowSpecParts and parseRemoteOrigin so malicious workflowspec imports are rejected before any git subprocess runs - Add '--' end-of-options separator to all git subprocess calls that take user-derived positional arguments: - git archive --remote=<url> <ref> -- <path> (remote_download_file.go) - git checkout -- <ref> (remote_download_file.go) - git ls-remote <url> -- <ref> (remote_resolve_sha.go) - Add early ref/path validation guards in downloadFileViaGit and downloadFileViaGitClone fallback paths - Add unit tests for ValidateGitRef and ValidateGitPath covering valid cases, empty inputs, leading-dash injection, and dotdot traversal Fixes #47940 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds centralized validation and Git argument separators to prevent remote-import argument injection.
Changes:
- Validates user-controlled Git refs and paths.
- Hardens Git fallback commands with option separators.
- Adds validator unit tests.
Show a summary per file
| File | Description |
|---|---|
pkg/gitutil/gitutil.go |
Adds ref and path validators. |
pkg/gitutil/gitutil_test.go |
Tests validator behavior. |
pkg/parser/remote_workflow_spec.go |
Validates parsed workflow specs. |
pkg/parser/import_remote.go |
Rejects unsafe remote refs. |
pkg/parser/remote_download_file.go |
Hardens archive and clone fallbacks. |
pkg/parser/remote_resolve_sha.go |
Hardens ref resolution fallback. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Medium
| // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix: git argument injection via unvalidated ref/path (VULN-001)
The overall approach is sound — ValidateGitRef/ValidateGitPath guards + -- end-of-options separators correctly close the CWE-88 argument-injection surface. Tests are thorough.
One blocking correctness bug and one consistency gap:
-
git checkout -- <sha>is semantically wrong (line 423,downloadFileViaGitClone). In git,--separates options from pathspecs, not from commit refs.git checkout -- <sha>tries to restore files matching the pathspec<sha>from the index, not check out that commit. The correct form isgit checkout <ref>— which is safe here becauseValidateGitRefalready blocks leading-dash values. -
downloadFileViaGitClonedoes not validatepath(line 386).downloadFileViaGitvalidates bothrefandpath; the clone fallback only validatesref. Low risk now (path is not passed to git commands in this path), but worth adding for defence-in-depth and consistency.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 30.6 AIC · ⌖ 7.59 AIC · ⊞ 5K
| // Now checkout the specific commit | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) | ||
| // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) |
There was a problem hiding this comment.
Bug: git checkout -- <sha> does not check out a commit
The -- separator tells git that what follows is a pathspec, not a ref. So git checkout -- <sha> attempts to restore files matching the pathspec <sha> from the index — it does not switch to that commit. This will silently fail or produce unexpected results.
The correct invocation for a SHA is:
checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref)This is safe because ValidateGitRef already blocks any ref starting with -, eliminating argument-injection risk, and a 40-char hex SHA can never look like a git flag. The -- guard is meaningful for git archive and git ls-remote where the ref is an arbitrary user-supplied branch/tag, but git checkout <commit> does not accept -- as a ref/pathspec delimiter in the same way.
@copilot please address this.
| func downloadFileViaGitClone(ctx context.Context, owner, repo, path, ref, host string) ([]byte, error) { | ||
| remoteLog.Printf("Attempting git clone fallback for %s/%s/%s@%s", owner, repo, path, ref) | ||
|
|
||
| if err := gitutil.ValidateGitRef(ref); err != nil { |
There was a problem hiding this comment.
downloadFileViaGitClone does not validate path
downloadFileViaGit validates both ref and path before any subprocess, but downloadFileViaGitClone only validates ref. While path is not passed to git commands in this function (so there is no direct argument-injection risk here), the inconsistency is worth closing: if the code is refactored later and path is passed to a subprocess, there would be no guard.
Add the missing guard after the ref check:
if err := gitutil.ValidateGitPath(path); err != nil {
return nil, fmt.Errorf("refusing git clone fallback: %w", err)
}@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (207 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
Test Quality DetailsTestValidateGitRef (9 test cases, table-driven)
TestValidateGitPath (5 test cases, table-driven)
Quality Signals
Verdict
|
There was a problem hiding this comment.
REQUEST_CHANGES — four blocking issues found
The security intent of this PR is correct, but the implementation introduces two command-semantic bugs and leaves two validation gaps.
Blocking findings
-
git checkout -- refis semantically wrong (remote_download_file.go:423):--makes git treat the argument as a pathspec, not a branch/commit. The SHA checkout will silently fail or restore a nonexistent file instead of switching to the commit. Remove--here;ValidateGitRefalready prevents leading-dash injection. -
git ls-remote ... -- refis not a defined interface (remote_resolve_sha.go:48,53):git ls-remotehas no--separator between remote and ref patterns. The--is treated as a literal ref to match, breaking ref resolution. Remove the--;ValidateGitRefcovers the injection risk. -
downloadFileViaGitClonemissingValidateGitPath(remote_download_file.go:388): sibling functiondownloadFileViaGitvalidates bothrefandpath; this one only validatesref. Thepathreachesos.ReadFileafterfilepath.Join, creating asymmetric coverage. -
ValidateGitPathdoes not block..traversal (gitutil.go:90–96): only leading-is rejected.subdir/../../etc/shadowpasses. Should also reject..-containing and absolute paths, consistent withValidateGitRef.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 50.4 AIC · ⌖ 5.15 AIC · ⊞ 5.7K
Comment /review to run again
| // Now checkout the specific commit | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) | ||
| // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) |
There was a problem hiding this comment.
git checkout -- ref is semantically broken: -- signals end-of-options to git checkout, so everything after it is treated as a pathspec (file to restore from HEAD), not a branch/commit to switch to. This will fail to actually check out the SHA.
💡 Suggested fix
Remove the -- separator for the commit-checkout case. ValidateGitRef already blocks dash-prefixed refs, so there is no injection risk:
// ValidateGitRef above guarantees ref does not start with -.
checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref)The -- separator only makes sense for git checkout <tree-ish> -- <pathspecs>, i.e. when you want to restore specific files, not switch HEAD. Using it here inverts the semantics.
| // The '--' end-of-options separator ensures ref is never parsed as a flag even | ||
| // if it begins with '-' (argument injection, CWE-88). ValidateGitRef above also | ||
| // rejects such values; '--' is kept as defence-in-depth. | ||
| cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, "--", ref) |
There was a problem hiding this comment.
git ls-remote does not support -- as a remote/refs separator: inserting -- between the remote URL and the ref pattern means git receives ["ls-remote", repoURL, "--", ref], treating "--" as a literal ref pattern to match and ref as an additional pattern — the actual ref may or may not be matched depending on git version.
💡 Suggested fix
git ls-remote does not define a -- end-of-options boundary between remote and refs. The injection risk is already eliminated by ValidateGitRef, which rejects dash-prefixed refs. Remove the spurious --:
cmd := exec.CommandContext(ctx, "git", "ls-remote", repoURL, ref)
// and the prefix loop:
cmd = exec.CommandContext(ctx, "git", "ls-remote", repoURL, prefix+ref)The same applies to line 53 in the prefix-retry loop.
|
|
||
| if err := gitutil.ValidateGitRef(ref); err != nil { | ||
| return nil, fmt.Errorf("refusing git clone fallback: %w", err) | ||
| } |
There was a problem hiding this comment.
downloadFileViaGitClone validates ref but not path: this is asymmetric with downloadFileViaGit which validates both. The unvalidated path is later joined with tmpDir and used in os.ReadFile, bypassing the injection check that exists in the sibling function.
💡 Suggested fix
Add a ValidateGitPath call immediately after the ValidateGitRef call:
if err := gitutil.ValidateGitRef(ref); err != nil {
return nil, fmt.Errorf("refusing git clone fallback: %w", err)
}
if err := gitutil.ValidateGitPath(path); err != nil {
return nil, fmt.Errorf("refusing git clone fallback: %w", err)
}While fileutil.ValidatePathWithinBase is called later (line 438), defense-in-depth and API symmetry both require the same upfront rejection that the archive path gets.
| } | ||
| if strings.HasPrefix(path, "-") { | ||
| return fmt.Errorf("invalid git path %q: paths must not start with '-' to prevent argument injection", path) | ||
| } |
There was a problem hiding this comment.
ValidateGitPath does not block .. traversal sequences: only leading - is rejected. A path like subdir/../../etc/shadow passes validation and, in the clone path where it is joined with tmpDir via filepath.Join, will escape the intended clone directory before ValidatePathWithinBase catches it.
💡 Suggested fix
Add a .. component check analogous to what ValidateGitRef already does:
func ValidateGitPath(path string) error {
if path == "" {
return errors.New("git path must not be empty")
}
if strings.HasPrefix(path, "-") {
return fmt.Errorf("invalid git path %q: paths must not start with '-'", path)
}
if strings.HasPrefix(path, "/") {
return fmt.Errorf("invalid git path %q: paths must not be absolute", path)
}
if strings.Contains(path, "..") {
return fmt.Errorf("invalid git path %q: paths must not contain '..'", path)
}
return nil
}ValidatePathWithinBase is a correct second layer, but ValidateGitPath should be self-sufficient.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness regression and several validation gaps.
📋 Key Themes & Highlights
Key Themes
- Critical regression (
/diagnosing-bugs):git checkout -- <sha>(line 423 ofremote_download_file.go) changes semantics from a detached HEAD checkout to a pathspec restore — SHA-based clones will fail at runtime after this PR. - Incomplete validation (
/tdd):downloadFileViaGitClonevalidatesrefbut notpath;ValidateGitPathis missing a..traversal guard that its siblingValidateGitRefhas;ValidateGitRefaccepts NUL bytes that can corruptls-remoteoutput parsing. - Silent error swallowing (
/diagnosing-bugs):parseRemoteOriginlogs and returnsnilon an invalid ref instead of propagating an error, making security rejections indistinguishable from normal parse failures in callers.
Positive Highlights
- ✅ Defence-in-depth design: validation at parse time and execution time, plus
--separators throughout — the right approach for CWE-88. - ✅
ValidateGitRefandValidateGitPathare well-named, in a sharedgitutilpackage, and easy to call consistently. - ✅ Test table structure is clear: valid inputs, empty, leading-dash, and traversal cases are well covered for
ValidateGitRef. - ✅
#nosec G204comments are accurate and well-explained.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54.9 AIC · ⌖ 5.31 AIC · ⊞ 6.7K
Comment /matt to run again
| // Now checkout the specific commit | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) | ||
| // Now checkout the specific commit; '--' prevents ref from being parsed as a flag. | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", "--", ref) |
There was a problem hiding this comment.
[/diagnosing-bugs] git checkout -- <sha> does not switch the working tree to a commit — it restores files named by that SHA from the index, making this checkout a no-op or error at runtime.
💡 Root cause and fix
git checkout <sha> (detached HEAD) is the correct incantation for switching to a specific commit. Adding -- before a SHA changes semantics entirely: git interprets the argument as a pathspec and tries to restore a file named after the 40-char hex string from the index — which will fail or silently do nothing.
In this branch ref is already validated to be a 40-char hex SHA by the isSHA guard above (len(ref) == 40 && gitutil.IsHexString(ref)), so ValidateGitRef already ensures no leading -. The -- is unnecessary here and actively breaks the operation:
// Remove the '--' — detached HEAD checkout doesn't use pathspec separator
checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref)If explicit defence-in-depth is desired, prefer git switch --detach <ref> which cannot be confused with a pathspec operation.
@copilot please address this.
|
|
||
| if err := gitutil.ValidateGitRef(ref); err != nil { | ||
| return nil, fmt.Errorf("refusing git clone fallback: %w", err) | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] downloadFileViaGitClone validates ref but not path — a path like -output=/etc/passwd is still passed to filepath.Join and os.ReadFile after cloning.
💡 Fix
downloadFileViaGit (the function above) validates both ref and path at lines 296–301, but downloadFileViaGitClone only adds ValidateGitRef. Although path is not passed to a subprocess here (it's used with filepath.Join + ValidatePathWithinBase), consistency with the sibling function is important and path traversal attacks via crafted paths could still reach ValidatePathWithinBase.
Add the missing guard immediately after the ref check:
if err := gitutil.ValidateGitPath(path); err != nil {
return nil, fmt.Errorf("refusing git clone fallback: %w", err)
}@copilot please address this.
| // Reject refs that would be unsafe to pass to git subprocesses. | ||
| if err := gitutil.ValidateGitRef(ref); err != nil { | ||
| importRemoteLog.Printf("Rejecting spec %q: invalid ref: %v", spec, err) | ||
| return nil |
There was a problem hiding this comment.
[/diagnosing-bugs] Returning nil on an invalid ref silently swallows the error — callers cannot distinguish a malformed spec from a not-found one, making injection attempts invisible in logs.
💡 Suggested approach
parseRemoteOrigin currently returns *remoteImportOrigin with no error channel. Silently returning nil means the rejection is only visible in a Printf log line — there's no way for the caller to surface "this ref was rejected for security reasons" vs. "this spec didn't parse".
Consider changing the signature to return an error:
func parseRemoteOrigin(spec string) (*remoteImportOrigin, error)or at minimum logging at Warn/Error level so security-relevant rejections are clearly distinguishable from parse failures.
@copilot please address this.
| if strings.HasPrefix(ref, "-") { | ||
| return fmt.Errorf("invalid git ref %q: refs must not start with '-' to prevent argument injection", ref) | ||
| } | ||
| if strings.Contains(ref, "..") { |
There was a problem hiding this comment.
[/tdd] ValidateGitRef does not reject refs containing a NUL byte (\x00), which git uses as a record separator in ls-remote output — a crafted ref could corrupt SHA parsing downstream.
💡 Suggested addition
NUL bytes in refs are invalid per the git ref spec and can confuse output parsers that split on \n\x00:
if strings.ContainsRune(ref, '\x00') {
return fmt.Errorf("invalid git ref %q: refs must not contain NUL bytes", ref)
}Add a corresponding test case:
{
name: "NUL byte is rejected",
ref: "main\x00evil",
expectError: true,
errContains: "NUL",
},@copilot please address this.
| require.Error(t, err, "expected error for ref %q", tt.ref) | ||
| assert.Contains(t, err.Error(), tt.errContains) | ||
| } else { | ||
| require.NoError(t, err, "unexpected error for ref %q", tt.ref) |
There was a problem hiding this comment.
[/tdd] TestValidateGitPath has no test case for path traversal (../etc/passwd) — ValidateGitPath currently accepts it, but downloadFileViaGitClone relies on ValidatePathWithinBase as a second defence. If that ever changes, traversal goes undetected at the validation layer.
💡 Suggested test case + fix
Add a test case and a corresponding guard in ValidateGitPath:
// In TestValidateGitPath:
{
name: "path traversal is rejected",
path: "../etc/passwd",
expectError: true,
errContains: "traversal",
},// In ValidateGitPath:
if strings.Contains(path, "..") {
return fmt.Errorf("invalid git path %q: paths must not contain '..'", path)
}This makes the guard self-contained and not dependent on a downstream ValidatePathWithinBase call.
@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the blocking fallback issues in a06d4c9: SHA clone fallback now uses detached checkout, clone fallback validates path, |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
User-controlled
refandpathvalues from workflowspec imports (owner/repo/path@<ref>) were passed directly as positional arguments to git subprocesses with no validation and no--end-of-options separator. Areflike--upload-pack=maliciousor apathlike--output=/etc/cron.d/pwnedwould be parsed by git as flags rather than values (CWE-88, argument injection). The auth-error fallback path — triggered on any token-less or unauthorized execution — is the primary sink.Changes
pkg/gitutil/gitutil.go— two new shared guards:ValidateGitRef: rejects empty, leading--, and..-containing refsValidateGitPath: rejects empty and leading--pathspkg/parser/remote_workflow_spec.go/import_remote.go— ref and path validated at parse time inparseWorkflowSpecPartsandparseRemoteOrigin, before any subprocess is reachedpkg/parser/remote_download_file.go— validation guards added to bothdownloadFileViaGitanddownloadFileViaGitClone;git archiveandgit checkoutcalls use--end-of-options:pkg/parser/remote_resolve_sha.go— ref validated beforegit ls-remote; all three call sites use--:pkg/gitutil/gitutil_test.go— unit tests for both validators covering valid inputs, empty, leading--, and..traversal cases