This is an automated email from the ASF dual-hosted git repository.
lizhimins pushed a commit to branch rocketmq-studio
in repository https://gitbox.apache.org/repos/asf/rocketmq-dashboard.git
The following commit(s) were added to refs/heads/rocketmq-studio by this push:
new 09f0adcad fix(message-history): scope result snapshots to operator
(#3213)
09f0adcad is described below
commit 09f0adcadc95cbc98f52df9f29c258c4aec2d967
Author: btlqql <[email protected]>
AuthorDate: Tue Sep 15 19:26:48 2026 +0800
fix(message-history): scope result snapshots to operator (#3213)
* fix(message-history): scope result snapshots to operator
`getMessageQueryResults` loaded the stored snapshot with
`messageQueryMapper.selectById(id)`, with no ownership predicate, while
every
other read path in the same service scopes by `eq("queried_by", queriedBy)`.
It was the only by-id read over query history that ignored the operator.
`id` is an auto-increment key, so it is enumerable, and `result_snapshot`
carries the message rows including `bornHost`, `storeHost` and `brokerName`
alongside the topic names. Any authenticated operator could therefore walk
ids
and read another operator's results together with the cluster topology
embedded
in them.
The lookup is now scoped to the current operator. A record owned by another
operator is reported as not found rather than forbidden: a 403 would confirm
that the record exists and turn enumeration into a directory of valid ids.
`QueryHistoryServiceTest` covers the owner path, the other-operator path
and the
bound parameters.
Fixes #4007
* fix(message-history): address review feedback
Assert the 404 code, not only the exception type and message. The code is
the
substance of `new BusinessException(404, ...)`; without pinning it, a later
change of 404 to 500 — or a split of "not found" from "not yours" into
404/403
that reintroduces the existence leak — would leave both tests green.
Use the static `assertThatThrownBy` import the file already uses for
`assertThat`, state the operator scoping in the `getMessageQueryResults`
Javadoc so the deliberate 404 is not mistaken for an oversight, fix the
import
ordering, and drop the fully-qualified `BusinessException` at the production
throw site now that the type is imported.
---
.../instance/message/QueryHistoryService.java | 13 ++++--
.../instance/message/QueryHistoryServiceTest.java | 47 +++++++++++++++++++++-
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git
a/server/src/main/java/org/apache/rocketmq/studio/instance/message/QueryHistoryService.java
b/server/src/main/java/org/apache/rocketmq/studio/instance/message/QueryHistoryService.java
index 759888de4..98055144c 100644
---
a/server/src/main/java/org/apache/rocketmq/studio/instance/message/QueryHistoryService.java
+++
b/server/src/main/java/org/apache/rocketmq/studio/instance/message/QueryHistoryService.java
@@ -29,6 +29,7 @@ import
org.apache.rocketmq.studio.persistence.entity.RmqTraceQuery;
import org.apache.rocketmq.studio.persistence.mapper.RmqMessageQueryMapper;
import org.apache.rocketmq.studio.persistence.mapper.RmqTraceQueryMapper;
import org.apache.rocketmq.studio.common.domain.PageResult;
+import org.apache.rocketmq.studio.common.exception.BusinessException;
import org.springframework.scheduling.annotation.Scheduled;
import org.springframework.stereotype.Service;
import org.springframework.util.StringUtils;
@@ -129,12 +130,18 @@ public class QueryHistoryService {
}
/**
- * Retrieves the stored result snapshot for a given history record.
+ * Retrieves the stored result snapshot for a history record owned by the
authenticated operator.
+ *
+ * <p>The lookup is scoped to the current authenticated operator. A record
owned by another
+ * operator is deliberately reported as not found (404) to prevent history
id enumeration.
*/
public List<MessageRecordVO> getMessageQueryResults(long id) {
- RmqMessageQuery query = messageQueryMapper.selectById(id);
+ String queriedBy = AuthenticatedUserContext.currentUsernameOrSystem();
+ RmqMessageQuery query = messageQueryMapper.selectOne(new
QueryWrapper<RmqMessageQuery>()
+ .eq("id", id)
+ .eq("queried_by", queriedBy));
if (query == null) {
- throw new
org.apache.rocketmq.studio.common.exception.BusinessException(404, "Query
history record not found");
+ throw new BusinessException(404, "Query history record not found");
}
String snapshot = query.getResultSnapshot();
if (!StringUtils.hasText(snapshot)) {
diff --git
a/server/src/test/java/org/apache/rocketmq/studio/instance/message/QueryHistoryServiceTest.java
b/server/src/test/java/org/apache/rocketmq/studio/instance/message/QueryHistoryServiceTest.java
index b29359f82..3d52d789d 100644
---
a/server/src/test/java/org/apache/rocketmq/studio/instance/message/QueryHistoryServiceTest.java
+++
b/server/src/test/java/org/apache/rocketmq/studio/instance/message/QueryHistoryServiceTest.java
@@ -17,10 +17,12 @@
package org.apache.rocketmq.studio.instance.message;
import com.baomidou.mybatisplus.core.conditions.Wrapper;
+import com.baomidou.mybatisplus.core.conditions.query.QueryWrapper;
import com.baomidou.mybatisplus.extension.plugins.pagination.Page;
import com.fasterxml.jackson.databind.ObjectMapper;
-import org.apache.rocketmq.studio.common.domain.PageResult;
import org.apache.rocketmq.studio.auth.AuthenticatedUserContext;
+import org.apache.rocketmq.studio.common.domain.PageResult;
+import org.apache.rocketmq.studio.common.exception.BusinessException;
import org.apache.rocketmq.studio.persistence.entity.RmqMessageQuery;
import org.apache.rocketmq.studio.persistence.entity.RmqTraceQuery;
import org.apache.rocketmq.studio.persistence.mapper.RmqMessageQueryMapper;
@@ -36,6 +38,7 @@ import java.time.ZoneOffset;
import java.util.List;
import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
@@ -241,6 +244,48 @@ class QueryHistoryServiceTest {
assertThat(queryCaptor.getValue().getCustomSqlSegment()).contains("queried_by");
}
+ @Test
+ void loadsResultSnapshotOnlyForTheAuthenticatedOperatorTest() {
+ AuthenticatedUserContext.setUsername("alice");
+ RmqMessageQuery entity = new RmqMessageQuery();
+ entity.setId(9L);
+ entity.setQueriedBy("alice");
+
entity.setResultSnapshot("[{\"msgId\":\"msg-9\",\"topic\":\"orders\"}]");
+ when(messageQueryMapper.selectOne(any())).thenReturn(entity);
+
+ List<MessageRecordVO> results = service.getMessageQueryResults(9L);
+
+ assertThat(results).singleElement().satisfies(result -> {
+ assertThat(result.getMsgId()).isEqualTo("msg-9");
+ assertThat(result.getTopic()).isEqualTo("orders");
+ });
+ ArgumentCaptor<QueryWrapper<RmqMessageQuery>> queryCaptor =
ArgumentCaptor.forClass(QueryWrapper.class);
+ verify(messageQueryMapper).selectOne(queryCaptor.capture());
+ assertThat(queryCaptor.getValue().getCustomSqlSegment())
+ .contains("id", "queried_by");
+ assertThat(queryCaptor.getValue().getParamNameValuePairs().values())
+ .contains(9L, "alice");
+ }
+
+ @Test
+ void hidesResultSnapshotOwnedByAnotherOperatorTest() {
+ AuthenticatedUserContext.setUsername("bob");
+ when(messageQueryMapper.selectOne(any())).thenReturn(null);
+
+ assertThatThrownBy(() -> service.getMessageQueryResults(9L))
+ .isInstanceOf(BusinessException.class)
+ .hasMessage("Query history record not found")
+ .extracting(e -> ((BusinessException) e).getCode())
+ .isEqualTo(404);
+
+ ArgumentCaptor<QueryWrapper<RmqMessageQuery>> queryCaptor =
ArgumentCaptor.forClass(QueryWrapper.class);
+ verify(messageQueryMapper).selectOne(queryCaptor.capture());
+ assertThat(queryCaptor.getValue().getCustomSqlSegment())
+ .contains("id", "queried_by");
+ assertThat(queryCaptor.getValue().getParamNameValuePairs().values())
+ .contains(9L, "bob");
+ }
+
@Test
void summarizesBothHistoryStreams() {
AuthenticatedUserContext.setUsername("alice");
