Copilot commented on code in PR #10803:
URL: https://github.com/apache/rocketmq/pull/10803#discussion_r3705549698
##########
proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/common/ResponseWriter.java:
##########
@@ -52,16 +54,17 @@ public <T> boolean writeResponse(StreamObserver<T>
observer, final T response) {
if (null == response) {
return false;
}
- log.debug("start to write response. response: {}", response);
+ String responseSummary = summarizeResponse(response);
+ log.debug("start to write response. response: {}", responseSummary);
if (isCancelled(observer)) {
- log.warn("client has cancelled the request. response to write:
{}", response);
+ log.warn("client has cancelled the request. response to write:
{}", responseSummary);
Review Comment:
responseSummary is computed unconditionally (including protobuf descriptor
lookups) even when debug/warn logging is disabled. Consider guarding the
summary construction behind the relevant log-level checks (and only computing
it on the cancellation path for the warn log) to avoid per-request overhead in
hot paths.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/common/ResponseWriter.java:
##########
@@ -76,5 +79,32 @@ public <T> boolean isCancelled(StreamObserver<T> observer) {
}
return false;
}
-}
+ static String summarizeResponse(Object response) {
+ if (response == null) {
+ return "null";
+ }
+ StringBuilder summary = new
StringBuilder(response.getClass().getSimpleName());
+ if (response instanceof Message) {
+ appendStatusCode(summary, (Message) response);
+ }
+ return summary.toString();
+ }
+
+ private static void appendStatusCode(StringBuilder summary, Message
response) {
+ Descriptors.FieldDescriptor statusField =
response.getDescriptorForType().findFieldByName("status");
+ if (statusField == null || !response.hasField(statusField)) {
+ return;
+ }
+ Object status = response.getField(statusField);
+ if (!(status instanceof Message)) {
+ summary.append("{status=").append(status).append('}');
+ return;
+ }
Review Comment:
This fallback appends the full `status` object via `toString()` when it
isn't a protobuf `Message`, which can reintroduce sensitive-data exposure (the
exact issue this PR aims to avoid). To keep summaries payload-free, avoid
interpolating arbitrary objects here; instead, append a non-sensitive indicator
(e.g., only note that status is present, or include only the status object's
class name).
--
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]
