From 64552aee9ba891ec040728ab8ef0d2d3eff1df7e Mon Sep 17 00:00:00 2001
From: MuoDoo <36472129+MuoDoo@users.noreply.github.com>
Date: Sat, 13 Jun 2026 11:07:59 +0800
Subject: [PATCH] fix(security): block git ref option injection (#112)
---
cmd/opencodereview/git.go | 2 +-
cmd/opencodereview/review_cmd.go | 31 ++++++++++++++++++++++++++
cmd/opencodereview/review_cmd_test.go | 26 ++++++++++++++++++++++
internal/diff/git.go | 6 ++---
internal/diff/git_test.go | 22 ++++++++++++++++++
internal/diff/parser.go | 2 +-
internal/tool/code_search.go | 1 +
internal/tool/code_search_test.go | 32 ++++++++++++++++++++++++++-
internal/tool/file_find.go | 2 +-
internal/tool/filereader.go | 4 ++--
10 files changed, 119 insertions(+), 9 deletions(-)
create mode 100644 cmd/opencodereview/review_cmd_test.go
diff --git a/cmd/opencodereview/git.go b/cmd/opencodereview/git.go
index ed7485f..0185b56 100644
--- a/cmd/opencodereview/git.go
+++ b/cmd/opencodereview/git.go
@@ -13,7 +13,7 @@ func runGitCmd(repoDir string, args ...string) ([]byte, error) {
}
func getCommitMessage(repoDir, commit string) (string, error) {
- out, err := runGitCmd(repoDir, "log", "-1", "--format=%B", commit)
+ out, err := runGitCmd(repoDir, "log", "-1", "--format=%B", "--end-of-options", commit)
if err != nil {
return "", fmt.Errorf("git log failed: %w", err)
}
diff --git a/cmd/opencodereview/review_cmd.go b/cmd/opencodereview/review_cmd.go
index 400c1c2..bf9f3c5 100644
--- a/cmd/opencodereview/review_cmd.go
+++ b/cmd/opencodereview/review_cmd.go
@@ -5,6 +5,7 @@ import (
"fmt"
"os"
"path/filepath"
+ "strings"
"time"
"github.com/open-code-review/open-code-review/internal/agent"
@@ -48,6 +49,9 @@ func runReview(args []string) error {
if err != nil {
return fmt.Errorf("resolve repo: %w", err)
}
+ if err := validateReviewRefs(repoDir, opts); err != nil {
+ return err
+ }
if opts.commit != "" && opts.background == "" {
if msg, err := getCommitMessage(repoDir, opts.commit); err == nil && msg != "" {
@@ -216,6 +220,33 @@ func requireGitRepo(dir string) error {
return nil
}
+func validateReviewRefs(repoDir string, opts reviewOptions) error {
+ refs := []struct {
+ flag string
+ ref string
+ }{
+ {"--from", opts.from},
+ {"--to", opts.to},
+ {"--commit", opts.commit},
+ }
+ for _, item := range refs {
+ if item.ref == "" {
+ continue
+ }
+ if strings.HasPrefix(item.ref, "-") {
+ return fmt.Errorf("%s value %q is not a valid git ref: refs must not start with '-'", item.flag, item.ref)
+ }
+ if out, err := runGitCmd(repoDir, "rev-parse", "--verify", "--end-of-options", item.ref+"^{commit}"); err != nil {
+ msg := strings.TrimSpace(string(out))
+ if msg != "" {
+ return fmt.Errorf("%s value %q is not a valid commit ref: %s", item.flag, item.ref, msg)
+ }
+ return fmt.Errorf("%s value %q is not a valid commit ref", item.flag, item.ref)
+ }
+ }
+ return nil
+}
+
func runPreview(repoDir string, opts reviewOptions, fileFilter *rules.FileFilter) error {
gitRunner := gitcmd.New(opts.maxGitProcs)
ag := agent.New(agent.Args{
diff --git a/cmd/opencodereview/review_cmd_test.go b/cmd/opencodereview/review_cmd_test.go
new file mode 100644
index 0000000..1daf95d
--- /dev/null
+++ b/cmd/opencodereview/review_cmd_test.go
@@ -0,0 +1,26 @@
+package main
+
+import (
+ "strings"
+ "testing"
+)
+
+func TestValidateReviewRefsRejectsOptionLikeCommit(t *testing.T) {
+ err := validateReviewRefs(t.TempDir(), reviewOptions{commit: "-O./pwn.sh"})
+ if err == nil {
+ t.Fatal("expected option-like --commit ref to be rejected")
+ }
+ if !strings.Contains(err.Error(), "--commit") || !strings.Contains(err.Error(), "must not start with '-'") {
+ t.Fatalf("unexpected error: %v", err)
+ }
+}
+
+func TestValidateReviewRefsRejectsOptionLikeRangeRef(t *testing.T) {
+ err := validateReviewRefs(t.TempDir(), reviewOptions{to: "-O./pwn.sh"})
+ if err == nil {
+ t.Fatal("expected option-like --to ref to be rejected")
+ }
+ if !strings.Contains(err.Error(), "--to") || !strings.Contains(err.Error(), "must not start with '-'") {
+ t.Fatalf("unexpected error: %v", err)
+ }
+}
diff --git a/internal/diff/git.go b/internal/diff/git.go
index 765516b..f56201d 100644
--- a/internal/diff/git.go
+++ b/internal/diff/git.go
@@ -115,14 +115,14 @@ func (p *Provider) GetDiff(ctx context.Context) ([]model.Diff, error) {
if base == "" {
return nil, fmt.Errorf("cannot find merge-base between %s and %s", p.from, p.to)
}
- out, err := p.runGit(ctx, "diff", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), base, p.to, "--")
+ out, err := p.runGit(ctx, "diff", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--end-of-options", base, p.to, "--")
if err != nil {
return nil, fmt.Errorf("git diff failed: %w", err)
}
combined.WriteString(out)
case ModeCommit:
- out, err := p.runGit(ctx, "show", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), p.commit)
+ out, err := p.runGit(ctx, "show", "--no-ext-diff", "--no-textconv", "--src-prefix=a/", "--dst-prefix=b/", "--no-color", "-U"+fmt.Sprint(DiffContextLines), "--end-of-options", p.commit)
if err != nil {
return nil, fmt.Errorf("git show failed: %w", err)
}
@@ -257,7 +257,7 @@ func (p *Provider) filterDiffs(diffs []model.Diff) []model.Diff {
// ---- Internal helpers ----
func (p *Provider) computeMergeBase(ctx context.Context, from, to string) string {
- out, err := p.runGit(ctx, "merge-base", from, to)
+ out, err := p.runGit(ctx, "merge-base", "--end-of-options", from, to)
if err != nil {
return ""
}
diff --git a/internal/diff/git_test.go b/internal/diff/git_test.go
index b414b4e..93f18b8 100644
--- a/internal/diff/git_test.go
+++ b/internal/diff/git_test.go
@@ -124,6 +124,28 @@ func TestCommitDiffSurvivesExternalDiffTool(t *testing.T) {
}
}
+func TestCommitDiffTreatsOptionLikeRefAsRevision(t *testing.T) {
+ repo := initRepoWithChange(t)
+ pagerPath := filepath.Join(repo, "pwn.sh")
+ proofPath := filepath.Join(repo, "PROOF")
+ if err := os.WriteFile(pagerPath, []byte("#!/bin/sh\nprintf pwned > PROOF\n"), 0755); err != nil {
+ t.Fatalf("write pager: %v", err)
+ }
+
+ runner := gitcmd.New(0)
+ provider := NewCommitProvider(repo, "-O./pwn.sh", runner)
+
+ _, err := provider.GetDiff(context.Background())
+ if err == nil {
+ t.Fatal("expected option-like commit ref to fail as an invalid revision")
+ }
+ if _, statErr := os.Stat(proofPath); statErr == nil {
+ t.Fatal("option-like commit ref was interpreted as a git show option")
+ } else if !os.IsNotExist(statErr) {
+ t.Fatal(statErr)
+ }
+}
+
// TestRangeDiffSurvivesExternalDiffTool covers the ModeRange call site
// (git diff ), which likewise must pass --no-ext-diff so that a
// user's external diff tool does not break range comparisons.
diff --git a/internal/diff/parser.go b/internal/diff/parser.go
index afcea79..97502aa 100644
--- a/internal/diff/parser.go
+++ b/internal/diff/parser.go
@@ -92,7 +92,7 @@ func finalizeDiff(ctx context.Context, d *model.Diff, repoDir string, ref string
return
}
if ref != "" {
- args := []string{"-c", "core.quotepath=false", "show", ref + ":" + d.NewPath}
+ args := []string{"-c", "core.quotepath=false", "show", "--end-of-options", ref + ":" + d.NewPath}
var output []byte
var err error
if runner != nil {
diff --git a/internal/tool/code_search.go b/internal/tool/code_search.go
index 1fbccb5..2c7c7a9 100644
--- a/internal/tool/code_search.go
+++ b/internal/tool/code_search.go
@@ -67,6 +67,7 @@ func (p *CodeSearchProvider) buildGrepArgs(searchText string, caseSensitive bool
cmdArgs = append(cmdArgs, "-e", searchText)
if ref := p.FileReader.Ref; ref != "" {
+ cmdArgs = append(cmdArgs, "--end-of-options")
cmdArgs = append(cmdArgs, ref)
}
diff --git a/internal/tool/code_search_test.go b/internal/tool/code_search_test.go
index 15404e3..ee4abb9 100644
--- a/internal/tool/code_search_test.go
+++ b/internal/tool/code_search_test.go
@@ -29,7 +29,14 @@ func TestBuildGrepArgs_CommitMode(t *testing.T) {
p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: "abc1234"})
args := p.buildGrepArgs("myFunc", false, false, []string{"pkg/"})
- assertContainsInOrder(t, args, "-e", "myFunc", "abc1234", "--", "pkg/")
+ assertContainsInOrder(t, args, "-e", "myFunc", "--end-of-options", "abc1234", "--", "pkg/")
+}
+
+func TestBuildGrepArgs_RefUsesEndOfOptions(t *testing.T) {
+ p := NewCodeSearch(&FileReader{RepoDir: "/tmp", Ref: "-O./pwn.sh"})
+ args := p.buildGrepArgs("myFunc", false, false, nil)
+
+ assertContainsInOrder(t, args, "-e", "myFunc", "--end-of-options", "-O./pwn.sh", "--")
}
func TestBuildGrepArgs_PatternStartingWithDash(t *testing.T) {
@@ -187,6 +194,29 @@ func TestGitGrep_CommitMode_WithPathspec(t *testing.T) {
}
}
+func TestGitGrep_OptionLikeRefDoesNotLaunchPager(t *testing.T) {
+ dir := setupTestRepo(t)
+ proofPath := filepath.Join(dir, "PROOF")
+ pagerPath := filepath.Join(dir, "pwn.sh")
+ if err := os.WriteFile(pagerPath, []byte("#!/bin/sh\nprintf pwned > PROOF\n"), 0755); err != nil {
+ t.Fatal(err)
+ }
+
+ p := NewCodeSearch(&FileReader{RepoDir: dir, Ref: "-O./pwn.sh", Mode: ModeCommit})
+ result, err := p.gitGrep(context.Background(), "Hello", false, false, []string{"hello.go"})
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !strings.Contains(result, "unable to resolve revision") && !strings.Contains(result, "Not a valid object name") {
+ t.Fatalf("expected invalid revision error, got: %s", result)
+ }
+ if _, err := os.Stat(proofPath); err == nil {
+ t.Fatal("option-like ref launched pager and created proof file")
+ } else if !os.IsNotExist(err) {
+ t.Fatal(err)
+ }
+}
+
func TestGitGrep_CommitMode_WithBadPathspec(t *testing.T) {
dir := setupTestRepo(t)
commit := getHeadCommit(t, dir)
diff --git a/internal/tool/file_find.go b/internal/tool/file_find.go
index 81a48d7..87ea5b3 100644
--- a/internal/tool/file_find.go
+++ b/internal/tool/file_find.go
@@ -72,7 +72,7 @@ func (p *FileFindProvider) listGitFiles(parentCtx context.Context) ([]string, er
var args []string
if ref := p.FileReader.Ref; ref != "" {
- args = []string{"ls-tree", "-r", "--name-only", ref}
+ args = []string{"ls-tree", "-r", "--name-only", "--end-of-options", ref}
} else {
args = []string{"ls-files", "--cached", "--others", "--exclude-standard"}
}
diff --git a/internal/tool/filereader.go b/internal/tool/filereader.go
index 580a954..d7cdfa9 100644
--- a/internal/tool/filereader.go
+++ b/internal/tool/filereader.go
@@ -127,7 +127,7 @@ func (fr *FileReader) readFromGitShow(parentCtx context.Context, path string) (s
ctx, cancel := context.WithTimeout(parentCtx, 30*time.Second)
defer cancel()
- args := []string{"-c", "core.quotepath=false", "show", fr.Ref + ":" + path}
+ args := []string{"-c", "core.quotepath=false", "show", "--end-of-options", fr.Ref + ":" + path}
if fr.Runner != nil {
output, err := fr.Runner.Output(ctx, fr.RepoDir, args...)
if err != nil {
@@ -213,7 +213,7 @@ func (fr *FileReader) readLinesFromDisk(path string, startLine, maxLines int) ([
}
func (fr *FileReader) readLinesFromGitShow(ctx context.Context, path string, startLine, maxLines int) ([]string, int, error) {
- args := []string{"-c", "core.quotepath=false", "show", fr.Ref + ":" + path}
+ args := []string{"-c", "core.quotepath=false", "show", "--end-of-options", fr.Ref + ":" + path}
var collected []string
var totalLines int