Copilot commented on code in PR #3387:
URL: https://github.com/apache/brpc/pull/3387#discussion_r3585559301
##########
src/brpc/details/http_message.cpp:
##########
@@ -644,12 +654,17 @@ void MakeRawHttpRequest(butil::IOBuf* request,
}
os << BRPC_CRLF;
}
- if (!h->content_type().empty()) {
+ if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) {
os << "Content-Type: " << h->content_type()
<< BRPC_CRLF;
}
Review Comment:
The PR description says dropped headers emit a warning, but a Content-Type
containing CR/LF is currently dropped silently. Logging a warning (without
writing the header) improves observability and keeps behavior consistent with
the other header-skip path.
##########
src/brpc/details/http_message.cpp:
##########
@@ -644,12 +654,17 @@ void MakeRawHttpRequest(butil::IOBuf* request,
}
os << BRPC_CRLF;
}
- if (!h->content_type().empty()) {
+ if (!h->content_type().empty() && !HeaderHasCRLF(h->content_type())) {
os << "Content-Type: " << h->content_type()
<< BRPC_CRLF;
}
for (HttpHeader::HeaderIterator it = h->HeaderBegin();
it != h->HeaderEnd(); ++it) {
+ if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) {
+ LOG(ERROR) << "Skip header `" << it->first
+ << "' containing CR/LF to avoid injection";
+ continue;
+ }
Review Comment:
When the header *name* contains CR/LF, the current LOG statement prints it
verbatim, which can lead to multi-line log injection. Log a sanitized/escaped
form (e.g., butil::ToPrintable) and consider WARNING level since this is
invalid input rather than an internal error.
##########
test/brpc_http_message_unittest.cpp:
##########
@@ -703,6 +703,31 @@ TEST(HttpMessageTest, serialize_http_response) {
<< butil::ToPrintable(response);
}
+TEST(HttpMessageTest, serialize_header_with_crlf_is_not_injected) {
+ // A header value carrying CR/LF must not terminate the current line and
+ // introduce extra header fields (HTTP request/response splitting).
+ butil::EndPoint ep;
+ ASSERT_EQ(0, butil::str2endpoint("127.0.0.1:1234", &ep));
+ butil::IOBuf content;
+ content.append("data");
+
+ brpc::HttpHeader req_header;
+ req_header.set_method(brpc::HTTP_METHOD_POST);
+ req_header.SetHeader("X-Evil", "a\r\nInjected: 1");
+ butil::IOBuf request;
+ MakeRawHttpRequest(&request, &req_header, ep, &content);
+ std::string request_str = request.to_string();
+ ASSERT_EQ(std::string::npos, request_str.find("Injected: 1")) <<
request_str;
+
+ brpc::HttpHeader res_header;
+ res_header.SetHeader("X-Evil", "a\r\nInjected: 1");
+ butil::IOBuf response;
+ content.append("data");
+ MakeRawHttpResponse(&response, &res_header, &content);
+ std::string response_str = response.to_string();
+ ASSERT_EQ(std::string::npos, response_str.find("Injected: 1")) <<
response_str;
+}
Review Comment:
This test covers CR/LF injection via a normal header field, but the PR also
changes Content-Type serialization. Adding explicit cases for Content-Type
(both request + response) would prevent regressions and avoid relying on
moved/modified IOBuf state between calls.
##########
src/brpc/details/http_message.cpp:
##########
@@ -733,12 +748,18 @@ void MakeRawHttpResponse(butil::IOBuf* response,
}
}
}
- if (!is_invalid_content && !h->content_type().empty()) {
+ if (!is_invalid_content && !h->content_type().empty() &&
+ !HeaderHasCRLF(h->content_type())) {
os << "Content-Type: " << h->content_type()
<< BRPC_CRLF;
}
for (HttpHeader::HeaderIterator it = h->HeaderBegin();
it != h->HeaderEnd(); ++it) {
+ if (HeaderHasCRLF(it->first) || HeaderHasCRLF(it->second)) {
+ LOG(ERROR) << "Skip header `" << it->first
+ << "' containing CR/LF to avoid injection";
+ continue;
+ }
Review Comment:
When the header *name* contains CR/LF, the current LOG statement prints it
verbatim, which can lead to multi-line log injection. Log a sanitized/escaped
form (e.g., butil::ToPrintable) and consider WARNING level since this is
invalid input rather than an internal error.
##########
src/brpc/details/http_message.cpp:
##########
@@ -733,12 +748,18 @@ void MakeRawHttpResponse(butil::IOBuf* response,
}
}
}
- if (!is_invalid_content && !h->content_type().empty()) {
+ if (!is_invalid_content && !h->content_type().empty() &&
+ !HeaderHasCRLF(h->content_type())) {
os << "Content-Type: " << h->content_type()
<< BRPC_CRLF;
}
Review Comment:
The PR description says dropped headers emit a warning, but a Content-Type
containing CR/LF is currently dropped silently in responses. Consider logging a
warning when Content-Type is rejected so operators can detect
malformed/untrusted upstream values.
--
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]
