Merge branch '787-githttp-lint' into 'main'

Lint fixes for githttp package

Closes #787

See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/1144

Merged-by: Igor Drozdov <[email protected]>
Approved-by: Igor Drozdov <[email protected]>
Reviewed-by: Igor Drozdov <[email protected]>
Reviewed-by: Ash McKenzie <[email protected]>
Co-authored-by: Ash McKenzie <[email protected]>
Co-authored-by: Archish <[email protected]>

Author: 3 people

internal/command/githttp/pull.go

@@ -1,9 +1,11 @@
+// Package githttp provides functionality to handle Git operations over HTTP(S) and SSH,
+// including executing Git commands like git-upload-pack and converting responses to the
+// expected format for SSH protocols. It integrates with GitLab's internal components
+// for secure access verification and data transfer.
 package githttp
 
 import (
-	"bytes"
 	"context"
-	"fmt"
 	"io"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/commandargs"
@@ -17,8 +19,9 @@ import (
 
 const pullService = "git-upload-pack"
 
-var uploadPackHttpPrefix = []byte("001e# service=git-upload-pack\n0000")
+var uploadPackHTTPPrefix = []byte("001e# service=git-upload-pack\n0000")
 
+// PullCommand handles the execution of a Git pull operation over HTTP(S) or SSH
 type PullCommand struct {
 	Config     *config.Config
 	ReadWriter *readwriter.ReadWriter
@@ -36,6 +39,12 @@ type PullCommand struct {
 // 5. Perform /git-upload-pack request and send this data
 // 6. Return the output to the user
 
+// ForInfoRefs returns the necessary Pull specifics for client.InfoRefs()
+func (c *PullCommand) ForInfoRefs() (*readwriter.ReadWriter, string, []byte) {
+	return c.ReadWriter, pullService, uploadPackHTTPPrefix
+}
+
+// Execute runs the pull command by determining the appropriate method (HTTP/SSH)
 func (c *PullCommand) Execute(ctx context.Context) error {
 	data := c.Response.Payload.Data
 	client := &git.Client{URL: data.PrimaryRepo, Headers: data.RequestHeaders}
@@ -48,39 +57,19 @@ func (c *PullCommand) Execute(ctx context.Context) error {
 		return c.requestSSHUploadPack(ctx, client)
 	}
 
-	if err := c.requestInfoRefs(ctx, client); err != nil {
+	if err := requestInfoRefs(ctx, client, c); err != nil {
 		return err
 	}
 
 	return c.requestUploadPack(ctx, client, data.GeoProxyFetchDirectToPrimaryWithOptions)
 }
 
-func (c *PullCommand) requestInfoRefs(ctx context.Context, client *git.Client) error {
-	response, err := client.InfoRefs(ctx, pullService)
-	if err != nil {
-		return err
-	}
-	defer response.Body.Close()
-
-	// Read the first bytes that contain 001e# service=git-upload-pack\n0000 string
-	// to convert HTTP(S) Git response to the one expected by SSH
-	p := make([]byte, len(uploadPackHttpPrefix))
-	_, err = response.Body.Read(p)
-	if err != nil || !bytes.Equal(p, uploadPackHttpPrefix) {
-		return fmt.Errorf("Unexpected git-upload-pack response")
-	}
-
-	_, err = io.Copy(c.ReadWriter.Out, response.Body)
-
-	return err
-}
-
 func (c *PullCommand) requestSSHUploadPack(ctx context.Context, client *git.Client) error {
 	response, err := client.SSHUploadPack(ctx, io.NopCloser(c.ReadWriter.In))
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() //nolint:errcheck
 
 	_, err = io.Copy(c.ReadWriter.Out, response.Body)
 
@@ -95,7 +84,7 @@ func (c *PullCommand) requestUploadPack(ctx context.Context, client *git.Client,
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() //nolint:errcheck
 
 	_, err = io.Copy(c.ReadWriter.Out, response.Body)
 
@@ -108,18 +97,27 @@ func (c *PullCommand) readFromStdin(pw *io.PipeWriter, geoProxyFetchDirectToPrim
 	for scanner.Scan() {
 		line := scanner.Bytes()
 
-		pw.Write(line)
+		_, err := pw.Write(line)
+		if err != nil {
+			log.WithError(err).Error("failed to write line")
+		}
 
 		if pktline.IsDone(line) {
 			break
 		}
 
 		if pktline.IsFlush(line) && geoProxyFetchDirectToPrimaryWithOptions {
-			pw.Write(pktline.PktDone())
+			_, err := pw.Write(pktline.PktDone())
+			if err != nil {
+				log.WithError(err).Error("failed to write packet done line")
+			}
 
 			break
 		}
 	}
 
-	pw.Close()
+	err := pw.Close()
+	if err != nil {
+		log.WithError(err).Error("failed to close writer")
+	}
 }

internal/command/githttp/pull_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/client/testserver"
@@ -104,7 +105,7 @@ func TestPullExecuteWithFailedInfoRefs(t *testing.T) {
 			desc:            "unexpected response",
 			statusCode:      http.StatusOK,
 			responseContent: "unexpected response",
-			expectedErr:     "Unexpected git-upload-pack response",
+			expectedErr:     "unexpected git-upload-pack response",
 		},
 	}
 
@@ -114,7 +115,7 @@ func TestPullExecuteWithFailedInfoRefs(t *testing.T) {
 				{
 					Path: "/info/refs",
 					Handler: func(w http.ResponseWriter, r *http.Request) {
-						require.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
+						assert.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
 
 						w.WriteHeader(tc.statusCode)
 						w.Write([]byte(tc.responseContent))
@@ -167,7 +168,7 @@ func setupPull(t *testing.T, uploadPackStatusCode int) string {
 		{
 			Path: "/info/refs",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
-				require.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
+				assert.Equal(t, "git-upload-pack", r.URL.Query().Get("service"))
 
 				w.Write([]byte(infoRefs))
 			},
@@ -176,10 +177,10 @@ func setupPull(t *testing.T, uploadPackStatusCode int) string {
 			Path: "/git-upload-pack",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				body, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
-				require.True(t, strings.HasSuffix(string(body), "0009done\n"))
+				assert.True(t, strings.HasSuffix(string(body), "0009done\n"))
 
 				w.WriteHeader(uploadPackStatusCode)
 			},
@@ -195,12 +196,12 @@ func setupSSHPull(t *testing.T, uploadPackStatusCode int) string {
 			Path: "/ssh-upload-pack",
 			Handler: func(w http.ResponseWriter, r *http.Request) {
 				body, err := io.ReadAll(r.Body)
-				require.NoError(t, err)
+				assert.NoError(t, err)
 				defer r.Body.Close()
 
-				require.True(t, strings.HasSuffix(string(body), "0009done\n"))
-				require.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
-				require.Equal(t, "token", r.Header.Get("Authorization"))
+				assert.True(t, strings.HasSuffix(string(body), "0009done\n"))
+				assert.Equal(t, "version=2", r.Header.Get("Git-Protocol"))
+				assert.Equal(t, "token", r.Header.Get("Authorization"))
 
 				w.Write([]byte("upload-pack-response"))
 				w.WriteHeader(uploadPackStatusCode)

internal/command/githttp/push.go

@@ -1,9 +1,7 @@
 package githttp
 
 import (
-	"bytes"
 	"context"
-	"fmt"
 	"io"
 
 	"gitlab.com/gitlab-org/gitlab-shell/v14/internal/command/commandargs"
@@ -15,10 +13,12 @@ import (
 	"gitlab.com/gitlab-org/labkit/log"
 )
 
-const service = "git-receive-pack"
+const pushService = "git-receive-pack"
 
-var receivePackHttpPrefix = []byte("001f# service=git-receive-pack\n0000")
+var receivePackHTTPPrefix = []byte("001f# service=git-receive-pack\n0000")
 
+// PushCommand handles the execution of a Git push operation,
+// including configuration, input/output handling, and access verification.
 type PushCommand struct {
 	Config     *config.Config
 	ReadWriter *readwriter.ReadWriter
@@ -35,6 +35,13 @@ type PushCommand struct {
 // 4. Read the send-pack data provided by user via SSH (stdinReader)
 // 5. Perform /git-receive-pack request and send this data
 // 6. Return the output to the user
+
+// ForInfoRefs returns the necessary Push specifics for client.InfoRefs()
+func (c *PushCommand) ForInfoRefs() (*readwriter.ReadWriter, string, []byte) {
+	return c.ReadWriter, pushService, receivePackHTTPPrefix
+}
+
+// Execute runs the push command by determining the appropriate method (HTTP/SSH)
 func (c *PushCommand) Execute(ctx context.Context) error {
 	data := c.Response.Payload.Data
 	client := &git.Client{URL: data.PrimaryRepo, Headers: data.RequestHeaders}
@@ -47,39 +54,19 @@ func (c *PushCommand) Execute(ctx context.Context) error {
 		return c.requestSSHReceivePack(ctx, client)
 	}
 
-	if err := c.requestInfoRefs(ctx, client); err != nil {
+	if err := requestInfoRefs(ctx, client, c); err != nil {
 		return err
 	}
 
 	return c.requestReceivePack(ctx, client)
 }
 
-func (c *PushCommand) requestInfoRefs(ctx context.Context, client *git.Client) error {
-	response, err := client.InfoRefs(ctx, service)
-	if err != nil {
-		return err
-	}
-	defer response.Body.Close()
-
-	// Read the first bytes that contain 001f# service=git-receive-pack\n0000 string
-	// to convert HTTP(S) Git response to the one expected by SSH
-	p := make([]byte, len(receivePackHttpPrefix))
-	_, err = response.Body.Read(p)
-	if err != nil || !bytes.Equal(p, receivePackHttpPrefix) {
-		return fmt.Errorf("Unexpected git-receive-pack response")
-	}
-
-	_, err = io.Copy(c.ReadWriter.Out, response.Body)
-
-	return err
-}
-
 func (c *PushCommand) requestSSHReceivePack(ctx context.Context, client *git.Client) error {
 	response, err := client.SSHReceivePack(ctx, io.NopCloser(c.ReadWriter.In))
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() //nolint:errcheck
 
 	_, err = io.Copy(c.ReadWriter.Out, response.Body)
 
@@ -94,7 +81,7 @@ func (c *PushCommand) requestReceivePack(ctx context.Context, client *git.Client
 	if err != nil {
 		return err
 	}
-	defer response.Body.Close()
+	defer response.Body.Close() //nolint:errcheck
 
 	_, err = io.Copy(c.ReadWriter.Out, response.Body)
 
@@ -107,7 +94,10 @@ func (c *PushCommand) readFromStdin(pw *io.PipeWriter) {
 	scanner := pktline.NewScanner(c.ReadWriter.In)
 	for scanner.Scan() {
 		line := scanner.Bytes()
-		pw.Write(line)
+		_, err := pw.Write(line)
+		if err != nil {
+			log.WithError(err).Error("failed to write line")
+		}
 
 		if pktline.IsFlush(line) {
 			break
@@ -119,8 +109,14 @@ func (c *PushCommand) readFromStdin(pw *io.PipeWriter) {
 	}
 
 	if needsPackData {
-		io.Copy(pw, c.ReadWriter.In)
+		_, err := io.Copy(pw, c.ReadWriter.In)
+		if err != nil {
+			log.WithError(err).Error("failed to copy")
+		}
 	}
 
-	pw.Close()
+	err := pw.Close()
+	if err != nil {
+		log.WithError(err).Error("failed to close writer")
+	}
 }