Skip to content

Commit 5e20bf4

Browse files
committed
fix(chat): resolve a caller-supplied conversation id through its owner
The v2 chat route used the caller-supplied conversationId verbatim, with no existence, owner, or workspace check, against a store keyed by bare text with no owner column. A caller who knew another user's conversation id reached that conversation. Ids now resolve through the same owner-scoped loader the web chat path uses, and anything unresolvable answers one uniform 404 before any lifecycle work runs. Omitting the id mints a server-issued conversation. The contract also accepted any 1-128 character string for a column typed uuid, so a malformed id raised a driver error and rendered 500 while an unknown but well-formed id rendered 404 - a shape oracle, and a 500 on ordinary input. The ownership predicate had no coverage anywhere: the route test mocked the module and the lifecycle test drove a chain mock that ignores its where clause, so deleting the owner condition left both suites green. It is now asserted by composition and by condition count, which is what catches a dropped condition. Also renames the reply's model identifier away from a term the project's own copy rules forbid on a user-facing surface.
1 parent ea6a6c2 commit 5e20bf4

5 files changed

Lines changed: 190 additions & 18 deletions

File tree

apps/sim/app/api/v2/chat/route.test.ts

Lines changed: 107 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const {
1616
mockGenerateId,
1717
mockRequestExplicitStreamAbort,
1818
mockResolveBillingAttribution,
19+
mockResolveOrCreateChat,
1920
mockRunHeadlessCopilotLifecycle,
2021
} = vi.hoisted(() => ({
2122
MockV2ApiKeyUnauthenticatedError: class MockV2ApiKeyUnauthenticatedError extends Error {},
@@ -35,6 +36,7 @@ const {
3536
mockCheckPreAuthRate: vi.fn(),
3637
mockGenerateId: vi.fn(),
3738
mockResolveBillingAttribution: vi.fn(),
39+
mockResolveOrCreateChat: vi.fn(),
3840
mockRequestExplicitStreamAbort: vi.fn().mockResolvedValue(undefined),
3941
mockRunHeadlessCopilotLifecycle: vi.fn(),
4042
}))
@@ -78,6 +80,10 @@ vi.mock('@/lib/copilot/chat/workspace-context', () => ({
7880
generateWorkspaceContext: vi.fn().mockResolvedValue('workspace context'),
7981
}))
8082

83+
vi.mock('@/lib/copilot/chat/lifecycle', () => ({
84+
resolveOrCreateChat: mockResolveOrCreateChat,
85+
}))
86+
8187
vi.mock('@/lib/copilot/chat/payload', () => ({
8288
buildIntegrationToolSchemas: vi.fn().mockResolvedValue([{ name: 'run_workflow' }]),
8389
}))
@@ -111,6 +117,17 @@ const personalAuth = {
111117
keyType: 'personal',
112118
}
113119

120+
/**
121+
* The route never echoes the caller's string back as the conversation id: it
122+
* reports whatever the owner-scoped resolver returns.
123+
*/
124+
const SERVER_ISSUED_CHAT_ID = 'chat-server-1'
125+
const OWNED_CONVERSATION_ID = '11111111-1111-4111-8111-111111111111'
126+
127+
function chatRow(id: string) {
128+
return { id, userId: 'user-1', workspaceId: 'workspace-1', workflowId: null, type: 'mothership' }
129+
}
130+
114131
const successResult = {
115132
success: true,
116133
content: 'Hello there',
@@ -144,6 +161,12 @@ describe('POST /api/v2/chat', () => {
144161
mockResolveBillingAttribution.mockResolvedValue(billingAttributionSnapshot)
145162
mockRequestExplicitStreamAbort.mockResolvedValue(undefined)
146163
mockRunHeadlessCopilotLifecycle.mockResolvedValue(successResult)
164+
mockResolveOrCreateChat.mockResolvedValue({
165+
chatId: SERVER_ISSUED_CHAT_ID,
166+
chat: chatRow(SERVER_ISSUED_CHAT_ID),
167+
conversationHistory: [],
168+
isNew: true,
169+
})
147170
})
148171

149172
it('rejects a missing or invalid API key', async () => {
@@ -187,15 +210,15 @@ describe('POST /api/v2/chat', () => {
187210
expect(mockRunHeadlessCopilotLifecycle).not.toHaveBeenCalled()
188211
})
189212

190-
it('runs one turn and answers the reply with a generated conversation id', async () => {
213+
it('runs one turn and answers the reply with a server-issued conversation id', async () => {
191214
const response = await callChat({ workspaceId: 'workspace-1', message: 'hi' })
192215

193216
expect(response.status).toBe(200)
194217
const body = await response.json()
195218
expect(body.data).toEqual({
196219
content: 'Hello there',
197-
model: 'mothership',
198-
conversationId: 'generated-1',
220+
model: 'sim',
221+
conversationId: SERVER_ISSUED_CHAT_ID,
199222
tokens: { prompt: 10, completion: 5, total: 15 },
200223
cost: { total: 0.01 },
201224
toolCalls: [{ name: 'run_workflow' }],
@@ -206,7 +229,7 @@ describe('POST /api/v2/chat', () => {
206229
messages: [{ role: 'user', content: 'hi' }],
207230
userId: 'user-1',
208231
workspaceId: 'workspace-1',
209-
chatId: 'generated-1',
232+
chatId: SERVER_ISSUED_CHAT_ID,
210233
mode: 'agent',
211234
isHosted: true,
212235
workspaceContext: 'workspace context',
@@ -216,7 +239,7 @@ describe('POST /api/v2/chat', () => {
216239
expect(options).toMatchObject({
217240
userId: 'user-1',
218241
workspaceId: 'workspace-1',
219-
chatId: 'generated-1',
242+
chatId: SERVER_ISSUED_CHAT_ID,
220243
goRoute: '/api/mothership/execute',
221244
autoExecuteTools: true,
222245
interactive: false,
@@ -230,17 +253,88 @@ describe('POST /api/v2/chat', () => {
230253
})
231254
})
232255

233-
it('continues the conversation the caller names', async () => {
256+
it('mints a server-issued conversation when the caller names none', async () => {
257+
const response = await callChat({ workspaceId: 'workspace-1', message: 'hi' })
258+
259+
expect(response.status).toBe(200)
260+
const body = await response.json()
261+
expect(body.data.conversationId).toBe(SERVER_ISSUED_CHAT_ID)
262+
const resolverInput = mockResolveOrCreateChat.mock.calls[0][0] as Record<string, unknown>
263+
expect(Object.hasOwn(resolverInput, 'chatId')).toBe(false)
264+
expect(resolverInput).toMatchObject({
265+
userId: 'user-1',
266+
workspaceId: 'workspace-1',
267+
type: 'mothership',
268+
})
269+
})
270+
271+
it('resolves a named conversation against the calling user and workspace before continuing it', async () => {
272+
mockResolveOrCreateChat.mockResolvedValue({
273+
chatId: OWNED_CONVERSATION_ID,
274+
chat: chatRow(OWNED_CONVERSATION_ID),
275+
conversationHistory: [],
276+
isNew: false,
277+
})
278+
234279
const response = await callChat({
235280
workspaceId: 'workspace-1',
236281
message: 'and then?',
237-
conversationId: 'conv-9',
282+
conversationId: OWNED_CONVERSATION_ID,
238283
})
239284

240285
expect(response.status).toBe(200)
286+
expect(mockResolveOrCreateChat).toHaveBeenCalledWith(
287+
expect.objectContaining({
288+
chatId: OWNED_CONVERSATION_ID,
289+
userId: 'user-1',
290+
workspaceId: 'workspace-1',
291+
})
292+
)
293+
const body = await response.json()
294+
expect(body.data.conversationId).toBe(OWNED_CONVERSATION_ID)
295+
expect(mockRunHeadlessCopilotLifecycle.mock.calls[0][0]).toMatchObject({
296+
chatId: OWNED_CONVERSATION_ID,
297+
})
298+
})
299+
300+
it('answers 404 and runs nothing when the resolver refuses the named conversation', async () => {
301+
mockResolveOrCreateChat.mockResolvedValue({
302+
chatId: OWNED_CONVERSATION_ID,
303+
chat: null,
304+
conversationHistory: [],
305+
isNew: false,
306+
})
307+
308+
const response = await callChat({
309+
workspaceId: 'workspace-1',
310+
message: 'and then?',
311+
conversationId: OWNED_CONVERSATION_ID,
312+
})
313+
314+
expect(response.status).toBe(404)
241315
const body = await response.json()
242-
expect(body.data.conversationId).toBe('conv-9')
243-
expect(mockRunHeadlessCopilotLifecycle.mock.calls[0][0]).toMatchObject({ chatId: 'conv-9' })
316+
expect(body.error.code).toBe('NOT_FOUND')
317+
expect(mockResolveOrCreateChat).toHaveBeenCalledWith(
318+
expect.objectContaining({
319+
chatId: OWNED_CONVERSATION_ID,
320+
userId: 'user-1',
321+
workspaceId: 'workspace-1',
322+
})
323+
)
324+
// No tokens may be billed against an id the caller could not be given.
325+
expect(mockRunHeadlessCopilotLifecycle).not.toHaveBeenCalled()
326+
})
327+
328+
it('rejects a malformed conversation id before resolving anything', async () => {
329+
const response = await callChat({
330+
workspaceId: 'workspace-1',
331+
message: 'and then?',
332+
conversationId: 'not-a-conversation-id',
333+
})
334+
335+
expect(response.status).toBe(400)
336+
expect(mockResolveOrCreateChat).not.toHaveBeenCalled()
337+
expect(mockRunHeadlessCopilotLifecycle).not.toHaveBeenCalled()
244338
})
245339

246340
it('answers a failed run as a 500 with the run error', async () => {
@@ -282,7 +376,10 @@ describe('POST /api/v2/chat', () => {
282376
expect(chunks.map((chunk) => chunk.content)).toEqual(['Hello', ' there'])
283377
const final = events.at(-1) as { type: string; data: Record<string, unknown> }
284378
expect(final.type).toBe('final')
285-
expect(final.data).toMatchObject({ content: 'Hello there', conversationId: 'generated-1' })
379+
expect(final.data).toMatchObject({
380+
content: 'Hello there',
381+
conversationId: SERVER_ISSUED_CHAT_ID,
382+
})
286383
})
287384

288385
it('ends the NDJSON stream with an error event when the run fails', async () => {

apps/sim/app/api/v2/chat/route.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
} from '@/lib/api/server/routes'
1414
import { resolveBillingAttribution } from '@/lib/billing/core/billing-attribution'
1515
import { chatOperations } from '@/lib/copilot/application/operations'
16+
import { resolveOrCreateChat } from '@/lib/copilot/chat/lifecycle'
1617
import { buildIntegrationToolSchemas } from '@/lib/copilot/chat/payload'
1718
import { generateWorkspaceContext } from '@/lib/copilot/chat/workspace-context'
1819
import { computeWorkspaceEntitlements } from '@/lib/copilot/entitlements'
@@ -48,6 +49,12 @@ const CHAT_STREAM_VALUE = 'ndjson'
4849
const CHAT_HEARTBEAT_INTERVAL_MS = 15_000
4950
const ndjsonEncoder = new TextEncoder()
5051

52+
/**
53+
* Model recorded on conversations this route creates, matching the model the
54+
* web Chat surface stamps on a new mothership conversation.
55+
*/
56+
const V2_CHAT_MODEL = 'claude-opus-4-8'
57+
5158
function isAbortError(error: unknown): boolean {
5259
return error instanceof Error && error.name === 'AbortError'
5360
}
@@ -80,7 +87,7 @@ function buildChatResultPayload(
8087

8188
return {
8289
content: result.content ?? '',
83-
model: 'mothership',
90+
model: 'sim',
8491
conversationId,
8592
tokens: result.usage
8693
? {
@@ -130,14 +137,35 @@ export const POST = withRouteHandler(
130137
if (!parsed.success) return parsed.response
131138
const { workspaceId, message, conversationId } = parsed.data.body
132139

133-
const chatId = conversationId || generateId()
134140
const messageId = generateId()
135141
const requestId = generateId()
136-
const reqLogger = logger.withMetadata({ chatId, messageId, requestId })
142+
let reqLogger = logger.withMetadata({ messageId, requestId })
137143

138144
try {
139145
const workspaceAccess = await assertActiveWorkspaceAccess(workspaceId, userId)
140146
const userPermission = workspaceAccess.permission
147+
148+
// A caller-supplied conversation id is a claim, not an identity: resolve
149+
// it through the same owner- and workspace-scoped loader the web Chat
150+
// surface uses, and refuse every id that does not resolve with the same
151+
// response so the refusal carries no information about the id. Omitting
152+
// the id mints a server-issued conversation instead of trusting one.
153+
const resolvedChat = await resolveOrCreateChat({
154+
...(conversationId ? { chatId: conversationId } : {}),
155+
userId,
156+
workspaceId,
157+
model: V2_CHAT_MODEL,
158+
type: 'mothership',
159+
})
160+
if (conversationId && !resolvedChat.chat) {
161+
return v2Error('NOT_FOUND', 'Conversation not found')
162+
}
163+
if (!resolvedChat.chat || !resolvedChat.chatId) {
164+
reqLogger.error('Failed to start a chat conversation', { userId, workspaceId })
165+
return v2Error('INTERNAL_ERROR', 'Internal server error')
166+
}
167+
const chatId = resolvedChat.chatId
168+
reqLogger = logger.withMetadata({ chatId, messageId, requestId })
141169
const secretMountPolicy = normalizeSecretMountPolicy(undefined)
142170

143171
let environmentContext: CopilotEnvironmentContext | undefined

apps/sim/lib/api/contracts/v2/chat.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,7 @@ export const v2ChatBodySchema = z.object({
2424
.describe('The message to send to Sim.'),
2525
conversationId: z
2626
.string()
27-
.min(1, 'conversationId cannot be empty')
28-
.max(128, 'conversationId cannot exceed 128 characters')
27+
.uuid('conversationId must be a valid conversation id')
2928
.optional()
3029
.describe('Conversation to continue; a new one starts when omitted.'),
3130
})
@@ -39,7 +38,7 @@ const v2ChatTokensSchema = z.object({
3938
export const v2ChatResultSchema = z.object({
4039
content: z.string(),
4140
conversationId: z.string(),
42-
model: z.string(),
41+
model: z.string().describe('Identifier of the agent that produced the reply.'),
4342
tokens: v2ChatTokensSchema.optional(),
4443
// untyped-response: cost is a billing passthrough whose shape is owned by the copilot backend, not this contract
4544
cost: z.unknown().optional(),

apps/sim/lib/copilot/chat/lifecycle.test.ts

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { dbChainMockFns, resetDbChainMock, workflowAuthzMockFns } from '@sim/testing'
4+
import { dbChainMockFns, resetDbChainMock, schemaMock, workflowAuthzMockFns } from '@sim/testing'
55
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
66

77
const {
@@ -135,6 +135,54 @@ describe('lifecycle copilot chat reads (cutover to copilot_messages)', () => {
135135
expect(result?.messages).toEqual([userMsg])
136136
})
137137

138+
it('scopes the chat lookup to the requesting user, not the chat id alone', async () => {
139+
dbChainMockFns.limit.mockResolvedValueOnce([chatRow])
140+
dbChainMockFns.orderBy.mockResolvedValueOnce([])
141+
142+
await getAccessibleCopilotChatWithMessages(CHAT_ID, USER_ID)
143+
144+
const predicate = dbChainMockFns.where.mock.calls[0]?.[0] as {
145+
type: string
146+
conditions: unknown[]
147+
}
148+
expect(predicate.type).toBe('and')
149+
// Three conditions exactly: dropping one silently widens the lookup, so the
150+
// count is asserted alongside the membership checks.
151+
expect(predicate.conditions).toHaveLength(3)
152+
expect(predicate.conditions).toContainEqual({
153+
type: 'eq',
154+
left: schemaMock.copilotChats.userId,
155+
right: USER_ID,
156+
})
157+
expect(predicate.conditions).toContainEqual({
158+
type: 'eq',
159+
left: schemaMock.copilotChats.id,
160+
right: CHAT_ID,
161+
})
162+
expect(predicate.conditions).toContainEqual({
163+
type: 'isNull',
164+
column: schemaMock.copilotChats.deletedAt,
165+
})
166+
})
167+
168+
it('resolveOrCreateChat scopes its existing-chat lookup to the requesting user', async () => {
169+
dbChainMockFns.limit.mockResolvedValueOnce([chatRow])
170+
dbChainMockFns.orderBy.mockResolvedValueOnce([])
171+
172+
await resolveOrCreateChat({ chatId: CHAT_ID, userId: USER_ID, model: 'm' })
173+
174+
const predicate = dbChainMockFns.where.mock.calls[0]?.[0] as {
175+
type: string
176+
conditions: unknown[]
177+
}
178+
expect(predicate.conditions).toHaveLength(3)
179+
expect(predicate.conditions).toContainEqual({
180+
type: 'eq',
181+
left: schemaMock.copilotChats.userId,
182+
right: USER_ID,
183+
})
184+
})
185+
138186
it('resolveOrCreateChat returns conversationHistory from the table for an existing chat', async () => {
139187
dbChainMockFns.limit.mockResolvedValueOnce([chatRow])
140188
dbChainMockFns.orderBy.mockResolvedValueOnce([{ content: userMsg }, { content: asstMsg }])

packages/sim-cli/src/commands/protocol/chat.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ function written(spy: WriteSpy): string {
102102

103103
const FINAL = {
104104
type: 'final',
105-
data: { content: 'Hello there', conversationId: 'conv-1', model: 'mothership' },
105+
data: { content: 'Hello there', conversationId: 'conv-1', model: 'sim' },
106106
}
107107

108108
describe('sim chat', () => {

0 commit comments

Comments
 (0)