zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3686922668
##########
src/proxy/hdrs/URL.cc:
##########
@@ -1188,6 +1188,22 @@ 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:
You're right, and I was wrong to act on this. `nullptr + 0` is well-defined
— [expr.add]/4 says adding 0 to a null pointer yields a null pointer, so `start
+ value.length()` with an empty view was never UB. The guard is reverted; the
empty case already fell out of the `i < end` loop conditions in
`url_is_strictly_compliant()` / `url_is_mostly_compliant()`, so behavior is
identical either way and the branch was pure noise.
My mistake was taking the Copilot finding at face value instead of checking
the standard. Thanks for the citation.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -326,6 +365,50 @@ Http2Stream::send_headers(Http2ConnectionState & /* cstate
ATS_UNUSED */)
this->_http_sm_id = this->_sm->sm_id;
}
+ // The fast path skips parse_req, so re-apply strict_uri_parsing on the
target.
+ // Evaluated last so path_get() (asserts REQUEST polarity) only sees a
REQUEST.
+ 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 a token method and Content-Length framing (RFC
9110 8.6),
+ // which the fast path would otherwise skip.
+ auto parse_req_would_accept = [&]() {
+ 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;
+ }
+ if (MIMEField *cl =
_receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_CONTENT_LENGTH));
cl != nullptr) {
+ auto value{cl->value_get()};
+ if (cl->has_dups() || value.empty() || std::any_of(value.begin(),
value.end(), [](char c) { return c < '0' || c > '9'; }) ||
+
_receive_header.field_find(static_cast<std::string_view>(MIME_FIELD_TRANSFER_ENCODING))
!= nullptr) {
+ return false;
+ }
+ }
+ return true;
+ };
Review Comment:
Confirmed and fixed. `url_parse_internet()` splits `user@host` into userinfo
+ host and validates only the host part, so the conversion succeeded and the
Host header kept the raw `:authority` value — which then never reached
`validate_hdr_host()`. The legacy path 400s it because
`http_parse_host_header()` → `validate_host_name()` rejects '@' (not in
`is_host_char`).
Rather than hand-roll the character check, `parse_req_would_accept()` now
calls `http_parse_host_header()` on the Host value and rejects duplicates,
which is exactly what `validate_hdr_host()` does — so parity is by construction
rather than by a second implementation that can drift. Such requests fall back
to serialize+parse and get the same 400 as before.
Worth noting the userinfo is itself an RFC 9113 8.3.1 violation
(`:authority` MUST NOT include it), so rejecting is correct on two counts.
--
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]
