diff --git a/git/client.go b/git/client.go index 560eccfdd..b2cfbce45 100644 --- a/git/client.go +++ b/git/client.go @@ -16,6 +16,7 @@ import ( "strings" "sync" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/safeexec" ) @@ -94,16 +95,65 @@ func (c *Client) Command(ctx context.Context, args ...string) (*Command, error) return &Command{cmd}, nil } +// CredentialPattern is used to inform AuthenticatedCommand which patterns Git should match +// against when trying to find credentials. It is a little over-engineered as a type because we +// want AuthenticatedCommand to have a clear compilation error when this is not provided, +// as opposed to using a string which might compile with `client.AuthenticatedCommand(ctx, "fetch")`. +// +// It is only usable when constructed by another function in the package because the empty pattern, +// without allMatching set to true, will result in an error in AuthenticatedCommand. +// +// Callers can currently opt-in to an slightly less secure mode for backwards compatibility by using +// AllMatchingCredentialsPattern. +type CredentialPattern struct { + allMatching bool // should only be constructable via AllMatchingCredentialsPattern + pattern string +} + +// AllMatchingCredentialsPattern allows for setting gh as credential helper for all hosts. +// However, we should endeavour to remove it as it's less secure. +var AllMatchingCredentialsPattern = CredentialPattern{allMatching: true, pattern: ""} +var disallowedCredentialPattern = CredentialPattern{allMatching: false, pattern: ""} + +// WM-TODO: Are there any funny remotes that might not resolve to a URL? +func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) { + gitURL, err := c.GetRemoteURL(ctx, remote) + if err != nil { + return CredentialPattern{}, err + } + return CredentialPatternFromGitURL(gitURL) +} + +func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) { + normalizedURL, err := ParseURL(gitURL) + if err != nil { + return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err) + } + return CredentialPattern{ + pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"), + }, nil +} + // AuthenticatedCommand is a wrapper around Command that included configuration to use gh // as the credential helper for git. -func (c *Client) AuthenticatedCommand(ctx context.Context, args ...string) (*Command, error) { - preArgs := []string{"-c", "credential.helper="} +func (c *Client) AuthenticatedCommand(ctx context.Context, credentialPattern CredentialPattern, args ...string) (*Command, error) { if c.GhPath == "" { // Assumes that gh is in PATH. c.GhPath = "gh" } credHelper := fmt.Sprintf("!%q auth git-credential", c.GhPath) - preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + + var preArgs []string + if credentialPattern == disallowedCredentialPattern { + return nil, fmt.Errorf("empty credential pattern is not allowed unless provided explicitly") + } else if credentialPattern == AllMatchingCredentialsPattern { + preArgs = []string{"-c", "credential.helper="} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.helper=%s", credHelper)) + } else { + preArgs = []string{"-c", fmt.Sprintf("credential.%s.helper=", credentialPattern.pattern)} + preArgs = append(preArgs, "-c", fmt.Sprintf("credential.%s.helper=%s", credentialPattern.pattern, credHelper)) + } + args = append(preArgs, args...) return c.Command(ctx, args...) } @@ -152,6 +202,19 @@ func (c *Client) UpdateRemoteURL(ctx context.Context, name, url string) error { return nil } +func (c *Client) GetRemoteURL(ctx context.Context, name string) (string, error) { + args := []string{"remote", "get-url", name} + cmd, err := c.Command(ctx, args...) + if err != nil { + return "", err + } + out, err := cmd.Output() + if err != nil { + return "", err + } + return strings.TrimSpace(string(out)), nil +} + func (c *Client) SetRemoteResolution(ctx context.Context, name, resolution string) error { args := []string{"config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution} cmd, err := c.Command(ctx, args...) @@ -549,7 +612,7 @@ func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods if refspec != "" { args = append(args, refspec) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -564,7 +627,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman if remote != "" && branch != "" { args = append(args, remote, branch) } - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -576,7 +639,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...CommandModifier) error { args := []string{"push", "--set-upstream", remote, ref} - cmd, err := c.AuthenticatedCommand(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, AllMatchingCredentialsPattern, args...) if err != nil { return err } @@ -587,6 +650,13 @@ func (c *Client) Push(ctx context.Context, remote string, ref string, mods ...Co } func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods ...CommandModifier) (string, error) { + // Note that even if this is an SSH clone URL, we are setting the pattern anyway. + // We could write some code to prevent this, but it also doesn't seem harmful. + pattern, err := CredentialPatternFromGitURL(cloneURL) + if err != nil { + return "", err + } + cloneArgs, target := parseCloneArgs(args) cloneArgs = append(cloneArgs, cloneURL) // If the args contain an explicit target, pass it to clone otherwise, @@ -601,7 +671,7 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods } } cloneArgs = append([]string{"clone"}, cloneArgs...) - cmd, err := c.AuthenticatedCommand(ctx, cloneArgs...) + cmd, err := c.AuthenticatedCommand(ctx, pattern, cloneArgs...) if err != nil { return "", err } diff --git a/git/client_test.go b/git/client_test.go index 1e3032317..0014ddc3f 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -64,17 +64,32 @@ func TestClientAuthenticatedCommand(t *testing.T) { tests := []struct { name string path string + pattern CredentialPattern wantArgs []string + wantErr error }{ { - name: "adds credential helper config options", + name: "when credential pattern allows for anything, credential helper matches everything", path: "path/to/gh", + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, + { + name: "when credential pattern is set, credential helper only matches that pattern", + path: "path/to/gh", + pattern: CredentialPattern{pattern: "https://github.com"}, + wantArgs: []string{"path/to/git", "-c", "credential.https://github.com.helper=", "-c", `credential.https://github.com.helper=!"path/to/gh" auth git-credential`, "fetch"}, + }, { name: "fallback when GhPath is not set", + pattern: AllMatchingCredentialsPattern, wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, }, + { + name: "errors when attempting to use an empty pattern that isn't marked all matching", + pattern: CredentialPattern{allMatching: false, pattern: ""}, + wantErr: fmt.Errorf("empty credential pattern is not allowed unless provided explicitly"), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -82,9 +97,12 @@ func TestClientAuthenticatedCommand(t *testing.T) { GhPath: tt.path, GitPath: "path/to/git", } - cmd, err := client.AuthenticatedCommand(context.Background(), "fetch") - assert.NoError(t, err) - assert.Equal(t, tt.wantArgs, cmd.Args) + cmd, err := client.AuthenticatedCommand(context.Background(), tt.pattern, "fetch") + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.Equal(t, tt.wantArgs, cmd.Args) }) } } @@ -1131,44 +1149,52 @@ func TestClientSetRemoteBranches(t *testing.T) { func TestClientFetch(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "fetch", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "fetch", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on fetch", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`: { + ExitStatus: 1, + Stderr: "fetch error message", + }, + }, + wantErrorMsg: "failed to run git: fetch error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Fetch(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1176,44 +1202,52 @@ func TestClientFetch(t *testing.T) { func TestClientPull(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "pull", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "pull", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on pull", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`: { + ExitStatus: 1, + Stderr: "pull error message", + }, + }, + wantErrorMsg: "failed to run git: pull error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Pull(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1221,44 +1255,52 @@ func TestClientPull(t *testing.T) { func TestClientPush(t *testing.T) { tests := []struct { - name string - mods []CommandModifier - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string - wantErrorMsg string + name string + mods []CommandModifier + commands mockedCommands + wantErrorMsg string }{ { - name: "push", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "push", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "accepts command modifiers", - mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, + name: "accepts command modifiers", + mods: []CommandModifier{WithRepoDir("/path/to/repo")}, + commands: map[args]commandResult{ + `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 0, + }, + }, }, { - name: "git error", - cmdExitStatus: 1, - cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, - wantErrorMsg: "failed to run git: git error message", + name: "git error on push", + commands: map[args]commandResult{ + `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`: { + ExitStatus: 1, + Stderr: "push error message", + }, + }, + wantErrorMsg: "failed to run git: push error message", }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + cmdCtx := createMockedCommandContext(t, tt.commands) client := Client{ GitPath: "path/to/git", commandContext: cmdCtx, } err := client.Push(context.Background(), "origin", "trunk", tt.mods...) - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { - assert.NoError(t, err) + require.NoError(t, err) } else { - assert.EqualError(t, err, tt.wantErrorMsg) + require.EqualError(t, err, tt.wantErrorMsg) } }) } @@ -1279,14 +1321,14 @@ func TestClientClone(t *testing.T) { { name: "clone", args: []string{}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { name: "accepts command modifiers", args: []string{}, mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantTarget: "cli", }, { @@ -1294,19 +1336,19 @@ func TestClientClone(t *testing.T) { args: []string{}, cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone https://github.com/cli/cli`, wantErrorMsg: "failed to run git: git error message", }, { name: "bare clone", args: []string{"--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli`, wantTarget: "cli.git", }, { name: "bare clone with explicit target", args: []string{"cli-bare", "--bare"}, - wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone --bare github.com/cli/cli cli-bare`, + wantCmdArgs: `path/to/git -c credential.https://github.com.helper= -c credential.https://github.com.helper=!"gh" auth git-credential clone --bare https://github.com/cli/cli cli-bare`, wantTarget: "cli-bare", }, } @@ -1317,7 +1359,7 @@ func TestClientClone(t *testing.T) { GitPath: "path/to/git", commandContext: cmdCtx, } - target, err := client.Clone(context.Background(), "github.com/cli/cli", tt.args, tt.mods...) + target, err := client.Clone(context.Background(), "https://github.com/cli/cli", tt.args, tt.mods...) assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) if tt.wantErrorMsg == "" { assert.NoError(t, err) @@ -1442,6 +1484,54 @@ func initRepo(t *testing.T, dir string) { assert.NoError(t, err) } +type args string + +type commandResult struct { + ExitStatus int `json:"exitStatus"` + Stdout string `json:"out"` + Stderr string `json:"err"` +} + +type mockedCommands map[args]commandResult + +// TestCommandMocking is an invoked test helper that emulates expected behavior for predefined shell commands, erroring when unexpected conditions are encountered. +func TestCommandMocking(t *testing.T) { + if os.Getenv("GH_WANT_HELPER_PROCESS_RICH") != "1" { + return + } + + jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") + if !ok { + fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(1) + } + + var commands mockedCommands + if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { + fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") + os.Exit(1) + } + + // The discarded args are those for the go test binary itself, e.g. `-test.run=TestHelperProcessRich` + realArgs := os.Args[3:] + + commandResult, ok := commands[args(strings.Join(realArgs, " "))] + if !ok { + fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) + os.Exit(1) + } + + if commandResult.Stdout != "" { + fmt.Fprint(os.Stdout, commandResult.Stdout) + } + + if commandResult.Stderr != "" { + fmt.Fprint(os.Stderr, commandResult.Stderr) + } + + os.Exit(commandResult.ExitStatus) +} + func TestHelperProcess(t *testing.T) { if os.Getenv("GH_WANT_HELPER_PROCESS") != "1" { return @@ -1465,6 +1555,87 @@ func TestHelperProcess(t *testing.T) { os.Exit(0) } +func TestCredentialPatternFromGitURL(t *testing.T) { + tests := []struct { + name string + gitURL string + wantErr bool + wantCredentialPattern CredentialPattern + }{ + { + name: "Given a well formed gitURL, it returns the corresponding CredentialPattern", + gitURL: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + allMatching: false, + }, + }, + { + name: "Given a malformed gitURL, it returns an error", + // This pattern is copied from the tests in ParseURL + // Unexpectedly, a non URL-like string did not error in ParseURL + gitURL: "ssh://git@[/tmp/git-repo", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + credentialPattern, err := CredentialPatternFromGitURL(tt.gitURL) + if tt.wantErr { + assert.ErrorContains(t, err, "failed to parse remote URL") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + +func TestCredentialPatternFromRemote(t *testing.T) { + tests := []struct { + name string + remote string + wantCredentialPattern CredentialPattern + wantErr bool + }{ + { + name: "Given a well formed remote, it returns the corresponding CredentialPattern", + remote: "https://github.com/OWNER/REPO", + wantCredentialPattern: CredentialPattern{ + pattern: "https://github.com", + allMatching: false, + }, + }, + { + name: "Given an error from GetRemoteURL, it returns that error", + remote: "foo remote", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var cmdCtx func(ctx context.Context, name string, args ...string) *exec.Cmd + if tt.wantErr { + _, cmdCtx = createCommandContext(t, 1, tt.remote, "GetRemoteURL error") + } else { + _, cmdCtx = createCommandContext(t, 0, tt.remote, "") + } + + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + credentialPattern, err := CredentialPatternFromRemote(context.Background(), &client, tt.remote) + if tt.wantErr { + assert.ErrorContains(t, err, "GetRemoteURL error") + } else { + assert.NoError(t, err) + assert.Equal(t, tt.wantCredentialPattern, credentialPattern) + } + }) + } +} + func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) (*exec.Cmd, commandCtx) { cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestHelperProcess", "--") cmd.Env = []string{ @@ -1479,3 +1650,21 @@ func createCommandContext(t *testing.T, exitStatus int, stdout, stderr string) ( return cmd } } + +func createMockedCommandContext(t *testing.T, commands mockedCommands) commandCtx { + marshaledCommands, err := json.Marshal(commands) + require.NoError(t, err) + + // invokes helper within current test binary, emulating desired behavior + return func(ctx context.Context, exe string, args ...string) *exec.Cmd { + cmd := exec.CommandContext(context.Background(), os.Args[0], "-test.run=TestCommandMocking", "--") + cmd.Env = []string{ + "GH_WANT_HELPER_PROCESS_RICH=1", + fmt.Sprintf("GH_HELPER_PROCESS_RICH_COMMANDS=%s", string(marshaledCommands)), + } + + cmd.Args = append(cmd.Args, exe) + cmd.Args = append(cmd.Args, args...) + return cmd + } +} diff --git a/go.mod b/go.mod index 40193c4f8..a95ccacc0 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/cenkalti/backoff/v4 v4.3.0 github.com/charmbracelet/glamour v0.7.0 github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c - github.com/cli/go-gh/v2 v2.11.0 + github.com/cli/go-gh/v2 v2.11.1 github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 diff --git a/go.sum b/go.sum index d068d9d11..a7d87de7d 100644 --- a/go.sum +++ b/go.sum @@ -95,8 +95,8 @@ github.com/charmbracelet/x/exp/term v0.0.0-20240425164147-ba2a9512b05f/go.mod h1 github.com/cli/browser v1.0.0/go.mod h1:IEWkHYbLjkhtjwwWlwTHW2lGxeS5gezEQBMLTwDHf5Q= github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= -github.com/cli/go-gh/v2 v2.11.0 h1:TERLYMMWderKBO3lBff/JIu2+eSly2oFRgN2WvO+3eA= -github.com/cli/go-gh/v2 v2.11.0/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= +github.com/cli/go-gh/v2 v2.11.1 h1:amAyfqMWQTBdue8iTmDUegGZK7c8kk6WCxD9l/wLtGI= +github.com/cli/go-gh/v2 v2.11.1/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs= diff --git a/internal/run/stub.go b/internal/run/stub.go index 49bf62d29..5cd3c6de5 100644 --- a/internal/run/stub.go +++ b/internal/run/stub.go @@ -9,7 +9,7 @@ import ( ) const ( - gitAuthRE = `-c credential.helper= -c credential.helper=!"[^"]+" auth git-credential ` + gitAuthRE = `-c credential(?:\..+)?\.helper= -c credential(?:\..+)?\.helper=!"[^"]+" auth git-credential ` ) type T interface { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index c35bcb845..abdebf0c8 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -249,7 +249,7 @@ func TestDevelopRun(t *testing.T) { expectedOut: heredoc.Doc(` Showing linked branches for OWNER/REPO#42 - + BRANCH URL foo https://github.com/OWNER/REPO/tree/foo bar https://github.com/OWNER/OTHER-REPO/tree/bar diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index f566cbe1d..c4549d89b 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -130,7 +130,14 @@ func checkoutRun(opts *CheckoutOptions) error { cmdQueue = append(cmdQueue, []string{"submodule", "update", "--init", "--recursive"}) } - err = executeCmds(opts.GitClient, cmdQueue) + // Note that although we will probably be fetching from the headRemote, in practice, PR checkout can only + // ever point to one host, and we know baseRemote must be populated, where headRemote might be nil (e.g. when + // it was deleted). + credentialPattern, err := git.CredentialPatternFromRemote(context.Background(), opts.GitClient, baseRemote.Name) + if err != nil { + return err + } + err = executeCmds(opts.GitClient, credentialPattern, cmdQueue) if err != nil { return err } @@ -240,13 +247,16 @@ func localBranchExists(client *git.Client, b string) bool { return err == nil } -func executeCmds(client *git.Client, cmdQueue [][]string) error { +func executeCmds(client *git.Client, credentialPattern git.CredentialPattern, cmdQueue [][]string) error { for _, args := range cmdQueue { var err error var cmd *git.Command - if args[0] == "fetch" || args[0] == "submodule" { - cmd, err = client.AuthenticatedCommand(context.Background(), args...) - } else { + switch args[0] { + case "submodule": + cmd, err = client.AuthenticatedCommand(context.Background(), credentialPattern, args...) + case "fetch": + cmd, err = client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) + default: cmd, err = client.Command(context.Background(), args...) } if err != nil { diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 1eda5dda2..32f7b5b80 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -72,6 +72,32 @@ func Test_checkoutRun(t *testing.T) { wantStderr string wantErr bool }{ + { + name: "checkout with ssh remote URL", + opts: &CheckoutOptions{ + SelectorArg: "123", + Finder: func() shared.PRFinder { + baseRepo, pr := stubPR("OWNER/REPO:master", "OWNER/REPO:feature") + finder := shared.NewMockFinder("123", pr, baseRepo) + return finder + }(), + Config: func() (gh.Config, error) { + return config.NewBlankConfig(), nil + }, + Branch: func() (string, error) { + return "main", nil + }, + }, + remotes: map[string]string{ + "origin": "OWNER/REPO", + }, + runStubs: func(cs *run.CommandStubber) { + cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") + cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") + cs.Register(`git checkout -b feature --track origin/feature`, 0, "") + }, + }, { name: "fork repo was deleted", opts: &CheckoutOptions{ @@ -94,6 +120,7 @@ func Test_checkoutRun(t *testing.T) { "origin": "OWNER/REPO", }, runStubs: func(cs *run.CommandStubber) { + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -124,6 +151,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git show-ref --verify -- refs/heads/foobar`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git checkout -b foobar --track origin/feature`, 0, "") }, @@ -151,6 +179,7 @@ func Test_checkoutRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git config branch\.foobar\.merge`, 1, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:foobar`, 0, "") cs.Register(`git checkout foobar`, 0, "") cs.Register(`git config branch\.foobar\.remote https://github.com/hubot/REPO.git`, 0, "") @@ -276,6 +305,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track origin/feature`, 0, "") @@ -296,6 +326,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -329,6 +360,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "") cs.Register(`git checkout -b feature --track robot-fork/feature`, 0, "") @@ -350,6 +382,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -373,6 +406,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -393,6 +427,7 @@ func TestPRCheckout_detachedHead(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git checkout feature`, 0, "") @@ -413,6 +448,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n") cs.Register(`git merge --ff-only FETCH_HEAD`, 0, "") @@ -450,6 +486,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "") cs.Register(`git config branch\.feature\.merge`, 1, "") cs.Register(`git checkout feature`, 0, "") @@ -472,6 +509,7 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -494,6 +532,7 @@ func TestPRCheckout_force(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git") cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "") cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "") cs.Register(`git checkout feature`, 0, "") @@ -517,6 +556,7 @@ func TestPRCheckout_detach(t *testing.T) { defer cmdTeardown(t) cs.Register(`git checkout --detach FETCH_HEAD`, 0, "") + cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git") cs.Register(`git fetch origin refs/pull/123/head`, 0, "") output, err := runCommand(http, nil, "", `123 --detach`) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 79c349aa4..ec7026759 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -676,7 +676,7 @@ func createFromLocal(opts *CreateOptions) error { } if opts.Push && repoType == bare { - cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), "push", baseRemote, "--mirror") + cmd, err := opts.GitClient.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, "push", baseRemote, "--mirror") if err != nil { return err } diff --git a/pkg/cmd/repo/sync/git.go b/pkg/cmd/repo/sync/git.go index 1f18d0fdb..0a0be1ed6 100644 --- a/pkg/cmd/repo/sync/git.go +++ b/pkg/cmd/repo/sync/git.go @@ -55,7 +55,7 @@ func (g *gitExecuter) CurrentBranch() (string, error) { func (g *gitExecuter) Fetch(remote, ref string) error { args := []string{"fetch", "-q", remote, ref} - cmd, err := g.client.AuthenticatedCommand(context.Background(), args...) + cmd, err := g.client.AuthenticatedCommand(context.Background(), git.AllMatchingCredentialsPattern, args...) if err != nil { return err }