From 5ad72db3bd60351da9ab42727952654e3ad5bcb2 Mon Sep 17 00:00:00 2001
From: "matej.risek" <matej.risek@hashicorp.com>
Date: Wed, 12 Jul 2023 14:40:09 +0200
Subject: plumbing: Do not swallow http message coming from VCS providers.

For diagnostics reasons we want to surface error messages coming from VCS providers.
That's why we introduce the reason field to Err struct in http package.
This field can be used by an end user of the library in order to better understand failures.
---
 plumbing/transport/http/common.go       | 18 ++++++++++++++++--
 plumbing/transport/http/common_test.go  | 19 +++++++++++++++++++
 plumbing/transport/http/receive_pack.go |  1 -
 plumbing/transport/http/upload_pack.go  |  1 -
 4 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/plumbing/transport/http/common.go b/plumbing/transport/http/common.go
index a7cdc1e..54126fe 100644
--- a/plumbing/transport/http/common.go
+++ b/plumbing/transport/http/common.go
@@ -406,14 +406,28 @@ func (a *TokenAuth) String() string {
 // Err is a dedicated error to return errors based on status code
 type Err struct {
 	Response *http.Response
+	Reason   string
 }
 
-// NewErr returns a new Err based on a http response
+// NewErr returns a new Err based on a http response and closes response body
+// if needed
 func NewErr(r *http.Response) error {
 	if r.StatusCode >= http.StatusOK && r.StatusCode < http.StatusMultipleChoices {
 		return nil
 	}
 
+	var reason string
+
+	// If a response message is present, add it to error
+	var messageBuffer bytes.Buffer
+	if r.Body != nil {
+		messageLength, _ := messageBuffer.ReadFrom(r.Body)
+		if messageLength > 0 {
+			reason = messageBuffer.String()
+		}
+		_ = r.Body.Close()
+	}
+
 	switch r.StatusCode {
 	case http.StatusUnauthorized:
 		return transport.ErrAuthenticationRequired
@@ -423,7 +437,7 @@ func NewErr(r *http.Response) error {
 		return transport.ErrRepositoryNotFound
 	}
 
-	return plumbing.NewUnexpectedError(&Err{r})
+	return plumbing.NewUnexpectedError(&Err{r, reason})
 }
 
 // StatusCode returns the status code of the response
diff --git a/plumbing/transport/http/common_test.go b/plumbing/transport/http/common_test.go
index 1517228..6bd018b 100644
--- a/plumbing/transport/http/common_test.go
+++ b/plumbing/transport/http/common_test.go
@@ -3,6 +3,7 @@ package http
 import (
 	"crypto/tls"
 	"fmt"
+	"io"
 	"log"
 	"net"
 	"net/http"
@@ -14,6 +15,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/transport"
 
 	fixtures "github.com/go-git/go-git-fixtures/v4"
@@ -90,6 +92,23 @@ func (s *ClientSuite) TestNewHTTPError40x(c *C) {
 		"unexpected client error.*")
 }
 
+func (s *ClientSuite) TestNewUnexpectedError(c *C) {
+	res := &http.Response{
+		StatusCode: 500,
+		Body:       io.NopCloser(strings.NewReader("Unexpected error")),
+	}
+
+	err := NewErr(res)
+	c.Assert(err, NotNil)
+	c.Assert(err, FitsTypeOf, &plumbing.UnexpectedError{})
+
+	unexpectedError, _ := err.(*plumbing.UnexpectedError)
+	c.Assert(unexpectedError.Err, FitsTypeOf, &Err{})
+
+	httpError, _ := unexpectedError.Err.(*Err)
+	c.Assert(httpError.Reason, Equals, "Unexpected error")
+}
+
 func (s *ClientSuite) Test_newSession(c *C) {
 	cl := NewClientWithOptions(nil, &ClientOptions{
 		CacheMaxEntries: 2,
diff --git a/plumbing/transport/http/receive_pack.go b/plumbing/transport/http/receive_pack.go
index 4387ecf..3e736cd 100644
--- a/plumbing/transport/http/receive_pack.go
+++ b/plumbing/transport/http/receive_pack.go
@@ -102,7 +102,6 @@ func (s *rpSession) doRequest(
 	}
 
 	if err := NewErr(res); err != nil {
-		_ = res.Body.Close()
 		return nil, err
 	}
 
diff --git a/plumbing/transport/http/upload_pack.go b/plumbing/transport/http/upload_pack.go
index 4f85145..3432618 100644
--- a/plumbing/transport/http/upload_pack.go
+++ b/plumbing/transport/http/upload_pack.go
@@ -100,7 +100,6 @@ func (s *upSession) doRequest(
 	}
 
 	if err := NewErr(res); err != nil {
-		_ = res.Body.Close()
 		return nil, err
 	}
 
-- 
cgit