Merge pull request #835 from matejrisek/feature/do-not-swallow-vcs-host-errors

plumbing: Do not swallow http message coming from VCS providers

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
 	}