Skip to content

Commit 2f413c4

Browse files
authored
transport/http2: use HTTP 400 for bad requests instead of 500 (#5804)
1 parent 5003029 commit 2f413c4

File tree

3 files changed

+25
-12
lines changed

3 files changed

+25
-12
lines changed

internal/transport/handler_server.go

+21-9
Original file line numberDiff line numberDiff line change
@@ -46,24 +46,32 @@ import (
4646
"google.golang.org/grpc/status"
4747
)
4848

49-
// NewServerHandlerTransport returns a ServerTransport handling gRPC
50-
// from inside an http.Handler. It requires that the http Server
51-
// supports HTTP/2.
49+
// NewServerHandlerTransport returns a ServerTransport handling gRPC from
50+
// inside an http.Handler, or writes an HTTP error to w and returns an error.
51+
// It requires that the http Server supports HTTP/2.
5252
func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []stats.Handler) (ServerTransport, error) {
5353
if r.ProtoMajor != 2 {
54-
return nil, errors.New("gRPC requires HTTP/2")
54+
msg := "gRPC requires HTTP/2"
55+
http.Error(w, msg, http.StatusBadRequest)
56+
return nil, errors.New(msg)
5557
}
5658
if r.Method != "POST" {
57-
return nil, errors.New("invalid gRPC request method")
59+
msg := fmt.Sprintf("invalid gRPC request method %q", r.Method)
60+
http.Error(w, msg, http.StatusBadRequest)
61+
return nil, errors.New(msg)
5862
}
5963
contentType := r.Header.Get("Content-Type")
6064
// TODO: do we assume contentType is lowercase? we did before
6165
contentSubtype, validContentType := grpcutil.ContentSubtype(contentType)
6266
if !validContentType {
63-
return nil, errors.New("invalid gRPC request content-type")
67+
msg := fmt.Sprintf("invalid gRPC request content-type %q", contentType)
68+
http.Error(w, msg, http.StatusBadRequest)
69+
return nil, errors.New(msg)
6470
}
6571
if _, ok := w.(http.Flusher); !ok {
66-
return nil, errors.New("gRPC requires a ResponseWriter supporting http.Flusher")
72+
msg := "gRPC requires a ResponseWriter supporting http.Flusher"
73+
http.Error(w, msg, http.StatusInternalServerError)
74+
return nil, errors.New(msg)
6775
}
6876

6977
st := &serverHandlerTransport{
@@ -79,7 +87,9 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []s
7987
if v := r.Header.Get("grpc-timeout"); v != "" {
8088
to, err := decodeTimeout(v)
8189
if err != nil {
82-
return nil, status.Errorf(codes.Internal, "malformed time-out: %v", err)
90+
msg := fmt.Sprintf("malformed time-out: %v", err)
91+
http.Error(w, msg, http.StatusBadRequest)
92+
return nil, status.Error(codes.Internal, msg)
8393
}
8494
st.timeoutSet = true
8595
st.timeout = to
@@ -97,7 +107,9 @@ func NewServerHandlerTransport(w http.ResponseWriter, r *http.Request, stats []s
97107
for _, v := range vv {
98108
v, err := decodeMetadataHeader(k, v)
99109
if err != nil {
100-
return nil, status.Errorf(codes.Internal, "malformed binary metadata: %v", err)
110+
msg := fmt.Sprintf("malformed binary metadata %q in header %q: %v", v, k, err)
111+
http.Error(w, msg, http.StatusBadRequest)
112+
return nil, status.Error(codes.Internal, msg)
101113
}
102114
metakv = append(metakv, k, v)
103115
}

internal/transport/handler_server_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func (s) TestHandlerTransport_NewServerHandlerTransport(t *testing.T) {
6363
Method: "GET",
6464
Header: http.Header{},
6565
},
66-
wantErr: "invalid gRPC request method",
66+
wantErr: `invalid gRPC request method "GET"`,
6767
},
6868
{
6969
name: "bad content type",
@@ -74,7 +74,7 @@ func (s) TestHandlerTransport_NewServerHandlerTransport(t *testing.T) {
7474
"Content-Type": {"application/foo"},
7575
},
7676
},
77-
wantErr: "invalid gRPC request content-type",
77+
wantErr: `invalid gRPC request content-type "application/foo"`,
7878
},
7979
{
8080
name: "not flusher",

server.go

+2-1
Original file line numberDiff line numberDiff line change
@@ -1008,7 +1008,8 @@ var _ http.Handler = (*Server)(nil)
10081008
func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
10091009
st, err := transport.NewServerHandlerTransport(w, r, s.opts.statsHandlers)
10101010
if err != nil {
1011-
http.Error(w, err.Error(), http.StatusInternalServerError)
1011+
// Errors returned from transport.NewServerHandlerTransport have
1012+
// already been written to w.
10121013
return
10131014
}
10141015
if !s.addConn(listenerAddressForServeHTTP, st) {

0 commit comments

Comments
 (0)