From cf5d47d249b76748fffb8bee94b8aee28df32fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=80=AA=E7=A8=8B=E4=BC=9F?= Date: Thu, 18 Jun 2026 02:03:47 +0800 Subject: [PATCH] fix(security): cross-workspace guard on isConversationOwner (#344) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the authorization asymmetry between list endpoints (which filter by workspaceId) and direct-access endpoints (which did not): a logged-in user could reach another workspace's system / IM / webchat-owned conversation by id and run any of messages / delete / rename / pin / setModel / clear / chat-files download on it. Per the maintainer's guidance on issue #344, workspaces are now treated as untrusted isolation boundaries — the fix is the cross-cutting hardening, kept out of feature work. Behavior change (only for shared, non-direct convs): - requester is a global admin (user.role=admin) → pass - requester is a member of the conversation's workspace → pass - otherwise → deny Preserved to avoid regressions: - direct owner (username == conv.username) → pass without lookup - convs without workspace_id (legacy rows) → legacy system-owner check - anonymous user (authService returns null, e.g. permitAll reconnect) → legacy system-owner check Callers in ConversationController / ChatController / SubagentController / GoalController / ApprovalController (18 sites) are unchanged — the signature stays isConversationOwner(conversationId, username). The workspace membership check is done via WorkspaceService.hasPermissionCached (Caffeine-backed, same cache the WorkspaceAccessInterceptor uses) and ignores the X-Workspace-Id header, which is client-controlled. Tests: 11 cases in ConversationServiceOwnershipWorkspaceTest covering each branch of the new logic. No caller-side test changes — the 66 caller tests (ConversationService*Test, ChatController*Test, SubagentController*Test, GoalController*Test, ApprovalController*Test) still pass. --- .../conversation/ConversationService.java | 68 ++++- ...ersationServiceOwnershipWorkspaceTest.java | 250 ++++++++++++++++++ 2 files changed, 307 insertions(+), 11 deletions(-) create mode 100644 mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java index 4b197821..a49b597d 100644 --- a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java +++ b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java @@ -17,6 +17,8 @@ import vip.mate.approval.MetadataDecision; import vip.mate.agent.repository.AgentMapper; import vip.mate.approval.model.ToolApprovalEntity; import vip.mate.approval.repository.ToolApprovalMapper; +import vip.mate.auth.model.UserEntity; +import vip.mate.auth.service.AuthService; import vip.mate.channel.model.ChannelSessionEntity; import vip.mate.channel.repository.ChannelSessionMapper; import vip.mate.task.model.AsyncTaskEntity; @@ -29,6 +31,7 @@ import vip.mate.workspace.conversation.repository.ConversationMapper; import vip.mate.workspace.conversation.repository.MessageMapper; import vip.mate.workspace.conversation.vo.ConversationVO; import vip.mate.workspace.conversation.vo.MessageVO; +import vip.mate.workspace.core.service.WorkspaceService; import java.io.IOException; import java.nio.file.Files; @@ -81,6 +84,8 @@ public class ConversationService { private final AsyncTaskMapper asyncTaskMapper; private final ChannelSessionMapper channelSessionMapper; private final ApplicationEventPublisher eventPublisher; + private final AuthService authService; + private final WorkspaceService workspaceService; /** * Optional spill store. Injected via a setter so the existing @RequiredArgsConstructor @@ -1516,14 +1521,37 @@ public class ConversationService { } /** - * Check whether a user owns the conversation, treating system-owned - * rows (e.g. from scheduled jobs / IM channels) and external channel - * principals ({@code webchat:}) as visible to every - * authenticated user — otherwise the console could list a webchat - * conversation but 403 on opening / deleting / renaming it. + * Check whether a user owns the conversation. Direct owners always pass; + * shared rows (system / IM / {@code webchat:} principals) are + * additionally gated by the requester's membership in the conversation's + * workspace, so they are not reachable cross-workspace by id. * - *

校验用户是否拥有该会话。定时任务产生的会话（username = system） - * 和 webchat 访客会话（username = webchat:<visitorId>）对所有登录用户可见。 + *

Cross-workspace guard (issue #344). The legacy contract let any + * logged-in user reach a system / IM / webchat-owned conversation by id — + * the list endpoints filtered by {@code workspaceId} but the direct-access + * endpoints did not. Under a multi-tenant model where workspaces are + * untrusted isolation boundaries, that asymmetry is a cross-workspace + * authorization gap. This method now also requires, for shared (non-direct) + * conversations, that the requester actually be a member of the + * conversation's workspace. + * + *

校验用户是否拥有该会话。直属会话直接放行;共享会话(system / IM / webchat) + * 额外要求请求者是该会话所属 workspace 的成员。 + * + *

分支: + *

    + *
  • 会话不存在 → false
    • + *
  • 请求者是该会话的直属 owner → true(自己的会话,workspace 隐式一致)
    • + *
  • 会话无 workspace_id(老数据)→ 仅看是否 system owner(维持旧行为,避免回归)
    • + *
  • 请求者用户记录不存在(permitAll 端点的匿名重连)→ 仅看是否 system owner(维持旧行为)
    • + *
  • 请求者是全局 admin(user.role=admin)→ true(横切覆盖,与具体 workspace 无关)
    • + *
  • 请求者非该会话 workspace 的成员 → false
    • + *
  • 否则 → system owner 检查(共享会话对本 workspace 成员可见)
    • + *
+ * + *

调用方签名不变;调用方若需在不查 DB 的情况下做 admin 例外,可在外层先短路, + * 但通常让本方法统一处理以避免散落的 admin 例外逻辑。注意:本方法不读 + * {@code X-Workspace-Id} header —— 该 header 客户端可伪造,以 DB 中的成员关系为准。 */ public boolean isConversationOwner(String conversationId, String username) { ConversationEntity conv = conversationMapper.selectOne( @@ -1532,10 +1560,28 @@ public class ConversationService { if (conv == null) { return false; } - String owner = conv.getUsername(); - return username.equals(owner) - || SYSTEM_USER.equals(owner) - || (owner != null && owner.startsWith(WEBCHAT_OWNER_PREFIX)); + // 直属 owner 一律放行:会话由该用户创建,workspace 自然一致,无需再做成员校验。 + if (username != null && username.equals(conv.getUsername())) { + return true; + } + // 共享会话(system / IM / webchat owner)以下收紧。 + Long convWorkspaceId = conv.getWorkspaceId(); + UserEntity requester = authService.findByUsername(username); + // 老数据无 workspace_id,或请求者为匿名(permitAll 端点重连场景):维持旧行为, + // 仅 system owner 可见。避免数据迁移未完成或匿名流式场景下回归。 + if (convWorkspaceId == null || requester == null) { + return SYSTEM_USER.equals(conv.getUsername()); + } + // 全局 admin 横切放行,覆盖所有 workspace。 + if ("admin".equalsIgnoreCase(requester.getRole())) { + return true; + } + // #344 的核心守卫:必须是该会话所属 workspace 的成员(viewer 或更高)。 + // 不读 X-Workspace-Id header —— 客户端可伪造;以 DB 成员关系为准。 + if (!workspaceService.hasPermissionCached(convWorkspaceId, requester.getId(), "viewer")) { + return false; + } + return SYSTEM_USER.equals(conv.getUsername()); } /** diff --git a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java new file mode 100644 index 00000000..b1df1669 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceOwnershipWorkspaceTest.java @@ -0,0 +1,250 @@ +package vip.mate.workspace.conversation; + +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.context.ApplicationEventPublisher; +import vip.mate.agent.repository.AgentMapper; +import vip.mate.approval.repository.ToolApprovalMapper; +import vip.mate.auth.model.UserEntity; +import vip.mate.auth.service.AuthService; +import vip.mate.channel.repository.ChannelSessionMapper; +import vip.mate.task.repository.AsyncTaskMapper; +import vip.mate.workspace.conversation.model.ConversationEntity; +import vip.mate.workspace.conversation.repository.ConversationMapper; +import vip.mate.workspace.conversation.repository.MessageMapper; +import vip.mate.workspace.core.service.WorkspaceService; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Pin the cross-workspace authorization guard added to + * {@link ConversationService#isConversationOwner(String, String)} in response + * to issue #344. + * + *

Pre-fix behavior: any logged-in user could reach a system / IM / webchat + * -owned conversation by id, regardless of which workspace the conversation + * lived in. List endpoints filtered by {@code workspaceId}, direct-access + * endpoints did not — an asymmetry that becomes a cross-workspace breach once + * workspaces are untrusted isolation boundaries. + * + *

Post-fix behavior: shared conversations are visible only to members of + * their own workspace (plus global admins, plus the legacy escape hatches for + * pre-workspace rows and anonymous permitAll reconnects). + * + *

Pure-Mockito (no Spring context) so the test stays fast and isolated. + * + * @author MateClaw Team + */ +@ExtendWith(MockitoExtension.class) +class ConversationServiceOwnershipWorkspaceTest { + + private static final String SYSTEM_CONV = "cron:daily-report"; + private static final String ALICE_CONV = "alice-uuid-1"; + private static final String WEBCHAT_CONV = "webchat:testkey1:vA"; + private static final long WS_TENANT_A = 10L; + private static final long WS_TENANT_B = 20L; + private static final long ALICE_USER_ID = 1001L; + private static final long BOB_USER_ID = 1002L; + private static final long ADMIN_USER_ID = 1003L; + + @Mock private ConversationMapper conversationMapper; + @Mock private MessageMapper messageMapper; + @Mock private AgentMapper agentMapper; + @Spy private ObjectMapper objectMapper = new ObjectMapper(); + @Mock private ToolApprovalMapper toolApprovalMapper; + @Mock private AsyncTaskMapper asyncTaskMapper; + @Mock private ChannelSessionMapper channelSessionMapper; + @Mock private ApplicationEventPublisher eventPublisher; + @Mock private AuthService authService; + @Mock private WorkspaceService workspaceService; + + @InjectMocks private ConversationService service; + + @BeforeEach + void stubUsers() { + // Lenient stubs — not every test needs both users (admin-only tests + // would trip strict-stubbing otherwise). + org.mockito.Mockito.lenient().when(authService.findByUsername("alice")) + .thenReturn(user(ALICE_USER_ID, "user")); + org.mockito.Mockito.lenient().when(authService.findByUsername("bob")) + .thenReturn(user(BOB_USER_ID, "user")); + } + + // ------------------------------------------------------------------ + // 1. Direct owner — no workspace check needed + // ------------------------------------------------------------------ + + @Test + @DisplayName("direct owner: always allowed, no membership lookup") + void directOwnerShortCircuits() { + when(conversationMapper.selectOne(any())).thenReturn(conv(ALICE_CONV, "alice", WS_TENANT_A)); + + assertThat(service.isConversationOwner(ALICE_CONV, "alice")).isTrue(); + + // Workspace membership is NOT consulted — alice owns it, end of story. + verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString()); + } + + // ------------------------------------------------------------------ + // 2. System conv, same-workspace member → allowed + // ------------------------------------------------------------------ + + @Test + @DisplayName("system conv in requester's workspace: member passes") + void systemConvSameWorkspaceMember() { + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A)); + when(workspaceService.hasPermissionCached(WS_TENANT_A, ALICE_USER_ID, "viewer")) + .thenReturn(true); + + assertThat(service.isConversationOwner(SYSTEM_CONV, "alice")).isTrue(); + } + + // ------------------------------------------------------------------ + // 3. System conv, cross-workspace user → DENIED (the #344 fix) + // ------------------------------------------------------------------ + + @Test + @DisplayName("system conv in another workspace: non-member rejected (#344)") + void systemConvCrossWorkspaceRejected() { + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A)); + // Bob is not a member of tenant A. + when(workspaceService.hasPermissionCached(WS_TENANT_A, BOB_USER_ID, "viewer")) + .thenReturn(false); + + assertThat(service.isConversationOwner(SYSTEM_CONV, "bob")).isFalse(); + } + + // ------------------------------------------------------------------ + // 4. Global admin bypass + // ------------------------------------------------------------------ + + @Test + @DisplayName("system conv in another workspace: global admin bypasses") + void systemConvAdminBypass() { + when(authService.findByUsername("admin")).thenReturn(user(ADMIN_USER_ID, "admin")); + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A)); + + // Admin skips the membership check entirely. + assertThat(service.isConversationOwner(SYSTEM_CONV, "admin")).isTrue(); + verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString()); + } + + // ------------------------------------------------------------------ + // 5. Anonymous / permitAll reconnect — user lookup returns null + // ------------------------------------------------------------------ + + @Test + @DisplayName("system conv + null user record (anonymous reconnect): legacy behavior preserved") + void anonymousReconnectLegacyFallback() { + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A)); + when(authService.findByUsername("anonymous")).thenReturn(null); + + // Pre-fix: anonymous could see system convs. Preserve that. + assertThat(service.isConversationOwner(SYSTEM_CONV, "anonymous")).isTrue(); + verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString()); + } + + @Test + @DisplayName("anonymous reconnect to a non-system conv: still rejected") + void anonymousReconnectNonSystemConv() { + when(conversationMapper.selectOne(any())).thenReturn(conv(ALICE_CONV, "alice", WS_TENANT_A)); + when(authService.findByUsername("anonymous")).thenReturn(null); + + assertThat(service.isConversationOwner(ALICE_CONV, "anonymous")).isFalse(); + } + + // ------------------------------------------------------------------ + // 6. Legacy rows without workspace_id — fall back to old logic + // ------------------------------------------------------------------ + + @Test + @DisplayName("conv without workspace_id: legacy system-owner check, no membership lookup") + void legacyConvWithoutWorkspace() { + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", null)); + + assertThat(service.isConversationOwner(SYSTEM_CONV, "bob")).isTrue(); + verify(workspaceService, never()).hasPermissionCached(anyLong(), anyLong(), anyString()); + } + + // ------------------------------------------------------------------ + // 7. Webchat convs — already isolated; verify the fix doesn't open them + // ------------------------------------------------------------------ + + @Test + @DisplayName("webchat conv: invisible to a JWT user even when they share the workspace") + void webchatConvInvisibleToJwtUser() { + when(conversationMapper.selectOne(any())).thenReturn(conv(WEBCHAT_CONV, "webchat:vA", WS_TENANT_A)); + when(workspaceService.hasPermissionCached(WS_TENANT_A, ALICE_USER_ID, "viewer")) + .thenReturn(true); + + // Alice is a member of the conv's workspace, but the conv is owned by + // "webchat:vA" — not "system" — so the final OR-clause returns false. + assertThat(service.isConversationOwner(WEBCHAT_CONV, "alice")).isFalse(); + } + + @Test + @DisplayName("webchat conv: global admin can still reach it (consistent with system convs)") + void webchatConvAdminBypass() { + when(authService.findByUsername("admin")).thenReturn(user(ADMIN_USER_ID, "admin")); + when(conversationMapper.selectOne(any())).thenReturn(conv(WEBCHAT_CONV, "webchat:vA", WS_TENANT_A)); + + assertThat(service.isConversationOwner(WEBCHAT_CONV, "admin")).isTrue(); + } + + // ------------------------------------------------------------------ + // 8. Edge cases + // ------------------------------------------------------------------ + + @Test + @DisplayName("conversation not found: false") + void notFound() { + when(conversationMapper.selectOne(any())).thenReturn(null); + + assertThat(service.isConversationOwner("missing", "alice")).isFalse(); + } + + @Test + @DisplayName("system conv + same-workspace member that the workspace service lost track of: rejected") + void systemConvMemberCacheMiss() { + when(conversationMapper.selectOne(any())).thenReturn(conv(SYSTEM_CONV, "system", WS_TENANT_A)); + // Membership cache returns false even though we'd expect this user to + // be a member — defense in depth: when in doubt, deny. + when(workspaceService.hasPermissionCached(eq(WS_TENANT_A), eq(ALICE_USER_ID), eq("viewer"))) + .thenReturn(false); + + assertThat(service.isConversationOwner(SYSTEM_CONV, "alice")).isFalse(); + } + + // ------------------------------------------------------------------ + // helpers + // ------------------------------------------------------------------ + + private static UserEntity user(long id, String role) { + UserEntity u = new UserEntity(); + u.setId(id); + u.setRole(role); + return u; + } + + private static ConversationEntity conv(String conversationId, String username, Long workspaceId) { + ConversationEntity c = new ConversationEntity(); + c.setConversationId(conversationId); + c.setUsername(username); + c.setWorkspaceId(workspaceId); + return c; + } +}