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