This is an automated email from the ASF dual-hosted git repository.
maskit pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 644a59c92e Return a 400 on chunk parse errors (#12192)
644a59c92e is described below
commit 644a59c92e8ea75f96107d7424e514a4216c00fd
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
---
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 205a6fb238..07bab04891 100644
--- a/src/proxy/http/HttpSM.cc
+++ b/src/proxy/http/HttpSM.cc
@@ -2750,6 +2750,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;
}
@@ -3102,6 +3105,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:
@@ -3116,6 +3120,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;
@@ -3791,11 +3798,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.
@@ -5893,6 +5901,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 1a37024161..5df0a6d334 100644
--- a/src/proxy/http/HttpTunnel.cc
+++ b/src/proxy/http/HttpTunnel.cc
@@ -1240,10 +1240,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) {
@@ -1393,6 +1390,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 f57861d1b2..ef58cc9976 100644
--- a/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
+++ b/tests/gold_tests/chunked_encoding/bad_chunked_encoding.test.py
@@ -183,6 +183,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 6e058a3d81..9bccd5699c 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
@@ -118,6 +118,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.
