Repository: quic-go/quic-go

Issue:
http3: duplicate pseudo-headers not detected
quic-go/http3 does not reject requests with duplicate pseudo-headers.

e.g.: a request with both `:method: GET` and `:method POST` is not rejected. the last occurring value is treated as the actual request-method.

a request like this should be treated as malformed, since this could cause trouble in reverse-proxies. however, in rfc 9114 this is not stated explicitly, see section 4.3.1: 

> All HTTP/3 requests MUST include exactly one value for the :method, :scheme, and :path pseudo-header fields, unless the request is a CONNECT request.

functionality should probably be implemented here:
https://github.com/quic-go/quic-go/blob/9ca74c70493a7268a8866495ab4dd8ec849f9111/http3/headers.go#L53-L75

Discussion:
@marten-seemann: Thanks for reporting @pittgi! Want to submit a fix?@pittgi: @marten-seemann Sure! I'll submit a fix asap

Solution patch:
diff --git a/connection.go b/connection.go
index 9c711d44..b266d7a4 100644
--- a/connection.go
+++ b/connection.go
@@ -1054,7 +1054,7 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed boo
 	}
 
 	var shouldSwitchPath bool
-	if pn == s.largestRcvdAppData && !addrsEqual(p.remoteAddr, s.RemoteAddr()) {
+	if !addrsEqual(p.remoteAddr, s.RemoteAddr()) {
 		if s.pathManager == nil {
 			s.pathManager = newPathManager(
 				s.connIDManager.GetConnIDForPath,
@@ -1075,9 +1075,11 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed boo
 			s.registerPackedShortHeaderPacket(probe, protocol.ECNNon, p.rcvTime)
 			s.sendQueue.SendProbe(buf, p.remoteAddr)
 		}
-	}
-
-	if shouldSwitchPath {
+		// We only switch paths in response to the highest-numbered non-probing packet,
+		// see section 9.3 of RFC 9000.
+		if !shouldSwitchPath || pn != s.largestRcvdAppData {
+			return true, nil
+		}
 		s.pathManager.SwitchToPath(p.remoteAddr)
 		s.sentPacketHandler.MigratedPath(p.rcvTime, protocol.ByteCount(s.config.InitialPacketSize))
 		maxPacketSize := protocol.ByteCount(protocol.MaxPacketBufferSize)
diff --git a/http3/headers.go b/http3/headers.go
index 05d13ff3..27af9500 100644
--- a/http3/headers.go
+++ b/http3/headers.go
@@ -55,24 +55,34 @@ func parseHeaders(headers []qpack.HeaderField, isRequest bool) (header, error) {
 				// all pseudo headers must appear before regular header fields, see section 4.3 of RFC 9114
 				return header{}, fmt.Errorf("received pseudo header %s after a regular header field", h.Name)
 			}
-			var isResponsePseudoHeader bool // pseudo headers are either valid for requests or for responses
+			var isResponsePseudoHeader bool  // pseudo headers are either valid for requests or for responses
+			var isDuplicatePseudoHeader bool // pseudo headers are allowed to appear exactly once
 			switch h.Name {
 			case ":path":
+				isDuplicatePseudoHeader = hdr.Path != ""
 				hdr.Path = h.Value
 			case ":method":
+				isDuplicatePseudoHeader = hdr.Method != ""
 				hdr.Method = h.Value
 			case ":authority":
+				isDuplicatePseudoHeader = hdr.Authority != ""
 				hdr.Authority = h.Value
 			case ":protocol":
+				isDuplicatePseudoHeader = hdr.Protocol != ""
 				hdr.Protocol = h.Value
 			case ":scheme":
+				isDuplicatePseudoHeader = hdr.Scheme != ""
 				hdr.Scheme = h.Value
 			case ":status":
+				isDuplicatePseudoHeader = hdr.Status != ""
 				hdr.Status = h.Value
 				isResponsePseudoHeader = true
 			default:
 				return header{}, fmt.Errorf("unknown pseudo header: %s", h.Name)
 			}
+			if isDuplicatePseudoHeader {
+				return header{}, fmt.Errorf("duplicate pseudo header: %s", h.Name)
+			}
 			if isRequest && isResponsePseudoHeader {
 				return header{}, fmt.Errorf("invalid request pseudo header: %s", h.Name)
 			}



Test patch:
diff --git a/http3/headers_test.go b/http3/headers_test.go
index a1cedc82..3f23cc3b 100644
--- a/http3/headers_test.go
+++ b/http3/headers_test.go
@@ -235,6 +235,18 @@ var _ = Describe("Request", func() {
 		Expect(err).To(MatchError(":protocol must be empty"))
 	})
 
+	It("errors with duplicate pseudo header in request", func() {
+		headers := []qpack.HeaderField{
+			{Name: ":path", Value: "/foo"},
+			{Name: ":authority", Value: "quic.clemente.io"},
+			{Name: ":method", Value: "GET"},
+			{Name: ":scheme", Value: "https"},
+			{Name: ":method", Value: "POST"},
+		}
+		_, err := requestFromHeaders(headers)
+		Expect(err).To(MatchError("duplicate pseudo header: :method"))
+	})
+
 	Context("regular HTTP CONNECT", func() {
 		It("handles CONNECT method", func() {
 			headers := []qpack.HeaderField{
@@ -432,4 +444,13 @@ var _ = Describe("Response", func() {
 		_, err := parseTrailers(headers)
 		Expect(err).To(MatchError("http3: received pseudo header in trailer: :status"))
 	})
+
+	It("errors with duplicate status header in response", func() {
+		headers := []qpack.HeaderField{
+			{Name: ":status", Value: "200"},
+			{Name: ":status", Value: "400"},
+		}
+		err := updateResponseFromHeaders(&http.Response{}, headers)
+		Expect(err).To(MatchError("duplicate pseudo header: :status"))
+	})
 })