Copilot commented on code in PR #3521:
URL: https://github.com/apache/brpc/pull/3521#discussion_r3932462400
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -426,6 +428,174 @@ TEST(HttpMessageTest, cl_and_te) {
}
}
+brpc::http_errno ParseHttpErrno(const char* buf) {
+ butil::IOBuf data;
+ data.append(buf);
+ brpc::HttpMessage http_message;
+ if (http_message.ParseFromIOBuf(data) == (ssize_t)data.size() &&
+ http_message.Completed()) {
+ return brpc::HPE_OK;
+ }
+ brpc::http_errno err = (brpc::http_errno)http_message._parser.http_errno;
+ return err != brpc::HPE_OK ? err : brpc::HPE_UNKNOWN;
+}
+
+TEST(HttpMessageTest, space_before_colon_of_framing_headers) {
+ const char* rejected[] = {
+ "POST / HTTP/1.1\r\nHost: a.com\r\nContent-Length : 5\r\n\r\nhello",
+ "POST / HTTP/1.1\r\nHost: a.com\r\nTransfer-Encoding :
chunked\r\n\r\n0\r\n\r\n",
+ "GET / HTTP/1.1\r\nHost: a.com\r\nConnection : close\r\n\r\n",
+ "GET / HTTP/1.1\r\nHost: a.com\r\nUpgrade : h2c\r\n\r\n",
+ };
+ for (size_t i = 0; i < arraysize(rejected); ++i) {
+ ASSERT_EQ(brpc::HPE_INVALID_HEADER_TOKEN, ParseHttpErrno(rejected[i]))
+ << rejected[i];
+ }
+
+ // Names without framing semantics keep the historical leniency.
+ ASSERT_FALSE(brpc::FLAGS_http_strict_header_token);
+ ASSERT_EQ(brpc::HPE_OK, ParseHttpErrno(
Review Comment:
This test assumes the process is started with the default value of
--http_strict_header_token (it asserts the flag is false). If someone runs the
test binary with --http_strict_header_token=true, this will fail even though
the parser behavior under test is unrelated to the binary’s flag configuration.
Prefer setting the flag explicitly (and ideally using FlagSaver for isolation)
instead of asserting the default.
This issue also appears on line 546 of the same file.
##########
src/brpc/details/http_parser.cpp:
##########
@@ -1633,10 +1685,18 @@ size_t http_parser_execute (http_parser *parser,
break;
case h_content_length:
+ if (ch == ' ') break;
+ parser->header_state = h_content_length_num;
+ /* FALLTHROUGH */
Review Comment:
Content-Length parsing treats only SP as optional whitespace. RFC 7230 OWS
includes HTAB as well, and the nearby comment refers to OWS. Consider accepting
'\t' consistently wherever trailing whitespace is handled for Content-Length.
This issue also appears in the following locations of the same file:
- line 1696
- line 1724
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
