fix: harden native http protocol boundaries

2026-09-21 02:54:42 +08:00
parent 3d5f225fb2
commit 22939fdb0b
4 changed files with 240 additions and 18 deletions
--- a/transport/native/server.go
+++ b/transport/native/server.go
@@ -479,23 +479,39 @@ func readRequest(reader *bufio.Reader, conn net.Conn, maxHeaderBytes int, maxBod
 		}
 		name := strings.TrimSpace(line[:index])
 		value := strings.TrimSpace(line[index+1:])
-		if !validToken(name) || strings.ContainsAny(value, "\r\n") {
+		if !validToken(name) || !validHeaderValue(value) {
 			return nil, true, &protocolError{status: http.StatusBadRequest, message: "malformed header"}
 		}
 		header.Add(name, value)
 	}
-
-	if value := header.Get("Expect"); value != "" {
-		return nil, true, &protocolError{status: http.StatusExpectationFailed, message: "unsupported expectation"}
+	if protoMajor == 1 && protoMinor == 1 && (len(header.Values("Host")) != 1 || header.Get("Host") == "") {
+		return nil, true, &protocolError{status: http.StatusBadRequest, message: "missing host header"}
 	}
-	if value := header.Get("Transfer-Encoding"); value != "" && !strings.EqualFold(value, "identity") {
-		return nil, true, &protocolError{status: http.StatusNotImplemented, message: "chunked requests are not supported"}
+
+	for _, value := range header.Values("Expect") {
+		if strings.TrimSpace(value) != "" {
+			return nil, true, &protocolError{status: http.StatusExpectationFailed, message: "unsupported expectation"}
+		}
+	}
+	for _, value := range header.Values("Transfer-Encoding") {
+		for _, coding := range strings.Split(value, ",") {
+			coding = strings.TrimSpace(coding)
+			if coding != "" && !strings.EqualFold(coding, "identity") {
+				return nil, true, &protocolError{status: http.StatusNotImplemented, message: "transfer encoding is not supported"}
+			}
+		}
 	}
 	contentLength := int64(0)
-	if value := header.Get("Content-Length"); value != "" {
-		contentLength, err = strconv.ParseInt(strings.TrimSpace(value), 10, 64)
-		if err != nil || contentLength < 0 {
-			return nil, true, &protocolError{status: http.StatusBadRequest, message: "invalid content length"}
+	contentLengthSeen := false
+	for _, value := range header.Values("Content-Length") {
+		for _, part := range strings.Split(value, ",") {
+			part = strings.TrimSpace(part)
+			parsed, parseErr := strconv.ParseInt(part, 10, 64)
+			if parseErr != nil || parsed < 0 || (contentLengthSeen && parsed != contentLength) {
+				return nil, true, &protocolError{status: http.StatusBadRequest, message: "invalid content length"}
+			}
+			contentLength = parsed
+			contentLengthSeen = true
 		}
 	}
 	if contentLength > maxBodyBytes {
@@ -523,8 +539,8 @@ func readRequest(reader *bufio.Reader, conn net.Conn, maxHeaderBytes int, maxBod
 		RequestURI:    target,
 		RemoteAddr:    conn.RemoteAddr().String(),
 	}
-	closeAfter := strings.EqualFold(header.Get("Connection"), "close") ||
-		(protoMajor == 1 && protoMinor == 0 && !strings.Contains(strings.ToLower(header.Get("Connection")), "keep-alive"))
+	closeAfter := headerHasToken(header.Values("Connection"), "close") ||
+		(protoMajor == 1 && protoMinor == 0 && !headerHasToken(header.Values("Connection"), "keep-alive"))
 	request.Close = closeAfter
 	return request, closeAfter, nil
 }
@@ -540,14 +556,43 @@ func validToken(value string) bool {
 	if value == "" {
 		return false
 	}
-	for _, char := range value {
-		if char <= ' ' || strings.ContainsRune("()<>@,;:\\\"/[]?={} \t", char) {
+	for index := 0; index < len(value); index++ {
+		char := value[index]
+		if !isTokenByte(char) {
 			return false
 		}
 	}
 	return true
 }

+func isTokenByte(char byte) bool {
+	if char >= '0' && char <= '9' || char >= 'A' && char <= 'Z' || char >= 'a' && char <= 'z' {
+		return true
+	}
+	return strings.ContainsRune("!#$%&'*+-.^_`|~", rune(char))
+}
+
+func validHeaderValue(value string) bool {
+	for index := 0; index < len(value); index++ {
+		char := value[index]
+		if (char < 0x20 && char != '\t') || char == 0x7f {
+			return false
+		}
+	}
+	return true
+}
+
+func headerHasToken(values []string, token string) bool {
+	for _, value := range values {
+		for _, item := range strings.Split(value, ",") {
+			if strings.EqualFold(strings.TrimSpace(item), token) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 func isTimeout(err error) bool {
 	var netErr net.Error
 	return errors.As(err, &netErr) && netErr.Timeout()
@@ -614,12 +659,12 @@ func (w *responseWriter) writeTo(conn net.Conn, closeAfter bool, timeout time.Du
 		w.wroteHeader = true
 	}
 	body := w.body.Bytes()
-	noBody := w.requestMethod == http.MethodHead || w.status == http.StatusNoContent ||
+	noBody := w.requestMethod == http.MethodHead || w.status == http.StatusNoContent || w.status == http.StatusResetContent ||
 		w.status == http.StatusNotModified || (w.status >= 100 && w.status < 200)
 	if noBody {
 		body = nil
 	}
-	if w.header.Get("Content-Length") == "" && w.status != http.StatusNoContent && w.status != http.StatusNotModified {
+	if w.header.Get("Content-Length") == "" && w.status >= 200 && w.status != http.StatusNoContent && w.status != http.StatusNotModified && w.status != http.StatusResetContent {
 		w.header.Set("Content-Length", strconv.Itoa(len(w.body.Bytes())))
 	}
 	if closeAfter {
@@ -637,8 +682,11 @@ func (w *responseWriter) writeTo(conn net.Conn, closeAfter bool, timeout time.Du
 	output.WriteString(http.StatusText(w.status))
 	output.WriteString("\r\n")
 	for _, key := range keys {
+		if !validToken(key) {
+			return errors.New("invalid response header name")
+		}
 		for _, value := range w.header.Values(key) {
-			if strings.ContainsAny(value, "\r\n") {
+			if !validHeaderValue(value) {
 				return errors.New("invalid response header")
 			}
 			output.WriteString(key)
--- a/transport/native/server_test.go
+++ b/transport/native/server_test.go
@@ -88,6 +88,179 @@ func TestServerRejectsUnsupportedBodyAndLimits(t *testing.T) {
 	}
 }

+func TestServerProtocolBoundaries(t *testing.T) {
+	application := omni.NewWithRouter(netrouter.New())
+	application.GET("/health", func(ctx *omni.Context) error { return ctx.OK("ok") })
+	server, listener, done := startServer(t, application)
+	defer stopServer(t, server, listener, done)
+
+	tests := []struct {
+		name    string
+		request string
+		status  int
+	}{
+		{
+			name:    "missing host",
+			request: "GET /health HTTP/1.1\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "duplicate host",
+			request: "GET /health HTTP/1.1\r\nHost: one\r\nHost: two\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "unsupported version",
+			request: "GET /health HTTP/2.0\r\nHost: test\r\n\r\n",
+			status:  http.StatusHTTPVersionNotSupported,
+		},
+		{
+			name:    "malformed header",
+			request: "GET /health HTTP/1.1\r\nHost: test\r\nBroken\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "folded header",
+			request: "GET /health HTTP/1.1\r\nHost: test\r\n X-Bad: value\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "control header value",
+			request: "GET /health HTTP/1.1\r\nHost: test\r\nX-Bad: value\x01\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "chunked",
+			request: "POST /health HTTP/1.1\r\nHost: test\r\nTransfer-Encoding: chunked\r\n\r\n",
+			status:  http.StatusNotImplemented,
+		},
+		{
+			name:    "expectation",
+			request: "GET /health HTTP/1.1\r\nHost: test\r\nExpect: 100-continue\r\n\r\n",
+			status:  http.StatusExpectationFailed,
+		},
+		{
+			name:    "conflicting content length",
+			request: "POST /health HTTP/1.1\r\nHost: test\r\nContent-Length: 1\r\nContent-Length: 2\r\n\r\na",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "invalid content length",
+			request: "POST /health HTTP/1.1\r\nHost: test\r\nContent-Length: nope\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+		{
+			name:    "malformed target",
+			request: "GET health HTTP/1.1\r\nHost: test\r\n\r\n",
+			status:  http.StatusBadRequest,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			response := rawResponse(t, listener.Addr().String(), test.request)
+			defer response.Body.Close()
+			if response.StatusCode != test.status {
+				t.Fatalf("expected status %d, got %d", test.status, response.StatusCode)
+			}
+			if !response.Close {
+				t.Fatal("protocol error should close the connection")
+			}
+		})
+	}
+}
+
+func TestServerHTTP10KeepAliveAndRequestMetadata(t *testing.T) {
+	application := omni.NewWithRouter(netrouter.New())
+	application.GET("/metadata", func(ctx *omni.Context) error {
+		return ctx.OK(map[string]string{
+			"method": ctx.Request().Method,
+			"host":   ctx.Request().Host,
+		})
+	})
+	server, listener, done := startServer(t, application)
+	defer stopServer(t, server, listener, done)
+
+	conn, err := net.Dial("tcp", listener.Addr().String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer conn.Close()
+	reader := bufio.NewReader(conn)
+	_, _ = io.WriteString(conn, "GET /metadata HTTP/1.0\r\nHost: legacy\r\nConnection: keep-alive\r\n\r\n")
+	response, err := http.ReadResponse(reader, &http.Request{Method: http.MethodGet})
+	if err != nil {
+		t.Fatal(err)
+	}
+	body, err := io.ReadAll(response.Body)
+	response.Body.Close()
+	if err != nil || response.StatusCode != http.StatusOK || response.Close || !strings.Contains(string(body), `"host":"legacy"`) {
+		t.Fatalf("unexpected HTTP/1.0 response: close=%v status=%d body=%s error=%v", response.Close, response.StatusCode, body, err)
+	}
+	_, _ = io.WriteString(conn, "GET /metadata HTTP/1.0\r\nHost: legacy\r\n\r\n")
+	response, err = http.ReadResponse(reader, &http.Request{Method: http.MethodGet})
+	if err != nil {
+		t.Fatal(err)
+	}
+	response.Body.Close()
+	if !response.Close {
+		t.Fatal("HTTP/1.0 response without keep-alive should close")
+	}
+}
+
+func TestServerSuppressesBodiesForHeadAndNoContent(t *testing.T) {
+	application := omni.NewWithRouter(netrouter.New())
+	application.Handle(http.MethodHead, "/head", func(ctx *omni.Context) error { return ctx.OK("hidden") })
+	application.GET("/empty", func(ctx *omni.Context) error {
+		ctx.Raw().WriteHeader(http.StatusNoContent)
+		return nil
+	})
+	server, listener, done := startServer(t, application)
+	defer stopServer(t, server, listener, done)
+
+	headResponse := rawResponseMethod(t, listener.Addr().String(), http.MethodHead, "HEAD /head HTTP/1.1\r\nHost: test\r\n\r\n")
+	headBody, err := io.ReadAll(headResponse.Body)
+	headResponse.Body.Close()
+	if err != nil || len(headBody) != 0 || headResponse.StatusCode != http.StatusOK || headResponse.ContentLength <= 0 {
+		t.Fatalf("unexpected HEAD response: status=%d contentLength=%d body=%q error=%v", headResponse.StatusCode, headResponse.ContentLength, headBody, err)
+	}
+	emptyResponse := rawResponse(t, listener.Addr().String(), "GET /empty HTTP/1.1\r\nHost: test\r\n\r\n")
+	emptyBody, err := io.ReadAll(emptyResponse.Body)
+	emptyResponse.Body.Close()
+	if err != nil || len(emptyBody) != 0 || emptyResponse.StatusCode != http.StatusNoContent || emptyResponse.ContentLength != 0 {
+		t.Fatalf("unexpected 204 response: status=%d contentLength=%d body=%q error=%v", emptyResponse.StatusCode, emptyResponse.ContentLength, emptyBody, err)
+	}
+}
+
+func rawResponse(t *testing.T, address, request string) *http.Response {
+	return rawResponseMethod(t, address, http.MethodGet, request)
+}
+
+func rawResponseMethod(t *testing.T, address, method, request string) *http.Response {
+	t.Helper()
+	conn, err := net.Dial("tcp", address)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, _ = io.WriteString(conn, request)
+	response, err := http.ReadResponse(bufio.NewReader(conn), &http.Request{Method: method})
+	if err != nil {
+		conn.Close()
+		t.Fatal(err)
+	}
+	response.Body = &closeBody{ReadCloser: response.Body, conn: conn}
+	return response
+}
+
+type closeBody struct {
+	io.ReadCloser
+	conn net.Conn
+}
+
+func (b *closeBody) Close() error {
+	err := b.ReadCloser.Close()
+	return errors.Join(err, b.conn.Close())
+}
+
 func TestServerShutdownCancelsActiveHandler(t *testing.T) {
 	started := make(chan struct{})
 	application := omni.NewWithRouter(netrouter.New())