Skip to content

Commit 95ae2f0

Browse files
johnstcnjohnstcn
authored
fix: do not inject GIT_USERNAME and GIT_PASSWORD into git clone URL (#141)
1 parent 9685dcc commit 95ae2f0

File tree

4 files changed

+200
-69
lines changed

4 files changed

+200
-69
lines changed

‎envbuilder.go

+2-7
Original file line numberDiff line numberDiff line change
@@ -365,13 +365,8 @@ func Run(ctx context.Context, options Options) error {
365365
}
366366

367367
if options.GitUsername != "" || options.GitPassword != "" {
368-
gitURL, err := url.Parse(options.GitURL)
369-
if err != nil {
370-
return fmt.Errorf("parse git url: %w", err)
371-
}
372-
gitURL.User = url.UserPassword(options.GitUsername, options.GitPassword)
373-
options.GitURL = gitURL.String()
374-
368+
// NOTE: we previously inserted the credentials into the repo URL.
369+
// This was removed in https://github.com/coder/envbuilder/pull/141
375370
cloneOpts.RepoAuth = &githttp.BasicAuth{
376371
Username: options.GitUsername,
377372
Password: options.GitPassword,

‎git_test.go

+157-45
Original file line numberDiff line numberDiff line change
@@ -2,74 +2,186 @@ package envbuilder_test
22

33
import (
44
"context"
5+
"fmt"
56
"io"
67
"net/http/httptest"
8+
"net/url"
79
"os"
10+
"regexp"
811
"testing"
912
"time"
1013

1114
"github.com/coder/envbuilder"
1215
"github.com/coder/envbuilder/gittest"
16+
"github.com/go-git/go-billy/v5"
1317
"github.com/go-git/go-billy/v5/memfs"
1418
"github.com/go-git/go-git/v5"
1519
"github.com/go-git/go-git/v5/plumbing/object"
20+
githttp "github.com/go-git/go-git/v5/plumbing/transport/http"
1621
"github.com/stretchr/testify/require"
1722
)
1823

1924
func TestCloneRepo(t *testing.T) {
2025
t.Parallel()
2126

22-
t.Run("Clones", func(t *testing.T) {
23-
t.Parallel()
27+
for _, tc := range []struct {
28+
name string
29+
srvUsername string
30+
srvPassword string
31+
username string
32+
password string
33+
mungeURL func(*string)
34+
expectError string
35+
expectClone bool
36+
}{
37+
{
38+
name: "no auth",
39+
expectClone: true,
40+
},
41+
{
42+
name: "auth",
43+
srvUsername: "user",
44+
srvPassword: "password",
45+
username: "user",
46+
password: "password",
47+
expectClone: true,
48+
},
49+
{
50+
name: "auth but no creds",
51+
srvUsername: "user",
52+
srvPassword: "password",
53+
expectClone: false,
54+
expectError: "authentication required",
55+
},
56+
{
57+
name: "invalid auth",
58+
srvUsername: "user",
59+
srvPassword: "password",
60+
username: "notuser",
61+
password: "notpassword",
62+
expectClone: false,
63+
expectError: "authentication required",
64+
},
65+
{
66+
name: "tokenish username",
67+
srvUsername: "tokentokentoken",
68+
srvPassword: "",
69+
username: "tokentokentoken",
70+
password: "",
71+
expectClone: true,
72+
},
73+
} {
74+
tc := tc
75+
t.Run(tc.name, func(t *testing.T) {
76+
t.Parallel()
2477

25-
serverFS := memfs.New()
26-
repo := gittest.NewRepo(t, serverFS)
27-
tree, err := repo.Worktree()
28-
require.NoError(t, err)
78+
// We do not overwrite a repo if one is already present.
79+
t.Run("AlreadyCloned", func(t *testing.T) {
80+
srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
81+
clientFS := memfs.New()
82+
// A repo already exists!
83+
_ = gittest.NewRepo(t, clientFS)
84+
cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
85+
Path: "/",
86+
RepoURL: srvURL,
87+
Storage: clientFS,
88+
})
89+
require.NoError(t, err)
90+
require.False(t, cloned)
91+
})
2992

30-
gittest.WriteFile(t, serverFS, "README.md", "Hello, world!")
31-
_, err = tree.Add("README.md")
32-
require.NoError(t, err)
33-
commit, err := tree.Commit("Wow!", &git.CommitOptions{
34-
Author: &object.Signature{
35-
Name: "Example",
36-
Email: "[email protected]",
37-
When: time.Now(),
38-
},
39-
})
40-
require.NoError(t, err)
41-
_, err = repo.CommitObject(commit)
42-
require.NoError(t, err)
93+
// Basic Auth
94+
t.Run("BasicAuth", func(t *testing.T) {
95+
t.Parallel()
96+
srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
97+
clientFS := memfs.New()
4398

44-
srv := httptest.NewServer(gittest.NewServer(serverFS))
99+
cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
100+
Path: "/workspace",
101+
RepoURL: srvURL,
102+
Storage: clientFS,
103+
RepoAuth: &githttp.BasicAuth{
104+
Username: tc.username,
105+
Password: tc.password,
106+
},
107+
})
108+
require.Equal(t, tc.expectClone, cloned)
109+
if tc.expectError != "" {
110+
require.ErrorContains(t, err, tc.expectError)
111+
return
112+
}
113+
require.NoError(t, err)
114+
require.True(t, cloned)
45115

46-
clientFS := memfs.New()
47-
cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
48-
Path: "/workspace",
49-
RepoURL: srv.URL,
50-
Storage: clientFS,
51-
})
52-
require.NoError(t, err)
53-
require.True(t, cloned)
116+
readme := mustRead(t, clientFS, "/workspace/README.md")
117+
require.Equal(t, "Hello, world!", readme)
118+
gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
119+
// Ensure we do not modify the git URL that folks pass in.
120+
require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(srvURL)), gitConfig)
121+
})
54122

55-
file, err := clientFS.OpenFile("/workspace/README.md", os.O_RDONLY, 0644)
56-
require.NoError(t, err)
57-
defer file.Close()
58-
content, err := io.ReadAll(file)
59-
require.NoError(t, err)
60-
require.Equal(t, "Hello, world!", string(content))
61-
})
123+
// In-URL-style auth e.g. http://user:password@host:port
124+
t.Run("InURL", func(t *testing.T) {
125+
t.Parallel()
126+
srvURL := setupGit(t, tc.srvUsername, tc.srvPassword)
127+
authURL, err := url.Parse(srvURL)
128+
require.NoError(t, err)
129+
authURL.User = url.UserPassword(tc.username, tc.password)
130+
clientFS := memfs.New()
62131

63-
t.Run("DoesntCloneIfRepoExists", func(t *testing.T) {
64-
t.Parallel()
65-
clientFS := memfs.New()
66-
gittest.NewRepo(t, clientFS)
67-
cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
68-
Path: "/",
69-
RepoURL: "https://example.com",
70-
Storage: clientFS,
132+
cloned, err := envbuilder.CloneRepo(context.Background(), envbuilder.CloneRepoOptions{
133+
Path: "/workspace",
134+
RepoURL: authURL.String(),
135+
Storage: clientFS,
136+
})
137+
require.Equal(t, tc.expectClone, cloned)
138+
if tc.expectError != "" {
139+
require.ErrorContains(t, err, tc.expectError)
140+
return
141+
}
142+
require.NoError(t, err)
143+
require.True(t, cloned)
144+
145+
readme := mustRead(t, clientFS, "/workspace/README.md")
146+
require.Equal(t, "Hello, world!", readme)
147+
gitConfig := mustRead(t, clientFS, "/workspace/.git/config")
148+
// Ensure we do not modify the git URL that folks pass in.
149+
require.Regexp(t, fmt.Sprintf(`(?m)^\s+url\s+=\s+%s\s*$`, regexp.QuoteMeta(authURL.String())), gitConfig)
150+
})
71151
})
72-
require.NoError(t, err)
73-
require.False(t, cloned)
152+
}
153+
}
154+
155+
func mustRead(t *testing.T, fs billy.Filesystem, path string) string {
156+
t.Helper()
157+
f, err := fs.OpenFile(path, os.O_RDONLY, 0644)
158+
require.NoError(t, err)
159+
content, err := io.ReadAll(f)
160+
require.NoError(t, err)
161+
return string(content)
162+
}
163+
164+
func setupGit(t *testing.T, user, pass string) (url string) {
165+
serverFS := memfs.New()
166+
repo := gittest.NewRepo(t, serverFS)
167+
tree, err := repo.Worktree()
168+
require.NoError(t, err)
169+
170+
gittest.WriteFile(t, serverFS, "README.md", "Hello, world!")
171+
_, err = tree.Add("README.md")
172+
require.NoError(t, err)
173+
commit, err := tree.Commit("Wow!", &git.CommitOptions{
174+
Author: &object.Signature{
175+
Name: "Example",
176+
Email: "[email protected]",
177+
When: time.Now(),
178+
},
74179
})
180+
require.NoError(t, err)
181+
_, err = repo.CommitObject(commit)
182+
require.NoError(t, err)
183+
184+
authMW := gittest.BasicAuthMW(user, pass)
185+
srv := httptest.NewServer(authMW(gittest.NewServer(serverFS)))
186+
return srv.URL
75187
}

‎gittest/gittest.go

+15
Original file line numberDiff line numberDiff line change
@@ -118,3 +118,18 @@ func WriteFile(t *testing.T, fs billy.Filesystem, path, content string) {
118118
err = file.Close()
119119
require.NoError(t, err)
120120
}
121+
122+
func BasicAuthMW(username, password string) func(http.Handler) http.Handler {
123+
return func(next http.Handler) http.Handler {
124+
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
125+
if username != "" || password != "" {
126+
authUser, authPass, ok := r.BasicAuth()
127+
if !ok || username != authUser || password != authPass {
128+
w.WriteHeader(http.StatusUnauthorized)
129+
return
130+
}
131+
}
132+
next.ServeHTTP(w, r)
133+
})
134+
}
135+
}

‎integration/integration_test.go

+26-17
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,37 @@ func TestSucceedsGitAuth(t *testing.T) {
7171
username: "kyle",
7272
password: "testing",
7373
})
74-
_, err := runEnvbuilder(t, options{env: []string{
74+
ctr, err := runEnvbuilder(t, options{env: []string{
7575
"GIT_URL=" + url,
7676
"DOCKERFILE_PATH=Dockerfile",
7777
"GIT_USERNAME=kyle",
7878
"GIT_PASSWORD=testing",
7979
}})
8080
require.NoError(t, err)
81+
gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config")
82+
require.Contains(t, gitConfig, url)
83+
}
84+
85+
func TestSucceedsGitAuthInURL(t *testing.T) {
86+
t.Parallel()
87+
gitURL := createGitServer(t, gitServerOptions{
88+
files: map[string]string{
89+
"Dockerfile": "FROM " + testImageAlpine,
90+
},
91+
username: "kyle",
92+
password: "testing",
93+
})
94+
95+
u, err := url.Parse(gitURL)
96+
require.NoError(t, err)
97+
u.User = url.UserPassword("kyle", "testing")
98+
ctr, err := runEnvbuilder(t, options{env: []string{
99+
"GIT_URL=" + u.String(),
100+
"DOCKERFILE_PATH=Dockerfile",
101+
}})
102+
require.NoError(t, err)
103+
gitConfig := execContainer(t, ctr, "cat /workspaces/.git/config")
104+
require.Contains(t, gitConfig, u.String())
81105
}
82106

83107
func TestBuildFromDevcontainerWithFeatures(t *testing.T) {
@@ -841,27 +865,12 @@ type gitServerOptions struct {
841865
func createGitServer(t *testing.T, opts gitServerOptions) string {
842866
t.Helper()
843867
if opts.authMW == nil {
844-
opts.authMW = checkBasicAuth(opts.username, opts.password)
868+
opts.authMW = gittest.BasicAuthMW(opts.username, opts.password)
845869
}
846870
srv := httptest.NewServer(opts.authMW(createGitHandler(t, opts)))
847871
return srv.URL
848872
}
849873

850-
func checkBasicAuth(username, password string) func(http.Handler) http.Handler {
851-
return func(next http.Handler) http.Handler {
852-
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
853-
if username != "" && password != "" {
854-
authUser, authPass, ok := r.BasicAuth()
855-
if !ok || username != authUser || password != authPass {
856-
w.WriteHeader(http.StatusUnauthorized)
857-
return
858-
}
859-
}
860-
next.ServeHTTP(w, r)
861-
})
862-
}
863-
}
864-
865874
func createGitHandler(t *testing.T, opts gitServerOptions) http.Handler {
866875
t.Helper()
867876
fs := memfs.New()

0 commit comments

Comments
 (0)