address PR comments

Author: johnstcn

envbuilder.go

@@ -361,13 +361,8 @@ func Run(ctx context.Context, options Options) error {
 		}
 
 		if options.GitUsername != "" || options.GitPassword != "" {
-			// Previously, we had been placing credentials in the URL
-			// as well as setting githttp.BasicAuth.
-			// This was removed as it would leak the credentials used
-			// to clone the repo into the resulting workspace.
-			// Users may still hard-code credentials directly into the
-			// git URL themselves, if required.
-
+			// 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,10 +2,12 @@ package envbuilder_test
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http/httptest"
 	"net/url"
 	"os"
+	"regexp"
 	"testing"
 	"time"
 
@@ -45,46 +47,29 @@ func TestCloneRepo(t *testing.T) {
 			expectClone: true,
 		},
 		{
-			name:        "invalid auth",
+			name:        "auth but no creds",
 			srvUsername: "user",
 			srvPassword: "password",
 			expectClone: false,
 			expectError: "authentication required",
 		},
 		{
-			name:        "invalid auth password",
-			srvUsername: "user",
-			srvPassword: "password",
-			username:    "user",
-			password:    "",
-			expectClone: false,
-			expectError: "authentication required",
-		},
-		{
-			name:        "invalid auth user",
+			name:        "invalid auth",
 			srvUsername: "user",
 			srvPassword: "password",
-			username:    "",
-			password:    "password",
+			username:    "notuser",
+			password:    "notpassword",
 			expectClone: false,
 			expectError: "authentication required",
 		},
 		{
-			name:        "user only",
-			srvUsername: "user",
+			name:        "tokenish username",
+			srvUsername: "tokentokentoken",
 			srvPassword: "",
-			username:    "user",
+			username:    "tokentokentoken",
 			password:    "",
 			expectClone: true,
 		},
-		{
-			name:        "password only",
-			srvUsername: "",
-			srvPassword: "password",
-			username:    "",
-			password:    "password",
-			expectClone: true,
-		},
 	} {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
@@ -131,7 +116,8 @@ func TestCloneRepo(t *testing.T) {
 				readme := mustRead(t, clientFS, "/workspace/README.md")
 				require.Equal(t, "Hello, world!", readme)
 				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
-				require.Contains(t, gitConfig, srvURL)
+				// 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)
 			})
 
 			// In-URL-style auth e.g. http://user:password@host:port
@@ -159,8 +145,8 @@ func TestCloneRepo(t *testing.T) {
 				readme := mustRead(t, clientFS, "/workspace/README.md")
 				require.Equal(t, "Hello, world!", readme)
 				gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
-				// We do not modify the git URL that folks pass in.
-				require.Contains(t, gitConfig, authURL.String())
+				// 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)
 			})
 		})
 	}

gittest/gittest.go

@@ -122,7 +122,7 @@ func WriteFile(t *testing.T, fs billy.Filesystem, path, content string) {
 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 != "" {
+			if username != "" || password != "" {
 				authUser, authPass, ok := r.BasicAuth()
 				if !ok || username != authUser || password != authPass {
 					w.WriteHeader(http.StatusUnauthorized)