This is an automated email from the ASF dual-hosted git repository. cmcfarlen pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 92dff8ea7f2d72b18405981258ed339b1f151130 Author: Masakazu Kitajo <[email protected]> AuthorDate: Wed May 14 16:53:50 2025 -0600 Return a 400 on chunk parse errors (#12192) * Return a 400 on chunk parse errors * Fix for an assertion failure * Add tests (cherry picked from commit 644a59c92e8ea75f96107d7424e514a4216c00fd) --- include/proxy/http/HttpTunnel.h | 1 + src/proxy/http/HttpSM.cc | 13 ++++++- src/proxy/http/HttpTunnel.cc | 6 ++-- .../chunked_encoding/bad_chunked_encoding.test.py | 21 ++++++++++++ .../replays/malformed_chunked_header.replay.yaml | 40 ++++++++++++++++++++++ 5 files changed, 76 insertions(+), 5 deletions(-) diff --git a/include/proxy/http/HttpTunnel.h b/include/proxy/http/HttpTunnel.h index e2d461d466..128c8fc591 100644 --- a/include/proxy/http/HttpTunnel.h +++ b/include/proxy/http/HttpTunnel.h @@ -48,6 +48,7 @@ #define HTTP_TUNNEL_EVENT_PRECOMPLETE (HTTP_TUNNEL_EVENTS_START + 2) #define HTTP_TUNNEL_EVENT_CONSUMER_DETACH (HTTP_TUNNEL_EVENTS_START + 3) #define HTTP_TUNNEL_EVENT_ACTIVITY_CHECK (HTTP_TUNNEL_EVENTS_START + 4) +#define HTTP_TUNNEL_EVENT_PARSE_ERROR (HTTP_TUNNEL_EVENTS_START + 5) #define HTTP_TUNNEL_STATIC_PRODUCER (VConnection *)!0 diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 0a5dc7ce3e..af01d7c798 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -2832,6 +2832,9 @@ HttpSM::tunnel_handler_post(int event, void *data) case HttpTransact::INACTIVE_TIMEOUT: call_transact_and_set_next_state(HttpTransact::PostInactiveTimeoutResponse); return 0; + case HttpTransact::PARSE_ERROR: + call_transact_and_set_next_state(HttpTransact::BadRequest); + return 0; default: break; } @@ -3184,6 +3187,7 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p) /* fallthru */ case VC_EVENT_EOS: + case HTTP_TUNNEL_EVENT_PARSE_ERROR: switch (event) { case VC_EVENT_INACTIVITY_TIMEOUT: @@ -3198,6 +3202,9 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p) case VC_EVENT_EOS: t_state.current.server->state = HttpTransact::TRANSACTION_COMPLETE; break; + case HTTP_TUNNEL_EVENT_PARSE_ERROR: + t_state.current.server->state = HttpTransact::PARSE_ERROR; + break; } Metrics::Counter::increment(http_rsb.origin_shutdown_tunnel_server); close_connection = true; @@ -3873,11 +3880,12 @@ HttpSM::tunnel_handler_post_ua(int event, HttpTunnelProducer *p) switch (event) { case VC_EVENT_INACTIVITY_TIMEOUT: case VC_EVENT_ACTIVE_TIMEOUT: + case HTTP_TUNNEL_EVENT_PARSE_ERROR: if (client_response_hdr_bytes == 0) { p->handler_state = HTTP_SM_POST_UA_FAIL; set_ua_abort(HttpTransact::ABORTED, event); - SMDbg(dbg_ctl_http_tunnel, "send 408 response to client to vc %p, tunnel vc %p", _ua.get_txn()->get_netvc(), p->vc); + SMDbg(dbg_ctl_http_tunnel, "send error response to client to vc %p, tunnel vc %p", _ua.get_txn()->get_netvc(), p->vc); tunnel.chain_abort_all(p); // Reset the inactivity timeout, otherwise the InactivityCop will callback again in the next second. @@ -5994,6 +6002,9 @@ HttpSM::set_ua_abort(HttpTransact::AbortState_t ua_abort, int event) case VC_EVENT_ERROR: t_state.client_info.state = HttpTransact::CONNECTION_ERROR; break; + case HTTP_TUNNEL_EVENT_PARSE_ERROR: + t_state.client_info.state = HttpTransact::PARSE_ERROR; + break; } } diff --git a/src/proxy/http/HttpTunnel.cc b/src/proxy/http/HttpTunnel.cc index cb654d3f1a..a6282059eb 100644 --- a/src/proxy/http/HttpTunnel.cc +++ b/src/proxy/http/HttpTunnel.cc @@ -1246,10 +1246,7 @@ HttpTunnel::producer_handler_chunked(int event, HttpTunnelProducer *p) if (p->chunked_handler.state == ChunkedHandler::CHUNK_READ_ERROR) { Dbg(dbg_ctl_http_tunnel, "[%" PRId64 "] producer_handler_chunked [%s chunk decoding error]", sm->sm_id, p->name); p->chunked_handler.truncation = true; - // FIX ME: we return EOS here since it will cause the - // the client to be reenabled. ERROR makes more - // sense but no reenables follow - return VC_EVENT_EOS; + return HTTP_TUNNEL_EVENT_PARSE_ERROR; } switch (event) { @@ -1399,6 +1396,7 @@ HttpTunnel::producer_handler(int event, HttpTunnelProducer *p) case VC_EVENT_ACTIVE_TIMEOUT: case VC_EVENT_INACTIVITY_TIMEOUT: case HTTP_TUNNEL_EVENT_CONSUMER_DETACH: + case HTTP_TUNNEL_EVENT_PARSE_ERROR: if (p->alive) { p->alive = false; if (p->read_vio) { diff --git a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py index 707813d2f0..5b63acf2d0 100644 --- a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py +++ b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py @@ -179,6 +179,27 @@ class MalformedChunkHeaderTest: r"(Unexpected chunked content for key 103: too small|Failed HTTP/1 transaction with key: 103)", "Verify that ATS closed the sixth transaction.") + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 1 with headers", "Verify that ATS returns a response for uuid:1 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 2 with headers", "Verify that ATS returns a response for uuid:2 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 3 with headers", "Verify that ATS returns a response for uuid:3 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 4 with headers", "Verify that ATS returns a response for uuid:4 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 5 with headers", "Verify that ATS returns a response for uuid:5 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 6 with headers", "Verify that ATS returns a response for uuid:6 transaction.") + + tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + r"Received an HTTP/1 400 response for key 7 with headers", "Verify that ATS returns a response for uuid:7 transaction.") + # ATS should close the connection before any body gets through. "def" # is the body sent by the server for each of these chunked cases. tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression("def", "Verify that the body never got through.") diff --git a/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml b/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml index ac0c35d9c3..0e3cbda394 100644 --- a/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml +++ b/tests/gold_tests/chunked_encoding/replays/malformed_chunked_header.replay.yaml @@ -138,6 +138,46 @@ sessions: server-response: status: 200 +- transactions: + - client-request: + method: "POST" + version: "1.1" + url: /malformed/chunk/header3 + headers: + fields: + - [ Host, example.com ] + - [ Transfer-Encoding, chunked ] + - [ uuid, 6 ] + content: + transfer: plain + encoding: uri + # Chunk header must end with a sequence of CRLF. + data: 7%0Dabcwxyz%0D%0A0%0D%0A%0D%0A + + # The connection will be dropped and this response will not go out. + server-response: + status: 200 + +- transactions: + - client-request: + method: "POST" + version: "1.1" + url: /malformed/chunk/header3 + headers: + fields: + - [ Host, example.com ] + - [ Transfer-Encoding, chunked ] + - [ uuid, 7 ] + content: + transfer: plain + encoding: uri + # Chunk header must end with a sequence of CRLF. + data: 3%0Aabcwxyz%0D%0A0%0D%0A%0D%0A + + # The connection will be dropped and this response will not go out. + server-response: + status: 200 + # # Now repeat the above two malformed chunk header tests, but on the server # side.
