Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4137244520
##########
include/proxy/http/HttpSM.h:
##########
@@ -739,13 +765,30 @@ HttpSM::get_cache_sm()
inline int
HttpSM::write_response_header_into_buffer(HTTPHdr *h, MIOBuffer *b)
{
- if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
+ if (_ua.get_txn()->supports_direct_header_passing()) {
Review Comment:
`setup_internal_transfer()` calls this helper before checking whether a
client transaction exists. Scheduled-update transactions can have no UA, so
dereferencing `_ua.get_txn()` here crashes instead of reaching the existing
no-UA branch in `setup_internal_transfer()`. Use the buffered path when the
transaction is null.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +964,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
Review Comment:
The direct-send branch skips the former `parse_req()`/`parse_resp()` of the
finalized server request or client response. Those parsers checked
`Content-Length` after header hooks; the request parser also checked `Host`. A
hook can now add conflicting or malformed length fields (or an invalid Host)
and have them encoded to an HTTP/2 peer, changing framing and request handling.
Validate the finalized header by direction before taking this branch, and
preserve the old parser/error handling when it is invalid.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,73 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the
REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) &&
url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing
(RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max =
static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max =
static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c)
{ return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req
makes the exact call.
+ if (method.size() +
static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims
whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max ||
(!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) ||
ParseRules::is_ws(value.back())))) {
Review Comment:
An ordinary HTTP/2 field name can contain `:` after its first character;
neither the header validator nor this gate rejects it. For example,
`content-length:extra: 0` was split at the first colon by the old `parse_req()`
path, but is copied as a distinct field name by this path. That changes request
interpretation and can forward an invalid name to an HTTP/2 origin. Fall back
to serialization and parsing for ordinary names containing `:` (or reject them
at HTTP/2 validation).
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,73 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here; re-apply strict_uri_parsing. Runs after the
REQUEST type
+ // check below, since path_get() asserts that polarity.
+ auto uri_ok = [&]() {
+ int const level = this->_sm->t_state.http_config_param->strict_uri_parsing;
+
+ return level == 0 ||
+ (url_is_uri_compliant(level, _receive_header.path_get()) &&
url_is_uri_compliant(level, _receive_header.query_get()) &&
+ url_is_uri_compliant(level, _receive_header.fragment_get()));
+ };
+
+ // parse_req also enforces token methods, Host and Content-Length framing
(RFC 9110 8.6),
+ // and the request line and header field size limits.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const line_max =
static_cast<size_t>(config->http_request_line_max_size);
+ auto const field_max =
static_cast<size_t>(config->http_hdr_field_max_size);
+ auto method{_receive_header.method_get()};
+
+ if (method.empty() || std::any_of(method.begin(), method.end(), [](char c)
{ return !ParseRules::is_token(c); })) {
+ return false;
+ }
+ // Upper bound of the serialized "METHOD URL HTTP/1.1\r\n", so parse_req
makes the exact call.
+ if (method.size() +
static_cast<size_t>(_receive_header.url_get()->length_get()) + 12 > line_max) {
+ return false;
+ }
+ for (auto const &field : _receive_header) {
+ auto name{field.name_get()};
+ auto value{field.value_get()};
+
+ // parse_req drops a line whose name starts with a non-token and trims
whitespace around values.
+ if (name.empty() || name.size() + value.size() > field_max ||
(!ParseRules::is_token(name.front()) && name.front() != '@') ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) ||
ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ // url_parse_internet() accepts the userinfo that RFC 9113 8.3.1 bans from
:authority,
+ // and the Host built from it never sees validate_hdr_host(). Mirror that
check here.
+ if (MIMEField *host =
_receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_HOST));
host != nullptr) {
+ std::string_view parsed_host;
+ int port = 0;
+ bool has_port = false;
+
+ if (host->has_dups() || !http_parse_host_header(host->value_get(),
parsed_host, port, has_port)) {
+ return false;
+ }
+ }
+ return validate_hdr_content_length(_receive_header.m_heap,
_receive_header.m_http) == ParseResult::DONE;
+ };
+
+ // A failed conversion leaves a \xffVOID method that only parse_req can turn
into a 400.
+ if (conversion_ok && !this->trailing_header_is_possible() &&
!this->is_outbound_connection() &&
+ _receive_header.type_get() == HTTPType::REQUEST && this->_sm != nullptr
&& this->read_vio.nbytes > 0 && uri_ok() &&
+ parse_req_would_accept()) {
+ // The stream owns _receive_header and outlives the handoff, so the pulled
pointer cannot dangle.
+ this->_is_parsed_receive_header_ready = true;
Review Comment:
If `signal_read_event()` cannot lock the HttpSM, the ready flag stays set
until its deferred callback runs. Meanwhile the session can resume reading
frames: a DATA frame marks later HEADERS as trailers, and `rcv_headers_frame()`
calls `reset_receive_headers()` on the same `_receive_header`. HttpSM then
copies the trailer (or an empty RESPONSE header) as its request. Preserve the
initial request in a separate header until HttpSM has copied it, or prevent
trailer decoding from replacing it while the handoff is pending.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +964,33 @@ Http2Stream::update_write_request(bool call_update)
// Process the new data
if (!this->parsing_header_done) {
- // Still parsing the request or response header
int bytes_used = 0;
ParseResult state;
- if (this->is_outbound_connection()) {
+ HTTPHdr *send_hdr = this->_pending_send_header();
+
+ if (send_hdr != nullptr) {
+ // Field-by-field, not copy(): copy() would wipe the create(HTTP_2_0)
pseudos that the
+ // 1.1->2 conversion fills. The ready flag re-arms per header (1xx
interim, retries).
+ if (this->is_outbound_connection()) {
+ this->_send_header.method_set(send_hdr->method_get());
+ this->_send_header.url_set(send_hdr->url_get());
+ } else {
+ this->_send_header.status_set(send_hdr->status_get());
+ }
+ for (auto &field : *send_hdr) {
+ MIMEField *f = this->_send_header.field_create(field.name_get());
+
+ f->value_set(this->_send_header.m_heap, this->_send_header.m_mime,
field.value_get());
+ this->_send_header.field_attach(f);
+ }
Review Comment:
Request or response hooks can add fields after the inbound request gate
runs. The old `parse_req()`/`parse_resp()` trimmed leading and trailing spaces
or tabs from their values; this direct copy keeps those bytes, so the emitted
HTTP/2 header can be rejected by a peer. Trim surrounding SP/HT when copying
each finalized field, as the old parser did.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
