Repository: quic-go/quic-go
Pull request #4646 (2024-08-28 07:47:43)

http3: reject the Transfer-Encoding header field
The use of Transfer-Encoding is forbidden in HTTP/3, see section 4.1 of RFC 9114.

@bt90 would you mind reviewing this PR?

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4646?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 84.83%. Comparing base [(`920bfb4`)](https://app.codecov.io/gh/quic-go/quic-go/commit/920bfb46af9db2ab23a94e54701b7a310ecc4928?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`cd6f123`)](https://app.codecov.io/gh/quic-go/quic-go/commit/cd6f1231dfed1d7c96c2b64b4b330bfd4260d1f3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4646      +/-   ##
==========================================
- Coverage   84.91%   84.83%   -0.08%     
==========================================
  Files         150      147       -3     
  Lines       14868    14589     -279     
==========================================
- Hits        12624    12376     -248     
+ Misses       1718     1705      -13     
+ Partials      526      508      -18     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4646?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

bt90: @marten-seemann i've created #4647 to track other headers we might want to reject

Patch:
diff --git a/http3/headers.go b/http3/headers.go
index c51f48d0..7c67342e 100644
--- a/http3/headers.go
+++ b/http3/headers.go
@@ -75,6 +75,8 @@ func parseHeaders(headers []qpack.HeaderField, isRequest bool) (header, error) {
 			}
 			readFirstRegularHeader = true
 			switch h.Name {
+			case "transfer-encoding":
+				return header{}, errors.New("invalid header field: Transfer-Encoding")
 			case "content-length":
 				// Ignore duplicate Content-Length headers.
 				// Fail if the duplicates differ.
diff --git a/http3/http_stream.go b/http3/http_stream.go
index dd715064..af46226b 100644
--- a/http3/http_stream.go
+++ b/http3/http_stream.go
@@ -245,11 +245,10 @@ func (s *requestStream) ReadResponse() (*http.Response, error) {
 	respBody := newResponseBody(s.stream, contentLength, s.reqDone)
 
 	// Rules for when to set Content-Length are defined in https://tools.ietf.org/html/rfc7230#section-3.3.2.
-	_, hasTransferEncoding := res.Header["Transfer-Encoding"]
 	isInformational := res.StatusCode >= 100 && res.StatusCode < 200
 	isNoContent := res.StatusCode == http.StatusNoContent
 	isSuccessfulConnect := s.isConnect && res.StatusCode >= 200 && res.StatusCode < 300
-	if !hasTransferEncoding && !isInformational && !isNoContent && !isSuccessfulConnect {
+	if !isInformational && !isNoContent && !isSuccessfulConnect {
 		res.ContentLength = -1
 		if clens, ok := res.Header["Content-Length"]; ok && len(clens) == 1 {
 			if clen64, err := strconv.ParseInt(clens[0], 10, 64); err == nil {
diff --git a/http3/response_writer.go b/http3/response_writer.go
index 8638ec57..71f6551e 100644
--- a/http3/response_writer.go
+++ b/http3/response_writer.go
@@ -117,11 +117,9 @@ func (w *responseWriter) sniffContentType(p []byte) {
 	// We can't use `w.header.Get` here since if the Content-Type was set to nil, we shouldn't do sniffing.
 	_, haveType := w.header["Content-Type"]
 
-	// If the Transfer-Encoding or Content-Encoding was set and is non-blank,
-	// we shouldn't sniff the body.
-	hasTE := w.header.Get("Transfer-Encoding") != ""
+	// If the Content-Encoding was set and is non-blank, we shouldn't sniff the body.
 	hasCE := w.header.Get("Content-Encoding") != ""
-	if !hasCE && !haveType && !hasTE && len(p) > 0 {
+	if !hasCE && !haveType && len(p) > 0 {
 		w.header.Set("Content-Type", http.DetectContentType(p))
 	}
 }


Test patch:
diff --git a/http3/headers_test.go b/http3/headers_test.go
index 65fa3837..e9bf7a76 100644
--- a/http3/headers_test.go
+++ b/http3/headers_test.go
@@ -359,6 +359,15 @@ var _ = Describe("Response", func() {
 		Expect(err).To(MatchError("invalid response pseudo header: :method"))
 	})
 
+	It("rejects the Transfer-Encoding header field", func() {
+		headers := []qpack.HeaderField{
+			{Name: ":status", Value: "404"},
+			{Name: "transfer-encoding", Value: "chunked"},
+		}
+		err := updateResponseFromHeaders(&http.Response{}, headers)
+		Expect(err).To(MatchError("invalid header field: Transfer-Encoding"))
+	})
+
 	It("parses trailers", func() {
 		headers := []qpack.HeaderField{
 			{Name: "content-length", Value: "42"},