From 5d91d54a17b427343137e650ed59074905baf0ef Mon Sep 17 00:00:00 2001 From: Scott Brown Date: Wed, 8 Jul 2026 13:03:46 -0600 Subject: [PATCH] feat: add security fuzz tests and harden input sinks Add native Go fuzz tests covering the untrusted-input boundaries (clone method flag, repository/organization names and URLs, path resolution, clone-URL construction, and API JSON decoding) to guard against path traversal and command/argument injection. The type-safety validators (IsValid) existed but were never called, so untrusted GitHub API data flowed straight into filepath.Join and exec.Command. Harden those sinks first so the fuzz tests assert real security invariants: - Tighten name validators to strict allowlists (reject "."/"..", path separators, leading/trailing '-', control/whitespace chars). - Strengthen URL validators to reject leading '-' and embedded whitespace/control chars. - Add pure choke-point helpers resolveRepoPath, buildCloneURL, and parseRepos, wired into CloneRepo/FetchAllRepos. - Use `git clone --` separator to prevent argument injection. Fuzzing is split into two tiers: - Lightweight (CI): `task fuzz-ci` runs a short burst per target and is wired into a new fuzz job in ci.yml; seed corpora also run under `task test`. - Heavyweight (ad hoc): `task fuzz` runs each target for a long, configurable duration (FUZZTIME, default 5m). Both tiers auto-discover every Fuzz* target since `go test -fuzz` fuzzes one target per invocation. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/ci.yml | 18 ++ CLAUDE.md | 25 +++ README.md | 22 +++ Taskfile.yml | 25 +++ fuzz_test.go | 344 +++++++++++++++++++++++++++++++++++++++ gitgrab.go | 191 +++++++++++++++++++--- 6 files changed, 604 insertions(+), 21 deletions(-) create mode 100644 fuzz_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aa06993..2203e9c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,24 @@ jobs: - name: Run tests run: task test + fuzz: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - name: Set up Go + uses: actions/setup-go@v6 + with: + go-version-file: 'go.mod' + + - name: Install Task + uses: arduino/setup-task@v3 + with: + version: 3.x + + - name: Run lightweight fuzz tests + run: task fuzz-ci + build: runs-on: ubuntu-latest steps: diff --git a/CLAUDE.md b/CLAUDE.md index 9dd48be..8e43bd6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,6 +29,24 @@ This is `gitgrab`, a CLI utility written in Go that clones all GitHub repositori - Tests use `httptest.NewRecorder()` for mock HTTP responses - Current test coverage: ~70% +### Security fuzz tests (`fuzz_test.go`) + +Native Go fuzz targets cover the untrusted-input boundaries and assert security +invariants (path containment, no argument/command injection, safe URL/name +handling, robust JSON decoding). They run in two tiers: + +- **Lightweight (CI)**: `task fuzz-ci` runs a short, time-bounded burst per + target (`FUZZTIME` default `15s`) and is wired into the `fuzz` job in + `.github/workflows/ci.yml`. The seed corpora also run as normal tests under + `task test`. +- **Heavyweight (ad hoc)**: `task fuzz` runs each target for a long duration + (`FUZZTIME` default `5m`, e.g. `task fuzz FUZZTIME=30m`) on a developer + machine. + +Both tiers loop over every `Fuzz*` function automatically (targets are +discovered by grep in the `fuzz-run` internal task), since `go test -fuzz` +only fuzzes one target per invocation. + ## Development Commands **Build the application (preferred method):** @@ -70,6 +88,13 @@ task coverage go test . -run TestName ``` +**Run fuzz tests:** +```bash +task fuzz-ci # Lightweight, time-bounded (CI tier) +task fuzz # Heavyweight ad hoc (defaults to 5m per target) +task fuzz FUZZTIME=30m # Override per-target duration +``` + **Security and code quality checks:** ```bash task check # Run all security scans diff --git a/README.md b/README.md index 735b221..35611c4 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,28 @@ task build # The binary will be created at .build/gitgrab ``` +## Security fuzz testing + +GitGrab is fuzz-tested at its untrusted-input boundaries (the clone-method +flag, GitHub-supplied repository/organization names and URLs, path resolution, +clone-URL construction, and API JSON decoding) to guard against path traversal +and command/argument injection. The fuzz targets live in `fuzz_test.go` and +come in two tiers: + +```bash +# Lightweight, time-bounded burst — runs in CI on every push/PR +task fuzz-ci + +# Heavyweight ad hoc run on a developer machine (defaults to 5m per target) +task fuzz + +# Override the per-target duration +task fuzz FUZZTIME=30m +``` + +The seed corpora also execute as ordinary unit tests during `task test`, so the +malicious inputs are checked on every test run even without a fuzzing pass. + ## Requirements - Go 1.24+ diff --git a/Taskfile.yml b/Taskfile.yml index bd26be2..9706a65 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -56,6 +56,31 @@ tasks: cmds: - go test ./... -cover + fuzz-ci: + desc: "Lightweight, time-bounded fuzzing for CI (short burst per target)" + cmds: + - task: fuzz-run + vars: { FUZZTIME: "{{.FUZZTIME | default \"15s\"}}" } + + fuzz: + desc: "Heavyweight ad hoc fuzzing (override duration with FUZZTIME, e.g. FUZZTIME=5m)" + cmds: + - task: fuzz-run + vars: { FUZZTIME: "{{.FUZZTIME | default \"5m\"}}" } + + fuzz-run: + internal: true + desc: "Runs every Fuzz* target for FUZZTIME each (go fuzz allows one target per run)" + vars: + FUZZTIME: '{{.FUZZTIME | default "15s"}}' + TARGETS: + sh: grep -rhoE '^func (Fuzz[A-Za-z0-9_]+)' *.go | awk '{print $2}' + cmds: + - for: { var: TARGETS } + cmd: | + echo "==> Fuzzing {{.ITEM}} for {{.FUZZTIME}}" + go test -run '^$' -fuzz '^{{.ITEM}}$' -fuzztime={{.FUZZTIME}} . + check: desc: "Run all security scans" deps: [ sast, vet, vuln ] diff --git a/fuzz_test.go b/fuzz_test.go new file mode 100644 index 0000000..8095589 --- /dev/null +++ b/fuzz_test.go @@ -0,0 +1,344 @@ +package gitgrab + +// Security fuzz tests for gitgrab. +// +// These targets fuzz the untrusted-input boundaries of the tool: the clone +// method flag, GitHub-supplied repository/organization names and URLs, the +// path that a repository name is resolved to, the clone URL handed to git, and +// the JSON decoding of GitHub API responses. Each target asserts a security +// *invariant* (path containment, no argument injection, no command splitting) +// rather than a fixed expected value, so they detect regressions that would +// re-open a traversal or injection hole. +// +// Two ways to run them: +// +// Lightweight (CI): the seed corpora below run as ordinary tests on every +// `go test`/`task test`, and `task fuzz-ci` adds a short, time-bounded fuzz +// burst per target. Fast and deterministic. +// +// Heavyweight (ad hoc, on a developer machine): `task fuzz` runs each target +// for a long, configurable duration, e.g. `task fuzz FUZZTIME=5m`. + +import ( + "os" + "path/filepath" + "strings" + "testing" + "unicode" +) + +// fixedTarget is an arbitrary, safe target directory used where the test needs +// a stable base for path resolution. resolveRepoPath does not touch the +// filesystem, so this need not exist. +const fixedTarget = "/tmp/gitgrab-target" + +// hasControlOrSpace reports whether s contains any control character, +// whitespace, or the Unicode replacement character (which marks invalid UTF-8). +func hasControlOrSpace(s string) bool { + for _, r := range s { + if r == unicode.ReplacementChar || unicode.IsControl(r) || unicode.IsSpace(r) { + return true + } + } + return false +} + +// FuzzParseCloneMethod checks that parsing the untrusted -m flag never panics +// and always yields a known clone method, defaulting to SSH on error. +func FuzzParseCloneMethod(f *testing.F) { + seeds := []string{ + "ssh", "http", "SSH", "HTTP", "Ssh", "hTtP", + "", " ", "ftp", "git", "https", "ssh ", " http", + "ssh\n", "http;rm -rf /", "-o", "--config", "$(id)", + "ssh\x00http", "SSH\t", strings.Repeat("s", 4096), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, s string) { + m, err := ParseCloneMethod(s) + + // The returned method must always be one of the known values. + if m != CloneMethodSSH && m != CloneMethodHTTP { + t.Fatalf("ParseCloneMethod(%q) returned unknown method %d", s, m) + } + if got := m.String(); got != "ssh" && got != "http" { + t.Fatalf("ParseCloneMethod(%q).String() = %q, want ssh|http", s, got) + } + + low := strings.ToLower(s) + if err == nil && low != "ssh" && low != "http" { + t.Fatalf("ParseCloneMethod(%q) returned nil error for invalid input", s) + } + if err != nil && m != CloneMethodSSH { + t.Fatalf("ParseCloneMethod(%q) errored but did not default to ssh", s) + } + }) +} + +// FuzzRepositoryNameValidation asserts that any repository name accepted by +// RepositoryName.IsValid is genuinely safe: no path separators, not "." or +// "..", no leading dash (flag injection), no control/whitespace characters, and +// that it resolves to a path contained directly within the target directory. +func FuzzRepositoryNameValidation(f *testing.F) { + seeds := []string{ + "repo", "my-repo", "my_repo", "repo.git", "a", "R2-D2", + "", ".", "..", "...", "-repo", "repo-", + "../../etc/passwd", "..\\..\\windows", "a/b", "a\\b", + "foo bar", "foo\tbar", "repo\n", "repo\x00", "$(whoami)", + "`id`", "repo;rm -rf /", "--upload-pack=x", "-o", "café", + strings.Repeat("a", 8192), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, name string) { + r := RepositoryName(name) + if !r.IsValid() { + return + } + + if strings.ContainsAny(name, `/\`) { + t.Fatalf("valid repo name %q contains a path separator", name) + } + if name == "." || name == ".." { + t.Fatalf("valid repo name %q is a traversal component", name) + } + if strings.HasPrefix(name, "-") { + t.Fatalf("valid repo name %q starts with '-' (flag injection risk)", name) + } + if hasControlOrSpace(name) { + t.Fatalf("valid repo name %q contains control/whitespace", name) + } + + // A valid name must resolve to a contained path. + p, err := resolveRepoPath(fixedTarget, r) + if err != nil { + t.Fatalf("valid repo name %q failed to resolve: %v", name, err) + } + if filepath.Dir(p) != filepath.Clean(fixedTarget) { + t.Fatalf("valid repo name %q resolved outside target: %q", name, p) + } + if filepath.Base(p) != name { + t.Fatalf("valid repo name %q resolved to base %q", name, filepath.Base(p)) + } + }) +} + +// FuzzOrganizationNameValidation asserts that any organization name accepted by +// OrganizationName.IsValid is safe to interpolate into a clone URL: no +// separators, '@', ':' or spaces, no leading dash, no control characters, and +// that the resulting private-repo HTTP URL is well-formed and contains the org +// intact. +func FuzzOrganizationNameValidation(f *testing.F) { + seeds := []string{ + "github", "my-org", "Octocat", "a", "org123", + "", "-org", "org-", "my org", "my/org", "my\\org", + "a@b", "a:b", "org\n", "org\x00", "$(id)", "café", + strings.Repeat("o", 8192), + } + for _, s := range seeds { + f.Add(s) + } + + f.Fuzz(func(t *testing.T, org string) { + o := OrganizationName(org) + if !o.IsValid() { + return + } + + if strings.ContainsAny(org, `/\@: `) { + t.Fatalf("valid org name %q contains a URL metacharacter", org) + } + if strings.HasPrefix(org, "-") { + t.Fatalf("valid org name %q starts with '-'", org) + } + if hasControlOrSpace(org) { + t.Fatalf("valid org name %q contains control/whitespace", org) + } + + // A valid org must produce a safe private-repo HTTP clone URL. + cfg := CloneConfig{ + Repository: Repository{Name: RepositoryName("repo"), Private: true}, + Organization: o, + Token: GitHubToken("ghp_safeToken"), + Method: CloneMethodHTTP, + } + u, err := buildCloneURL(cfg) + if err != nil { + t.Fatalf("valid org name %q failed URL build: %v", org, err) + } + if !isSafeURL(u) { + t.Fatalf("valid org name %q produced unsafe URL %q", org, u) + } + if !strings.Contains(u, "/"+org+"/") { + t.Fatalf("org %q not present intact in URL %q", org, u) + } + }) +} + +// FuzzResolveRepoPath is the core path-traversal guard. For ANY target and +// name, if resolveRepoPath succeeds the result must sit directly inside the +// cleaned target directory and must not escape it via "..". +func FuzzResolveRepoPath(f *testing.F) { + seeds := []struct{ target, name string }{ + {"/tmp/x", "repo"}, + {"/tmp/x", "../escape"}, + {"/tmp/x", "../../etc/passwd"}, + {"/tmp/x", ".."}, + {"/tmp/x", "."}, + {"/tmp/x", "a/b"}, + {"/tmp/x", "a\\b"}, + {"", "repo"}, + {".", "repo"}, + {"relative/dir", "repo"}, + {"/tmp/x", "repo\x00/../../etc"}, + {"/a/b/c", "sub/../../../../root"}, + } + for _, s := range seeds { + f.Add(s.target, s.name) + } + + f.Fuzz(func(t *testing.T, target, name string) { + p, err := resolveRepoPath(target, RepositoryName(name)) + if err != nil { + return + } + + clean := filepath.Clean(target) + if filepath.Dir(p) != clean { + t.Fatalf("resolveRepoPath(%q, %q) = %q; parent %q != target %q", + target, name, p, filepath.Dir(p), clean) + } + if filepath.Base(p) != name { + t.Fatalf("resolveRepoPath(%q, %q) = %q; base %q != name", + target, name, p, filepath.Base(p)) + } + if rel, rerr := filepath.Rel(clean, p); rerr == nil { + if rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + t.Fatalf("resolveRepoPath(%q, %q) escapes target: rel=%q", + target, name, rel) + } + } + }) +} + +// FuzzBuildCloneURL asserts that whatever combination of untrusted fields is +// supplied, any URL buildCloneURL returns is safe to pass to `git clone`: +// non-empty, no leading dash, and free of whitespace/control/NUL characters +// that could split or corrupt the command. +func FuzzBuildCloneURL(f *testing.F) { + // Args: ssh, clone, name, org, token, private, useHTTP. + seeds := []struct { + ssh, clone, name, org, token string + private, useHTTP bool + }{ + {"git@github.com:o/r.git", "https://github.com/o/r.git", "r", "o", "ghp_x", false, false}, + {"git@github.com:o/r.git", "https://github.com/o/r.git", "r", "o", "ghp_x", true, true}, + {"git@github.com:o/r.git", "https://github.com/o/r.git", "r", "o", "ghp_x", false, true}, + {"-oProxyCommand=id", "https://github.com/o/r.git", "r", "o", "ghp_x", false, false}, + {"git@github.com:o/r.git", "--upload-pack=id", "r", "o", "ghp_x", false, true}, + {"ext::sh -c id", "https://github.com/o/r.git", "r", "o", "ghp_x", false, false}, + {"git@github.com:o/r.git", "https://x/r.git", "r", "o", "tok en", true, true}, + {"git@github.com:o/r.git", "https://x/r.git", "r", "o", "tok\nen", true, true}, + {"git@ github.com", "https://git hub.com/r.git", "r", "o", "ghp_x", false, false}, + {"", "", "", "", "", true, true}, + } + for _, s := range seeds { + f.Add(s.ssh, s.clone, s.name, s.org, s.token, s.private, s.useHTTP) + } + + f.Fuzz(func(t *testing.T, ssh, clone, name, org, token string, private, useHTTP bool) { + method := CloneMethodSSH + if useHTTP { + method = CloneMethodHTTP + } + cfg := CloneConfig{ + Repository: Repository{ + Name: RepositoryName(name), + CloneURL: HTTPURL(clone), + SSHURL: SSHURL(ssh), + Private: private, + }, + TargetDir: fixedTarget, + Token: GitHubToken(token), + Organization: OrganizationName(org), + Method: method, + } + + u, err := buildCloneURL(cfg) + if err != nil { + return + } + if !isSafeURL(u) { + t.Fatalf("buildCloneURL returned unsafe URL %q", u) + } + if strings.HasPrefix(u, "-") { + t.Fatalf("buildCloneURL returned URL that git reads as an option: %q", u) + } + if strings.ContainsAny(u, " \t\r\n\x00") { + t.Fatalf("buildCloneURL returned URL with command-splitting char: %q", u) + } + }) +} + +// FuzzParseRepos feeds arbitrary bytes to the GitHub API response decoder. It +// must never panic, and every repository it successfully decodes must, when run +// through the real sinks, either be rejected or produce a contained path and a +// safe clone URL. This ties the JSON boundary to the downstream safety checks. +func FuzzParseRepos(f *testing.F) { + seeds := []string{ + `[]`, + `[{"name":"repo","clone_url":"https://github.com/o/repo.git","ssh_url":"git@github.com:o/repo.git","private":false,"default_branch":"main"}]`, + `[{"name":"../../etc/passwd","ssh_url":"git@github.com:o/x.git"}]`, + `[{"name":"-oProxyCommand=id"}]`, + `[{"name":"repo","ssh_url":"ext::sh -c id"}]`, + `[{"name":"repo","ssh_url":"git@github.com:o/r.git\n--config=x"}]`, + `[{"name":"a","private":true},{"name":"b"}]`, + `{"not":"an array"}`, + `null`, + `[123, "string", null]`, + `[{"name":`, + `[] trailing garbage`, + strings.Repeat("[", 4096), + `[{"name":"` + strings.Repeat("a", 65536) + `"}]`, + } + for _, s := range seeds { + f.Add([]byte(s)) + } + + f.Fuzz(func(t *testing.T, data []byte) { + repos, err := parseRepos(data) + if err != nil { + return + } + + for _, repo := range repos { + // Path sink: never escapes the target when it succeeds. + if p, perr := resolveRepoPath(fixedTarget, repo.Name); perr == nil { + if filepath.Dir(p) != filepath.Clean(fixedTarget) { + t.Fatalf("decoded repo %q resolved outside target: %q", repo.Name, p) + } + if filepath.Base(p) != repo.Name.String() { + t.Fatalf("decoded repo %q resolved to base %q", repo.Name, filepath.Base(p)) + } + } + + // URL sink: never returns an injectable URL for any method. + for _, m := range []CloneMethod{CloneMethodSSH, CloneMethodHTTP} { + cfg := CloneConfig{ + Repository: repo, + TargetDir: fixedTarget, + Token: GitHubToken("ghp_safeToken"), + Organization: OrganizationName("safeorg"), + Method: m, + } + if u, uerr := buildCloneURL(cfg); uerr == nil && !isSafeURL(u) { + t.Fatalf("decoded repo %q produced unsafe URL %q (method %s)", repo.Name, u, m) + } + } + } + }) +} diff --git a/gitgrab.go b/gitgrab.go index 24d8b62..3160f07 100644 --- a/gitgrab.go +++ b/gitgrab.go @@ -1,6 +1,7 @@ package gitgrab import ( + "bytes" "encoding/json" "fmt" "io" @@ -9,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "unicode" ) // CloneMethod represents the method used to clone repositories @@ -60,7 +62,7 @@ func (u HTTPURL) String() string { } func (u HTTPURL) IsValid() bool { - return strings.HasPrefix(string(u), "https://") + return strings.HasPrefix(string(u), "https://") && isSafeURL(string(u)) } func (u SSHURL) String() string { @@ -68,7 +70,25 @@ func (u SSHURL) String() string { } func (u SSHURL) IsValid() bool { - return strings.HasPrefix(string(u), "git@") + return strings.HasPrefix(string(u), "git@") && isSafeURL(string(u)) +} + +// isSafeURL reports whether a URL is safe to hand to `git clone` as a +// positional argument. It rejects the empty string, any leading '-' (which git +// would interpret as an option), and any embedded whitespace or control +// characters that could split or corrupt the command. This does not attempt +// full URL validation — it is a defensive guard against argument injection via +// hostile GitHub API responses. +func isSafeURL(s string) bool { + if s == "" || strings.HasPrefix(s, "-") { + return false + } + for _, r := range s { + if r == unicode.ReplacementChar || unicode.IsSpace(r) || unicode.IsControl(r) { + return false + } + } + return true } // GitHubToken represents a GitHub authentication token @@ -93,9 +113,13 @@ func (o OrganizationName) String() string { return string(o) } +// IsValid reports whether the organization name is safe to interpolate into a +// URL path. GitHub logins are limited to alphanumerics and single hyphens, and +// may not begin or end with a hyphen. Anything else (path separators, spaces, +// URL/shell metacharacters, control characters) is rejected so a hostile API +// response or flag value cannot smuggle in traversal or injection payloads. func (o OrganizationName) IsValid() bool { - s := string(o) - return len(s) > 0 && !strings.ContainsAny(s, " /\\") + return isSafeName(string(o), false) } // RepositoryName represents a repository name @@ -105,9 +129,44 @@ func (r RepositoryName) String() string { return string(r) } +// IsValid reports whether the repository name is safe to use as a single path +// component and to interpolate into a clone URL. GitHub repository names allow +// alphanumerics plus '-', '_', and '.', but the names "." and ".." are +// rejected because they would escape the target directory when joined into a +// path. Path separators, spaces, and other metacharacters are also rejected. func (r RepositoryName) IsValid() bool { s := string(r) - return len(s) > 0 && !strings.ContainsAny(s, " /\\") + if s == "." || s == ".." { + return false + } + return isSafeName(s, true) +} + +// isSafeName is the shared allowlist check for GitHub-style identifiers used in +// paths and URLs. When allowExtra is true the characters '_' and '.' are also +// permitted (repository names), otherwise only alphanumerics and '-' are +// allowed (organization logins). A leading or trailing '-' is always rejected, +// matching GitHub's own rules and avoiding names that could be parsed as CLI +// flags. The empty string is never valid. +func isSafeName(s string, allowExtra bool) bool { + if s == "" { + return false + } + if strings.HasPrefix(s, "-") || strings.HasSuffix(s, "-") { + return false + } + for _, r := range s { + switch { + case r >= 'a' && r <= 'z': + case r >= 'A' && r <= 'Z': + case r >= '0' && r <= '9': + case r == '-': + case allowExtra && (r == '_' || r == '.'): + default: + return false + } + } + return true } // BranchName represents a git branch name @@ -194,8 +253,13 @@ func (gc *GitHubClient) FetchAllRepos(orgName OrganizationName) ([]Repository, e return nil, fmt.Errorf("API request failed: %s - %s", resp.Status, string(body)) } - var repos []Repository - if err := json.NewDecoder(resp.Body).Decode(&repos); err != nil { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response: %v", err) + } + + repos, err := parseRepos(body) + if err != nil { return nil, fmt.Errorf("failed to decode response: %v", err) } @@ -210,6 +274,91 @@ func (gc *GitHubClient) FetchAllRepos(orgName OrganizationName) ([]Repository, e return allRepos, nil } +// parseRepos decodes a single page of the GitHub "list org repos" response. +// It is separated from FetchAllRepos so the JSON decoding path can be +// fuzzed against hostile or malformed API responses without any network I/O. +// Decoding is strict: unexpected trailing data after the JSON array is +// rejected rather than silently ignored. +func parseRepos(data []byte) ([]Repository, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + var repos []Repository + if err := dec.Decode(&repos); err != nil { + return nil, err + } + if dec.More() { + return nil, fmt.Errorf("unexpected trailing data after JSON array") + } + return repos, nil +} + +// resolveRepoPath joins a repository name onto the target directory, returning +// an error if the repository name is not a safe single path component or if the +// result would escape targetDir (path traversal). It is the single choke point +// for turning an untrusted repository name into a filesystem path, and is pure +// so it can be fuzzed directly. +func resolveRepoPath(targetDir string, name RepositoryName) (string, error) { + if !name.IsValid() { + return "", fmt.Errorf("invalid repository name: %q", name.String()) + } + + repoPath := filepath.Join(targetDir, name.String()) + + // Defense in depth: even though IsValid already forbids separators and + // "..", confirm the joined path stays directly within targetDir. + cleanTarget := filepath.Clean(targetDir) + if parent := filepath.Dir(repoPath); parent != cleanTarget { + return "", fmt.Errorf("repository path escapes target directory: %q", repoPath) + } + + return repoPath, nil +} + +// buildCloneURL determines the git clone URL for a repository based on the +// chosen clone method, validating every component that originates from +// untrusted input (the GitHub API response and CLI flags). It never embeds a +// value that would be interpreted by git as an option or that contains +// command-splitting characters. It is pure so it can be fuzzed directly. +func buildCloneURL(config CloneConfig) (string, error) { + repo := config.Repository + + if config.Method == CloneMethodSSH { + if !repo.SSHURL.IsValid() { + return "", fmt.Errorf("invalid ssh url for %q", repo.Name.String()) + } + return repo.SSHURL.String(), nil + } + + // HTTP method. + if !repo.Private { + if !repo.CloneURL.IsValid() { + return "", fmt.Errorf("invalid clone url for %q", repo.Name.String()) + } + return repo.CloneURL.String(), nil + } + + // Private repo over HTTP: build a token-authenticated URL from validated + // components so a hostile name/org cannot inject into the URL or command. + if !config.Organization.IsValid() { + return "", fmt.Errorf("invalid organization name: %q", config.Organization.String()) + } + if !repo.Name.IsValid() { + return "", fmt.Errorf("invalid repository name: %q", repo.Name.String()) + } + if config.Token.IsEmpty() { + return "", fmt.Errorf("token required for private repository over http") + } + if !isSafeURL(config.Token.String()) { + return "", fmt.Errorf("token contains invalid characters") + } + + url := fmt.Sprintf("https://%s@github.com/%s/%s.git", + config.Token, config.Organization, repo.Name) + if !isSafeURL(url) { + return "", fmt.Errorf("constructed clone url is invalid") + } + return url, nil +} + func getCurrentBranch(repoPath string) (string, error) { cmd := exec.Command("git", "-C", repoPath, "branch", "--show-current") output, err := cmd.Output() @@ -221,8 +370,13 @@ func getCurrentBranch(repoPath string) (string, error) { } func CloneRepo(config CloneConfig) error { - repoPath := filepath.Join(config.TargetDir, config.Repository.Name.String()) - + // Validate the repository name and resolve it to a contained path before + // it is ever used as a filesystem path or passed to git. + repoPath, err := resolveRepoPath(config.TargetDir, config.Repository.Name) + if err != nil { + return fmt.Errorf("failed to clone %s: %v", config.Repository.Name, err) + } + // Check if directory already exists if _, err := os.Stat(repoPath); err == nil { fmt.Printf(" Directory %s already exists, updating...\n", config.Repository.Name) @@ -285,20 +439,15 @@ func CloneRepo(config CloneConfig) error { return nil } - // Prepare clone URL based on clone method - var cloneURL string - if config.Method == CloneMethodSSH { - cloneURL = config.Repository.SSHURL.String() - } else { - if config.Repository.Private { - cloneURL = fmt.Sprintf("https://%s@github.com/%s/%s.git", config.Token, config.Organization, config.Repository.Name) - } else { - cloneURL = config.Repository.CloneURL.String() - } + // Prepare and validate the clone URL based on clone method. + cloneURL, err := buildCloneURL(config) + if err != nil { + return fmt.Errorf("failed to clone %s: %v", config.Repository.Name, err) } - // Execute git clone - cmd := exec.Command("git", "clone", cloneURL, repoPath) + // Execute git clone. The "--" separator prevents a URL or path that begins + // with "-" from being interpreted by git as an option (argument injection). + cmd := exec.Command("git", "clone", "--", cloneURL, repoPath) cmd.Stdout = nil // Suppress output cmd.Stderr = nil // Suppress error output