From 98ab1f258767cb53279f5fc3d9ec8f8265fc44ef Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 15 Nov 2022 13:14:37 +0200 Subject: [PATCH] Authenticate network git commands (#6541) --- git/client.go | 60 ++++++++++----------------- git/client_test.go | 32 +++++++------- internal/run/stub.go | 17 ++++++++ pkg/cmd/gist/clone/clone_test.go | 10 ++--- pkg/cmd/issue/develop/develop_test.go | 5 ++- pkg/cmd/pr/checkout/checkout.go | 9 +++- pkg/cmd/pr/checkout/checkout_test.go | 10 ++++- pkg/cmd/pr/create/create_test.go | 10 ++++- pkg/cmd/pr/merge/merge_test.go | 5 ++- pkg/cmd/repo/clone/clone_test.go | 10 ++--- pkg/cmd/repo/create/create_test.go | 5 ++- pkg/cmd/repo/fork/fork_test.go | 5 ++- 12 files changed, 104 insertions(+), 74 deletions(-) diff --git a/git/client.go b/git/client.go index 41d0f9341..343d1300d 100644 --- a/git/client.go +++ b/git/client.go @@ -317,16 +317,6 @@ func (c *Client) DeleteLocalBranch(ctx context.Context, branch string) error { return nil } -func (c *Client) HasLocalBranch(ctx context.Context, branch string) bool { - args := []string{"rev-parse", "--verify", "refs/heads/" + branch} - cmd, err := c.Command(ctx, args...) - if err != nil { - return false - } - _, err = cmd.Output() - return err == nil -} - func (c *Client) CheckoutBranch(ctx context.Context, branch string) error { args := []string{"checkout", branch} cmd, err := c.Command(ctx, args...) @@ -354,14 +344,14 @@ func (c *Client) CheckoutNewBranch(ctx context.Context, remoteName, branch strin return nil } +func (c *Client) HasLocalBranch(ctx context.Context, branch string) bool { + _, err := c.revParse(ctx, "--verify", "refs/heads/"+branch) + return err == nil +} + // ToplevelDir returns the top-level directory path of the current repository. func (c *Client) ToplevelDir(ctx context.Context) (string, error) { - args := []string{"rev-parse", "--show-toplevel"} - cmd, err := c.Command(ctx, args...) - if err != nil { - return "", err - } - out, err := cmd.Output() + out, err := c.revParse(ctx, "--show-toplevel") if err != nil { return "", err } @@ -369,12 +359,7 @@ func (c *Client) ToplevelDir(ctx context.Context) (string, error) { } func (c *Client) GitDir(ctx context.Context) (string, error) { - args := []string{"rev-parse", "--git-dir"} - cmd, err := c.Command(ctx, args...) - if err != nil { - return "", err - } - out, err := cmd.Output() + out, err := c.revParse(ctx, "--git-dir") if err != nil { return "", err } @@ -383,12 +368,7 @@ func (c *Client) GitDir(ctx context.Context) (string, error) { // Show current directory relative to the top-level directory of repository. func (c *Client) PathFromRoot(ctx context.Context) string { - args := []string{"rev-parse", "--show-prefix"} - cmd, err := c.Command(ctx, args...) - if err != nil { - return "" - } - out, err := cmd.Output() + out, err := c.revParse(ctx, "--show-prefix") if err != nil { return "" } @@ -398,12 +378,20 @@ func (c *Client) PathFromRoot(ctx context.Context) string { return "" } +func (c *Client) revParse(ctx context.Context, args ...string) ([]byte, error) { + args = append([]string{"rev-parse"}, args...) + cmd, err := c.Command(ctx, args...) + if err != nil { + return nil, err + } + return cmd.Output() +} + // Below are commands that make network calls and need authentication credentials supplied from gh. func (c *Client) Fetch(ctx context.Context, remote string, refspec string, mods ...CommandModifier) error { args := []string{"fetch", remote, refspec} - // TODO: Use AuthenticatedCommand - cmd, err := c.Command(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, args...) if err != nil { return err } @@ -418,8 +406,7 @@ func (c *Client) Pull(ctx context.Context, remote, branch string, mods ...Comman if remote != "" && branch != "" { args = append(args, remote, branch) } - // TODO: Use AuthenticatedCommand - cmd, err := c.Command(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, args...) if err != nil { return err } @@ -431,8 +418,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} - // TODO: Use AuthenticatedCommand - cmd, err := c.Command(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, args...) if err != nil { return err } @@ -453,8 +439,7 @@ func (c *Client) Clone(ctx context.Context, cloneURL string, args []string, mods target = path.Base(strings.TrimSuffix(cloneURL, ".git")) } cloneArgs = append([]string{"clone"}, cloneArgs...) - // TODO: Use AuthenticatedCommand - cmd, err := c.Command(ctx, cloneArgs...) + cmd, err := c.AuthenticatedCommand(ctx, cloneArgs...) if err != nil { return "", err } @@ -474,8 +459,7 @@ func (c *Client) AddRemote(ctx context.Context, name, urlStr string, trackingBra args = append(args, "-t", branch) } args = append(args, "-f", name, urlStr) - // TODO: Use AuthenticatedCommand - cmd, err := c.Command(ctx, args...) + cmd, err := c.AuthenticatedCommand(ctx, args...) if err != nil { return nil, err } diff --git a/git/client_test.go b/git/client_test.go index c8ea57246..e12789c8c 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -67,11 +67,11 @@ func TestClientAuthenticatedCommand(t *testing.T) { { name: "adds credential helper config options", path: "path/to/gh", - wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", "credential.helper=!\"path/to/gh\" auth git-credential", "fetch"}, + wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"path/to/gh" auth git-credential`, "fetch"}, }, { name: "fallback when GhPath is not set", - wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", "credential.helper=!\"gh\" auth git-credential", "fetch"}, + wantArgs: []string{"path/to/git", "-c", "credential.helper=", "-c", `credential.helper=!"gh" auth git-credential`, "fetch"}, }, } for _, tt := range tests { @@ -846,18 +846,18 @@ func TestClientFetch(t *testing.T) { }{ { name: "fetch", - wantCmdArgs: `path/to/git fetch origin trunk`, + wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, }, { name: "accepts command modifiers", mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo fetch origin trunk`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential fetch origin trunk`, }, { name: "git error", cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git fetch origin trunk`, + 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", }, } @@ -891,18 +891,18 @@ func TestClientPull(t *testing.T) { }{ { name: "pull", - wantCmdArgs: `path/to/git pull --ff-only origin trunk`, + wantCmdArgs: `path/to/git -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")}, - wantCmdArgs: `path/to/git -C /path/to/repo pull --ff-only origin trunk`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential pull --ff-only origin trunk`, }, { name: "git error", cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git pull --ff-only origin trunk`, + 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", }, } @@ -936,18 +936,18 @@ func TestClientPush(t *testing.T) { }{ { name: "push", - wantCmdArgs: `path/to/git push --set-upstream origin trunk`, + wantCmdArgs: `path/to/git -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")}, - wantCmdArgs: `path/to/git -C /path/to/repo push --set-upstream origin trunk`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential push --set-upstream origin trunk`, }, { name: "git error", cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git push --set-upstream origin trunk`, + 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", }, } @@ -982,20 +982,20 @@ func TestClientClone(t *testing.T) { }{ { name: "clone", - wantCmdArgs: `path/to/git clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, wantTarget: "cli", }, { name: "accepts command modifiers", mods: []CommandModifier{WithRepoDir("/path/to/repo")}, - wantCmdArgs: `path/to/git -C /path/to/repo clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -C /path/to/repo -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, wantTarget: "cli", }, { name: "git error", cmdExitStatus: 1, cmdStderr: "git error message", - wantCmdArgs: `path/to/git clone github.com/cli/cli`, + wantCmdArgs: `path/to/git -c credential.helper= -c credential.helper=!"gh" auth git-credential clone github.com/cli/cli`, wantErrorMsg: "failed to run git: git error message", }, } @@ -1089,7 +1089,7 @@ func TestClientAddRemote(t *testing.T) { url: "URL", dir: "DIRECTORY", branches: []string{}, - wantCmdArgs: `path/to/git -C DIRECTORY remote add -f test URL`, + wantCmdArgs: `path/to/git -C DIRECTORY -c credential.helper= -c credential.helper=!"gh" auth git-credential remote add -f test URL`, }, { title: "fetch specific branches only", @@ -1097,7 +1097,7 @@ func TestClientAddRemote(t *testing.T) { url: "URL", dir: "DIRECTORY", branches: []string{"trunk", "dev"}, - wantCmdArgs: `path/to/git -C DIRECTORY remote add -t trunk -t dev -f test URL`, + wantCmdArgs: `path/to/git -C DIRECTORY -c credential.helper= -c credential.helper=!"gh" auth git-credential remote add -t trunk -t dev -f test URL`, }, } for _, tt := range tests { diff --git a/internal/run/stub.go b/internal/run/stub.go index bcb359cee..13a506c11 100644 --- a/internal/run/stub.go +++ b/internal/run/stub.go @@ -8,6 +8,10 @@ import ( "strings" ) +const ( + gitAuthRE = `-c credential.helper= -c credential.helper=!"[^"]+" auth git-credential ` +) + type T interface { Helper() Errorf(string, ...interface{}) @@ -71,6 +75,9 @@ func (cs *CommandStubber) Register(pattern string, exitStatus int, output string if len(pattern) < 1 { panic("cannot use empty regexp pattern") } + if strings.HasPrefix(pattern, "git") { + pattern = addGitAuthentication(pattern) + } cs.stubs = append(cs.stubs, &commandStub{ pattern: regexp.MustCompile(pattern), exitStatus: exitStatus, @@ -114,3 +121,13 @@ func (s *commandStub) Output() ([]byte, error) { } return []byte(s.stdout), nil } + +// Inject git authentication string for specific git commands. +func addGitAuthentication(s string) string { + pattern := regexp.MustCompile(`( fetch | pull | push | clone | remote add.+-f | submodule )`) + loc := pattern.FindStringIndex(s) + if loc == nil { + return s + } + return s[:loc[0]+1] + gitAuthRE + s[loc[0]+1:] +} diff --git a/pkg/cmd/gist/clone/clone_test.go b/pkg/cmd/gist/clone/clone_test.go index cd6f71b6e..58ce55b52 100644 --- a/pkg/cmd/gist/clone/clone_test.go +++ b/pkg/cmd/gist/clone/clone_test.go @@ -2,7 +2,6 @@ package clone import ( "net/http" - "strings" "testing" "github.com/cli/cli/v2/git" @@ -26,7 +25,10 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) Config: func() (config.Config, error) { return config.NewBlankConfig(), nil }, - GitClient: &git.Client{GitPath: "some/path/git"}, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, } cmd := NewCmdClone(fac, nil) @@ -97,9 +99,7 @@ func Test_GistClone(t *testing.T) { cs, restore := run.Stub() defer restore(t) - cs.Register(`git clone`, 0, "", func(s []string) { - assert.Equal(t, tt.want, strings.Join(s, " ")) - }) + cs.Register(tt.want, 0, "") output, err := runCloneCommand(httpClient, tt.args) if err != nil { diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index 2bf11e0d7..1cd7526f6 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -569,7 +569,10 @@ func Test_developRun(t *testing.T) { return remotes, nil } - opts.GitClient = &git.Client{GitPath: "some/path/git"} + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } cmdStubs, cmdTeardown := run.Stub() defer cmdTeardown(t) diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index 839104c2f..193eea735 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -242,8 +242,13 @@ func localBranchExists(client *git.Client, b string) bool { func executeCmds(client *git.Client, cmdQueue [][]string) error { for _, args := range cmdQueue { - // TODO: Use AuthenticatedCommand - cmd, err := client.Command(context.Background(), args...) + var err error + var cmd *git.Command + if args[0] == "fetch" || args[0] == "submodule" { + cmd, err = client.AuthenticatedCommand(context.Background(), args...) + } else { + cmd, err = client.Command(context.Background(), args...) + } if err != nil { return err } diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 7540bbacf..cc9aed0f0 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -197,7 +197,10 @@ func Test_checkoutRun(t *testing.T) { return remotes, nil } - opts.GitClient = &git.Client{GitPath: "some/path/git"} + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } err := checkoutRun(opts) if (err != nil) != tt.wantErr { @@ -236,7 +239,10 @@ func runCommand(rt http.RoundTripper, remotes context.Remotes, branch string, cl Branch: func() (string, error) { return branch, nil }, - GitClient: &git.Client{GitPath: "some/path/git"}, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, } cmd := NewCmdCheckout(factory, nil) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 1a709ee35..125c479b3 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -894,7 +894,10 @@ func Test_createRun(t *testing.T) { return branch, nil } opts.Finder = shared.NewMockFinder(branch, nil, nil) - opts.GitClient = &git.Client{GitPath: "some/path/git"} + opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } cleanSetup := func() {} if tt.setup != nil { cleanSetup = tt.setup(&opts, t) @@ -1011,7 +1014,10 @@ func Test_determineTrackingBranch(t *testing.T) { tt.cmdStubs(cs) - gitClient := &git.Client{GitPath: "some/path/git"} + gitClient := &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } ref := determineTrackingBranch(gitClient, tt.remotes, "feature") tt.assert(ref, t) }) diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 0e3ced350..b2a514d82 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -270,7 +270,10 @@ func runCommand(rt http.RoundTripper, branch string, isTTY bool, cli string) (*t }, }, nil }, - GitClient: &git.Client{GitPath: "some/path/git"}, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, } cmd := NewCmdMerge(factory, nil) diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go index c6ec872be..4e7750522 100644 --- a/pkg/cmd/repo/clone/clone_test.go +++ b/pkg/cmd/repo/clone/clone_test.go @@ -2,7 +2,6 @@ package clone import ( "net/http" - "strings" "testing" "github.com/cli/cli/v2/git" @@ -105,7 +104,10 @@ func runCloneCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) Config: func() (config.Config, error) { return config.NewBlankConfig(), nil }, - GitClient: &git.Client{GitPath: "some/path/git"}, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, } cmd := NewCmdClone(fac, nil) @@ -202,9 +204,7 @@ func Test_RepoClone(t *testing.T) { cs, restore := run.Stub() defer restore(t) - cs.Register(`git clone`, 0, "", func(s []string) { - assert.Equal(t, tt.want, strings.Join(s, " ")) - }) + cs.Register(tt.want, 0, "") output, err := runCloneCommand(httpClient, tt.args) if err != nil { diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index 702d7eba5..636f447c1 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -493,7 +493,10 @@ func Test_createRun(t *testing.T) { return config.NewBlankConfig(), nil } - tt.opts.GitClient = &git.Client{GitPath: "some/path/git"} + tt.opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } ios, _, stdout, stderr := iostreams.Test() ios.SetStdinTTY(tt.tty) diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index e5ffda36b..e913583e8 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -700,7 +700,10 @@ func TestRepoFork(t *testing.T) { return tt.remotes, nil } - tt.opts.GitClient = &git.Client{GitPath: "some/path/git"} + tt.opts.GitClient = &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + } //nolint:staticcheck // SA1019: prompt.InitAskStubber is deprecated: use NewAskStubber as, teardown := prompt.InitAskStubber()