fix(conversation): exclude malformed conversationIds from admin list/page

conversationId ending in ":" (e.g. webchat:<key8>: with empty visitorId,
from older webchat versions) leaks into the admin console via the
'webchat:%' username LIKE, then 500/403s on open because the trailing ":"
makes some reverse proxies strip the path tail — landing a GET on the
@DeleteMapping variant of /{conversationId} (issue #369).

Add applyMalformedIdGuard — a NOT LIKE '%:' clause — to both listConversations
(lenient + strict overloads) and pageConversations so these rows never
surface. isConversationOwner already rejects unknown ids with 403, so no
change is needed on the direct-access endpoints; once the rows are out of
the lists, admin can no longer reach them.

The two existing strict/non-admin assertions changed from "no LIKE keyword"
to "no webchat:% param value" — applyMalformedIdGuard emits a NOT LIKE
itself, so the LIKE keyword is now present in every query.

Tests cover the guard on lenient, page, and strict paths.
This commit is contained in:
倪程伟 2026-06-18 11:38:09 +08:00 committed by matevip
parent ffef9bab00
commit f70e56cfc3
2 changed files with 78 additions and 8 deletions

View File

@ -150,6 +150,7 @@ public class ConversationService {
boolean includeWebchat = includeChannelPrincipals && isGlobalAdmin(username);
LambdaQueryWrapper<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
.and(w -> applyOwnerScope(w, username, includeWebchat))
.and(this::applyMalformedIdGuard)
.isNull(ConversationEntity::getParentConversationId)
.orderByDesc(ConversationEntity::getPinned)
.orderByDesc(ConversationEntity::getLastActiveTime);
@ -203,6 +204,17 @@ public class ConversationService {
}
}
/**
* Exclude rows whose conversationId ends in ":" malformed (e.g.
* {@code webchat:<key8>:} with empty visitorId, from older versions).
* Showing them in the console surfaces threads that 500/403 on open
* because the trailing ":" makes some reverse proxies strip the path
* tail (issue #369).
*/
private void applyMalformedIdGuard(LambdaQueryWrapper<ConversationEntity> w) {
w.notLike(ConversationEntity::getConversationId, "%:");
}
/**
* Whether the user is a global admin (role=admin), resolved from the DB
* never from client-controlled data. Gates webchat row visibility in the
@ -239,6 +251,7 @@ public class ConversationService {
// conversation (issue #344), so non-admins must not see those rows.
LambdaQueryWrapper<ConversationEntity> wrapper = new LambdaQueryWrapper<ConversationEntity>()
.and(w -> applyOwnerScope(w, username, isGlobalAdmin(username)))
.and(this::applyMalformedIdGuard)
.isNull(ConversationEntity::getParentConversationId)
.orderByDesc(ConversationEntity::getPinned)
.orderByDesc(ConversationEntity::getLastActiveTime);

View File

@ -79,7 +79,7 @@ class ConversationServiceWebchatVisibilityTest {
}
@Test
@DisplayName("lenient list, non-admin: excludes webchat principals (no LIKE clause)")
@DisplayName("lenient list, non-admin: excludes webchat principals (no 'webchat:%' param)")
void lenientListNonAdminExcludesWebchat() {
when(authService.findByUsername("alice")).thenReturn(user("member"));
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
@ -88,12 +88,14 @@ class ConversationServiceWebchatVisibilityTest {
service.listConversations("alice", 1L, true);
String sql = captor.getValue().getTargetSql();
assertThat(sql).doesNotContainIgnoringCase("like");
// The malformed-id guard still emits a NOT LIKE, so we assert on the
// param value instead of the LIKE keyword.
assertThat(captor.getValue().getParamNameValuePairs().values())
.doesNotContain("webchat:%");
}
@Test
@DisplayName("strict list excludes webchat principals (no LIKE clause, no role lookup)")
@DisplayName("strict list excludes webchat principals (no 'webchat:%' param, no role lookup)")
void strictListExcludesWebchat() {
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
@ -101,8 +103,8 @@ class ConversationServiceWebchatVisibilityTest {
service.listConversations("admin", 1L); // strict 2-arg
String sql = captor.getValue().getTargetSql();
assertThat(sql).doesNotContainIgnoringCase("like");
assertThat(captor.getValue().getParamNameValuePairs().values())
.doesNotContain("webchat:%");
}
@Test
@ -133,8 +135,63 @@ class ConversationServiceWebchatVisibilityTest {
service.pageConversations("alice", 1L, 1, 20, null);
String sql = captor.getValue().getTargetSql();
assertThat(sql).doesNotContainIgnoringCase("like");
assertThat(captor.getValue().getParamNameValuePairs().values())
.doesNotContain("webchat:%");
}
// ------------------------------------------------------------------
// Malformed conversationId guard rows whose id ends in ":" (e.g. an
// empty-visitorId webchat thread) are filtered out of every admin list
// query, regardless of role. Surfacing them triggers 500/403 on open
// because the trailing ":" confuses some reverse proxies (issue #369).
// ------------------------------------------------------------------
@Test
@DisplayName("lenient list: applies NOT LIKE '%:' guard to filter malformed ids")
void lenientListAppliesMalformedIdGuard() {
when(authService.findByUsername("admin")).thenReturn(user("admin"));
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
when(conversationMapper.selectList(captor.capture())).thenReturn(List.of());
service.listConversations("admin", 1L, true);
// Assert on the rendered SQL (not the param values, which MyBatis-Plus
// percent-escapes internally) so the test stays independent of that
// implementation detail.
String sql = captor.getValue().getTargetSql().toLowerCase();
assertThat(sql).contains("not like");
assertThat(sql).contains("conversation_id");
}
@Test
@DisplayName("page query: applies the same NOT LIKE '%:' guard")
void pageAppliesMalformedIdGuard() {
when(authService.findByUsername("admin")).thenReturn(user("admin"));
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
when(conversationMapper.selectPage(any(Page.class), captor.capture()))
.thenReturn(new Page<>());
service.pageConversations("admin", 1L, 1, 20, null);
String sql = captor.getValue().getTargetSql().toLowerCase();
assertThat(sql).contains("not like");
assertThat(sql).contains("conversation_id");
}
@Test
@DisplayName("strict list also applies the guard — malformed ids never leak to owner-only views")
void strictListAppliesMalformedIdGuard() {
ArgumentCaptor<LambdaQueryWrapper<ConversationEntity>> captor =
ArgumentCaptor.forClass(LambdaQueryWrapper.class);
when(conversationMapper.selectList(captor.capture())).thenReturn(List.of());
service.listConversations("admin", 1L); // strict 2-arg
String sql = captor.getValue().getTargetSql().toLowerCase();
assertThat(sql).contains("not like");
assertThat(sql).contains("conversation_id");
}
private static UserEntity user(String role) {