Copilot commented on code in PR #3524:
URL: https://github.com/apache/brpc/pull/3524#discussion_r3950808322
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1396,24 +1422,34 @@ int
H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
char* endptr = nullptr;
const int sc = strtol(pair.value.c_str(), &endptr, 10);
if (*endptr != '\0') {
- LOG(ERROR) << "Invalid status=" << pair.value;
- return -1;
+ LOG(ERROR) << "Invalid status=" << pair.value
+ << ", stream_id=" << _stream_id;
+ _rejected_error = H2_PROTOCOL_ERROR;
+ } else {
+ h.set_status_code(sc);
}
- h.set_status_code(sc);
}
break;
default:
break;
}
if (!matched) {
- LOG(ERROR) << "Unknown name=`" << name << '\'';
- return -1;
+ LOG(ERROR) << "Unknown pseudo-header=`" << name
+ << "', stream_id=" << _stream_id;
+ _rejected_error = H2_PROTOCOL_ERROR;
}
} else if (name[0] == 'c' &&
strcmp(name + 1, /*c*/"ontent-type") == 0) {
h.set_content_type(pair.value);
} else {
h.AppendHeader(pair.name, pair.value);
+ if (FLAGS_http_max_header_count > 0 &&
+ h.HeaderCount() > FLAGS_http_max_header_count) {
+ LOG(ERROR) << "Too many headers, max="
+ << FLAGS_http_max_header_count
+ << ", stream_id=" << _stream_id;
Review Comment:
Similar to the HTTP/1 path, this introduces an unconditional `LOG(ERROR)` on
a client-controlled condition. Consider rate-limiting/sampling to avoid
log-flooding during attacks; the stream can still be rejected via
`_rejected_error` without emitting per-request ERROR logs.
##########
src/brpc/details/http_message.cpp:
##########
@@ -131,6 +134,14 @@ int HttpMessage::on_header_value(http_parser *parser,
http_message->_cur_value =
&header.AddHeader(http_message->_cur_header);
}
+
+ if (FLAGS_http_max_header_count > 0 &&
+ header.HeaderCount() > FLAGS_http_max_header_count) {
+ LOG(ERROR) << "Too many headers, max="
+ << FLAGS_http_max_header_count;
+ return -1;
+ }
Review Comment:
This adds a new unconditional `LOG(ERROR)` on a client-triggerable parse
failure. Since the intent is to mitigate DoS, attackers can likely turn this
into log amplification. Consider rate-limiting (e.g., `LOG_EVERY_N`,
`LOG_FIRST_N`, or a sampled/throttled logger) and/or lowering severity (e.g.,
WARNING) while still returning `-1`.
##########
src/brpc/uri.cpp:
##########
@@ -17,25 +17,25 @@
#include <ctype.h> // isalnum
-
#include <unordered_set>
-
+#include <gflags/gflags.h>
#include "brpc/log.h"
#include "brpc/details/http_parser.h" // http_parser_parse_url
#include "brpc/uri.h" // URI
namespace brpc {
+DEFINE_uint32(http_max_query_count, 1000,
+ "Reject an URL carrying more than so many query parameters. "
+ "0 lifts the limit.");
Review Comment:
PR description says \"Setting flag to <= 0 lifts the limit\", but the
implementation uses `DEFINE_uint32`, so negative values are impossible and only
`0` lifts the limit. Either update the PR description/documentation to say \"0
lifts the limit\" everywhere, or change the flag type to a signed integer and
implement the `<= 0` behavior.
##########
src/brpc/uri.h:
##########
@@ -99,8 +99,9 @@ class URI {
void set_port(int port) { _port = port; }
void SetHostAndPort(const std::string& host_and_optional_port);
// Set path/query/fragment with the input in form of "path?query#fragment"
- void SetH2Path(const char* h2_path);
- void SetH2Path(const std::string& path) { SetH2Path(path.c_str()); }
+ // Returns 0 on success, -1 otherwise and status() is set.
+ int SetH2Path(const char* h2_path);
+ int SetH2Path(const std::string& path) { return SetH2Path(path.c_str()); }
Review Comment:
Changing `SetH2Path` from `void` to `int` is a public API break for
downstream users including source compatibility (calls used in statement
position won’t compile). If backward compatibility is required, consider
restoring the original `void SetH2Path(...)` signature as a wrapper (that sets
status and ignores the return), and add a new
`TrySetH2Path(...)`/`SetH2PathAndGetStatus(...)` (or similar) that returns
`int`.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
Review Comment:
Using `ASSERT_*` inside a helper function can produce surprising control
flow: it only returns from the helper, and the calling test continues
executing. Consider changing these to `EXPECT_*` (so the test continues but
records the failure), or have the helper return `bool/Status` and
`ASSERT_TRUE(...)` in the test body where failure should abort the test.
##########
test/brpc_http_rpc_protocol_unittest.cpp:
##########
@@ -1940,6 +1942,231 @@ TEST_F(HttpTest,
h2_header_list_budget_resets_per_block) {
delete sctx;
}
+// Literal header field with a new name, with both lengths in a single 7-bit
+// prefix octet. `first_octet` selects the representation: 0x00 is "without
+// indexing" (RFC 7541 6.2.2), 0x40 is "with incremental indexing" (6.2.1)
+// which also adds the field to the dynamic table.
+// 0x80 of a length octet is the Huffman flag and a length of 128 or more needs
+// the multi-octet form, so refuse what does not fit instead of emitting a
+// corrupt header block.
+void AppendLiteralHeader(butil::IOBuf* out, const std::string& name,
+ const std::string& value, uint8_t first_octet = 0x00)
{
+ ASSERT_LT(name.size(), 0x80u);
+ ASSERT_LT(value.size(), 0x80u);
+ uint8_t prefix[] = { first_octet, (uint8_t)name.size() };
+ out->append(prefix, sizeof(prefix));
+ out->append(name);
+ uint8_t value_len = (uint8_t)value.size();
+ out->append(&value_len, 1);
+ out->append(value);
+}
+
+// Feed `payload` to `sctx` as one complete HEADERS block, the way
+// H2Context::Consume() would. A non-zero stream_id in the result means the
+// frame handler asked for a RST_STREAM, a zero one means a GOAWAY that closes
+// the whole connection.
+brpc::policy::H2ParseResult ConsumeHeadersBlock(
+ brpc::policy::H2StreamContext* sctx, const butil::IOBuf& payload,
+ int stream_id) {
+ brpc::policy::H2FrameHead head;
+ head.payload_size = payload.size();
+ head.type = brpc::policy::H2_FRAME_HEADERS;
+ head.flags = 0x4; // H2_FLAGS_END_HEADERS
+ head.stream_id = stream_id;
Review Comment:
`payload.size()` is `size_t` and `head.payload_size` is likely a fixed-width
integer; this assignment can cause narrowing warnings on some toolchains.
Consider an explicit cast (and/or an `ASSERT_LE(payload.size(),
max_payload_size)` if applicable) to keep the test warning-clean.
--
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]
