http/requestid: fix boundary conditions

We don't want to allow arbitrarily long request ID-s.

Don't log inside a package.

Change-Id: I66bafd58ca7c587e82f25c3e8653ba79085c0080

Author: egonelbre

http/doc.go

@@ -6,12 +6,9 @@ package requestid
 import (
 	"context"
 	"crypto/rand"
-	"encoding/base64"
-	"fmt"
-	"log"
 	"net/http"
 
-	"github.com/spacemonkeygo/monkit/v3"
+	"storj.io/common/base58"
 )
 
 // contextKey is the key that holds the unique request ID in a request context.
@@ -20,13 +17,25 @@ type contextKey struct{}
 // HeaderKey is the header key for the request ID.
 const HeaderKey = "X-Request-Id"
 
+// MaxRequestID is the maximum allowed length for a request id.
+const MaxRequestID = 64
+
 // AddToContext uses adds a unique requestid to the context and the response headers
 // of each request.
 func AddToContext(h http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		requestID := r.Header.Get(HeaderKey)
+		if len(requestID) > MaxRequestID {
+			requestID = ""
+		}
 		if requestID == "" {
-			requestID = generateRequestID()
+			var err error
+			requestID, err = generateRandomID()
+			if err != nil {
+				// If we fail to generate a random ID, then don't use one.
+				h.ServeHTTP(w, r)
+				return
+			}
 		}
 
 		w.Header().Set(HeaderKey, requestID)
@@ -48,15 +57,11 @@ func Propagate(ctx context.Context, req *http.Request) {
 	req.Header.Set(HeaderKey, FromContext(ctx))
 }
 
-// generateRequestID generates a random request ID using crypto/rand.
-// in case of an unlikely error, it falls back to using monkit.NewId().
-func generateRequestID() string {
-	idBytes := make([]byte, 16)
-	_, err := rand.Read(idBytes)
+func generateRandomID() (string, error) {
+	var data [8]byte
+	_, err := rand.Read(data[:])
 	if err != nil {
-		log.Printf("error generating request ID: %v", err)
-		return fmt.Sprintf("%x", monkit.NewId())
+		return "", err
 	}
-
-	return base64.RawURLEncoding.EncodeToString(idBytes)
+	return base58.Encode(data[:]), nil
 }

http/requestid/requestid.go

@@ -17,30 +17,46 @@ import (
 func TestAddToContext(t *testing.T) {
 	ctx := testcontext.New(t)
 
-	request, err := http.NewRequestWithContext(ctx, "GET", "", http.NoBody)
-	require.NoError(t, err)
-
-	rw := httptest.NewRecorder()
-
 	var requestID string
-	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	handler := AddToContext(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusOK)
 		require.NotNil(t, r.Context().Value(contextKey{}), "RequestId should not be nil")
 		require.NotEqual(t, "", r.Context().Value(contextKey{}).(string), "RequestId not set in Context")
 
 		requestID = r.Context().Value(contextKey{}).(string)
+	}))
+
+	t.Run("success", func(t *testing.T) {
+		rw := httptest.NewRecorder()
+
+		request, err := http.NewRequestWithContext(ctx, "GET", "", http.NoBody)
+		require.NoError(t, err)
+		handler.ServeHTTP(rw, request)
+
+		require.NotEqual(t, "", rw.Header().Get(HeaderKey), "RequestId is not set in response header")
+		require.Equal(t, requestID, rw.Header().Get(HeaderKey), "Correct RequestId is not set in response header")
 	})
 
-	newHandler := AddToContext(handler)
-	newHandler.ServeHTTP(rw, request)
+	t.Run("too-long", func(t *testing.T) {
+		rw := httptest.NewRecorder()
+
+		request, err := http.NewRequestWithContext(ctx, "GET", "", http.NoBody)
+		require.NoError(t, err)
+		const tooLongKey = "01234567890123456789012345678901234567890123456789012345678901234567890123456789"
+		request.Header.Set(HeaderKey, tooLongKey)
+
+		handler.ServeHTTP(rw, request)
 
-	require.NotEqual(t, "", rw.Header().Get(HeaderKey), "RequestId is not set in response header")
-	require.Equal(t, requestID, rw.Header().Get(HeaderKey), "Correct RequestId is not set in response header")
+		require.NotEqual(t, "", rw.Header().Get(HeaderKey), "RequestId is not set in response header")
+		require.NotEqual(t, tooLongKey, requestID)
+	})
 }
 
 func TestPropagate(t *testing.T) {
 	ctx := testcontext.New(t)
 
+	require.Equal(t, "", FromContext(ctx))
+
 	requestID := "test-request-id"
 	reqctx := context.WithValue(ctx, contextKey{}, requestID)