Harden git clone argv and config file permissions

internal/cli/repo.go

@@ -26,6 +26,13 @@ var repoCmd = &cobra.Command{
 	Short: "Manage repositories",
 }
 
+// gitCloneArgs builds the argv for git clone. The -- separator stops git
+// from parsing a server-supplied CloneURL as an option (a malicious forge
+// could otherwise return something like --upload-pack=...).
+func gitCloneArgs(url string) []string {
+	return []string{"clone", "--", url}
+}
+
 func init() {
 	rootCmd.AddCommand(repoCmd)
 	repoCmd.AddCommand(repoViewCmd())
@@ -267,7 +274,7 @@ func repoCreateCmd() *cobra.Command {
 			_, _ = fmt.Fprintf(os.Stdout, "%s\n", repo.HTMLURL)
 
 			if flagClone && repo.CloneURL != "" {
-				cloneCmd := exec.CommandContext(cmd.Context(), "git", "clone", repo.CloneURL)
+				cloneCmd := exec.CommandContext(cmd.Context(), "git", gitCloneArgs(repo.CloneURL)...)
 				cloneCmd.Stdout = os.Stdout
 				cloneCmd.Stderr = os.Stderr
 				return cloneCmd.Run()
@@ -435,7 +442,7 @@ func repoForkCmd() *cobra.Command {
 			_, _ = fmt.Fprintf(os.Stdout, "%s\n", r.HTMLURL)
 
 			if flagClone && r.CloneURL != "" {
-				cloneCmd := exec.CommandContext(cmd.Context(), "git", "clone", r.CloneURL)
+				cloneCmd := exec.CommandContext(cmd.Context(), "git", gitCloneArgs(r.CloneURL)...)
 				cloneCmd.Stdout = os.Stdout
 				cloneCmd.Stderr = os.Stderr
 				return cloneCmd.Run()
@@ -472,7 +479,7 @@ func repoCloneCmd() *cobra.Command {
 				cloneURL = r.HTMLURL + ".git"
 			}
 
-			cloneCmd := exec.CommandContext(cmd.Context(), "git", "clone", cloneURL)
+			cloneCmd := exec.CommandContext(cmd.Context(), "git", gitCloneArgs(cloneURL)...)
 			cloneCmd.Stdout = os.Stdout
 			cloneCmd.Stderr = os.Stderr
 			return cloneCmd.Run()

internal/cli/repo_test.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"bytes"
+	"slices"
 	"strings"
 	"testing"
 )
@@ -105,3 +106,47 @@ func TestDomainFromFlags(t *testing.T) {
 	}
 	flagForgeType = "" // reset
 }
+
+func TestGitCloneArgs(t *testing.T) {
+	tests := []struct {
+		name string
+		url  string
+		want []string
+	}{
+		{
+			name: "https url",
+			url:  "https://github.com/owner/repo.git",
+			want: []string{"clone", "--", "https://github.com/owner/repo.git"},
+		},
+		{
+			name: "ssh url",
+			url:  "git@github.com:owner/repo.git",
+			want: []string{"clone", "--", "git@github.com:owner/repo.git"},
+		},
+		{
+			name: "url that looks like a git option",
+			url:  "--upload-pack=evil",
+			want: []string{"clone", "--", "--upload-pack=evil"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := gitCloneArgs(tt.url)
+			if !slices.Equal(got, tt.want) {
+				t.Errorf("gitCloneArgs(%q) = %v, want %v", tt.url, got, tt.want)
+			}
+
+			// The url must appear after the -- separator so git cannot
+			// parse a server-supplied CloneURL as an option.
+			sep := slices.Index(got, "--")
+			urlIdx := slices.Index(got, tt.url)
+			if sep == -1 {
+				t.Fatal("expected -- separator in argv")
+			}
+			if urlIdx <= sep {
+				t.Errorf("url at index %d is not after -- at index %d", urlIdx, sep)
+			}
+		})
+	}
+}

internal/config/config.go

@@ -6,13 +6,15 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"sync"
 )
 
 const (
 	dirPermissions  = 0700
 	filePermissions = 0600
+	goosWindows     = "windows"
 )
 
 type Config struct {
@@ -245,7 +247,15 @@ func writeINI(path string, sections map[string]map[string]string) error {
 		writeSection(&b, name, kv)
 	}
 
-	return os.WriteFile(path, []byte(b.String()), filePermissions)
+	if err := os.WriteFile(path, []byte(b.String()), filePermissions); err != nil {
+		return err
+	}
+	// os.WriteFile only applies the mode on creation. Tighten existing files
+	// too, since they hold tokens.
+	if runtime.GOOS != goosWindows {
+		_ = os.Chmod(path, filePermissions)
+	}
+	return nil
 }
 
 func writeSection(b *strings.Builder, name string, kv map[string]string) {

internal/config/config_test.go

@@ -268,7 +268,7 @@ func TestSetDomain(t *testing.T) {
 	}
 
 	// Verify file permissions (skip on Windows, which doesn't support Unix perms)
-	if runtime.GOOS != "windows" {
+	if runtime.GOOS != goosWindows {
 		info, err := os.Stat(path)
 		if err != nil {
 			t.Fatalf("stat: %v", err)
@@ -279,6 +279,40 @@ func TestSetDomain(t *testing.T) {
 	}
 }
 
+func TestSetDomainTightensExistingPermissions(t *testing.T) {
+	if runtime.GOOS == goosWindows {
+		t.Skip("unix permissions not enforced on windows")
+	}
+
+	dir := t.TempDir()
+	t.Setenv("XDG_CONFIG_HOME", dir)
+
+	// Pre-create the config file with overly permissive mode. os.WriteFile
+	// only applies the mode bits on creation, so a file restored from a
+	// backup or created by hand can be left readable by other users even
+	// after we write a token into it.
+	cfgDir := filepath.Join(dir, "forge")
+	if err := os.MkdirAll(cfgDir, 0700); err != nil {
+		t.Fatal(err)
+	}
+	path := filepath.Join(cfgDir, "config")
+	if err := os.WriteFile(path, []byte("[github.com]\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := SetDomain("github.com", "ghp_secret", ""); err != nil {
+		t.Fatalf("SetDomain: %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat: %v", err)
+	}
+	if got := info.Mode().Perm(); got != 0600 {
+		t.Errorf("expected SetDomain to tighten existing file to 0600, got %o", got)
+	}
+}
+
 func TestSetDomainPreservesExistingConfig(t *testing.T) {
 	dir := t.TempDir()
 	t.Setenv("XDG_CONFIG_HOME", dir)