1
0
mirror of https://github.com/facebook/proxygen.git synced 2026-10-24 01:33:54 +03:00

Preserve bodies on non-upgrade WebSocket CONNECT responses

Summary:
`HTTP1xCodec` serializes WebSocket upgrades as `GET`, including requests represented as HTTP/2 `CONNECT`. Treating those requests as ordinary CONNECT tunnels discards bodies on responses that do not switch protocols. A non-upgrade response with `Content-Length` then fails coroutine body-length validation, turning the expected HTTP response into a 502. This includes an ordinary `200 OK` fallback as well as a rejected handshake.

Exclude WebSocket upgrades from ordinary CONNECT handling so non-upgrade response bodies are parsed normally. Keep Content-Length validation and successful upgrade handling intact. Add codec and coroutine-session tests for `401` and ordinary `200 OK` body preservation, plus a successful translated-upgrade test. Declare the direct `//folly:conv` build dependencies required by autodeps and regenerate the codec CMake dependency list. The repository mirrors these changes into `xplat/proxygen`.

This was found while investigating the Model API canary failure recorded in D124573071. A production origin logged `Content-Length/body mismatch: expected=104, actual=0` at the original failed-test time. This fix is independent of the Hatch retry change.

Use portable `folly::to<std::string>` conversions in the regression tests and a checked `folly::to<uint32_t>` conversion for the fixture body length.

Reviewed By: hanidamlaj

Differential Revision: D125794637

fbshipit-source-id: 5a37a7473d03196f2a071724d628372ec9c2aa04
This commit is contained in:
Oded Argon
2026-10-21 04:50:41 -07:00
committed by meta-codesync[bot]
parent 2f5ead1655
commit 724b010a3f
4 changed files with 151 additions and 1 deletions

View File

@@ -76,6 +76,7 @@ proxygen_add_library(proxygen_http_codec_http1x_codec
proxygen_http_codec_util
proxygen_http_http_headers
Folly::folly_base64
Folly::folly_conv
Folly::folly_io_iobuf
Folly::folly_random
Folly::folly_ssl_openssl_hash

View File

@@ -461,7 +461,9 @@ void HTTP1xCodec::generateHeader(
DCHECK_EQ(txn, egressTxnID_);
requestPending_ = true;
responsePending_ = true;
connectRequest_ = (msg.getMethod() == HTTPMethod::CONNECT);
// proxying downstream non-h1 ws CONNECT must be serialized as GET
connectRequest_ = (msg.getMethod() == HTTPMethod::CONNECT) &&
!msg.isEgressWebsocketUpgrade();
headRequest_ = (msg.getMethod() == HTTPMethod::HEAD);
expectNoResponseBody_ = connectRequest_ || headRequest_;
} else {

View File

@@ -8,6 +8,7 @@
#include <proxygen/lib/http/codec/HTTP1xCodec.h>
#include <folly/Conv.h>
#include <folly/String.h>
#include <folly/base64.h>
#include <folly/portability/GTest.h>
@@ -981,6 +982,101 @@ TEST(HTTP1xCodecTest, WebsocketUpgrade) {
EXPECT_NE(ws_accept_header, empty_string);
}
TEST(HTTP1xCodecTest, WebsocketConnectRejectionBody) {
HTTP1xCodec codec(TransportDirection::UPSTREAM);
HTTP1xCodecCallback callbacks;
codec.setCallback(&callbacks);
HTTPMessage req;
req.setHTTPVersion(2, 0);
req.setMethod(HTTPMethod::CONNECT);
req.setURL("/websocket");
req.setEgressWebsocketUpgrade();
folly::IOBufQueue writeBuf;
codec.generateHeader(writeBuf, codec.createStream(), req);
HTTP1xCodec peer(TransportDirection::DOWNSTREAM);
HTTP1xCodecCallback peerCallbacks;
peer.setCallback(&peerCallbacks);
peer.onIngress(*writeBuf.front());
ASSERT_NE(peerCallbacks.msg_, nullptr);
EXPECT_EQ(peerCallbacks.msg_->getMethod(), HTTPMethod::GET);
const string body = "Unauthorized";
auto response = folly::IOBuf::copyBuffer(
"HTTP/1.1 401 Unauthorized\r\nContent-Length: " +
folly::to<std::string>(body.size()) + "\r\n\r\n" + body);
codec.onIngress(*response);
ASSERT_NE(callbacks.msg_, nullptr);
EXPECT_EQ(callbacks.msg_->getStatusCode(), 401);
EXPECT_EQ(callbacks.bodyLen, body.size());
EXPECT_EQ(callbacks.messageComplete, 1);
EXPECT_EQ(callbacks.errors, 0);
}
TEST(HTTP1xCodecTest, WebsocketConnectNonUpgrade200Body) {
HTTP1xCodec codec(TransportDirection::UPSTREAM);
HTTP1xCodecCallback callbacks;
codec.setCallback(&callbacks);
HTTPMessage req;
req.setHTTPVersion(2, 0);
req.setMethod(HTTPMethod::CONNECT);
req.setURL("/websocket");
req.setEgressWebsocketUpgrade();
folly::IOBufQueue writeBuf;
codec.generateHeader(writeBuf, codec.createStream(), req);
const string body = "ordinary response";
auto response = folly::IOBuf::copyBuffer(
"HTTP/1.1 200 OK\r\nContent-Length: " +
folly::to<std::string>(body.size()) + "\r\n\r\n" + body);
codec.onIngress(*response);
ASSERT_NE(callbacks.msg_, nullptr);
EXPECT_EQ(callbacks.msg_->getStatusCode(), 200);
EXPECT_FALSE(callbacks.msg_->isIngressWebsocketUpgrade());
EXPECT_EQ(callbacks.bodyLen, body.size());
EXPECT_EQ(callbacks.messageComplete, 1);
EXPECT_EQ(callbacks.errors, 0);
}
TEST(HTTP1xCodecTest, WebsocketConnectUpgrade) {
HTTP1xCodec codec(TransportDirection::UPSTREAM);
HTTP1xCodec peer(TransportDirection::DOWNSTREAM);
HTTP1xCodecCallback callbacks;
HTTP1xCodecCallback peerCallbacks;
codec.setCallback(&callbacks);
peer.setCallback(&peerCallbacks);
HTTPMessage req;
req.setHTTPVersion(2, 0);
req.setMethod(HTTPMethod::CONNECT);
req.setURL("/websocket");
req.setEgressWebsocketUpgrade();
folly::IOBufQueue writeBuf;
const auto streamID = codec.createStream();
codec.generateHeader(writeBuf, streamID, req);
peer.onIngress(*writeBuf.front());
HTTPMessage resp;
resp.setHTTPVersion(1, 1);
resp.setStatusCode(101);
resp.setEgressWebsocketUpgrade();
writeBuf.reset();
peer.generateHeader(writeBuf, streamID, resp);
codec.onIngress(*writeBuf.front());
auto data = folly::IOBuf::copyBuffer("upgraded-protocol-data");
codec.onIngress(*data);
ASSERT_NE(callbacks.msg_, nullptr);
EXPECT_EQ(callbacks.msg_->getStatusCode(), 101);
EXPECT_TRUE(callbacks.msg_->isIngressWebsocketUpgrade());
EXPECT_EQ(callbacks.bodyLen, data->length());
EXPECT_EQ(callbacks.errors, 0);
}
// A rejected (non-101) response to an HTTP/1.1 Upgrade request closes the
// connection so trailing upgraded-protocol bytes are never reparsed as a
// pipelined request.

View File

@@ -12,6 +12,7 @@
#include "proxygen/lib/http/coro/test/Mocks.h"
#include <folly/logging/xlog.h>
#include <folly/Conv.h>
#include <folly/ExceptionWrapper.h>
#include <folly/coro/Timeout.h>
@@ -970,6 +971,56 @@ CO_TEST_P_X(H1UpstreamSessionTest, DrainAfterFailedUpgrade) {
}
}
CO_TEST_P_X(H1UpstreamSessionTest, WebsocketConnectRejectionBody) {
const std::string body = "Unauthorized";
const auto bodyLength = folly::to<uint32_t>(body.size());
auto req =
HTTPFixedSource::makeFixedRequest("/websocket", HTTPMethod::CONNECT);
req->msg_->setHTTPVersion(2, 0);
req->msg_->setEgressWebsocketUpgrade();
auto responseSource = co_await co_awaitTry(session_->sendRequest(req));
XCHECK(!responseSource.hasException());
auto resp = makeResponse(401, true);
resp.getHeaders().set(HTTP_HEADER_CONTENT_LENGTH,
folly::to<std::string>(bodyLength));
serializeResponse(*responseSource->getStreamID(),
std::move(resp),
folly::IOBuf::copyBuffer(body));
auto response = co_await expectResponse(
std::move(*responseSource), 401, bodyLength, true);
XCHECK_NE(response, nullptr);
auto responseBody = response->body_.move();
XCHECK(responseBody);
EXPECT_EQ(responseBody->moveToFbString(), body);
}
CO_TEST_P_X(H1UpstreamSessionTest, WebsocketConnectNonUpgrade200Body) {
const std::string body = "ordinary response";
const auto bodyLength = folly::to<uint32_t>(body.size());
auto req =
HTTPFixedSource::makeFixedRequest("/websocket", HTTPMethod::CONNECT);
req->msg_->setHTTPVersion(2, 0);
req->msg_->setEgressWebsocketUpgrade();
auto responseSource = co_await co_awaitTry(session_->sendRequest(req));
XCHECK(!responseSource.hasException());
auto resp = makeResponse(200, true);
resp.getHeaders().set(HTTP_HEADER_CONTENT_LENGTH,
folly::to<std::string>(bodyLength));
serializeResponse(*responseSource->getStreamID(),
std::move(resp),
folly::IOBuf::copyBuffer(body));
auto response = co_await expectResponse(
std::move(*responseSource), 200, bodyLength, true);
XCHECK_NE(response, nullptr);
auto responseBody = response->body_.move();
XCHECK(responseBody);
EXPECT_EQ(responseBody->moveToFbString(), body);
}
TEST_P(H2QUpstreamSessionTest, ReceiveGoaway) {
// Client receives GOAWAY, no EOF. Even though the peer is allowing for
// more streams, this session doesn't have any in-flight, so it egresses