From 6c2d9757b13516b2b1541172105c354522c37af7 Mon Sep 17 00:00:00 2001 From: onee-only Date: Wed, 18 Jun 2025 23:52:26 +0900 Subject: [PATCH 1/6] plumbing: transport, Close http server on t.CleanUp --- plumbing/transport/http/common_test.go | 9 +++++---- plumbing/transport/http/dumb_test.go | 9 +-------- plumbing/transport/http/proxy_test.go | 3 +-- plumbing/transport/http/receive_pack_test.go | 9 +-------- plumbing/transport/http/upload_pack_test.go | 9 +-------- 5 files changed, 9 insertions(+), 30 deletions(-) diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go index ddb7e6206..7b6a4dcba 100644 --- a/plumbing/transport/http/common_test.go +++ b/plumbing/transport/http/common_test.go @@ -225,7 +225,7 @@ func newEndpoint(t testing.TB, port int, name string) *transport.Endpoint { return ep } -func setupServer(t testing.TB, smart bool) (server *http.Server, base string, port int) { +func setupServer(t testing.TB, smart bool) (base string, port int) { l, err := net.Listen("tcp", "localhost:0") require.NoError(t, err) @@ -239,6 +239,7 @@ func setupServer(t testing.TB, smart bool) (server *http.Server, base string, po out, err := cmd.CombinedOutput() require.NoError(t, err) + var server *http.Server if smart { server = &http.Server{ // TODO: Implement a go-git middleware and use it here. @@ -253,10 +254,10 @@ func setupServer(t testing.TB, smart bool) (server *http.Server, base string, po } } + t.Cleanup(func() { require.NoError(t, server.Close()) }) + go func() { - if err := server.Serve(l); err != http.ErrServerClosed { - t.Fatalf("error http starting server: %v", err) - } + require.ErrorIs(t, server.Serve(l), http.ErrServerClosed) }() return diff --git a/plumbing/transport/http/dumb_test.go b/plumbing/transport/http/dumb_test.go index 90e49c64a..f4ebe9f28 100644 --- a/plumbing/transport/http/dumb_test.go +++ b/plumbing/transport/http/dumb_test.go @@ -1,7 +1,6 @@ package http import ( - "net/http" "testing" fixtures "github.com/go-git/go-git-fixtures/v5" @@ -18,7 +17,6 @@ import ( type DumbSuite struct { test.UploadPackSuite - server *http.Server } func TestDumbSuite(t *testing.T) { @@ -27,8 +25,7 @@ func TestDumbSuite(t *testing.T) { } func (s *DumbSuite) SetupTest() { - server, base, port := setupServer(s.T(), false) - s.server = server + base, port := setupServer(s.T(), false) s.Client = NewTransport(&TransportOptions{ // Set to true to use the dumb transport. @@ -53,10 +50,6 @@ func (s *DumbSuite) SetupTest() { s.Require().NoError(err) } -func (s *DumbSuite) TearDownTest() { - s.Require().NoError(s.server.Close()) -} - // The following tests are not applicable to the dumb transport as it does not // support reference and capability advertisement. diff --git a/plumbing/transport/http/proxy_test.go b/plumbing/transport/http/proxy_test.go index 789873ba9..bfb394718 100644 --- a/plumbing/transport/http/proxy_test.go +++ b/plumbing/transport/http/proxy_test.go @@ -33,8 +33,7 @@ func (s *ProxySuite) TestAdvertisedReferences() { defer httpListener.Close() defer proxyServer.Close() - server, base, port := setupServer(s.T(), true) - defer server.Close() + base, port := setupServer(s.T(), true) endpoint := newEndpoint(s.T(), port, "basic.git") dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), base, "basic.git") diff --git a/plumbing/transport/http/receive_pack_test.go b/plumbing/transport/http/receive_pack_test.go index 24c89f5c4..d369e0777 100644 --- a/plumbing/transport/http/receive_pack_test.go +++ b/plumbing/transport/http/receive_pack_test.go @@ -1,7 +1,6 @@ package http import ( - "net/http" "testing" "github.com/go-git/go-git/v6/internal/transport/test" @@ -17,12 +16,10 @@ func TestReceivePackSuite(t *testing.T) { type ReceivePackSuite struct { test.ReceivePackSuite - server *http.Server } func (s *ReceivePackSuite) SetupTest() { - server, base, port := setupServer(s.T(), true) - s.server = server + base, port := setupServer(s.T(), true) s.Client = DefaultTransport @@ -37,7 +34,3 @@ func (s *ReceivePackSuite) SetupTest() { s.NonExistentEndpoint = newEndpoint(s.T(), port, "non-existent.git") } - -func (s *ReceivePackSuite) TearDownTest() { - s.Require().NoError(s.server.Close()) -} diff --git a/plumbing/transport/http/upload_pack_test.go b/plumbing/transport/http/upload_pack_test.go index e744b81fc..c105b7850 100644 --- a/plumbing/transport/http/upload_pack_test.go +++ b/plumbing/transport/http/upload_pack_test.go @@ -2,7 +2,6 @@ package http import ( "context" - "net/http" "net/url" "testing" @@ -21,12 +20,10 @@ func TestUploadPackSuite(t *testing.T) { type UploadPackSuite struct { test.UploadPackSuite - server *http.Server } func (s *UploadPackSuite) SetupTest() { - server, base, port := setupServer(s.T(), true) - s.server = server + base, port := setupServer(s.T(), true) s.Client = DefaultTransport @@ -43,10 +40,6 @@ func (s *UploadPackSuite) SetupTest() { s.NonExistentStorer = memory.NewStorage() } -func (s *UploadPackSuite) TearDownTest() { - s.Require().NoError(s.server.Close()) -} - // Overwritten, different behaviour for HTTP. func (s *UploadPackSuite) TestAdvertisedReferencesNotExists() { r, err := s.Client.NewSession(s.Storer, s.NonExistentEndpoint, s.EmptyAuth) From 5da84f8c16e901b5262a841186d32a60ce98a99a Mon Sep 17 00:00:00 2001 From: onee-only Date: Thu, 19 Jun 2025 11:48:51 +0900 Subject: [PATCH 2/6] plumbing: transport, Remove unrelated UploadPackSuite from SuiteCommon --- plumbing/transport/ssh/auth_method_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plumbing/transport/ssh/auth_method_test.go b/plumbing/transport/ssh/auth_method_test.go index 5fb3bff83..d0d85c47d 100644 --- a/plumbing/transport/ssh/auth_method_test.go +++ b/plumbing/transport/ssh/auth_method_test.go @@ -20,9 +20,7 @@ func TestSuiteCommon(t *testing.T) { } type ( - SuiteCommon struct { - UploadPackSuite - } + SuiteCommon struct{ suite.Suite } mockKnownHosts struct{} mockKnownHostsWithCert struct{} From 487ed500bceac36eb3e17434199b740c0bbfddd7 Mon Sep 17 00:00:00 2001 From: onee-only Date: Thu, 19 Jun 2025 12:40:48 +0900 Subject: [PATCH 3/6] internal: transport, Add ListenTCP as test utility --- internal/transport/http/test/test_utils.go | 22 +++++++------ internal/transport/test/common.go | 17 ++++++++++ plumbing/transport/http/common_test.go | 9 +++--- plumbing/transport/http/proxy_test.go | 6 ++-- plumbing/transport/ssh/proxy_test.go | 30 +++++------------- plumbing/transport/ssh/upload_pack_test.go | 36 ++++++++++------------ 6 files changed, 60 insertions(+), 60 deletions(-) diff --git a/internal/transport/http/test/test_utils.go b/internal/transport/http/test/test_utils.go index 66c2ecd40..15b283807 100644 --- a/internal/transport/http/test/test_utils.go +++ b/internal/transport/http/test/test_utils.go @@ -4,7 +4,6 @@ import ( "crypto/tls" "embed" "encoding/base64" - "errors" "fmt" "io" "net" @@ -14,17 +13,16 @@ import ( "testing" "github.com/elazarl/goproxy" + "github.com/go-git/go-git/v6/internal/transport/test" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) //go:embed testdata/certs/* var certs embed.FS // Make sure you close the server after the test. -func SetupProxyServer(t *testing.T, handler http.Handler, isTls, schemaAddr bool) (string, *http.Server, net.Listener) { - httpListener, err := net.Listen("tcp", "127.0.0.1:0") - assert.NoError(t, err) - +func SetupProxyServer(t *testing.T, handler http.Handler, isTls, schemaAddr bool) (string, *http.Server) { schema := "http" if isTls { schema = "https" @@ -35,11 +33,15 @@ func SetupProxyServer(t *testing.T, handler http.Handler, isTls, schemaAddr bool addr = schema + "://localhost:%d" } - httpProxyAddr := fmt.Sprintf(addr, httpListener.Addr().(*net.TCPAddr).Port) + httpListener := test.ListenTCP(t) + port := httpListener.Addr().(*net.TCPAddr).Port + + httpProxyAddr := fmt.Sprintf(addr, port) proxyServer := http.Server{ Addr: httpProxyAddr, Handler: handler, } + if isTls { certf, err := certs.Open("testdata/certs/server.crt") assert.NoError(t, err) @@ -71,11 +73,11 @@ func SetupProxyServer(t *testing.T, handler http.Handler, isTls, schemaAddr bool } else { err = proxyServer.Serve(httpListener) } - if err != nil && !errors.Is(err, http.ErrServerClosed) { - panic(err) - } + + require.ErrorIs(t, err, http.ErrServerClosed) }() - return httpProxyAddr, &proxyServer, httpListener + + return httpProxyAddr, &proxyServer } func SetupHTTPProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { diff --git a/internal/transport/test/common.go b/internal/transport/test/common.go index b9da047a1..c2c088b30 100644 --- a/internal/transport/test/common.go +++ b/internal/transport/test/common.go @@ -36,3 +36,20 @@ func FreePort() (int, error) { return l.Addr().(*net.TCPAddr).Port, l.Close() } + +// ListenTCP listens localhost:0. +// It reserves the listener to be closed on t.CleanUp. +func ListenTCP(t testing.TB) *net.TCPListener { + t.Helper() + l, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + + t.Cleanup(func() { + err := l.Close() + if err != nil { + require.ErrorIs(t, err, net.ErrClosed) + } + }) + + return l.(*net.TCPListener) +} diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go index 7b6a4dcba..185053285 100644 --- a/plumbing/transport/http/common_test.go +++ b/plumbing/transport/http/common_test.go @@ -13,6 +13,7 @@ import ( "strings" "testing" + "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing" "github.com/go-git/go-git/v6/plumbing/cache" "github.com/go-git/go-git/v6/plumbing/transport" @@ -226,14 +227,12 @@ func newEndpoint(t testing.TB, port int, name string) *transport.Endpoint { } func setupServer(t testing.TB, smart bool) (base string, port int) { - l, err := net.Listen("tcp", "localhost:0") - require.NoError(t, err) + l := test.ListenTCP(t) port = l.Addr().(*net.TCPAddr).Port base = filepath.Join(t.TempDir(), fmt.Sprintf("go-git-http-%d", port)) - require.NoError(t, err) - err = os.MkdirAll(base, 0o755) - require.NoError(t, err) + + require.NoError(t, os.MkdirAll(base, 0o755)) cmd := exec.Command("git", "--exec-path") out, err := cmd.CombinedOutput() diff --git a/plumbing/transport/http/proxy_test.go b/plumbing/transport/http/proxy_test.go index bfb394718..0442d0af9 100644 --- a/plumbing/transport/http/proxy_test.go +++ b/plumbing/transport/http/proxy_test.go @@ -29,8 +29,7 @@ func (s *ProxySuite) TestAdvertisedReferences() { proxy.Verbose = true test.SetupHTTPProxy(proxy, &proxiedRequests) - httpProxyAddr, proxyServer, httpListener := test.SetupProxyServer(s.T(), proxy, false, true) - defer httpListener.Close() + httpProxyAddr, proxyServer := test.SetupProxyServer(s.T(), proxy, false, true) defer proxyServer.Close() base, port := setupServer(s.T(), true) @@ -61,8 +60,7 @@ func (s *ProxySuite) TestAdvertisedReferences() { atomic.StoreInt32(&proxiedRequests, 0) test.SetupHTTPSProxy(proxy, &proxiedRequests) - httpsProxyAddr, tlsProxyServer, httpsListener := test.SetupProxyServer(s.T(), proxy, true, true) - defer httpsListener.Close() + httpsProxyAddr, tlsProxyServer := test.SetupProxyServer(s.T(), proxy, true, true) defer tlsProxyServer.Close() endpoint, err = transport.NewEndpoint("https://github.com/git-fixtures/basic.git") diff --git a/plumbing/transport/ssh/proxy_test.go b/plumbing/transport/ssh/proxy_test.go index 4ac5768f6..98a0ae82d 100644 --- a/plumbing/transport/ssh/proxy_test.go +++ b/plumbing/transport/ssh/proxy_test.go @@ -3,19 +3,15 @@ package ssh import ( "context" "fmt" - "log" "net" - "os" "sync/atomic" "github.com/armon/go-socks5" - "github.com/gliderlabs/ssh" - "github.com/go-git/go-git/v6/internal/transport/ssh/test" + fixtures "github.com/go-git/go-git-fixtures/v5" ttest "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/storage/filesystem" - fixtures "github.com/go-git/go-git-fixtures/v5" stdssh "golang.org/x/crypto/ssh" ) @@ -26,33 +22,23 @@ type ProxySuite struct { var socksProxiedRequests int32 func (s *ProxySuite) TestCommand() { - socksListener, err := net.Listen("tcp", "localhost:0") - s.NoError(err) + socksListener := ttest.ListenTCP(s.T()) socksServer, err := socks5.New(&socks5.Config{ AuthMethods: []socks5.Authenticator{socks5.UserPassAuthenticator{ - Credentials: socks5.StaticCredentials{ - "user": "pass", - }, + Credentials: socks5.StaticCredentials{"user": "pass"}, }}, Rules: TestProxyRule{}, }) - s.NoError(err) - go func() { - socksServer.Serve(socksListener) - }() - socksProxyAddr := fmt.Sprintf("socks5://localhost:%d", socksListener.Addr().(*net.TCPAddr).Port) + s.Require().NoError(err) - sshListener, err := net.Listen("tcp", "localhost:0") - s.NoError(err) - sshServer := &ssh.Server{Handler: test.HandlerSSH} go func() { - log.Fatal(sshServer.Serve(sshListener)) + s.Require().ErrorIs(socksServer.Serve(socksListener), net.ErrClosed) }() - s.port = sshListener.Addr().(*net.TCPAddr).Port - s.base, err = os.MkdirTemp(s.T().TempDir(), fmt.Sprintf("go-git-ssh-%d", s.port)) - s.NoError(err) + socksProxyAddr := fmt.Sprintf("socks5://localhost:%d", socksListener.Addr().(*net.TCPAddr).Port) + + s.base, s.port, _ = setupTest(s.T()) DefaultAuthBuilder = func(user string) (AuthMethod, error) { return &Password{User: user}, nil diff --git a/plumbing/transport/ssh/upload_pack_test.go b/plumbing/transport/ssh/upload_pack_test.go index 826c4f365..ae33d2c0a 100644 --- a/plumbing/transport/ssh/upload_pack_test.go +++ b/plumbing/transport/ssh/upload_pack_test.go @@ -1,9 +1,9 @@ package ssh import ( - "errors" "fmt" - "log" + "net" + "os" "path/filepath" "runtime" "testing" @@ -59,11 +59,6 @@ func (s *UploadPackSuite) SetupTest() { } func setupTest(t testing.TB, opts ...ssh.Option) (base string, port int, client transport.Transport) { - var err error - port, err = test.FreePort() - require.NoError(t, err) - base = t.TempDir() - sshconfig := &stdssh.ClientConfig{ HostKeyCallback: stdssh.InsecureIgnoreHostKey(), } @@ -71,28 +66,31 @@ func setupTest(t testing.TB, opts ...ssh.Option) (base string, port int, client r := &runner{config: sshconfig} client = transport.NewPackTransport(r) - server := startServer(t, port, opts...) - t.Cleanup(func() { - server.Close() - }) - return + addr := startServer(t, opts...) + + base, err := os.MkdirTemp(t.TempDir(), fmt.Sprintf("go-git-ssh-%d", addr.Port)) + require.NoError(t, err) + + return base, addr.Port, client } -func startServer(t testing.TB, port int, opts ...ssh.Option) *ssh.Server { +func startServer(t testing.TB, opts ...ssh.Option) *net.TCPAddr { t.Helper() + + l := test.ListenTCP(t) + server := &ssh.Server{Handler: testutils.HandlerSSH} for _, opt := range opts { opt(server) } - server.Addr = fmt.Sprintf(":%d", port) + go func() { - err := server.ListenAndServe() - if !errors.Is(err, ssh.ErrServerClosed) { - log.Fatal(err) - } + require.ErrorIs(t, server.Serve(l), ssh.ErrServerClosed) }() - return server + t.Cleanup(func() { require.NoError(t, server.Close()) }) + + return l.Addr().(*net.TCPAddr) } func newEndpoint(t testing.TB, base string, port int, name string) *transport.Endpoint { From b1319b4a5740264df1b60825d06af6e0ed6bdb1b Mon Sep 17 00:00:00 2001 From: onee-only Date: Thu, 19 Jun 2025 12:52:35 +0900 Subject: [PATCH 4/6] plumbing: transport, Remove duplicate tests --- internal/transport/http/proxy_test.go | 61 ------------- internal/transport/ssh/test/proxy_test.go | 99 --------------------- plumbing/transport/http/proxy_test.go | 57 +++++++----- plumbing/transport/http/upload_pack_test.go | 17 ---- plumbing/transport/ssh/proxy_test.go | 40 +++++---- plumbing/transport/ssh/upload_pack_test.go | 2 +- 6 files changed, 59 insertions(+), 217 deletions(-) delete mode 100644 internal/transport/http/proxy_test.go delete mode 100644 internal/transport/ssh/test/proxy_test.go diff --git a/internal/transport/http/proxy_test.go b/internal/transport/http/proxy_test.go deleted file mode 100644 index c7ff7e744..000000000 --- a/internal/transport/http/proxy_test.go +++ /dev/null @@ -1,61 +0,0 @@ -package http - -import ( - "context" - "fmt" - "os" - "sync/atomic" - "testing" - - "github.com/elazarl/goproxy" - "github.com/go-git/go-git/v6/internal/transport/http/test" - "github.com/go-git/go-git/v6/plumbing/transport" - "github.com/go-git/go-git/v6/plumbing/transport/http" - "github.com/go-git/go-git/v6/storage/memory" - "github.com/stretchr/testify/suite" -) - -type ProxySuite struct { - suite.Suite -} - -func TestProxySuite(t *testing.T) { - suite.Run(t, new(ProxySuite)) -} - -// This test tests proxy support via an env var, i.e. `HTTPS_PROXY`. -// Its located in a separate package because golang caches the value -// of proxy env vars leading to misleading/unexpected test results. -func (s *ProxySuite) TestAdvertisedReferences() { - var proxiedRequests int32 - - proxy := goproxy.NewProxyHttpServer() - proxy.Verbose = true - test.SetupHTTPSProxy(proxy, &proxiedRequests) - - httpsProxyAddr, tlsProxyServer, httpsListener := test.SetupProxyServer(s.T(), proxy, true, false) - defer httpsListener.Close() - defer tlsProxyServer.Close() - - os.Setenv("HTTPS_PROXY", fmt.Sprintf("https://user:pass@%s", httpsProxyAddr)) - defer os.Unsetenv("HTTPS_PROXY") - - st := memory.NewStorage() - endpoint, err := transport.NewEndpoint("https://github.com/git-fixtures/basic.git") - s.NoError(err) - endpoint.InsecureSkipTLS = true - - client := http.DefaultTransport - session, err := client.NewSession(st, endpoint, nil) - s.NoError(err) - conn, err := session.Handshake(context.Background(), transport.UploadPackService) - s.NoError(err) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - info, err := conn.GetRemoteRefs(ctx) - s.NoError(err) - s.NotNil(info) - proxyUsed := atomic.LoadInt32(&proxiedRequests) > 0 - s.True(proxyUsed) -} diff --git a/internal/transport/ssh/test/proxy_test.go b/internal/transport/ssh/test/proxy_test.go deleted file mode 100644 index 1f230e88c..000000000 --- a/internal/transport/ssh/test/proxy_test.go +++ /dev/null @@ -1,99 +0,0 @@ -package test - -import ( - "context" - "fmt" - "log" - "net" - "os" - "sync/atomic" - "testing" - - "github.com/armon/go-socks5" - "github.com/gliderlabs/ssh" - "github.com/go-git/go-git/v6/internal/transport/test" - "github.com/go-git/go-git/v6/plumbing/transport" - ggssh "github.com/go-git/go-git/v6/plumbing/transport/ssh" - "github.com/go-git/go-git/v6/storage/memory" - "github.com/stretchr/testify/suite" - - fixtures "github.com/go-git/go-git-fixtures/v5" - stdssh "golang.org/x/crypto/ssh" -) - -type ProxyEnvSuite struct { - suite.Suite - port int - base string -} - -func TestProxyEnvSuite(t *testing.T) { - suite.Run(t, new(ProxyEnvSuite)) -} - -var socksProxiedRequests int32 - -// This test tests proxy support via an env var, i.e. `ALL_PROXY`. -// Its located in a separate package because golang caches the value -// of proxy env vars leading to misleading/unexpected test results. -func (s *ProxyEnvSuite) TestCommand() { - socksListener, err := net.Listen("tcp", "localhost:0") - s.NoError(err) - - socksServer, err := socks5.New(&socks5.Config{ - Rules: TestProxyRule{}, - }) - s.NoError(err) - go func() { - socksServer.Serve(socksListener) - }() - socksProxyAddr := fmt.Sprintf("socks5://localhost:%d", socksListener.Addr().(*net.TCPAddr).Port) - os.Setenv("ALL_PROXY", socksProxyAddr) - defer os.Unsetenv("ALL_PROXY") - - sshListener, err := net.Listen("tcp", "localhost:0") - s.NoError(err) - sshServer := &ssh.Server{Handler: HandlerSSH} - go func() { - log.Fatal(sshServer.Serve(sshListener)) - }() - - s.port = sshListener.Addr().(*net.TCPAddr).Port - s.base, err = os.MkdirTemp(s.T().TempDir(), fmt.Sprintf("go-git-ssh-%d", s.port)) - s.NoError(err) - - ggssh.DefaultAuthBuilder = func(user string) (ggssh.AuthMethod, error) { - return &ggssh.Password{User: user}, nil - } - - st := memory.NewStorage() - fs := test.PrepareRepository(s.T(), fixtures.Basic().One(), s.base, "basic.git") - - client := ggssh.NewTransport(&stdssh.ClientConfig{ - HostKeyCallback: stdssh.InsecureIgnoreHostKey(), - }) - r, err := client.NewSession(st, s.newEndpoint(fs.Root()), nil) - s.Require().NoError(err) - conn, err := r.Handshake(context.Background(), transport.UploadPackService) - s.Require().NoError(err) - defer func() { s.Nil(conn.Close()) }() - - info, err := conn.GetRemoteRefs(context.TODO()) - s.NoError(err) - s.NotNil(info) - proxyUsed := atomic.LoadInt32(&socksProxiedRequests) > 0 - s.True(proxyUsed) -} - -func (s *ProxyEnvSuite) newEndpoint(name string) *transport.Endpoint { - ep, err := transport.NewEndpoint(fmt.Sprintf("ssh://git@localhost:%d/%s", s.port, name)) - s.NoError(err) - return ep -} - -type TestProxyRule struct{} - -func (dr TestProxyRule) Allow(ctx context.Context, req *socks5.Request) (context.Context, bool) { - atomic.AddInt32(&socksProxiedRequests, 1) - return ctx, true -} diff --git a/plumbing/transport/http/proxy_test.go b/plumbing/transport/http/proxy_test.go index 0442d0af9..3f7720f14 100644 --- a/plumbing/transport/http/proxy_test.go +++ b/plumbing/transport/http/proxy_test.go @@ -14,15 +14,18 @@ import ( "github.com/stretchr/testify/suite" ) +// This test tests proxy support via an env var, i.e. `HTTPS_PROXY`. +// Its located in a separate package because golang caches the value +// of proxy env vars leading to misleading/unexpected test results. func TestProxySuite(t *testing.T) { suite.Run(t, new(ProxySuite)) } type ProxySuite struct { - UploadPackSuite + suite.Suite } -func (s *ProxySuite) TestAdvertisedReferences() { +func (s *ProxySuite) TestAdvertisedReferencesHTTP() { var proxiedRequests int32 proxy := goproxy.NewProxyHttpServer() @@ -35,36 +38,43 @@ func (s *ProxySuite) TestAdvertisedReferences() { base, port := setupServer(s.T(), true) endpoint := newEndpoint(s.T(), port, "basic.git") - dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), base, "basic.git") endpoint.Proxy = transport.ProxyOptions{ URL: httpProxyAddr, Username: "user", Password: "pass", } + client := NewTransport(nil) + dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), base, "basic.git") st := filesystem.NewStorage(dotgit, nil) - s.Client = NewTransport(nil) - session, err := s.Client.NewSession(st, endpoint, nil) - s.Nil(err) + + session, err := client.NewSession(st, endpoint, nil) + s.Require().NoError(err) conn, err := session.Handshake(context.Background(), transport.UploadPackService) - s.NoError(err) + s.Require().NoError(err) ctx, cancel := context.WithCancel(context.Background()) defer cancel() info, err := conn.GetRemoteRefs(ctx) - s.Nil(err) + s.NoError(err) s.NotNil(info) + proxyUsed := atomic.LoadInt32(&proxiedRequests) > 0 - s.Equal(true, proxyUsed) + s.True(proxyUsed) +} + +func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { + var proxiedRequests int32 - atomic.StoreInt32(&proxiedRequests, 0) + proxy := goproxy.NewProxyHttpServer() + proxy.Verbose = true test.SetupHTTPSProxy(proxy, &proxiedRequests) httpsProxyAddr, tlsProxyServer := test.SetupProxyServer(s.T(), proxy, true, true) defer tlsProxyServer.Close() - endpoint, err = transport.NewEndpoint("https://github.com/git-fixtures/basic.git") - s.Nil(err) + endpoint, err := transport.NewEndpoint("https://github.com/git-fixtures/basic.git") + s.Require().NoError(err) endpoint.Proxy = transport.ProxyOptions{ URL: httpsProxyAddr, Username: "user", @@ -72,14 +82,21 @@ func (s *ProxySuite) TestAdvertisedReferences() { } endpoint.InsecureSkipTLS = true - session, err = s.Client.NewSession(st, endpoint, nil) - s.Nil(err) - conn, err = session.Handshake(context.Background(), transport.UploadPackService) - s.NoError(err) + client := NewTransport(nil) + dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), s.T().TempDir(), "basic.git") + st := filesystem.NewStorage(dotgit, nil) - info, err = conn.GetRemoteRefs(ctx) - s.Nil(err) + session, err := client.NewSession(st, endpoint, nil) + s.Require().NoError(err) + conn, err := session.Handshake(context.Background(), transport.UploadPackService) + s.Require().NoError(err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + info, err := conn.GetRemoteRefs(ctx) + s.NoError(err) s.NotNil(info) - proxyUsed = atomic.LoadInt32(&proxiedRequests) > 0 - s.Equal(true, proxyUsed) + + proxyUsed := atomic.LoadInt32(&proxiedRequests) > 0 + s.True(proxyUsed) } diff --git a/plumbing/transport/http/upload_pack_test.go b/plumbing/transport/http/upload_pack_test.go index c105b7850..e65503888 100644 --- a/plumbing/transport/http/upload_pack_test.go +++ b/plumbing/transport/http/upload_pack_test.go @@ -49,23 +49,6 @@ func (s *UploadPackSuite) TestAdvertisedReferencesNotExists() { s.Nil(conn) } -// func (s *UploadPackSuite) TestuploadPackRequestToReader() { -// r := &transport.FetchRequest{} -// r.Wants = append(r.Wants, plumbing.NewHash("d82f291cde9987322c8a0c81a325e1ba6159684c")) -// r.Wants = append(r.Wants, plumbing.NewHash("2b41ef280fdb67a9b250678686a0c3e03b0a9989")) -// r.Haves = append(r.Haves, plumbing.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5")) -// -// sr, err := uploadPackRequestToReader(r) -// s.Nil(err) -// b, _ := io.ReadAll(sr) -// s.Equal(string(b), -// "0032want 2b41ef280fdb67a9b250678686a0c3e03b0a9989\n"+ -// "0032want d82f291cde9987322c8a0c81a325e1ba6159684c\n0000"+ -// "0032have 6ecf0ef2c2dffb796033e5a02219af86ec6584e5\n"+ -// "0009done\n", -// ) -// } - func (s *UploadPackSuite) TestAdvertisedReferencesRedirectPath() { endpoint, _ := transport.NewEndpoint("https://gitlab.com/gitlab-org/gitter/webapp") diff --git a/plumbing/transport/ssh/proxy_test.go b/plumbing/transport/ssh/proxy_test.go index 98a0ae82d..f2e4f9c89 100644 --- a/plumbing/transport/ssh/proxy_test.go +++ b/plumbing/transport/ssh/proxy_test.go @@ -4,31 +4,43 @@ import ( "context" "fmt" "net" - "sync/atomic" + "testing" "github.com/armon/go-socks5" - fixtures "github.com/go-git/go-git-fixtures/v5" ttest "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing/transport" - "github.com/go-git/go-git/v6/storage/filesystem" + "github.com/stretchr/testify/suite" stdssh "golang.org/x/crypto/ssh" ) type ProxySuite struct { - UploadPackSuite + suite.Suite } -var socksProxiedRequests int32 +func TestProxySuite(t *testing.T) { + suite.Run(t, new(ProxySuite)) +} + +type TestProxyRule struct{ proxiedRequests int } + +func (dr *TestProxyRule) Allow(ctx context.Context, req *socks5.Request) (context.Context, bool) { + dr.proxiedRequests++ + return ctx, true +} +// This test tests proxy support via an env var, i.e. `ALL_PROXY`. +// Its located in a separate package because golang caches the value +// of proxy env vars leading to misleading/unexpected test results. func (s *ProxySuite) TestCommand() { socksListener := ttest.ListenTCP(s.T()) + rule := TestProxyRule{} socksServer, err := socks5.New(&socks5.Config{ AuthMethods: []socks5.Authenticator{socks5.UserPassAuthenticator{ Credentials: socks5.StaticCredentials{"user": "pass"}, }}, - Rules: TestProxyRule{}, + Rules: &rule, }) s.Require().NoError(err) @@ -38,16 +50,13 @@ func (s *ProxySuite) TestCommand() { socksProxyAddr := fmt.Sprintf("socks5://localhost:%d", socksListener.Addr().(*net.TCPAddr).Port) - s.base, s.port, _ = setupTest(s.T()) + base, port, _ := setupTest(s.T()) DefaultAuthBuilder = func(user string) (AuthMethod, error) { return &Password{User: user}, nil } - ep := newEndpoint(s.T(), s.base, s.port, "basic.git") - basic := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), s.base, "basic.git") - s.Storer = filesystem.NewStorage(basic, nil) - s.NoError(err) + ep := newEndpoint(s.T(), base, port, "") ep.Proxy = transport.ProxyOptions{ URL: socksProxyAddr, Username: "user", @@ -61,13 +70,6 @@ func (s *ProxySuite) TestCommand() { } _, err = runner.Command(context.TODO(), transport.UploadPackService.String(), ep, nil) s.NoError(err) - proxyUsed := atomic.LoadInt32(&socksProxiedRequests) > 0 - s.True(proxyUsed) -} -type TestProxyRule struct{} - -func (dr TestProxyRule) Allow(ctx context.Context, req *socks5.Request) (context.Context, bool) { - atomic.AddInt32(&socksProxiedRequests, 1) - return ctx, true + s.True(rule.proxiedRequests > 0) } diff --git a/plumbing/transport/ssh/upload_pack_test.go b/plumbing/transport/ssh/upload_pack_test.go index ae33d2c0a..a36d668a0 100644 --- a/plumbing/transport/ssh/upload_pack_test.go +++ b/plumbing/transport/ssh/upload_pack_test.go @@ -48,7 +48,7 @@ func (s *UploadPackSuite) TearDownSuite() { } func (s *UploadPackSuite) SetupTest() { - s.base, s.port, s.Client = setupTest(s.T()) + s.base, s.port, s.Client = setupTest(s.T(), s.opts...) s.Endpoint = newEndpoint(s.T(), s.base, s.port, "basic.git") s.EmptyEndpoint = newEndpoint(s.T(), s.base, s.port, "empty.git") s.NonExistentEndpoint = newEndpoint(s.T(), s.base, s.port, "non-existent.git") From 7a5e048becccf52a7850f8addc302a36d8b3305a Mon Sep 17 00:00:00 2001 From: onee-only Date: Thu, 19 Jun 2025 13:34:46 +0900 Subject: [PATCH 5/6] internal: transport, Move protocol-specific test utils to each package --- internal/transport/http/test/test_utils.go | 132 ---------------- internal/transport/ssh/test/test_utils.go | 83 ----------- plumbing/transport/http/proxy_test.go | 141 ++++++++++++++++-- .../transport/http}/testdata/certs/server.crt | 0 .../transport/http}/testdata/certs/server.key | 0 plumbing/transport/ssh/proxy_test.go | 4 +- plumbing/transport/ssh/upload_pack_test.go | 78 +++++++++- 7 files changed, 209 insertions(+), 229 deletions(-) delete mode 100644 internal/transport/http/test/test_utils.go delete mode 100644 internal/transport/ssh/test/test_utils.go rename {internal/transport/http/test => plumbing/transport/http}/testdata/certs/server.crt (100%) rename {internal/transport/http/test => plumbing/transport/http}/testdata/certs/server.key (100%) diff --git a/internal/transport/http/test/test_utils.go b/internal/transport/http/test/test_utils.go deleted file mode 100644 index 15b283807..000000000 --- a/internal/transport/http/test/test_utils.go +++ /dev/null @@ -1,132 +0,0 @@ -package test - -import ( - "crypto/tls" - "embed" - "encoding/base64" - "fmt" - "io" - "net" - "net/http" - "strings" - "sync/atomic" - "testing" - - "github.com/elazarl/goproxy" - "github.com/go-git/go-git/v6/internal/transport/test" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -//go:embed testdata/certs/* -var certs embed.FS - -// Make sure you close the server after the test. -func SetupProxyServer(t *testing.T, handler http.Handler, isTls, schemaAddr bool) (string, *http.Server) { - schema := "http" - if isTls { - schema = "https" - } - - addr := "localhost:%d" - if schemaAddr { - addr = schema + "://localhost:%d" - } - - httpListener := test.ListenTCP(t) - port := httpListener.Addr().(*net.TCPAddr).Port - - httpProxyAddr := fmt.Sprintf(addr, port) - proxyServer := http.Server{ - Addr: httpProxyAddr, - Handler: handler, - } - - if isTls { - certf, err := certs.Open("testdata/certs/server.crt") - assert.NoError(t, err) - defer certf.Close() - keyf, err := certs.Open("testdata/certs/server.key") - assert.NoError(t, err) - defer keyf.Close() - cert, err := io.ReadAll(certf) - assert.NoError(t, err) - key, err := io.ReadAll(keyf) - assert.NoError(t, err) - keyPair, err := tls.X509KeyPair(cert, key) - assert.NoError(t, err) - cfg := &tls.Config{ - NextProtos: []string{"http/1.1"}, - Certificates: []tls.Certificate{keyPair}, - } - - // Due to how golang manages http/2 when provided with custom TLS config, - // servers and clients running in the same process leads to issues. - // Ref: https://github.com/golang/go/issues/21336 - proxyServer.TLSConfig = cfg - } - - go func() { - var err error - if isTls { - err = proxyServer.ServeTLS(httpListener, "", "") - } else { - err = proxyServer.Serve(httpListener) - } - - require.ErrorIs(t, err, http.ErrServerClosed) - }() - - return httpProxyAddr, &proxyServer -} - -func SetupHTTPProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { - // The request is being forwarded to the local test git server in this handler. - var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { - if strings.Contains(req.Host, "localhost") { - user, pass, _ := ParseBasicAuth(req.Header.Get("Proxy-Authorization")) - if user != "user" || pass != "pass" { - return req, goproxy.NewResponse(req, goproxy.ContentTypeText, http.StatusUnauthorized, "") - } - atomic.AddInt32(proxiedRequests, 1) - return req, nil - } - // Reject if it isn't our request. - return req, goproxy.NewResponse(req, goproxy.ContentTypeText, http.StatusForbidden, "") - } - proxy.OnRequest().Do(proxyHandler) -} - -func SetupHTTPSProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { - var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { - if strings.Contains(host, "github.com") { - user, pass, _ := ParseBasicAuth(ctx.Req.Header.Get("Proxy-Authorization")) - if user != "user" || pass != "pass" { - return goproxy.RejectConnect, host - } - atomic.AddInt32(proxiedRequests, 1) - return goproxy.OkConnect, host - } - // Reject if it isn't our request. - return goproxy.RejectConnect, host - } - proxy.OnRequest().HandleConnect(proxyHandler) -} - -// adapted from https://github.com/golang/go/blob/2ef70d9d0f98832c8103a7968b195e560a8bb262/src/net/http/request.go#L959 -func ParseBasicAuth(auth string) (username, password string, ok bool) { - const prefix = "Basic " - if len(auth) < len(prefix) || !strings.EqualFold(auth[:len(prefix)], prefix) { - return "", "", false - } - c, err := base64.StdEncoding.DecodeString(auth[len(prefix):]) - if err != nil { - return "", "", false - } - cs := string(c) - username, password, ok = strings.Cut(cs, ":") - if !ok { - return "", "", false - } - return username, password, true -} diff --git a/internal/transport/ssh/test/test_utils.go b/internal/transport/ssh/test/test_utils.go deleted file mode 100644 index 83129d75a..000000000 --- a/internal/transport/ssh/test/test_utils.go +++ /dev/null @@ -1,83 +0,0 @@ -package test - -import ( - "fmt" - "io" - "os/exec" - "runtime" - "strings" - "sync" - - "github.com/gliderlabs/ssh" -) - -func HandlerSSH(s ssh.Session) { - cmd, stdin, stderr, stdout, err := buildCommand(s.Command()) - if err != nil { - fmt.Println(err) - return - } - - if err := cmd.Start(); err != nil { - fmt.Println(err) - return - } - - go func() { - defer stdin.Close() - io.Copy(stdin, s) - }() - - var wg sync.WaitGroup - wg.Add(2) - - go func() { - defer wg.Done() - io.Copy(s.Stderr(), stderr) - }() - - go func() { - defer wg.Done() - io.Copy(s, stdout) - }() - - wg.Wait() - - if err := cmd.Wait(); err != nil { - return - } - -} - -func buildCommand(c []string) (cmd *exec.Cmd, stdin io.WriteCloser, stderr, stdout io.ReadCloser, err error) { - if len(c) != 2 { - err = fmt.Errorf("invalid command") - return - } - - // fix for Windows environments - var path string - if runtime.GOOS == "windows" { - path = strings.Replace(c[1], "/C:", "C:", 1) - } else { - path = c[1] - } - - cmd = exec.Command(c[0], path) - stdout, err = cmd.StdoutPipe() - if err != nil { - return - } - - stdin, err = cmd.StdinPipe() - if err != nil { - return - } - - stderr, err = cmd.StderrPipe() - if err != nil { - return - } - - return -} diff --git a/plumbing/transport/http/proxy_test.go b/plumbing/transport/http/proxy_test.go index 3f7720f14..d6816767a 100644 --- a/plumbing/transport/http/proxy_test.go +++ b/plumbing/transport/http/proxy_test.go @@ -2,15 +2,24 @@ package http import ( "context" + "crypto/tls" + "embed" + "encoding/base64" + "fmt" + "io" + "net" + "net/http" + "strings" "sync/atomic" "testing" "github.com/elazarl/goproxy" fixtures "github.com/go-git/go-git-fixtures/v5" - "github.com/go-git/go-git/v6/internal/transport/http/test" - ttest "github.com/go-git/go-git/v6/internal/transport/test" + "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/storage/filesystem" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" ) @@ -29,10 +38,10 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTP() { var proxiedRequests int32 proxy := goproxy.NewProxyHttpServer() - proxy.Verbose = true - test.SetupHTTPProxy(proxy, &proxiedRequests) - httpProxyAddr, proxyServer := test.SetupProxyServer(s.T(), proxy, false, true) + setupHTTPProxy(proxy, &proxiedRequests) + + httpProxyAddr, proxyServer := setupProxyServer(s.T(), proxy, false, true) defer proxyServer.Close() base, port := setupServer(s.T(), true) @@ -45,7 +54,7 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTP() { } client := NewTransport(nil) - dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), base, "basic.git") + dotgit := test.PrepareRepository(s.T(), fixtures.Basic().One(), base, "basic.git") st := filesystem.NewStorage(dotgit, nil) session, err := client.NewSession(st, endpoint, nil) @@ -67,10 +76,9 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { var proxiedRequests int32 proxy := goproxy.NewProxyHttpServer() - proxy.Verbose = true - test.SetupHTTPSProxy(proxy, &proxiedRequests) + setupHTTPSProxy(proxy, &proxiedRequests) - httpsProxyAddr, tlsProxyServer := test.SetupProxyServer(s.T(), proxy, true, true) + httpsProxyAddr, tlsProxyServer := setupProxyServer(s.T(), proxy, true, true) defer tlsProxyServer.Close() endpoint, err := transport.NewEndpoint("https://github.com/git-fixtures/basic.git") @@ -83,7 +91,7 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { endpoint.InsecureSkipTLS = true client := NewTransport(nil) - dotgit := ttest.PrepareRepository(s.T(), fixtures.Basic().One(), s.T().TempDir(), "basic.git") + dotgit := test.PrepareRepository(s.T(), fixtures.Basic().One(), s.T().TempDir(), "basic.git") st := filesystem.NewStorage(dotgit, nil) session, err := client.NewSession(st, endpoint, nil) @@ -100,3 +108,116 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { proxyUsed := atomic.LoadInt32(&proxiedRequests) > 0 s.True(proxyUsed) } + +//go:embed testdata/certs/* +var certs embed.FS + +// Make sure you close the server after the test. +func setupProxyServer(t testing.TB, handler http.Handler, isTls, schemaAddr bool) (string, *http.Server) { + schema := "http" + if isTls { + schema = "https" + } + + addr := "localhost:%d" + if schemaAddr { + addr = schema + "://localhost:%d" + } + + httpListener := test.ListenTCP(t) + port := httpListener.Addr().(*net.TCPAddr).Port + + httpProxyAddr := fmt.Sprintf(addr, port) + proxyServer := http.Server{ + Addr: httpProxyAddr, + Handler: handler, + } + + if isTls { + certf, err := certs.Open("testdata/certs/server.crt") + assert.NoError(t, err) + defer certf.Close() + keyf, err := certs.Open("testdata/certs/server.key") + assert.NoError(t, err) + defer keyf.Close() + cert, err := io.ReadAll(certf) + assert.NoError(t, err) + key, err := io.ReadAll(keyf) + assert.NoError(t, err) + keyPair, err := tls.X509KeyPair(cert, key) + assert.NoError(t, err) + cfg := &tls.Config{ + NextProtos: []string{"http/1.1"}, + Certificates: []tls.Certificate{keyPair}, + } + + // Due to how golang manages http/2 when provided with custom TLS config, + // servers and clients running in the same process leads to issues. + // Ref: https://github.com/golang/go/issues/21336 + proxyServer.TLSConfig = cfg + } + + go func() { + var err error + if isTls { + err = proxyServer.ServeTLS(httpListener, "", "") + } else { + err = proxyServer.Serve(httpListener) + } + + require.ErrorIs(t, err, http.ErrServerClosed) + }() + + return httpProxyAddr, &proxyServer +} + +func setupHTTPProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { + // The request is being forwarded to the local test git server in this handler. + var proxyHandler goproxy.FuncReqHandler = func(req *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { + if strings.Contains(req.Host, "localhost") { + user, pass, _ := parseBasicAuth(req.Header.Get("Proxy-Authorization")) + if user != "user" || pass != "pass" { + return req, goproxy.NewResponse(req, goproxy.ContentTypeText, http.StatusUnauthorized, "") + } + atomic.AddInt32(proxiedRequests, 1) + return req, nil + } + // Reject if it isn't our request. + return req, goproxy.NewResponse(req, goproxy.ContentTypeText, http.StatusForbidden, "") + } + proxy.OnRequest().Do(proxyHandler) +} + +func setupHTTPSProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { + var proxyHandler goproxy.FuncHttpsHandler = func(host string, ctx *goproxy.ProxyCtx) (*goproxy.ConnectAction, string) { + if strings.Contains(host, "github.com") { + user, pass, _ := parseBasicAuth(ctx.Req.Header.Get("Proxy-Authorization")) + if user != "user" || pass != "pass" { + return goproxy.RejectConnect, host + } + atomic.AddInt32(proxiedRequests, 1) + return goproxy.OkConnect, host + } + // Reject if it isn't our request. + return goproxy.RejectConnect, host + } + proxy.OnRequest().HandleConnect(proxyHandler) +} + +// adapted from https://github.com/golang/go/blob/2ef70d9d0f98832c8103a7968b195e560a8bb262/src/net/http/request.go#L959 +func parseBasicAuth(auth string) (username, password string, ok bool) { + const prefix = "Basic " + if len(auth) < len(prefix) || !strings.EqualFold(auth[:len(prefix)], prefix) { + return "", "", false + } + c, err := base64.StdEncoding.DecodeString(auth[len(prefix):]) + if err != nil { + return "", "", false + } + cs := string(c) + username, password, ok = strings.Cut(cs, ":") + if !ok { + return "", "", false + } + return username, password, true +} diff --git a/internal/transport/http/test/testdata/certs/server.crt b/plumbing/transport/http/testdata/certs/server.crt similarity index 100% rename from internal/transport/http/test/testdata/certs/server.crt rename to plumbing/transport/http/testdata/certs/server.crt diff --git a/internal/transport/http/test/testdata/certs/server.key b/plumbing/transport/http/testdata/certs/server.key similarity index 100% rename from internal/transport/http/test/testdata/certs/server.key rename to plumbing/transport/http/testdata/certs/server.key diff --git a/plumbing/transport/ssh/proxy_test.go b/plumbing/transport/ssh/proxy_test.go index f2e4f9c89..99226573c 100644 --- a/plumbing/transport/ssh/proxy_test.go +++ b/plumbing/transport/ssh/proxy_test.go @@ -7,7 +7,7 @@ import ( "testing" "github.com/armon/go-socks5" - ttest "github.com/go-git/go-git/v6/internal/transport/test" + "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/stretchr/testify/suite" @@ -33,7 +33,7 @@ func (dr *TestProxyRule) Allow(ctx context.Context, req *socks5.Request) (contex // Its located in a separate package because golang caches the value // of proxy env vars leading to misleading/unexpected test results. func (s *ProxySuite) TestCommand() { - socksListener := ttest.ListenTCP(s.T()) + socksListener := test.ListenTCP(s.T()) rule := TestProxyRule{} socksServer, err := socks5.New(&socks5.Config{ diff --git a/plumbing/transport/ssh/upload_pack_test.go b/plumbing/transport/ssh/upload_pack_test.go index a36d668a0..4da14f524 100644 --- a/plumbing/transport/ssh/upload_pack_test.go +++ b/plumbing/transport/ssh/upload_pack_test.go @@ -2,13 +2,16 @@ package ssh import ( "fmt" + "io" "net" "os" + "os/exec" "path/filepath" "runtime" + "strings" + "sync" "testing" - testutils "github.com/go-git/go-git/v6/internal/transport/ssh/test" "github.com/go-git/go-git/v6/internal/transport/test" "github.com/go-git/go-git/v6/plumbing/transport" "github.com/go-git/go-git/v6/storage/filesystem" @@ -79,7 +82,7 @@ func startServer(t testing.TB, opts ...ssh.Option) *net.TCPAddr { l := test.ListenTCP(t) - server := &ssh.Server{Handler: testutils.HandlerSSH} + server := &ssh.Server{Handler: handlerSSH} for _, opt := range opts { opt(server) } @@ -100,3 +103,74 @@ func newEndpoint(t testing.TB, base string, port int, name string) *transport.En require.NoError(t, err) return ep } + +func handlerSSH(s ssh.Session) { + cmd, stdin, stderr, stdout, err := buildCommand(s.Command()) + if err != nil { + fmt.Println(err) + return + } + + if err := cmd.Start(); err != nil { + fmt.Println(err) + return + } + + go func() { + defer stdin.Close() + io.Copy(stdin, s) + }() + + var wg sync.WaitGroup + wg.Add(2) + + go func() { + defer wg.Done() + io.Copy(s.Stderr(), stderr) + }() + + go func() { + defer wg.Done() + io.Copy(s, stdout) + }() + + wg.Wait() + + if err := cmd.Wait(); err != nil { + return + } + +} + +func buildCommand(c []string) (cmd *exec.Cmd, stdin io.WriteCloser, stderr, stdout io.ReadCloser, err error) { + if len(c) != 2 { + err = fmt.Errorf("invalid command") + return + } + + // fix for Windows environments + var path string + if runtime.GOOS == "windows" { + path = strings.Replace(c[1], "/C:", "C:", 1) + } else { + path = c[1] + } + + cmd = exec.Command(c[0], path) + stdout, err = cmd.StdoutPipe() + if err != nil { + return + } + + stdin, err = cmd.StdinPipe() + if err != nil { + return + } + + stderr, err = cmd.StderrPipe() + if err != nil { + return + } + + return +} From 9fbc38a773ae59d7b437c72cafecaa68b6cdc3ae Mon Sep 17 00:00:00 2001 From: onee-only Date: Thu, 19 Jun 2025 14:14:26 +0900 Subject: [PATCH 6/6] plumbing: transport, Ensure goroutines are finished in test --- plumbing/transport/http/common_test.go | 8 +++++++- plumbing/transport/http/proxy_test.go | 18 ++++++++++++------ plumbing/transport/ssh/proxy_test.go | 7 +++++++ plumbing/transport/ssh/upload_pack_test.go | 13 +++++++++++-- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go index 185053285..1ed364327 100644 --- a/plumbing/transport/http/common_test.go +++ b/plumbing/transport/http/common_test.go @@ -253,11 +253,17 @@ func setupServer(t testing.TB, smart bool) (base string, port int) { } } - t.Cleanup(func() { require.NoError(t, server.Close()) }) + done := make(chan struct{}) go func() { + defer func() { close(done) }() require.ErrorIs(t, server.Serve(l), http.ErrServerClosed) }() + t.Cleanup(func() { + require.NoError(t, server.Close()) + <-done + }) + return } diff --git a/plumbing/transport/http/proxy_test.go b/plumbing/transport/http/proxy_test.go index d6816767a..88ca2d089 100644 --- a/plumbing/transport/http/proxy_test.go +++ b/plumbing/transport/http/proxy_test.go @@ -41,8 +41,7 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTP() { setupHTTPProxy(proxy, &proxiedRequests) - httpProxyAddr, proxyServer := setupProxyServer(s.T(), proxy, false, true) - defer proxyServer.Close() + httpProxyAddr := setupProxyServer(s.T(), proxy, false, true) base, port := setupServer(s.T(), true) @@ -78,8 +77,7 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { proxy := goproxy.NewProxyHttpServer() setupHTTPSProxy(proxy, &proxiedRequests) - httpsProxyAddr, tlsProxyServer := setupProxyServer(s.T(), proxy, true, true) - defer tlsProxyServer.Close() + httpsProxyAddr := setupProxyServer(s.T(), proxy, true, true) endpoint, err := transport.NewEndpoint("https://github.com/git-fixtures/basic.git") s.Require().NoError(err) @@ -113,7 +111,7 @@ func (s *ProxySuite) TestAdvertisedReferencesHTTPS() { var certs embed.FS // Make sure you close the server after the test. -func setupProxyServer(t testing.TB, handler http.Handler, isTls, schemaAddr bool) (string, *http.Server) { +func setupProxyServer(t testing.TB, handler http.Handler, isTls, schemaAddr bool) string { schema := "http" if isTls { schema = "https" @@ -157,7 +155,10 @@ func setupProxyServer(t testing.TB, handler http.Handler, isTls, schemaAddr bool proxyServer.TLSConfig = cfg } + done := make(chan struct{}) + go func() { + defer func() { close(done) }() var err error if isTls { err = proxyServer.ServeTLS(httpListener, "", "") @@ -168,7 +169,12 @@ func setupProxyServer(t testing.TB, handler http.Handler, isTls, schemaAddr bool require.ErrorIs(t, err, http.ErrServerClosed) }() - return httpProxyAddr, &proxyServer + t.Cleanup(func() { + require.NoError(t, proxyServer.Close()) + <-done + }) + + return httpProxyAddr } func setupHTTPProxy(proxy *goproxy.ProxyHttpServer, proxiedRequests *int32) { diff --git a/plumbing/transport/ssh/proxy_test.go b/plumbing/transport/ssh/proxy_test.go index 99226573c..13e997716 100644 --- a/plumbing/transport/ssh/proxy_test.go +++ b/plumbing/transport/ssh/proxy_test.go @@ -44,10 +44,17 @@ func (s *ProxySuite) TestCommand() { }) s.Require().NoError(err) + done := make(chan struct{}) go func() { + defer func() { close(done) }() s.Require().ErrorIs(socksServer.Serve(socksListener), net.ErrClosed) }() + defer func() { + s.Require().NoError(socksListener.Close()) + <-done + }() + socksProxyAddr := fmt.Sprintf("socks5://localhost:%d", socksListener.Addr().(*net.TCPAddr).Port) base, port, _ := setupTest(s.T()) diff --git a/plumbing/transport/ssh/upload_pack_test.go b/plumbing/transport/ssh/upload_pack_test.go index 4da14f524..6958e1984 100644 --- a/plumbing/transport/ssh/upload_pack_test.go +++ b/plumbing/transport/ssh/upload_pack_test.go @@ -87,11 +87,20 @@ func startServer(t testing.TB, opts ...ssh.Option) *net.TCPAddr { opt(server) } + done := make(chan struct{}) + go func() { - require.ErrorIs(t, server.Serve(l), ssh.ErrServerClosed) + defer func() { close(done) }() + require.ErrorIs(t, server.Serve(l), net.ErrClosed) }() - t.Cleanup(func() { require.NoError(t, server.Close()) }) + t.Cleanup(func() { + // server.Serve(l) tracks the given listener, and server.Close() closes all tracked listeners. + // If the test finishes too early and calls server.Close() before the listener is tracked, + // server.Serve() may hang. Therefore, we should close the listener directly. + require.NoError(t, l.Close()) + <-done + }) return l.Addr().(*net.TCPAddr) }