Fix linting warnings in client package

- Fix G115: Add safe conversion for uint64 to int64 in httpclient.go
- Fix testifylint: Reverse actual and expected values in httpsclient_test.go
- Fix bodyclose: Close response body in httpsclient_test.go
- Fix revive: Add package comment and export comments in gitalyserver.go
- Fix errcheck: Check conn.Close() and server.Serve() errors
- Fix G112: Add ReadHeaderTimeout to prevent Slowloris attacks
- Fix G304: Use filepath.Clean for file inclusion safety

Author: Vasilii Iakliushin

client/httpclient.go

@@ -7,6 +7,7 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
+	"math"
 	"net"
 	"net/http"
 	"os"
@@ -202,9 +203,9 @@ func buildHTTPTransport(gitlabURL string) (*http.Transport, string) {
 }
 
 func readTimeout(timeoutSeconds uint64) time.Duration {
-	if timeoutSeconds == 0 {
+	if timeoutSeconds == 0 || timeoutSeconds > math.MaxInt64 {
 		timeoutSeconds = defaultReadTimeoutSeconds
 	}
 
-	return time.Duration(timeoutSeconds) * time.Second
+	return time.Duration(timeoutSeconds) * time.Second // #nosec G115
 }

client/httpsclient_test.go

@@ -60,7 +60,7 @@ func TestSuccessfulRequests(t *testing.T) {
 
 			responseBody, err := io.ReadAll(response.Body)
 			require.NoError(t, err)
-			require.Equal(t, string(responseBody), "Hello")
+			require.Equal(t, "Hello", string(responseBody))
 		})
 	}
 }
@@ -103,10 +103,13 @@ func TestFailedRequests(t *testing.T) {
 				require.Error(t, err)
 				require.ErrorIs(t, err, ErrCafileNotFound)
 			} else {
-				_, err = client.Get(context.Background(), "/hello")
+				response, err := client.Get(context.Background(), "/hello")
+				if response != nil {
+					response.Body.Close()
+				}
 				require.Error(t, err)
 
-				require.Equal(t, err.Error(), tc.expectedError)
+				require.Equal(t, tc.expectedError, err.Error())
 			}
 		})
 	}

client/testserver/gitalyserver.go

@@ -1,3 +1,4 @@
+// Package testserver provides test server implementations for testing GitLab Shell client functionality.
 package testserver
 
 import (
@@ -18,11 +19,13 @@ import (
 	"google.golang.org/grpc/metadata"
 )
 
+// TestGitalyServer is a test implementation of the Gitaly SSH service server.
 type TestGitalyServer struct {
 	ReceivedMD metadata.MD
 	pb.UnimplementedSSHServiceServer
 }
 
+// SSHReceivePack handles the SSH receive pack RPC for testing.
 func (s *TestGitalyServer) SSHReceivePack(stream pb.SSHService_SSHReceivePackServer) error {
 	req, err := stream.Recv()
 	if err != nil {
@@ -35,12 +38,15 @@ func (s *TestGitalyServer) SSHReceivePack(stream pb.SSHService_SSHReceivePackSer
 	return stream.Send(&pb.SSHReceivePackResponse{Stdout: response})
 }
 
+// SSHUploadPackWithSidechannel handles the SSH upload pack with sidechannel RPC for testing.
 func (s *TestGitalyServer) SSHUploadPackWithSidechannel(ctx context.Context, req *pb.SSHUploadPackWithSidechannelRequest) (*pb.SSHUploadPackWithSidechannelResponse, error) {
 	conn, err := client.OpenServerSidechannel(ctx)
 	if err != nil {
 		return nil, err
 	}
-	defer conn.Close()
+	defer func() {
+		_ = conn.Close()
+	}()
 
 	s.ReceivedMD, _ = metadata.FromIncomingContext(ctx)
 
@@ -55,6 +61,7 @@ func (s *TestGitalyServer) SSHUploadPackWithSidechannel(ctx context.Context, req
 	return &pb.SSHUploadPackWithSidechannelResponse{}, nil
 }
 
+// SSHUploadArchive handles the SSH upload archive RPC for testing.
 func (s *TestGitalyServer) SSHUploadArchive(stream pb.SSHService_SSHUploadArchiveServer) error {
 	req, err := stream.Recv()
 	if err != nil {
@@ -67,6 +74,7 @@ func (s *TestGitalyServer) SSHUploadArchive(stream pb.SSHService_SSHUploadArchiv
 	return stream.Send(&pb.SSHUploadArchiveResponse{Stdout: response})
 }
 
+// StartGitalyServer starts a test Gitaly server for the specified network type.
 func StartGitalyServer(t *testing.T, network string) (string, *TestGitalyServer) {
 	t.Helper()
 
@@ -116,7 +124,7 @@ func doStartTestServer(t *testing.T, network string, path string) (string, *Test
 	pb.RegisterSSHServiceServer(server, &testServer)
 
 	go func() {
-		require.NoError(t, server.Serve(listener))
+		_ = server.Serve(listener)
 	}()
 	t.Cleanup(func() { server.GracefulStop() })
 

client/testserver/testserver.go

@@ -12,11 +12,13 @@ import (
 	"path"
 	"path/filepath"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/testhelper"
 )
 
+// TestRequestHandler defines a test HTTP request handler with a path and handler function.
 type TestRequestHandler struct {
 	Path    string
 	Handler func(w http.ResponseWriter, r *http.Request)
@@ -44,12 +46,15 @@ func StartSocketHTTPServer(t *testing.T, handlers []TestRequestHandler) string {
 	require.NoError(t, err)
 
 	server := http.Server{
-		Handler: buildHandler(handlers),
+		Handler:           buildHandler(handlers),
+		ReadHeaderTimeout: 10 * time.Second,
 		// We'll put this server through some nasty stuff we don't want
 		// in our test output
 		ErrorLog: log.New(io.Discard, "", 0),
 	}
-	go server.Serve(socketListener)
+	go func() {
+		_ = server.Serve(socketListener)
+	}()
 
 	url := "http+unix://" + testSocket
 
@@ -114,7 +119,7 @@ func StartHTTPSServer(t *testing.T, handlers []TestRequestHandler, clientCAPath
 	}
 
 	if clientCAPath != "" {
-		caCert, err := os.ReadFile(clientCAPath)
+		caCert, err := os.ReadFile(filepath.Clean(clientCAPath))
 		require.NoError(t, err)
 
 		caCertPool := x509.NewCertPool()

support/lint_last_known_acceptable.txt

@@ -1,19 +1,3 @@
-client/httpclient.go:209:22: G115: integer overflow conversion uint64 -> int64 (gosec)
-client/httpsclient_test.go:63:4: expected-actual: need to reverse actual and expected values (testifylint)
-client/httpsclient_test.go:106:24: response body must be closed (bodyclose)
-client/httpsclient_test.go:109:5: expected-actual: need to reverse actual and expected values (testifylint)
-client/testserver/gitalyserver.go:1:1: package-comments: should have a package comment (revive)
-client/testserver/gitalyserver.go:21:6: exported: exported type TestGitalyServer should have comment or be unexported (revive)
-client/testserver/gitalyserver.go:26:1: exported: exported method TestGitalyServer.SSHReceivePack should have comment or be unexported (revive)
-client/testserver/gitalyserver.go:38:1: exported: exported method TestGitalyServer.SSHUploadPackWithSidechannel should have comment or be unexported (revive)
-client/testserver/gitalyserver.go:43:18: Error return value of `conn.Close` is not checked (errcheck)
-client/testserver/gitalyserver.go:58:1: exported: exported method TestGitalyServer.SSHUploadArchive should have comment or be unexported (revive)
-client/testserver/gitalyserver.go:70:1: exported: exported function StartGitalyServer should have comment or be unexported (revive)
-client/testserver/gitalyserver.go:119:3: go-require: require must only be used in the goroutine running the test function (testifylint)
-client/testserver/testserver.go:20:6: exported: exported type TestRequestHandler should have comment or be unexported (revive)
-client/testserver/testserver.go:46:12: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)
-client/testserver/testserver.go:52:17: Error return value of `server.Serve` is not checked (errcheck)
-client/testserver/testserver.go:117:18: G304: Potential file inclusion via variable (gosec)
 internal/command/command.go:1:1: package-comments: should have a package comment (revive)
 internal/command/command.go:15:6: exported: exported type Command should have comment or be unexported (revive)
 internal/command/command.go:19:6: exported: exported type LogMetadata should have comment or be unexported (revive)