Drop Content-Length when HTTP1xCodec sends Transfer-Encoding: chunked

proxygen/lib/http/codec/HTTP1xCodec.cpp

@@ -766,7 +766,13 @@ void HTTP1xCodec::generateHeader(
     appendLiteral(writeBuf, len, "\r\n");
   }
 
-  if (!deferredContentLength.empty()) {
+  // A message sent with Transfer-Encoding must not also carry Content-Length
+  // (RFC 9112 section 6.1): the body goes out chunked, so a peer framing it
+  // by Content-Length instead would read the chunk headers as body bytes and
+  // parse whatever follows as another message.
+  const bool sentTransferEncodingChunked =
+      hasTransferEncodingChunked && mayChunkEgress_;
+  if (!deferredContentLength.empty() && !sentTransferEncodingChunked) {
     appendLiteral(writeBuf, len, "Content-Length: ");
     appendString(writeBuf, len, deferredContentLength);
     appendString(writeBuf, len, CRLF);

proxygen/lib/http/codec/test/HTTP1xCodecTest.cpp

@@ -810,6 +810,76 @@ TEST(HTTP1xCodecTest, TestBadTransferEncodingHeader) {
             "www.facebook.com");
 }
 
+// A request that arrives with both Transfer-Encoding: chunked and
+// Content-Length is framed by the former on ingress and re-chunked on egress,
+// so the Content-Length it carried must not go out with it (RFC 9112 section
+// 6.1): a backend that framed the body by Content-Length instead would parse
+// the tail of the chunked body as a second request.
+TEST(HTTP1xCodecTest, TestChunkedRequestWithContentLengthNotForwarded) {
+  HTTP1xCodec downstream(TransportDirection::DOWNSTREAM);
+  HTTP1xCodecCallback callbacks;
+  downstream.setCallback(&callbacks);
+
+  auto reqBuf = folly::IOBuf::copyBuffer(
+      "POST /upload HTTP/1.1\r\n"
+      "Host: www.facebook.com\r\n"
+      "Transfer-Encoding: chunked\r\n"
+      "Content-Length: 5\r\n"
+      "\r\n"
+      "5\r\nhello\r\n0\r\n\r\n");
+  downstream.onIngress(*reqBuf);
+  EXPECT_EQ(callbacks.errors, 0);
+  EXPECT_EQ(callbacks.headersComplete, 1);
+  EXPECT_EQ(callbacks.messageComplete, 1);
+  EXPECT_EQ(callbacks.bodyLen, 5);
+  ASSERT_NE(callbacks.msg_, nullptr);
+  EXPECT_TRUE(callbacks.msg_->getIsChunked());
+
+  HTTP1xCodec upstream(TransportDirection::UPSTREAM);
+  folly::IOBufQueue writeBuf(folly::IOBufQueue::cacheChainLength());
+  upstream.generateHeader(writeBuf, upstream.createStream(), *callbacks.msg_);
+  auto forwarded = writeBuf.move()->moveToFbString().toStdString();
+  EXPECT_THAT(forwarded, HasSubstr("Transfer-Encoding: chunked\r\n"));
+  EXPECT_THAT(forwarded, Not(HasSubstr("Content-Length")));
+}
+
+// Same for a response relayed to an HTTP/1.1 client.
+TEST(HTTP1xCodecTest, TestChunkedResponseWithContentLengthNotForwarded) {
+  HTTP1xCodec upstream(TransportDirection::UPSTREAM);
+  HTTP1xCodecCallback callbacks;
+  upstream.setCallback(&callbacks);
+  folly::IOBufQueue writeBuf(folly::IOBufQueue::cacheChainLength());
+  upstream.generateHeader(
+      writeBuf, upstream.createStream(), getGetRequest("/foo"));
+
+  auto respBuf = folly::IOBuf::copyBuffer(
+      "HTTP/1.1 200 OK\r\n"
+      "Transfer-Encoding: chunked\r\n"
+      "Content-Length: 5\r\n"
+      "\r\n"
+      "5\r\nhello\r\n0\r\n\r\n");
+  upstream.onIngress(*respBuf);
+  EXPECT_EQ(callbacks.errors, 0);
+  EXPECT_EQ(callbacks.headersComplete, 1);
+  EXPECT_EQ(callbacks.messageComplete, 1);
+  EXPECT_EQ(callbacks.bodyLen, 5);
+  ASSERT_NE(callbacks.msg_, nullptr);
+
+  // The client-facing codec has to have seen an HTTP/1.1 request to be allowed
+  // to send a chunked response.
+  HTTP1xCodec downstream(TransportDirection::DOWNSTREAM);
+  HTTP1xCodecCallback downstreamCallbacks;
+  downstream.setCallback(&downstreamCallbacks);
+  downstream.onIngress(*getSimpleRequestData());
+  EXPECT_EQ(downstreamCallbacks.headersComplete, 1);
+
+  writeBuf.move();
+  downstream.generateHeader(writeBuf, 1, *callbacks.msg_);
+  auto relayed = writeBuf.move()->moveToFbString().toStdString();
+  EXPECT_THAT(relayed, HasSubstr("Transfer-Encoding: chunked\r\n"));
+  EXPECT_THAT(relayed, Not(HasSubstr("Content-Length")));
+}
+
 // url_ accumulates per message, so a rejection on a pipelined request must
 // report that request's URL rather than one carried over from its predecessor.
 TEST(HTTP1xCodecTest, TestPartialMsgBackfilledForPipelinedRequest) {

proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp

@@ -3513,15 +3513,20 @@ TEST_F(HTTP2DownstreamSessionTest, Test304ContentLength) {
 // chunked with wrong content-length
 TEST_F(HTTPDownstreamSessionTest, HttpShortContentLength) {
   InSequence enforceOrder;
-  auto req = getPostRequest(10);
-  req.setIsChunked(true);
-  req.getHeaders().add(HTTP_HEADER_TRANSFER_ENCODING, "chunked");
-  auto streamID = sendRequest(req, false);
-  clientCodec_->generateChunkHeader(requests_, streamID, 20);
-  clientCodec_->generateBody(
-      requests_, streamID, makeBuf(20), HTTPCodec::NoPadding, false);
-  clientCodec_->generateChunkTerminator(requests_, streamID);
-  clientCodec_->generateEOM(requests_, streamID);
+  // Written out by hand: HTTP1xCodec does not send Content-Length alongside
+  // Transfer-Encoding: chunked, so clientCodec_ cannot produce this request.
+  string req = folly::to<string>(
+      "POST / HTTP/1.1\r\n"
+      "Host: www.foo.com\r\n"
+      "Content-Length: 10\r\n"
+      "Transfer-Encoding: chunked\r\n"
+      "\r\n"
+      "14\r\n",
+      string(20, 'a'),
+      "\r\n"
+      "0\r\n"
+      "\r\n");
+  requests_.append(req.data(), req.size());
   auto handler1 = addSimpleStrictHandler();
 
   handler1->expectHeaders();