zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4138603214
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,70 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped here, so re-apply its checks. Both run after the
REQUEST type check
+ // below, since the accessors assert 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()));
+ };
+
+ 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:
Right, thanks. mime_parser_parse() checks every character of a name that
isn't a well-known header. The gate now falls back to parse_req for any name
with a non-field-name character, which also covers ':'. Added an x(foo case
that expects a 400 and checks the origin never sees it.
##########
src/proxy/http/HttpSM.cc:
##########
@@ -7196,7 +7213,15 @@ HttpSM::setup_server_send_request()
// We need a reader so bytes don't fall off the end of
// the buffer
IOBufferReader *buf_start = server_entry->write_buffer->alloc_reader();
- server_request_hdr_bytes = hdr_length =
write_header_into_buffer(&t_state.hdr_info.server_request,
server_entry->write_buffer);
+
+ if (server_txn->supports_direct_header_passing()) {
+ _server_request_header_is_ready = true;
+ // Only the write buffer is empty; retry decisions and stats need the real
size.
+ server_request_hdr_bytes = t_state.hdr_info.server_request.length_get();
+ hdr_length = 0;
Review Comment:
Not changing this. On the previous HTTP/2 path, do_io_write() reparsed the
header synchronously and advanced ndone before any timeout could fire, so a
stalled stream was already classified as INACTIVE_TIMEOUT. The direct path
gives the same classification. The only difference is a cross-thread deferral
window of microseconds, and the effect there is not retrying, which can't
duplicate a request.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,68 @@ 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) {
+ if (field.name_get().size() + field.value_get().size() > field_max) {
+ return false;
+ }
+ }
Review Comment:
Correction to my earlier reply: parse_req() does reject x(foo. For names
that aren't well-known headers, mime_parser_parse() checks every character with
is_http_field_name(). The gate now falls back to parse_req for any such name.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -850,15 +959,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:
Correction to my earlier reply: the previous outbound reparse would have
rejected x(foo, not forwarded it. The request fails either way (the HTTP/2
origin rejects the invalid name), and the name comes from plugin code, so I'm
still leaving the outbound copy as it is.
--
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]
