From ebf99d9ff788155732a9cbc22e9ab33df1613d7d Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sat, 21 Mar 2026 20:34:11 +0000 Subject: [PATCH] fix(M2-005,M2-006): enforce user ownership at repo level for conversations and agents (#293) Co-authored-by: Jason Woltje Co-committed-by: Jason Woltje --- .../src/__tests__/resource-ownership.test.ts | 6 +- .../src/agent/agent-configs.controller.ts | 9 ++- .../conversations/conversations.controller.ts | 43 +++++++------ packages/brain/src/agents.ts | 41 ++++++++++-- packages/brain/src/conversations.ts | 63 ++++++++++++++++--- 5 files changed, 122 insertions(+), 40 deletions(-) diff --git a/apps/gateway/src/__tests__/resource-ownership.test.ts b/apps/gateway/src/__tests__/resource-ownership.test.ts index bfc241f..821a7a6 100644 --- a/apps/gateway/src/__tests__/resource-ownership.test.ts +++ b/apps/gateway/src/__tests__/resource-ownership.test.ts @@ -57,11 +57,13 @@ function createBrain() { describe('Resource ownership checks', () => { it('forbids access to another user conversation', async () => { const brain = createBrain(); - brain.conversations.findById.mockResolvedValue({ id: 'conv-1', userId: 'user-2' }); + // The repo enforces ownership via the WHERE clause; it returns undefined when the + // conversation does not belong to the requesting user. + brain.conversations.findById.mockResolvedValue(undefined); const controller = new ConversationsController(brain as never); await expect(controller.findOne('conv-1', { id: 'user-1' })).rejects.toBeInstanceOf( - ForbiddenException, + NotFoundException, ); }); diff --git a/apps/gateway/src/agent/agent-configs.controller.ts b/apps/gateway/src/agent/agent-configs.controller.ts index 79a233a..b15812e 100644 --- a/apps/gateway/src/agent/agent-configs.controller.ts +++ b/apps/gateway/src/agent/agent-configs.controller.ts @@ -62,7 +62,11 @@ export class AgentConfigsController { if (!agent.isSystem && agent.ownerId !== user.id) { throw new ForbiddenException('Agent does not belong to the current user'); } - const updated = await this.brain.agents.update(id, dto); + + // Pass ownerId for user agents so the repo WHERE clause enforces ownership. + // For system agents (admin path) pass undefined so the WHERE matches only on id. + const ownerId = agent.isSystem ? undefined : user.id; + const updated = await this.brain.agents.update(id, dto, ownerId); if (!updated) throw new NotFoundException('Agent not found'); return updated; } @@ -78,7 +82,8 @@ export class AgentConfigsController { if (agent.ownerId !== user.id) { throw new ForbiddenException('Agent does not belong to the current user'); } - const deleted = await this.brain.agents.remove(id); + // Pass ownerId so the repo WHERE clause enforces ownership at the DB level. + const deleted = await this.brain.agents.remove(id, user.id); if (!deleted) throw new NotFoundException('Agent not found'); } } diff --git a/apps/gateway/src/conversations/conversations.controller.ts b/apps/gateway/src/conversations/conversations.controller.ts index 72a66d6..732edff 100644 --- a/apps/gateway/src/conversations/conversations.controller.ts +++ b/apps/gateway/src/conversations/conversations.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, Delete, + ForbiddenException, Get, HttpCode, HttpStatus, @@ -16,7 +17,6 @@ import type { Brain } from '@mosaic/brain'; import { BRAIN } from '../brain/brain.tokens.js'; import { AuthGuard } from '../auth/auth.guard.js'; import { CurrentUser } from '../auth/current-user.decorator.js'; -import { assertOwner } from '../auth/resource-ownership.js'; import { CreateConversationDto, UpdateConversationDto, @@ -35,7 +35,9 @@ export class ConversationsController { @Get(':id') async findOne(@Param('id') id: string, @CurrentUser() user: { id: string }) { - return this.getOwnedConversation(id, user.id); + const conversation = await this.brain.conversations.findById(id, user.id); + if (!conversation) throw new NotFoundException('Conversation not found'); + return conversation; } @Post() @@ -53,8 +55,7 @@ export class ConversationsController { @Body() dto: UpdateConversationDto, @CurrentUser() user: { id: string }, ) { - await this.getOwnedConversation(id, user.id); - const conversation = await this.brain.conversations.update(id, dto); + const conversation = await this.brain.conversations.update(id, user.id, dto); if (!conversation) throw new NotFoundException('Conversation not found'); return conversation; } @@ -62,15 +63,16 @@ export class ConversationsController { @Delete(':id') @HttpCode(HttpStatus.NO_CONTENT) async remove(@Param('id') id: string, @CurrentUser() user: { id: string }) { - await this.getOwnedConversation(id, user.id); - const deleted = await this.brain.conversations.remove(id); + const deleted = await this.brain.conversations.remove(id, user.id); if (!deleted) throw new NotFoundException('Conversation not found'); } @Get(':id/messages') async listMessages(@Param('id') id: string, @CurrentUser() user: { id: string }) { - await this.getOwnedConversation(id, user.id); - return this.brain.conversations.findMessages(id); + // Verify ownership explicitly to return a clear 404 rather than an empty list. + const conversation = await this.brain.conversations.findById(id, user.id); + if (!conversation) throw new NotFoundException('Conversation not found'); + return this.brain.conversations.findMessages(id, user.id); } @Post(':id/messages') @@ -79,19 +81,16 @@ export class ConversationsController { @Body() dto: SendMessageDto, @CurrentUser() user: { id: string }, ) { - await this.getOwnedConversation(id, user.id); - return this.brain.conversations.addMessage({ - conversationId: id, - role: dto.role, - content: dto.content, - metadata: dto.metadata, - }); - } - - private async getOwnedConversation(id: string, userId: string) { - const conversation = await this.brain.conversations.findById(id); - if (!conversation) throw new NotFoundException('Conversation not found'); - assertOwner(conversation.userId, userId, 'Conversation'); - return conversation; + const message = await this.brain.conversations.addMessage( + { + conversationId: id, + role: dto.role, + content: dto.content, + metadata: dto.metadata, + }, + user.id, + ); + if (!message) throw new ForbiddenException('Conversation not found or access denied'); + return message; } } diff --git a/packages/brain/src/agents.ts b/packages/brain/src/agents.ts index 6a0a179..852a32a 100644 --- a/packages/brain/src/agents.ts +++ b/packages/brain/src/agents.ts @@ -1,4 +1,4 @@ -import { eq, or, type Db, agents } from '@mosaic/db'; +import { eq, and, or, type Db, agents } from '@mosaic/db'; export type Agent = typeof agents.$inferSelect; export type NewAgent = typeof agents.$inferInsert; @@ -27,6 +27,10 @@ export function createAgentsRepo(db: Db) { return db.select().from(agents).where(eq(agents.isSystem, true)); }, + /** + * Return only agents the user may access: their own agents plus all system agents. + * Never returns other users' private agents. + */ async findAccessible(ownerId: string): Promise { return db .select() @@ -39,17 +43,44 @@ export function createAgentsRepo(db: Db) { return rows[0]!; }, - async update(id: string, data: Partial): Promise { + /** + * Update an agent. + * + * For user-owned agents pass `ownerId` — the WHERE clause will enforce ownership so that + * one user cannot overwrite another user's agent. For system agents the caller must + * omit `ownerId` (admin-only path) and the WHERE clause only matches on `id`. + * + * Returns undefined when no row was matched (not found or ownership mismatch). + */ + async update( + id: string, + data: Partial, + ownerId?: string, + ): Promise { + const condition = + ownerId !== undefined + ? and(eq(agents.id, id), eq(agents.ownerId, ownerId)) + : eq(agents.id, id); + const rows = await db .update(agents) .set({ ...data, updatedAt: new Date() }) - .where(eq(agents.id, id)) + .where(condition) .returning(); return rows[0]; }, - async remove(id: string): Promise { - const rows = await db.delete(agents).where(eq(agents.id, id)).returning(); + /** + * Delete a user-owned agent, scoped to the given owner. + * Will not match system agents even if the id is correct, because system agents have + * `ownerId = null` which cannot equal a real user id. + * Returns false when no row was matched (not found, wrong owner, or system agent). + */ + async remove(id: string, ownerId: string): Promise { + const rows = await db + .delete(agents) + .where(and(eq(agents.id, id), eq(agents.ownerId, ownerId))) + .returning(); return rows.length > 0; }, }; diff --git a/packages/brain/src/conversations.ts b/packages/brain/src/conversations.ts index ce07dcf..f1bc88b 100644 --- a/packages/brain/src/conversations.ts +++ b/packages/brain/src/conversations.ts @@ -1,4 +1,4 @@ -import { eq, asc, desc, type Db, conversations, messages } from '@mosaic/db'; +import { eq, and, asc, desc, type Db, conversations, messages } from '@mosaic/db'; /** Maximum number of conversations returned per list query. */ const MAX_CONVERSATIONS = 200; @@ -21,8 +21,15 @@ export function createConversationsRepo(db: Db) { .limit(MAX_CONVERSATIONS); }, - async findById(id: string): Promise { - const rows = await db.select().from(conversations).where(eq(conversations.id, id)); + /** + * Find a conversation by ID, scoped to the given user. + * Returns undefined if the conversation does not exist or belongs to a different user. + */ + async findById(id: string, userId: string): Promise { + const rows = await db + .select() + .from(conversations) + .where(and(eq(conversations.id, id), eq(conversations.userId, userId))); return rows[0]; }, @@ -31,21 +38,47 @@ export function createConversationsRepo(db: Db) { return rows[0]!; }, - async update(id: string, data: Partial): Promise { + /** + * Update a conversation, scoped to the given user. + * Returns undefined if the conversation does not exist or belongs to a different user. + */ + async update( + id: string, + userId: string, + data: Partial, + ): Promise { const rows = await db .update(conversations) .set({ ...data, updatedAt: new Date() }) - .where(eq(conversations.id, id)) + .where(and(eq(conversations.id, id), eq(conversations.userId, userId))) .returning(); return rows[0]; }, - async remove(id: string): Promise { - const rows = await db.delete(conversations).where(eq(conversations.id, id)).returning(); + /** + * Delete a conversation, scoped to the given user. + * Returns false if the conversation does not exist or belongs to a different user. + */ + async remove(id: string, userId: string): Promise { + const rows = await db + .delete(conversations) + .where(and(eq(conversations.id, id), eq(conversations.userId, userId))) + .returning(); return rows.length > 0; }, - async findMessages(conversationId: string): Promise { + /** + * Find messages for a conversation, scoped to the given user. + * Returns an empty array if the conversation does not exist or belongs to a different user. + */ + async findMessages(conversationId: string, userId: string): Promise { + // Verify ownership of the parent conversation before returning messages. + const conv = await db + .select() + .from(conversations) + .where(and(eq(conversations.id, conversationId), eq(conversations.userId, userId))); + if (conv.length === 0) return []; + return db .select() .from(messages) @@ -54,7 +87,19 @@ export function createConversationsRepo(db: Db) { .limit(MAX_MESSAGES); }, - async addMessage(data: NewMessage): Promise { + /** + * Add a message to a conversation, scoped to the given user. + * Verifies the parent conversation belongs to the user before inserting. + * Returns undefined if the conversation does not exist or belongs to a different user. + */ + async addMessage(data: NewMessage, userId: string): Promise { + // Verify ownership of the parent conversation before inserting the message. + const conv = await db + .select() + .from(conversations) + .where(and(eq(conversations.id, data.conversationId), eq(conversations.userId, userId))); + if (conv.length === 0) return undefined; + const rows = await db.insert(messages).values(data).returning(); return rows[0]!; },