Copilot commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r4147829841
##########
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:
std::string_view::data()` may be `nullptr` for an empty view (e.g.,
default-constructed), and the subsequent pointer arithmetic / relational
comparisons inside the compliance helpers can become undefined behavior. This
is particularly relevant because callers can legitimately pass empty
path/query/fragment views. Fix by handling `value.empty()` early (e.g., return
`true` for strictness levels that treat empty as acceptable, or pass a non-null
stable pointer for the empty case) so `start/end` are always derived from a
valid buffer.
##########
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:
reported_client_response_hdr_bytes()` and `_direct_response_hdr_bytes` are
`int`, but the surrounding counters/APIs are `int64_t` (e.g.,
`TSHttpTxnClientRespHdrBytesGet`, transaction logging). This can truncate large
values and breaks type consistency. Use `int64_t` (or the same typedef/type as
`client_response_hdr_bytes`) for `_direct_response_hdr_bytes` and make
`reported_client_response_hdr_bytes()` return `int64_t` to avoid
overflow/truncation.
##########
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:
reported_client_response_hdr_bytes()` and `_direct_response_hdr_bytes` are
`int`, but the surrounding counters/APIs are `int64_t` (e.g.,
`TSHttpTxnClientRespHdrBytesGet`, transaction logging). This can truncate large
values and breaks type consistency. Use `int64_t` (or the same typedef/type as
`client_response_hdr_bytes`) for `_direct_response_hdr_bytes` and make
`reported_client_response_hdr_bytes()` return `int64_t` to avoid
overflow/truncation.
##########
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:
This embeds a sizable re-implementation of `parse_req`-adjacent validation
in a lambda inside `send_headers()`, which will be hard to keep in sync with
future parser/security changes. Consider extracting this into a shared helper
(e.g., a utility function near `HTTPHdr::parse_req`/validation code or the
version-converter layer) so it can be reused and updated centrally, and
unit-tested directly.
--
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]
