Omit access tokens from Git errors (#28)

Author: a-roberts

.gitignore

@@ -1 +1 @@
-./services
+services

pkg/avancement/service_manager.go

@@ -2,6 +2,7 @@ package avancement
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log"
 	"net/url"
@@ -97,7 +98,7 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName, mes
 	} else {
 		source, errorSource = s.checkoutSourceRepo(fromURL, fromBranch)
 		if errorSource != nil {
-			return fmt.Errorf("failed to checkout repo: %w", errorSource)
+			return git.GitError("error checking out source repository from Git", fromURL)
 		}
 		reposToDelete = append(reposToDelete, source)
 		if newBranchName == "" {
@@ -107,15 +108,19 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName, mes
 
 	destination, err := s.checkoutDestinationRepo(toURL, newBranchName)
 	if err != nil {
-		return fmt.Errorf("failed to checkout repo: %w", err)
+		if git.IsGitError(err) {
+			return git.GitError(fmt.Sprintf("failed to clone destination repository, error: %s", err.Error()), toURL)
+		}
+		// This would be a checkout error as the clone error gives us the above gitError instead
+		return fmt.Errorf("failed to checkout destination repository, error: %w", err)
 	}
 	reposToDelete = append(reposToDelete, destination)
 
 	var copied []string
 	if isLocal {
 		copied, err = local.CopyConfig(serviceName, localSource, destination)
 		if err != nil {
-			return fmt.Errorf("failed to setup local repo: %w", err)
+			return fmt.Errorf("failed to set up local repository: %w", err)
 		}
 	} else {
 		copied, err = git.CopyService(serviceName, source, destination)
@@ -141,13 +146,14 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName, mes
 	}
 
 	if err := destination.Push(newBranchName); err != nil {
-		return fmt.Errorf("failed to push: %w", err)
+		return fmt.Errorf("failed to push to Git repository - check the access token is correct with sufficient permissions: %w", err)
 	}
 
 	ctx := context.Background()
 	pr, err := createPullRequest(ctx, fromURL, toURL, newBranchName, commitMsg, s.clientFactory(s.author.Token), isLocal)
 	if err != nil {
-		return fmt.Errorf("failed to create a pull-request for branch %s in %v: %w", newBranchName, toURL, err)
+		message := fmt.Sprintf("failed to create a pull-request for branch %s, error: %s", newBranchName, err)
+		return git.GitError(message, toURL)
 	}
 	log.Printf("created PR %d", pr.Number)
 	return nil
@@ -156,23 +162,26 @@ func (s *ServiceManager) Promote(serviceName, fromURL, toURL, newBranchName, mes
 func (s *ServiceManager) checkoutSourceRepo(repoURL, branch string) (git.Repo, error) {
 	repo, err := s.cloneRepo(repoURL, branch)
 	if err != nil {
-		return nil, fmt.Errorf("failed to clone source repo %s: %w", repoURL, err)
+		if git.IsGitError(err) {
+			return nil, git.GitError("failed to clone source repository", repoURL)
+		}
+		return nil, err
 	}
 	err = repo.Checkout(branch)
 	if err != nil {
-		return nil, fmt.Errorf("failed to checkout branch %s in repo %s: %w", branch, repoURL, err)
+		return nil, git.GitError(fmt.Sprintf("failed to checkout branch %s, error: %s", branch, err.Error()), repoURL)
 	}
 	return repo, nil
 }
 
 func (s *ServiceManager) checkoutDestinationRepo(repoURL, branch string) (git.Repo, error) {
 	repo, err := s.cloneRepo(repoURL, branch)
 	if err != nil {
-		return nil, fmt.Errorf("failed to clone destination repo %s: %w", repoURL, err)
+		return nil, git.GitError(err.Error(), repoURL)
 	}
 	err = repo.CheckoutAndCreate(branch)
 	if err != nil {
-		return nil, fmt.Errorf("failed to checkout branch %s in repo %s: %w", branch, repoURL, err)
+		return nil, fmt.Errorf("failed to checkout branch %s: %w", branch, err)
 	}
 	return repo, nil
 }
@@ -185,7 +194,8 @@ func (s *ServiceManager) cloneRepo(repoURL, branch string) (git.Repo, error) {
 	}
 	repo, err := s.repoFactory(repoURL, path.Join(s.cacheDir, encode(repoURL, branch)), s.debug)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create repo for URL %s: %w", repoURL, err)
+		message := fmt.Sprintf("failed to clone repository, error is: %s", err.Error())
+		return nil, git.GitError(message, repoURL)
 	}
 	err = repo.Clone()
 	if err != nil {
@@ -205,7 +215,7 @@ func createPullRequest(ctx context.Context, fromURL, toURL, newBranchName, commi
 		// TODO: improve this error message
 		return nil, err
 	}
-	// TODO: come up with a better way of generating the repo URL (this
+	// TODO: come up with a better way of generating the repository URL (this
 	// only works for GitHub)
 	pr, _, err := client.PullRequests.Create(ctx, fmt.Sprintf("%s/%s", user, repo), prInput)
 	return pr, err
@@ -217,8 +227,11 @@ func encode(gitURL, branch string) string {
 
 func addCredentialsIfNecessary(s string, a *git.Author) (string, error) {
 	parsed, err := url.Parse(s)
+	// If this does error (e.g. if there's a leading ":" character") one would see
+	// parse ":[email protected]": missing protocol scheme
+	// thus surfacing the token. So intentionally display a generic parse problem
 	if err != nil {
-		return "", fmt.Errorf("failed to parse git repo url %v: %w", s, err)
+		return "", errors.New("failed to parse git repository url")
 	}
 	if parsed.Scheme != "https" || parsed.User != nil {
 		return s, nil

pkg/avancement/service_manager_test.go

@@ -13,6 +13,7 @@ import (
 	fakescm "github.com/jenkins-x/go-scm/scm/driver/fake"
 	"github.com/rhd-gitops-example/services/pkg/git"
 	"github.com/rhd-gitops-example/services/pkg/git/mock"
+	"github.com/rhd-gitops-example/services/test"
 )
 
 const (
@@ -281,3 +282,26 @@ func (s *mockSource) AddFiles(name string) {
 	}
 	s.files = append(s.files, path.Join(s.localPath, name))
 }
+
+func TestRepositoryCloneErrorOmitsToken(t *testing.T) {
+	dstBranch := "test-branch"
+	author := &git.Author{Name: "Testing User", Email: "[email protected]", Token: "test-token"}
+	client, _ := fakescm.NewDefault()
+	fakeClientFactory := func(s string) *scm.Client {
+		return client
+	}
+	sm := New("tmp", author)
+	sm.clientFactory = fakeClientFactory
+
+	sm.repoFactory = func(url, _ string, _ bool) (git.Repo, error) {
+		// This actually causes the error and results in trying to create a repository
+		// which can surface the token
+		errorMessage := fmt.Errorf("failed to clone repository %s: exit status 128", dev)
+		return nil, errorMessage
+	}
+	err := sm.Promote("my-service", dev, staging, dstBranch, "", false)
+	if err != nil {
+		devRepoToUseInError := fmt.Sprintf(".*%s", dev)
+		test.AssertErrorMatch(t, devRepoToUseInError, err)
+	}
+}

pkg/git/errors.go

@@ -0,0 +1,46 @@
+package git
+
+import (
+	"errors"
+	"fmt"
+	"net/url"
+)
+
+// We can choose to throw one of these errors if we want to handle it
+// in a particularly special way. This will be useful for Git API calls where
+// we know there's going to be a repository URL involved and it's not
+// some other error (e.g. to do with file permissions)
+type gitError struct {
+	msg string
+}
+
+// GitError returns an error whereby the user part of the url is removed (e.g. an access token)
+// Accepts the message to use and the url to remove the user part from
+func GitError(msg, url string) error {
+	cleanedURL, err := CleanURL(url)
+	if err != nil {
+		return err
+	}
+	return gitError{msg: fmt.Sprintf("%s. repository URL = %s", msg, cleanedURL)}
+}
+
+// IsGitError performs a cast to see if an error really is of type gitError
+func IsGitError(err error) bool {
+	_, ok := err.(gitError)
+	return ok
+}
+
+// CleanURL removes the .User part of the string (typically an access token)
+func CleanURL(inputURL string) (string, error) {
+	parsedURL, parseErr := url.Parse(inputURL)
+	if parseErr != nil {
+		// Intentionally don't display any real parse error as it would contain any token
+		return "", errors.New("failed to parse git repository URL, check it's well-formed")
+	}
+	parsedURL.User = nil
+	return parsedURL.String(), nil
+}
+
+func (e gitError) Error() string {
+	return e.msg
+}

pkg/git/errors_test.go

@@ -0,0 +1,25 @@
+package git
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestCleanURLRemovesTokenFromError(t *testing.T) {
+	url := "https://[email protected]/my-repo"
+	cleanedURL, err := CleanURL(url)
+	if err != nil {
+		t.Fatal("got an error cleaning a well-formed URL")
+	}
+	if strings.Contains(cleanedURL, "mytoken") {
+		t.Fatal("error still contains access token")
+	}
+}
+
+func TestCleanURLThrowsErrorIfInvalidURL(t *testing.T) {
+	url := ":_ https://[email protected]/my-repo"
+	cleanedURL, err := CleanURL(url)
+	if err == nil {
+		t.Fatalf("did not get an an error cleaning a bad URL, cleaned URL is: %s", cleanedURL)
+	}
+}

pkg/git/repository.go

@@ -2,6 +2,7 @@ package git
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -45,8 +46,8 @@ func (r *Repository) Clone() error {
 	if err != nil {
 		return fmt.Errorf("error creating the cache dir %s: %w", r.LocalPath, err)
 	}
-	out, err := r.execGit(r.LocalPath, nil, "clone", r.RepoURL)
-	log.Printf("%s\n", out)
+	// Intentionally omit output as it can contain an access token
+	_, err = r.execGit(r.LocalPath, nil, "clone", r.RepoURL)
 	return err
 }
 
@@ -144,11 +145,12 @@ func (r *Repository) execGit(workingDir string, env []string, args ...string) ([
 func repoName(u string) (string, error) {
 	parsed, err := url.Parse(u)
 	if err != nil {
-		return "", fmt.Errorf("failed to parse URL '%s': %w", u, err)
+		// Don't surface the URL as it could contain a token
+		return "", errors.New("failed to parse the URL when determining repository name")
 	}
 	parts := strings.Split(parsed.Path, "/")
 	if len(parts) < 3 {
-		return "", fmt.Errorf("could not identify repo: %s", u)
+		return "", fmt.Errorf("could not identify repository name: %s", u)
 	}
 	return strings.TrimSuffix(parts[len(parts)-1], ".git"), nil
 }

pkg/git/repository_test.go

@@ -24,7 +24,7 @@ func TestRepoName(t *testing.T) {
 		wantErr  string
 	}{
 		{testRepository, "staging", ""},
-		{"https://github.com/mnuttall", "", "could not identify repo: https://github.com/mnuttall"},
+		{"https://github.com/mnuttall", "", "could not identify repository name: https://github.com/mnuttall"},
 	}
 
 	for _, tt := range nameTests {
@@ -259,7 +259,7 @@ func TestDebugEnabled(t *testing.T) {
 	r, err := NewRepository(testRepository, path.Join(tempDir, "path"), true)
 	assertNoError(t, err)
 	if !r.debug {
-	  t.Fatalf("Debug not set to true")
+		t.Fatalf("Debug not set to true")
 	}
 }