[ISSUE #10760] Avoid logging full system message data - #10761
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens Proxy system-message failure logging (Issue #10760) by avoiding logging full system-message payload objects, replacing them with a compact summary and adding a regression test around heartbeat summaries.
Changes:
- Replace error logs in
AbstractSystemMessageSyncer.sendSystemMessageto log a compactdataSummaryinstead of fulldata. - Add
summarizeSystemMessageDatawith special handling forHeartbeatSyncerDatato avoid dumping subscription/channel details. - Add a unit test asserting the summary includes minimal fields and excludes sensitive heartbeat payload contents.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| proxy/src/main/java/org/apache/rocketmq/proxy/service/sysmessage/AbstractSystemMessageSyncer.java | Switch failure logs to dataSummary and introduce a heartbeat-aware summarizer. |
| proxy/src/test/java/org/apache/rocketmq/proxy/service/sysmessage/HeartbeatSyncerTest.java | Add regression coverage ensuring heartbeat summaries don’t include subscription/channel payload details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return data.getClass().getSimpleName(); | ||
| } |
| return "HeartbeatSyncerData{" | ||
| + "heartbeatType=" + heartbeatData.getHeartbeatType() | ||
| + ", clientId=" + heartbeatData.getClientId() | ||
| + ", group=" + heartbeatData.getGroup() | ||
| + ", subscriptionCount=" + subscriptionCount | ||
| + ", channelDataPresent=" + (heartbeatData.getChannelData() != null) | ||
| + '}'; |
| protected void sendSystemMessage(Object data) { | ||
| String targetTopic = this.getBroadcastTopicName(); | ||
| String dataSummary = summarizeSystemMessageData(data); | ||
| try { |
| if (throwable != null) { | ||
| log.error("send system message failed. data: {}, topic: {}", data, getBroadcastTopicName(), throwable); | ||
| log.error("send system message failed. dataSummary: {}, topic: {}", | ||
| dataSummary, getBroadcastTopicName(), throwable); | ||
| return; | ||
| } | ||
| if (SendStatus.SEND_OK != result.getSendStatus()) { | ||
| log.error("send system message failed. data: {}, topic: {}, sendResult:{}", data, getBroadcastTopicName(), result); | ||
| log.error("send system message failed. dataSummary: {}, topic: {}, sendResult:{}", | ||
| dataSummary, getBroadcastTopicName(), result); | ||
| } |
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Replaces full system-message data logging in AbstractSystemMessageSyncer failure paths with a compact summarizeSystemMessageData() method, preventing sensitive subscription details and channel data from appearing in error logs.
Findings
- [Info]
proxy/src/main/java/.../sysmessage/AbstractSystemMessageSyncer.java:113-145— ThesummarizeSystemMessageDatamethod is well-structured: null-safe, type-specific forHeartbeatSyncerData, and falls back to class simple name for unknown types. The null check ongetSubscriptionDataSet()is correct. - [Info] The fallback path (non-HeartbeatSyncerData types) returns only
getClass().getSimpleName(), which intentionally sacrifices debug detail for safety. This is an acceptable security trade-off. If other message types need richer summaries in the future, the method is easy to extend. - [Info]
proxy/src/test/java/.../sysmessage/HeartbeatSyncerTest.java— Thorough regression test verifying that sensitive fields (topic, tag, channel data) are excluded while diagnostic fields (type, clientId, group, subscriptionCount) are retained.
Verdict
Clean security fix with good test coverage and a well-designed summary method. LGTM.
Automated review by github-manager-bot
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #10761 +/- ##
=============================================
- Coverage 48.60% 48.50% -0.11%
+ Complexity 13690 13654 -36
=============================================
Files 1381 1381
Lines 101464 101479 +15
Branches 13187 13191 +4
=============================================
- Hits 49318 49221 -97
- Misses 46158 46240 +82
- Partials 5988 6018 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM. Code changes look good.
Automated review by github-manager-bot
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix with proper validation and test coverage. LGTM.
Automated review by github-manager-bot
|
Rebased onto the latest develop and verified the review points are present: heartbeat summaries contain only type/count/presence diagnostics, summary generation is defensive, callback logs retain the captured target topic, and anonymous classes fall back to their fully-qualified class name. Verified with: mvn -q -pl proxy -am -Dtest=HeartbeatSyncerTest -Dsurefire.failIfNoSpecifiedTests=false test |
bb03dc2 to
634950a
Compare
|
Consolidated into #10805 to keep heartbeat and system-message sync logging changes in one review. |
Fixes #10760.
What changed
datalogging inAbstractSystemMessageSyncerfailure paths with a compactdataSummary.HeartbeatSyncerDatawithout dumping subscription details or channel data.Validation
JAVA_HOME=$(/usr/libexec/java_home -v 1.8) mvn -pl proxy -Dtest=HeartbeatSyncerTest test