Merge branch 'main' of gitlab.com:gitlab-community/gitlab-shell into 797-console-lint

internal/command/shared/disallowedcommand/disallowedcommand.go

+// Package disallowedcommand provides an error for handling disallowed commands.
 package disallowedcommand
 
 import "errors"
 
 var (
-	Error = errors.New("Disallowed command")
+	// Error is returned when a disallowed command is encountered.
+	Error = errors.New("Disallowed command") //nolint:stylecheck // Used to display the error message to the user.
 )

internal/command/uploadarchive/gitalycall_test.go

@@ -21,7 +21,7 @@ func TestUploadArchive(t *testing.T) {
 	for _, network := range []string{"unix", "tcp", "dns"} {
 		t.Run(fmt.Sprintf("via %s network", network), func(t *testing.T) {
 			gitalyAddress, testServer := testserver.StartGitalyServer(t, network)
-			t.Log(fmt.Sprintf("Server address: %s", gitalyAddress))
+			t.Logf("Server address: %s", gitalyAddress)
 
 			requests := requesthandlers.BuildAllowedWithGitalyHandlers(t, gitalyAddress)
 			url := testserver.StartHTTPServer(t, requests)
@@ -29,7 +29,7 @@ func TestUploadArchive(t *testing.T) {
 			output := &bytes.Buffer{}
 			input := &bytes.Buffer{}
 
-			userId := "1"
+			userID := "1"
 			repo := "group/repo"
 
 			env := sshenv.Env{
@@ -39,7 +39,7 @@ func TestUploadArchive(t *testing.T) {
 			}
 
 			args := &commandargs.Shell{
-				GitlabKeyId: userId,
+				GitlabKeyId: userID,
 				CommandType: commandargs.UploadArchive,
 				SshArgs:     []string{"git-upload-archive", repo},
 				Env:         env,

internal/gitlabnet/git/client_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	httpclient "gitlab.com/gitlab-org/gitlab-shell/v14/client"
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -16,6 +17,7 @@ var customHeaders = map[string]string{
 	"Authorization": "Bearer: token",
 	"Header-One":    "Value-Two",
 }
+var refsBody = "0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n"
 
 func TestInfoRefs(t *testing.T) {
 	client := setup(t)
@@ -53,7 +55,6 @@ func TestReceivePack(t *testing.T) {
 func TestUploadPack(t *testing.T) {
 	client := setup(t)
 
-	refsBody := "0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n"
 	response, err := client.UploadPack(context.Background(), bytes.NewReader([]byte(refsBody)))
 	require.NoError(t, err)
 	defer response.Body.Close()
@@ -67,7 +68,6 @@ func TestUploadPack(t *testing.T) {
 func TestSSHUploadPack(t *testing.T) {
 	client := setup(t)
 
-	refsBody := "0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n"
 	response, err := client.SSHUploadPack(context.Background(), bytes.NewReader([]byte(refsBody)))
 	require.NoError(t, err)
 	defer response.Body.Close()
@@ -81,7 +81,6 @@ func TestSSHUploadPack(t *testing.T) {
 func TestSSHReceivePack(t *testing.T) {
 	client := setup(t)
 
-	refsBody := "0032want 0a53e9ddeaddad63ad106860237bbf53411d11a7\n"
 	response, err := client.SSHReceivePack(context.Background(), bytes.NewReader([]byte(refsBody)))
 	require.NoError(t, err)
 	defer response.Body.Close()
@@ -157,8 +156,8 @@ func setup(t *testing.T) *Client {
 		{
 			Path: "/info/refs",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
-				require.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
+				assert.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
+				assert.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
 
 				w.Write([]byte(r.URL.Query().Get("service")))
 			},
@@ -166,13 +165,13 @@ func setup(t *testing.T) *Client {
 		{
 			Path: "/git-receive-pack",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
-				require.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
-				require.Equal(t, "application/x-git-receive-pack-request", r.Header.Get("Content-Type"))
-				require.Equal(t, "application/x-git-receive-pack-result", r.Header.Get("Accept"))
+				assert.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
+				assert.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
+				assert.Equal(t, "application/x-git-receive-pack-request", r.Header.Get("Content-Type"))
+				assert.Equal(t, "application/x-git-receive-pack-result", r.Header.Get("Accept"))
 
 				body, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
 				w.Write([]byte("git-receive-pack: "))
@@ -182,13 +181,13 @@ func setup(t *testing.T) *Client {
 		{
 			Path: "/git-upload-pack",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
-				require.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
-				require.Equal(t, "application/x-git-upload-pack-request", r.Header.Get("Content-Type"))
-				require.Equal(t, "application/x-git-upload-pack-result", r.Header.Get("Accept"))
+				assert.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
+				assert.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
+				assert.Equal(t, "application/x-git-upload-pack-request", r.Header.Get("Content-Type"))
+				assert.Equal(t, "application/x-git-upload-pack-result", r.Header.Get("Accept"))
 
 				_, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
 				w.Write([]byte("git-upload-pack: content"))
@@ -197,11 +196,11 @@ func setup(t *testing.T) *Client {
 		{
 			Path: sshUploadPackPath,
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
-				require.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
+				assert.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
+				assert.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
 
 				_, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
 				w.Write([]byte("ssh-upload-pack: content"))
@@ -210,11 +209,11 @@ func setup(t *testing.T) *Client {
 		{
 			Path: sshReceivePackPath,
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
-				require.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
+				assert.Equal(t, customHeaders["Authorization"], r.Header.Get("Authorization"))
+				assert.Equal(t, customHeaders["Header-One"], r.Header.Get("Header-One"))
 
 				_, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
 				w.Write([]byte("ssh-receive-pack: content"))

internal/gitlabnet/lfsauthenticate/client_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -26,10 +27,10 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				b, err := io.ReadAll(r.Body)
 				defer r.Body.Close()
-				require.NoError(t, err)
+				assert.NoError(t, err)
 
 				var request *Request
-				require.NoError(t, json.Unmarshal(b, &request))
+				assert.NoError(t, json.Unmarshal(b, &request))
 
 				switch request.KeyID {
 				case keyID:
@@ -39,7 +40,7 @@ func setup(t *testing.T) []testserver.TestRequestHandler {
 						"repository_http_path": "https://gitlab.com/repo/path",
 						"expires_in":           1800,
 					}
-					require.NoError(t, json.NewEncoder(w).Encode(body))
+					assert.NoError(t, json.NewEncoder(w).Encode(body))
 				case "forbidden":
 					w.WriteHeader(http.StatusForbidden)
 				case "broken":
@@ -88,7 +89,7 @@ func TestFailedRequests(t *testing.T) {
 			_, err = client.Authenticate(context.Background(), operation, repo, "")
 			require.Error(t, err)
 
-			require.Equal(t, tc.expectedOutput, err.Error())
+			assert.Equal(t, tc.expectedOutput, err.Error())
 		})
 	}
 }
@@ -128,7 +129,7 @@ func TestSuccessfulRequests(t *testing.T) {
 				ExpiresIn: 1800,
 			}
 
-			require.Equal(t, expectedResponse, response)
+			assert.Equal(t, expectedResponse, response)
 		})
 	}
 }

internal/gitlabnet/lfstransfer/client.go

+// Package lfstransfer provides functionality for handling LFS (Large File Storage) transfers.
 package lfstransfer
 
 import (
@@ -17,6 +18,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/gitlabnet"
 )
 
+// Client holds configuration, arguments, and authentication details for the client.
 type Client struct {
 	config *config.Config
 	args   *commandargs.Shell
@@ -25,6 +27,7 @@ type Client struct {
 	header string
 }
 
+// BatchAction represents an action for a batch operation with metadata.
 type BatchAction struct {
 	Href      string            `json:"href"`
 	Header    map[string]string `json:"header,omitempty"`
@@ -32,6 +35,7 @@ type BatchAction struct {
 	ExpiresIn int               `json:"expires_in,omitempty"`
 }
 
+// BatchObject represents an object in a batch operation with its metadata and actions.
 type BatchObject struct {
 	Oid           string                  `json:"oid,omitempty"`
 	Size          int64                   `json:"size"`
@@ -50,6 +54,7 @@ type batchRequest struct {
 	HashAlgorithm string         `json:"hash_algo,omitempty"`
 }
 
+// BatchResponse contains batch operation results and the hash algorithm used.
 type BatchResponse struct {
 	Objects       []*BatchObject `json:"objects"`
 	HashAlgorithm string         `json:"hash_algo,omitempty"`
@@ -79,10 +84,12 @@ type listLocksVerifyRequest struct {
 	Ref    *batchRef `json:"ref,omitempty"`
 }
 
+// LockOwner represents the owner of a lock.
 type LockOwner struct {
 	Name string `json:"name"`
 }
 
+// Lock represents a lock with its ID, path, timestamp, and owner details.
 type Lock struct {
 	ID       string     `json:"id"`
 	Path     string     `json:"path"`
@@ -90,19 +97,23 @@ type Lock struct {
 	Owner    *LockOwner `json:"owner"`
 }
 
+// ListLocksResponse contains a list of locks and a cursor for pagination.
 type ListLocksResponse struct {
 	Locks      []*Lock `json:"locks,omitempty"`
 	NextCursor string  `json:"next_cursor,omitempty"`
 }
 
+// ListLocksVerifyResponse provides lists of locks for "ours" and "theirs" with a cursor for pagination.
 type ListLocksVerifyResponse struct {
 	Ours       []*Lock `json:"ours,omitempty"`
 	Theirs     []*Lock `json:"theirs,omitempty"`
 	NextCursor string  `json:"next_cursor,omitempty"`
 }
 
+// ClientHeader specifies the content type for Git LFS JSON requests.
 var ClientHeader = "application/vnd.git-lfs+json"
 
+// NewClient creates a new Client instance using the provided configuration and credentials.
 func NewClient(config *config.Config, args *commandargs.Shell, href string, auth string) (*Client, error) {
 	return &Client{config: config, args: args, href: href, auth: auth, header: ClientHeader}, nil
 }
@@ -121,6 +132,7 @@ func newHTTPClient() *retryablehttp.Client {
 	return client
 }
 
+// Batch performs a batch operation on objects and returns the result.
 func (c *Client) Batch(operation string, reqObjects []*BatchObject, ref string, reqHashAlgo string) (*BatchResponse, error) {
 	// FIXME: This causes tests to fail
 	// if ref == "" {
@@ -211,6 +223,7 @@ func (c *Client) PutObject(_, href string, headers map[string]string, r io.Reade
 	return nil
 }
 
+// Lock acquires a lock for the specified path with an optional reference name.
 func (c *Client) Lock(path, refname string) (*Lock, error) {
 	var ref *batchRef
 	if refname != "" {
@@ -268,6 +281,7 @@ func (c *Client) Lock(path, refname string) (*Lock, error) {
 	}
 }
 
+// Unlock releases the lock with the given id, optionally forcing the unlock.
 func (c *Client) Unlock(id string, force bool, refname string) (*Lock, error) {
 	var ref *batchRef
 	if refname != "" {
@@ -318,6 +332,7 @@ func (c *Client) Unlock(id string, force bool, refname string) (*Lock, error) {
 	}
 }
 
+// ListLocksVerify retrieves locks for the given path and id, with optional pagination.
 func (c *Client) ListLocksVerify(path, id, cursor string, limit int, ref string) (*ListLocksVerifyResponse, error) {
 	url, err := url.Parse(c.href)
 	if err != nil {

internal/gitlabnet/personalaccesstoken/client_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client"
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -27,7 +28,7 @@ func initialize(t *testing.T) {
 				b, err := io.ReadAll(r.Body)
 				defer r.Body.Close()
 
-				require.NoError(t, err)
+				assert.NoError(t, err)
 
 				var requestBody *RequestBody
 				json.Unmarshal(b, &requestBody)

internal/logger/logger.go

+// Package logger provides logging configuration utilities for the gitlab-shell
 package logger
 
 import (
@@ -52,7 +53,7 @@ func buildOpts(cfg *config.Config) []log.LoggerOption {
 // opened for writing.
 func Configure(cfg *config.Config) io.Closer {
 	var closer io.Closer = io.NopCloser(nil)
-	err := fmt.Errorf("No logfile specified")
+	err := fmt.Errorf("no logfile specified")
 
 	if cfg.LogFile != "" {
 		closer, err = log.Initialize(buildOpts(cfg)...)
@@ -66,7 +67,7 @@ func Configure(cfg *config.Config) io.Closer {
 			syslogLogger.Print(msg)
 		} else {
 			msg := fmt.Sprintf("%s: Unable to configure logging: %v, %v\n", progName, err.Error(), syslogLoggerErr.Error())
-			fmt.Fprintf(os.Stderr, msg)
+			fmt.Fprintln(os.Stderr, msg)
 		}
 
 		cfg.LogFile = "/dev/null"

internal/metrics/metrics.go

+// Package metrics provides Prometheus metrics for monitoring gitlab-shell components.
 package metrics
 
 import (
@@ -34,6 +35,7 @@ const (
 )
 
 var (
+	// SshdSessionDuration is a histogram of latencies for connections to gitlab-shell sshd.
 	SshdSessionDuration = promauto.NewHistogram(
 		prometheus.HistogramOpts{
 			Namespace: namespace,
@@ -48,6 +50,7 @@ var (
 		},
 	)
 
+	// SshdSessionEstablishedDuration is a histogram of latencies until session established to gitlab-shell sshd.
 	SshdSessionEstablishedDuration = promauto.NewHistogram(
 		prometheus.HistogramOpts{
 			Namespace: namespace,
@@ -62,6 +65,7 @@ var (
 		},
 	)
 
+	// SshdConnectionsInFlight is a gauge of connections currently being served by gitlab-shell sshd.
 	SshdConnectionsInFlight = promauto.NewGauge(
 		prometheus.GaugeOpts{
 			Namespace: namespace,
@@ -71,6 +75,7 @@ var (
 		},
 	)
 
+	// SshdHitMaxSessions is the number of times the concurrent sessions limit was hit in gitlab-shell sshd.
 	SshdHitMaxSessions = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Namespace: namespace,
@@ -80,6 +85,7 @@ var (
 		},
 	)
 
+	// SliSshdSessionsTotal is the number of SSH sessions that have been established.
 	SliSshdSessionsTotal = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Name: sliSshdSessionsTotalName,
@@ -87,6 +93,7 @@ var (
 		},
 	)
 
+	// SliSshdSessionsErrorsTotal is the number of SSH sessions that have failed.
 	SliSshdSessionsErrorsTotal = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Name: sliSshdSessionsErrorsTotalName,
@@ -94,6 +101,7 @@ var (
 		},
 	)
 
+	// GitalyConnectionsTotal is a counter for the number of Gitaly connections that have been established,
 	GitalyConnectionsTotal = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Namespace: namespace,
@@ -147,6 +155,7 @@ var (
 		},
 	)
 
+	// LfsHTTPConnectionsTotal is the number of LFS over HTTP connections that have been established.
 	LfsHTTPConnectionsTotal = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Name: lfsHTTPConnectionsTotalName,
@@ -154,6 +163,7 @@ var (
 		},
 	)
 
+	// LfsSSHConnectionsTotal is the number of LFS over SSH connections that have been established.
 	LfsSSHConnectionsTotal = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Name: lfsSSHConnectionsTotalName,
@@ -162,6 +172,7 @@ var (
 	)
 )
 
+// NewRoundTripper wraps an http.RoundTripper to instrument it with Prometheus metrics.
 func NewRoundTripper(next http.RoundTripper) promhttp.RoundTripperFunc {
 	rt := next
 

internal/pktline/pktline.go

+// Package pktline provides utility functions for working with the Git pkt-line format.
 package pktline
 
 // Utility functions for working with the Git pkt-line format. See
@@ -27,6 +28,7 @@ func NewScanner(r io.Reader) *bufio.Scanner {
 	return scanner
 }
 
+// IsRefRemoval checks if the packet represents a reference removal.
 func IsRefRemoval(pkt []byte) bool {
 	return branchRemovalPktRegexp.Match(pkt)
 }

internal/sshd/connection.go

@@ -168,7 +168,15 @@ func (c *connection) sendKeepAliveMsg(ctx context.Context, sconn *ssh.ServerConn
 		case <-ticker.C:
 			ctxlog.Debug("connection: sendKeepAliveMsg: send keepalive message to a client")
 
-			_, _, _ = sconn.SendRequest(KeepAliveMsg, true, nil)
+			status, payload, err := sconn.SendRequest(KeepAliveMsg, true, nil)
+			if err != nil {
+				ctxlog.Errorf("Error occurred while sending request :%v", err)
+				return
+			}
+
+			if status {
+				ctxlog.Debugf("connection: sendKeepAliveMsg: payload: %v", string(payload))
+			}
 		}
 	}
 }

internal/sshd/connection_test.go

@@ -78,7 +78,7 @@ func (f *fakeConn) SendRequest(name string, _ bool, _ []byte) (bool, []byte, err
 
 	f.sentRequestName = name
 
-	return true, nil, nil
+	return true, []byte("I am a response"), nil
 }
 
 func setup(newChannel *fakeNewChannel) (*connection, chan ssh.NewChannel) {

internal/sshd/gssapi_unsupported.go

@@ -8,6 +8,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/config"
 )
 
+// NewGSSAPIServer initializes and returns a new OSGSSAPIServer.
 func NewGSSAPIServer(c *config.GSSAPIConfig) (*OSGSSAPIServer, error) {
 	s := &OSGSSAPIServer{
 		ServicePrincipalName: c.ServicePrincipalName,
@@ -16,18 +17,22 @@ func NewGSSAPIServer(c *config.GSSAPIConfig) (*OSGSSAPIServer, error) {
 	return s, nil
 }
 
+// OSGSSAPIServer represents a server that handles GSSAPI requests.
 type OSGSSAPIServer struct {
 	ServicePrincipalName string
 }
 
+// AcceptSecContext returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) AcceptSecContext([]byte) ([]byte, string, bool, error) {
 	return []byte{}, "", false, errors.New("gssapi is unsupported")
 }
 
+// VerifyMIC returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) VerifyMIC([]byte, []byte) error {
 	return errors.New("gssapi is unsupported")
 }
 
+// DeleteSecContext returns an error indicating that GSSAPI is unsupported.
 func (*OSGSSAPIServer) DeleteSecContext() error {
 	return errors.New("gssapi is unsupported")
 }

internal/sshd/server_config_test.go

@@ -60,8 +60,9 @@ func TestHostKeyAndCerts(t *testing.T) {
 	data, err := os.ReadFile(path.Join(testRoot, "certs/valid/server.pub"))
 	require.NoError(t, err)
 
-	publicKey, _, _, _, err := ssh.ParseAuthorizedKey(data)
+	publicKey, comment, _, _, err := ssh.ParseAuthorizedKey(data)
 	require.NoError(t, err)
+	require.NotNil(t, comment)
 	require.NotNil(t, publicKey)
 	cert, ok := cfg.hostKeyToCertMap[string(publicKey.Marshal())]
 	require.True(t, ok)

internal/sshd/sshd_test.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/pires/go-proxyproto"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
 
@@ -409,16 +410,16 @@ func setupServerWithContext(ctx context.Context, t *testing.T, cfg *config.Confi
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				correlationID = r.Header.Get("X-Request-Id")
 
-				require.NotEmpty(t, correlationID)
-				require.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
+				assert.NotEmpty(t, correlationID)
+				assert.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
 
 				fmt.Fprint(w, `{"id": 1000, "key": "key"}`)
 			},
 		}, {
 			Path: "/api/v4/internal/discover",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, correlationID, r.Header.Get("X-Request-Id"))
-				require.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
+				assert.Equal(t, correlationID, r.Header.Get("X-Request-Id"))
+				assert.Equal(t, xForwardedFor, r.Header.Get("X-Forwarded-For"))
 
 				fmt.Fprint(w, `{"id": 1000, "name": "Test User", "username": "test-user"}`)
 			},