fix: do not inject GIT_USERNAME and GIT_PASSWORD into git clone URL

envbuilder.go

@@ -361,13 +361,8 @@ func Run(ctx context.Context, options Options) error {
 		}
 
 		if options.GitUsername != "" || options.GitPassword != "" {
-			gitURL, err := url.Parse(options.GitURL)
-			if err != nil {
-				return fmt.Errorf("parse git url: %w", err)
-			}
-			gitURL.User = url.UserPassword(options.GitUsername, options.GitPassword)
-			options.GitURL = gitURL.String()
-
+			// NOTE: we previously inserted the credentials into the repo URL.
+			// This was removed in https://github.com/coder/envbuilder/pull/141
 			cloneOpts.RepoAuth = &githttp.BasicAuth{
 				Username: options.GitUsername,
 				Password: options.GitPassword,

git_test.go

@@ -2,74 +2,186 @@ package envbuilder_test
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http/httptest"
+	"net/url"
 	"os"
+	"regexp"
 	"testing"
 	"time"
 
 	"github.com/coder/envbuilder"
 	"github.com/coder/envbuilder/gittest"
+	"github.com/go-git/go-billy/v5"
 	"github.com/go-git/go-billy/v5/memfs"
 	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing/object"
+	githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
 	"github.com/stretchr/testify/require"
 )
 
 func TestCloneRepo(t *testing.T) {
 	t.Parallel()
 
-	t.Run("Clones", func(t *testing.T) {
-		t.Parallel()
+	for _, tc := range []struct {
+		name        string
+		srvUsername string
+		srvPassword string
+		username    string
+		password    string
+		mungeURL    func(*string)
+		expectError string
+		expectClone bool
+	}{
+		{
+			name:        "no auth",
+			expectClone: true,
+		},
+		{
+			name:        "auth",
+			srvUsername: "user",
+			srvPassword: "password",
+			username:    "user",
+			password:    "password",
+			expectClone: true,
+		},
+		{
+			name:        "auth but no creds",
+			srvUsername: "user",
+			srvPassword: "password",
+			expectClone: false,
+			expectError: "authentication required",
+		},
+		{
+			name:        "invalid auth",
+			srvUsername: "user",
+			srvPassword: "password",
+			username:    "notuser",
+			password:    "notpassword",
+			expectClone: false,
+			expectError: "authentication required",
+		},
+		{
+			name:        "tokenish username",
+			srvUsername: "tokentokentoken",
+			srvPassword: "",
+			username:    "tokentokentoken",
+			password:    "",
+			expectClone: true,
+		},
+	} {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
 
-		serverFS := memfs.New()
-		repo := gittest.NewRepo(t, serverFS)
-		tree, err := repo.Worktree()
-		require.NoError(t, err)
+			// We do not overwrite a repo if one is already present.
+			t.Run("AlreadyCloned", func(t *testing.T) {
+				srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
+				clientFS := memfs.New()
+				// A repo already exists!
+				_ = gittest.NewRepo(t, clientFS)
+				cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
+					Path:    "/",
+					RepoURL: srvURL,
+					Storage: clientFS,
+				})
+				require.NoError(t, err)
+				require.False(t, cloned)
+			})
 
-		gittest.WriteFile(t, serverFS, "README.md", "Hello, world!")
-		_, err = tree.Add("README.md")
-		require.NoError(t, err)
-		commit, err := tree.Commit("Wow!", &git.CommitOptions{
-			Author: &object.Signature{
-				Name:  "Example",
-				Email: "[email protected]",
-				When:  time.Now(),
-			},
-		})
-		require.NoError(t, err)
-		_, err = repo.CommitObject(commit)
-		require.NoError(t, err)
+			// Basic Auth
+			t.Run("BasicAuth", func(t *testing.T) {
+				t.Parallel()
+				srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
+				clientFS := memfs.New()
 
-		srv := httptest.NewServer(gittest.NewServer(serverFS))
+				cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
+					Path:    "/workspace",
+					RepoURL: srvURL,
+					Storage: clientFS,
+					RepoAuth: &githttp.BasicAuth{
+						Username: tc.username,
+						Password: tc.password,
+					},
+				})
+				require.Equal(t, tc.expectClone, cloned)
+				if tc.expectError != "" {
+					require.ErrorContains(t, err, tc.expectError)
+					return
+				}
+				require.NoError(t, err)
+				require.True(t, cloned)
 
-		clientFS := memfs.New()
-		cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
-			Path:    "/workspace",
-			RepoURL: srv.URL,
-			Storage: clientFS,
-		})
-		require.NoError(t, err)
-		require.True(t, cloned)
+				readme := mustRead(t, clientFS, "/workspace/README.md")
+				require.Equal(t, "Hello, world!", readme)
+				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
+				// Ensure we do not modify the git URL that folks pass in.
+				require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(srvURL)), gitConfig)
+			})
 
-		file, err := clientFS.OpenFile("/workspace/README.md", os.O_RDONLY, 0644)
-		require.NoError(t, err)
-		defer file.Close()
-		content, err := io.ReadAll(file)
-		require.NoError(t, err)
-		require.Equal(t, "Hello, world!", string(content))
-	})
+			// In-URL-style auth e.g. http://user:password@host:port
+			t.Run("InURL", func(t *testing.T) {
+				t.Parallel()
+				srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
+				authURL, err := url.Parse(srvURL)
+				require.NoError(t, err)
+				authURL.User = url.UserPassword(tc.username, tc.password)
+				clientFS := memfs.New()
 
-	t.Run("DoesntCloneIfRepoExists", func(t *testing.T) {
-		t.Parallel()
-		clientFS := memfs.New()
-		gittest.NewRepo(t, clientFS)
-		cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
-			Path:    "/",
-			RepoURL: "https://example.com",
-			Storage: clientFS,
+				cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
+					Path:    "/workspace",
+					RepoURL: authURL.String(),
+					Storage: clientFS,
+				})
+				require.Equal(t, tc.expectClone, cloned)
+				if tc.expectError != "" {
+					require.ErrorContains(t, err, tc.expectError)
+					return
+				}
+				require.NoError(t, err)
+				require.True(t, cloned)
+
+				readme := mustRead(t, clientFS, "/workspace/README.md")
+				require.Equal(t, "Hello, world!", readme)
+				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
+				// Ensure we do not modify the git URL that folks pass in.
+				require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(authURL.String())), gitConfig)
+			})
 		})
-		require.NoError(t, err)
-		require.False(t, cloned)
+	}
+}
+
+func mustRead(t *testing.T, fs billy.Filesystem, path string) string {
+	t.Helper()
+	f, err := fs.OpenFile(path, os.O_RDONLY, 0644)
+	require.NoError(t, err)
+	content, err := io.ReadAll(f)
+	require.NoError(t, err)
+	return string(content)
+}
+
+func setupGit(t *testing.T, user, pass string) (url string) {
+	serverFS := memfs.New()
+	repo := gittest.NewRepo(t, serverFS)
+	tree, err := repo.Worktree()
+	require.NoError(t, err)
+
+	gittest.WriteFile(t, serverFS, "README.md", "Hello, world!")
+	_, err = tree.Add("README.md")
+	require.NoError(t, err)
+	commit, err := tree.Commit("Wow!", &git.CommitOptions{
+		Author: &object.Signature{
+			Name:  "Example",
+			Email: "[email protected]",
+			When:  time.Now(),
+		},
 	})
+	require.NoError(t, err)
+	_, err = repo.CommitObject(commit)
+	require.NoError(t, err)
+
+	authMW := gittest.BasicAuthMW(user, pass)
+	srv := httptest.NewServer(authMW(gittest.NewServer(serverFS)))
+	return srv.URL
 }

gittest/gittest.go

@@ -118,3 +118,18 @@ func WriteFile(t *testing.T, fs billy.Filesystem, path, content string) {
 	err = file.Close()
 	require.NoError(t, err)
 }
+
+func BasicAuthMW(username, password string) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if username != "" || password != "" {
+				authUser, authPass, ok := r.BasicAuth()
+				if !ok || username != authUser || password != authPass {
+					w.WriteHeader(http.StatusUnauthorized)
+					return
+				}
+			}
+			next.ServeHTTP(w, r)
+		})
+	}
+}

integration/integration_test.go

@@ -71,13 +71,37 @@ func TestSucceedsGitAuth(t *testing.T) {
 		username: "kyle",
 		password: "testing",
 	})
-	_, err := runEnvbuilder(t, options{env: []string{
+	ctr, err := runEnvbuilder(t, options{env: []string{
 		"GIT_URL=" + url,
 		"DOCKERFILE_PATH=Dockerfile",
 		"GIT_USERNAME=kyle",
 		"GIT_PASSWORD=testing",
 	}})
 	require.NoError(t, err)
+	gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config")
+	require.Contains(t, gitConfig, url)
+}
+
+func TestSucceedsGitAuthInURL(t *testing.T) {
+	t.Parallel()
+	gitURL := createGitServer(t, gitServerOptions{
+		files: map[string]string{
+			"Dockerfile": "FROM " + testImageAlpine,
+		},
+		username: "kyle",
+		password: "testing",
+	})
+
+	u, err := url.Parse(gitURL)
+	require.NoError(t, err)
+	u.User = url.UserPassword("kyle", "testing")
+	ctr, err := runEnvbuilder(t, options{env: []string{
+		"GIT_URL=" + u.String(),
+		"DOCKERFILE_PATH=Dockerfile",
+	}})
+	require.NoError(t, err)
+	gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config")
+	require.Contains(t, gitConfig, u.String())
 }
 
 func TestBuildFromDevcontainerWithFeatures(t *testing.T) {
@@ -756,27 +780,12 @@ type gitServerOptions struct {
 func createGitServer(t *testing.T, opts gitServerOptions) string {
 	t.Helper()
 	if opts.authMW == nil {
-		opts.authMW = checkBasicAuth(opts.username, opts.password)
+		opts.authMW = gittest.BasicAuthMW(opts.username, opts.password)
 	}
 	srv := httptest.NewServer(opts.authMW(createGitHandler(t, opts)))
 	return srv.URL
 }
 
-func checkBasicAuth(username, password string) func(http.Handler) http.Handler {
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			if username != "" && password != "" {
-				authUser, authPass, ok := r.BasicAuth()
-				if !ok || username != authUser || password != authPass {
-					w.WriteHeader(http.StatusUnauthorized)
-					return
-				}
-			}
-			next.ServeHTTP(w, r)
-		})
-	}
-}
-
 func createGitHandler(t *testing.T, opts gitServerOptions) http.Handler {
 	t.Helper()
 	fs := memfs.New()