[bugfix] Add back removed ValidateRequest() before backoff-retry loop (#1805)v0.9.0-rc2

* add back removed ValidateRequest() before backoff-retry loop

Signed-off-by: kim <grufwub@gmail.com>

* include response body in error response log

Signed-off-by: kim <grufwub@gmail.com>

* improved error response body draining

Signed-off-by: kim <grufwub@gmail.com>

* add more code commenting

Signed-off-by: kim <grufwub@gmail.com>

* move new error response logic to gtserror, handle instead in transport.Transport{} impl

Signed-off-by: kim <grufwub@gmail.com>

* appease ye oh mighty linter

Signed-off-by: kim <grufwub@gmail.com>

* fix mockhttpclient not setting request in http response

Signed-off-by: kim <grufwub@gmail.com>

---------

Signed-off-by: kim <grufwub@gmail.com>

2 files changed, 78 insertions, 5 deletions

diff --git a/internal/httpclient/client.go b/internal/httpclient/client.go
index dd1a1bd6b..efbf4cd18 100644
--- a/internal/httpclient/client.go
+++ b/internal/httpclient/client.go
@@ -41,6 +41,9 @@ import (
 )
 
 var (
+	// ErrInvalidRequest is returned if a given HTTP request is invalid and cannot be performed.
+	ErrInvalidRequest = errors.New("invalid http request")
+
 	// ErrInvalidNetwork is returned if the request would not be performed over TCP
 	ErrInvalidNetwork = errors.New("invalid network type")
 
@@ -90,6 +93,9 @@ type Config struct {
 //     cases to protect against forged / unknown content-lengths
 //   - protection from server side request forgery (SSRF) by only dialing
 //     out to known public IP prefixes, configurable with allows/blocks
+//   - retry-backoff logic for error temporary HTTP error responses
+//   - optional request signing
+//   - request logging
 type Client struct {
 	client   http.Client
 	badHosts cache.Cache[string, struct{}]
@@ -156,14 +162,14 @@ func New(cfg Config) *Client {
 	return &c
 }
 
-// Do ...
+// Do will essentially perform http.Client{}.Do() with retry-backoff functionality.
 func (c *Client) Do(r *http.Request) (*http.Response, error) {
 	return c.DoSigned(r, func(r *http.Request) error {
 		return nil // no request signing
 	})
 }
 
-// DoSigned ...
+// DoSigned will essentially perform http.Client{}.Do() with retry-backoff functionality and requesting signing..
 func (c *Client) DoSigned(r *http.Request, sign SignFunc) (rsp *http.Response, err error) {
 	const (
 		// max no. attempts.
@@ -173,6 +179,11 @@ func (c *Client) DoSigned(r *http.Request, sign SignFunc) (rsp *http.Response, e
 		baseBackoff = 2 * time.Second
 	)
 
+	// First validate incoming request.
+	if err := ValidateRequest(r); err != nil {
+		return nil, err
+	}
+
 	// Get request hostname.
 	host := r.URL.Hostname()
 
@@ -234,8 +245,8 @@ func (c *Client) DoSigned(r *http.Request, sign SignFunc) (rsp *http.Response, e
 				return rsp, nil
 			}
 
-			// Generate error from status code for logging
-			err = errors.New(`http response "` + rsp.Status + `"`)
+			// Create loggable error from response status code.
+			err = fmt.Errorf(`http response: %s`, rsp.Status)
 
 			// Search for a provided "Retry-After" header value.
 			if after := rsp.Header.Get("Retry-After"); after != "" {
@@ -307,7 +318,7 @@ func (c *Client) DoSigned(r *http.Request, sign SignFunc) (rsp *http.Response, e
 	return
 }
 
-// do ...
+// do wraps http.Client{}.Do() to provide safely limited response bodies.
 func (c *Client) do(req *http.Request) (*http.Response, error) {
 	// Perform the HTTP request.
 	rsp, err := c.client.Do(req)
diff --git a/internal/httpclient/validate.go b/internal/httpclient/validate.go
new file mode 100644
index 000000000..881d3f699
--- /dev/null
+++ b/internal/httpclient/validate.go
@@ -0,0 +1,62 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package httpclient
+
+import (
+	"fmt"
+	"net/http"
+	"strings"
+
+	"golang.org/x/net/http/httpguts"
+)
+
+// ValidateRequest performs the same request validation logic found in the default
+// net/http.Transport{}.roundTrip() function, but pulls it out into this separate
+// function allowing validation errors to be wrapped under a single error type.
+func ValidateRequest(r *http.Request) error {
+	switch {
+	case r.URL == nil:
+		return fmt.Errorf("%w: nil url", ErrInvalidRequest)
+	case r.Header == nil:
+		return fmt.Errorf("%w: nil header", ErrInvalidRequest)
+	case r.URL.Host == "":
+		return fmt.Errorf("%w: empty url host", ErrInvalidRequest)
+	case r.URL.Scheme != "http" && r.URL.Scheme != "https":
+		return fmt.Errorf("%w: unsupported protocol %q", ErrInvalidRequest, r.URL.Scheme)
+	case strings.IndexFunc(r.Method, func(r rune) bool { return !httpguts.IsTokenRune(r) }) != -1:
+		return fmt.Errorf("%w: invalid method %q", ErrInvalidRequest, r.Method)
+	}
+
+	for key, values := range r.Header {
+		// Check field key name is valid
+		if !httpguts.ValidHeaderFieldName(key) {
+			return fmt.Errorf("%w: invalid header field name %q", ErrInvalidRequest, key)
+		}
+
+		// Check each field value is valid
+		for i := 0; i < len(values); i++ {
+			if !httpguts.ValidHeaderFieldValue(values[i]) {
+				return fmt.Errorf("%w: invalid header field value %q", ErrInvalidRequest, values[i])
+			}
+		}
+	}
+
+	// ps. kim wrote this
+
+	return nil
+}