From 6a6c330922392f5c9f13a69504f7b7a7f4794d5c Mon Sep 17 00:00:00 2001
From: Sean Barag <barag@cockroachlabs.com>
Date: Fri, 18 Nov 2022 09:21:43 -0800
Subject: [PATCH] transport/http2: use HTTP 400 for bad requests instead of 500

Non-servicable HTTP requests from clients previously resulted in an HTTP
500 response, but no error exists within the server; the client simply
sent a malformed request. Detect bad requests and return HTTP 400 Bad
Request instead of HTTP 500 Internal Server Error.

fixes #5802
---
 internal/transport/handler_server.go | 30 +++++++++++++++++++---------
 server.go                            |  3 ++-
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/internal/transport/handler_server.go b/internal/transport/handler_server.go
index fb272235d81..17e2f0ab6e3 100644
--- a/internal/transport/handler_server.go
+++ b/internal/transport/handler_server.go
@@ -46,24 +46,32 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-// NewServerHandlerTransport returns a ServerTransport handling gRPC
-// from inside an http.Handler. It requires that the http Server
-// supports HTTP/2.
+// NewServerHandlerTransport returns a ServerTransport handling gRPC from
+// inside an http.Handler, or writes an HTTP error to w and returns an error.
+// It requires that the http Server supports HTTP/2.
 func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []stats.Handler) (ServerTransport, error) {
 	if r.ProtoMajor != 2 {
-		return nil, errors.New("gRPC requires HTTP/2")
+		msg := "gRPC requires HTTP/2"
+		http.Error(w, msg, http.StatusBadRequest)
+		return nil, errors.New(msg)
 	}
 	if r.Method != "POST" {
-		return nil, errors.New("invalid gRPC request method")
+		msg := "invalid gRPC request method"
+		http.Error(w, msg, http.StatusBadRequest)
+		return nil, errors.New(msg)
 	}
 	contentType := r.Header.Get("Content-Type")
 	// TODO: do we assume contentType is lowercase? we did before
 	contentSubtype, validContentType := grpcutil.ContentSubtype(contentType)
 	if !validContentType {
-		return nil, errors.New("invalid gRPC request content-type")
+		msg := "invalid gRPC request content-type"
+		http.Error(w, msg, http.StatusBadRequest)
+		return nil, errors.New(msg)
 	}
 	if _, ok := w.(http.Flusher); !ok {
-		return nil, errors.New("gRPC requires a ResponseWriter supporting http.Flusher")
+		msg := "gRPC requires a ResponseWriter supporting http.Flusher"
+		http.Error(w, msg, http.StatusInternalServerError)
+		return nil, errors.New(msg)
 	}
 
 	st := &serverHandlerTransport{
@@ -79,7 +87,9 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []s
 	if v := r.Header.Get("grpc-timeout"); v != "" {
 		to, err := decodeTimeout(v)
 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "malformed time-out: %v", err)
+			msg := fmt.Sprintf("malformed time-out: %v", err)
+			http.Error(w, msg, http.StatusBadRequest)
+			return nil, status.Error(codes.Internal, msg)
 		}
 		st.timeoutSet = true
 		st.timeout = to
@@ -97,7 +107,9 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []s
 		for _, v := range vv {
 			v, err := decodeMetadataHeader(k, v)
 			if err != nil {
-				return nil, status.Errorf(codes.Internal, "malformed binary metadata: %v", err)
+				msg := fmt.Sprintf("malformed binary metadata: %v", err)
+				http.Error(w, msg, http.StatusBadRequest)
+				return nil, status.Error(codes.Internal, msg)
 			}
 			metakv = append(metakv, k, v)
 		}
diff --git a/server.go b/server.go
index f4dde72b41f..bc6be574d8e 100644
--- a/server.go
+++ b/server.go
@@ -1008,7 +1008,8 @@ var _ http.Handler = (*Server)(nil)
 func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	st, err := transport.NewServerHandlerTransport(w, r, s.opts.statsHandlers)
 	if err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		// Errors returned from transport.NewServerHandlerTransport have
+		// already been written to w.
 		return
 	}
 	if !s.addConn(listenerAddressForServeHTTP, st) {