zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4149703355
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1222,14 +1222,27 @@ url_is_mostly_compliant(const char *start, const char
*end)
} // namespace UrlImpl
using namespace UrlImpl;
+bool
+url_is_uri_compliant(int strict_uri_parsing, std::string_view value)
+{
+ const char *start = value.data();
+ const char *end = start + value.length();
+
+ switch (strict_uri_parsing) {
+ case 1:
+ return url_is_strictly_compliant(start, end);
+ case 2:
+ return url_is_mostly_compliant(start, end);
+ default:
+ return true;
+ }
+}
Review Comment:
The current code is correct: C++ defines `nullptr + 0` as a null pointer
([expr.add]/4.1), two null pointers compare equal so `i < end` is false, and
neither compliance loop dereferences outside its body. We added the
`value.empty()` early return anyway, since it makes that explicit.
##########
include/proxy/http/HttpSM.h:
##########
@@ -636,6 +636,12 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
IOBufferReader *_netvc_reader = nullptr;
MIOBuffer *_netvc_read_buffer = nullptr;
+ bool _client_response_header_is_ready = false;
+ bool _server_request_header_is_ready = false;
+
+ // Direct-passed headers bypass the tunnel, so client_response_hdr_bytes
stays 0.
+ int _direct_response_hdr_bytes = 0;
+
Review Comment:
Same as the earlier threads on this: every HttpSM header byte counter is an
`int`, and so are `TSHttpTxnClientRespHdrBytesGet()` and
`HTTPHdr::length_get()`, which `_direct_response_hdr_bytes` is set from. An
`int64_t` here couldn't hold a larger value than its `int` source, so we're
keeping `int`.
##########
include/proxy/http/HttpSM.h:
##########
@@ -652,6 +658,25 @@ class HttpSM : public Continuation, public
PluginUserArgs<TS_USER_ARGS_TXN>
int client_transaction_priority_weight() const;
int client_transaction_priority_dependence() const;
+ HTTPHdr *get_client_response_header();
+ HTTPHdr *get_server_request_header();
+
+ int
+ reported_client_response_hdr_bytes() const
+ {
+ return client_response_hdr_bytes + _direct_response_hdr_bytes;
+ }
Review Comment:
Same as the earlier threads on this: every HttpSM header byte counter is an
`int`, and so are `TSHttpTxnClientRespHdrBytesGet()` and
`HTTPHdr::length_get()`, which `_direct_response_hdr_bytes` is set from. An
`int64_t` here couldn't hold a larger value than its `int` source, so we're
keeping `int`.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -327,6 +372,53 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // parse_req is skipped on this path, so re-apply its checks. Call it only
after the REQUEST
+ // type check below: the accessors assert it.
+ auto parse_req_would_accept = [&]() {
+ auto const *config = this->_sm->t_state.http_config_param;
+ auto const level = config->strict_uri_parsing;
+ 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::ranges::all_of(method, &ParseRules::is_token))
{
+ return false;
+ }
+ if (!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())) {
+ 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 or rejects a name with any non-field-name character,
and trims values.
+ if (name.empty() || name.size() + value.size() > field_max ||
!std::ranges::all_of(name, &ParseRules::is_http_field_name) ||
+ (!value.empty() && (ParseRules::is_ws(value.front()) ||
ParseRules::is_ws(value.back())))) {
+ return false;
+ }
+ }
+ return validate_hdr_host(_receive_header.m_http) == ParseResult::DONE &&
+ validate_hdr_content_length(_receive_header.m_heap,
_receive_header.m_http) == ParseResult::DONE;
+ };
Review Comment:
Done: the checks now live in `HTTPHdr::parse_req_would_accept()` in HTTP.cc,
next to the `validate_hdr_host()` and `validate_hdr_content_length()` it calls.
`HdrParseReqWouldAccept` in test_Hdrs.cc covers each rejection and the
field-size boundary.
--
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]
