fix(M2-005,M2-006): enforce user ownership at repo level for conversations and agents
ConversationsRepo: add userId parameter to findById, update, remove, findMessages, and addMessage so every query filters by conversations.userId in the WHERE clause. This prevents cross-user data access even if the controller layer were bypassed. AgentsRepo: add optional ownerId parameter to update (enforced for user-owned agents, omitted for admin system-agent path) and required ownerId to remove so the DELETE WHERE clause always scopes to the requesting user's agents. Controller call sites updated to pass userId/ownerId to the repo methods. The resource-ownership unit test updated to reflect that findById now returns undefined (not a foreign-user object) when ownership is checked at the DB layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -57,11 +57,13 @@ function createBrain() {
|
|||||||
describe('Resource ownership checks', () => {
|
describe('Resource ownership checks', () => {
|
||||||
it('forbids access to another user conversation', async () => {
|
it('forbids access to another user conversation', async () => {
|
||||||
const brain = createBrain();
|
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);
|
const controller = new ConversationsController(brain as never);
|
||||||
|
|
||||||
await expect(controller.findOne('conv-1', { id: 'user-1' })).rejects.toBeInstanceOf(
|
await expect(controller.findOne('conv-1', { id: 'user-1' })).rejects.toBeInstanceOf(
|
||||||
ForbiddenException,
|
NotFoundException,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -62,7 +62,11 @@ export class AgentConfigsController {
|
|||||||
if (!agent.isSystem && agent.ownerId !== user.id) {
|
if (!agent.isSystem && agent.ownerId !== user.id) {
|
||||||
throw new ForbiddenException('Agent does not belong to the current user');
|
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');
|
if (!updated) throw new NotFoundException('Agent not found');
|
||||||
return updated;
|
return updated;
|
||||||
}
|
}
|
||||||
@@ -78,7 +82,8 @@ export class AgentConfigsController {
|
|||||||
if (agent.ownerId !== user.id) {
|
if (agent.ownerId !== user.id) {
|
||||||
throw new ForbiddenException('Agent does not belong to the current user');
|
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');
|
if (!deleted) throw new NotFoundException('Agent not found');
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ import {
|
|||||||
Body,
|
Body,
|
||||||
Controller,
|
Controller,
|
||||||
Delete,
|
Delete,
|
||||||
|
ForbiddenException,
|
||||||
Get,
|
Get,
|
||||||
HttpCode,
|
HttpCode,
|
||||||
HttpStatus,
|
HttpStatus,
|
||||||
@@ -16,7 +17,6 @@ import type { Brain } from '@mosaic/brain';
|
|||||||
import { BRAIN } from '../brain/brain.tokens.js';
|
import { BRAIN } from '../brain/brain.tokens.js';
|
||||||
import { AuthGuard } from '../auth/auth.guard.js';
|
import { AuthGuard } from '../auth/auth.guard.js';
|
||||||
import { CurrentUser } from '../auth/current-user.decorator.js';
|
import { CurrentUser } from '../auth/current-user.decorator.js';
|
||||||
import { assertOwner } from '../auth/resource-ownership.js';
|
|
||||||
import {
|
import {
|
||||||
CreateConversationDto,
|
CreateConversationDto,
|
||||||
UpdateConversationDto,
|
UpdateConversationDto,
|
||||||
@@ -35,7 +35,9 @@ export class ConversationsController {
|
|||||||
|
|
||||||
@Get(':id')
|
@Get(':id')
|
||||||
async findOne(@Param('id') id: string, @CurrentUser() user: { id: string }) {
|
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()
|
@Post()
|
||||||
@@ -53,8 +55,7 @@ export class ConversationsController {
|
|||||||
@Body() dto: UpdateConversationDto,
|
@Body() dto: UpdateConversationDto,
|
||||||
@CurrentUser() user: { id: string },
|
@CurrentUser() user: { id: string },
|
||||||
) {
|
) {
|
||||||
await this.getOwnedConversation(id, user.id);
|
const conversation = await this.brain.conversations.update(id, user.id, dto);
|
||||||
const conversation = await this.brain.conversations.update(id, dto);
|
|
||||||
if (!conversation) throw new NotFoundException('Conversation not found');
|
if (!conversation) throw new NotFoundException('Conversation not found');
|
||||||
return conversation;
|
return conversation;
|
||||||
}
|
}
|
||||||
@@ -62,15 +63,16 @@ export class ConversationsController {
|
|||||||
@Delete(':id')
|
@Delete(':id')
|
||||||
@HttpCode(HttpStatus.NO_CONTENT)
|
@HttpCode(HttpStatus.NO_CONTENT)
|
||||||
async remove(@Param('id') id: string, @CurrentUser() user: { id: string }) {
|
async remove(@Param('id') id: string, @CurrentUser() user: { id: string }) {
|
||||||
await this.getOwnedConversation(id, user.id);
|
const deleted = await this.brain.conversations.remove(id, user.id);
|
||||||
const deleted = await this.brain.conversations.remove(id);
|
|
||||||
if (!deleted) throw new NotFoundException('Conversation not found');
|
if (!deleted) throw new NotFoundException('Conversation not found');
|
||||||
}
|
}
|
||||||
|
|
||||||
@Get(':id/messages')
|
@Get(':id/messages')
|
||||||
async listMessages(@Param('id') id: string, @CurrentUser() user: { id: string }) {
|
async listMessages(@Param('id') id: string, @CurrentUser() user: { id: string }) {
|
||||||
await this.getOwnedConversation(id, user.id);
|
// Verify ownership explicitly to return a clear 404 rather than an empty list.
|
||||||
return this.brain.conversations.findMessages(id);
|
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')
|
@Post(':id/messages')
|
||||||
@@ -79,19 +81,16 @@ export class ConversationsController {
|
|||||||
@Body() dto: SendMessageDto,
|
@Body() dto: SendMessageDto,
|
||||||
@CurrentUser() user: { id: string },
|
@CurrentUser() user: { id: string },
|
||||||
) {
|
) {
|
||||||
await this.getOwnedConversation(id, user.id);
|
const message = await this.brain.conversations.addMessage(
|
||||||
return this.brain.conversations.addMessage({
|
{
|
||||||
conversationId: id,
|
conversationId: id,
|
||||||
role: dto.role,
|
role: dto.role,
|
||||||
content: dto.content,
|
content: dto.content,
|
||||||
metadata: dto.metadata,
|
metadata: dto.metadata,
|
||||||
});
|
},
|
||||||
}
|
user.id,
|
||||||
|
);
|
||||||
private async getOwnedConversation(id: string, userId: string) {
|
if (!message) throw new ForbiddenException('Conversation not found or access denied');
|
||||||
const conversation = await this.brain.conversations.findById(id);
|
return message;
|
||||||
if (!conversation) throw new NotFoundException('Conversation not found');
|
|
||||||
assertOwner(conversation.userId, userId, 'Conversation');
|
|
||||||
return conversation;
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -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 Agent = typeof agents.$inferSelect;
|
||||||
export type NewAgent = typeof agents.$inferInsert;
|
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 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<Agent[]> {
|
async findAccessible(ownerId: string): Promise<Agent[]> {
|
||||||
return db
|
return db
|
||||||
.select()
|
.select()
|
||||||
@@ -39,17 +43,44 @@ export function createAgentsRepo(db: Db) {
|
|||||||
return rows[0]!;
|
return rows[0]!;
|
||||||
},
|
},
|
||||||
|
|
||||||
async update(id: string, data: Partial<NewAgent>): Promise<Agent | undefined> {
|
/**
|
||||||
|
* 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<NewAgent>,
|
||||||
|
ownerId?: string,
|
||||||
|
): Promise<Agent | undefined> {
|
||||||
|
const condition =
|
||||||
|
ownerId !== undefined
|
||||||
|
? and(eq(agents.id, id), eq(agents.ownerId, ownerId))
|
||||||
|
: eq(agents.id, id);
|
||||||
|
|
||||||
const rows = await db
|
const rows = await db
|
||||||
.update(agents)
|
.update(agents)
|
||||||
.set({ ...data, updatedAt: new Date() })
|
.set({ ...data, updatedAt: new Date() })
|
||||||
.where(eq(agents.id, id))
|
.where(condition)
|
||||||
.returning();
|
.returning();
|
||||||
return rows[0];
|
return rows[0];
|
||||||
},
|
},
|
||||||
|
|
||||||
async remove(id: string): Promise<boolean> {
|
/**
|
||||||
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<boolean> {
|
||||||
|
const rows = await db
|
||||||
|
.delete(agents)
|
||||||
|
.where(and(eq(agents.id, id), eq(agents.ownerId, ownerId)))
|
||||||
|
.returning();
|
||||||
return rows.length > 0;
|
return rows.length > 0;
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|||||||
@@ -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. */
|
/** Maximum number of conversations returned per list query. */
|
||||||
const MAX_CONVERSATIONS = 200;
|
const MAX_CONVERSATIONS = 200;
|
||||||
@@ -21,8 +21,15 @@ export function createConversationsRepo(db: Db) {
|
|||||||
.limit(MAX_CONVERSATIONS);
|
.limit(MAX_CONVERSATIONS);
|
||||||
},
|
},
|
||||||
|
|
||||||
async findById(id: string): Promise<Conversation | undefined> {
|
/**
|
||||||
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<Conversation | undefined> {
|
||||||
|
const rows = await db
|
||||||
|
.select()
|
||||||
|
.from(conversations)
|
||||||
|
.where(and(eq(conversations.id, id), eq(conversations.userId, userId)));
|
||||||
return rows[0];
|
return rows[0];
|
||||||
},
|
},
|
||||||
|
|
||||||
@@ -31,21 +38,47 @@ export function createConversationsRepo(db: Db) {
|
|||||||
return rows[0]!;
|
return rows[0]!;
|
||||||
},
|
},
|
||||||
|
|
||||||
async update(id: string, data: Partial<NewConversation>): Promise<Conversation | undefined> {
|
/**
|
||||||
|
* 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<NewConversation>,
|
||||||
|
): Promise<Conversation | undefined> {
|
||||||
const rows = await db
|
const rows = await db
|
||||||
.update(conversations)
|
.update(conversations)
|
||||||
.set({ ...data, updatedAt: new Date() })
|
.set({ ...data, updatedAt: new Date() })
|
||||||
.where(eq(conversations.id, id))
|
.where(and(eq(conversations.id, id), eq(conversations.userId, userId)))
|
||||||
.returning();
|
.returning();
|
||||||
return rows[0];
|
return rows[0];
|
||||||
},
|
},
|
||||||
|
|
||||||
async remove(id: string): Promise<boolean> {
|
/**
|
||||||
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<boolean> {
|
||||||
|
const rows = await db
|
||||||
|
.delete(conversations)
|
||||||
|
.where(and(eq(conversations.id, id), eq(conversations.userId, userId)))
|
||||||
|
.returning();
|
||||||
return rows.length > 0;
|
return rows.length > 0;
|
||||||
},
|
},
|
||||||
|
|
||||||
async findMessages(conversationId: string): Promise<Message[]> {
|
/**
|
||||||
|
* 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<Message[]> {
|
||||||
|
// 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
|
return db
|
||||||
.select()
|
.select()
|
||||||
.from(messages)
|
.from(messages)
|
||||||
@@ -54,7 +87,19 @@ export function createConversationsRepo(db: Db) {
|
|||||||
.limit(MAX_MESSAGES);
|
.limit(MAX_MESSAGES);
|
||||||
},
|
},
|
||||||
|
|
||||||
async addMessage(data: NewMessage): Promise<Message> {
|
/**
|
||||||
|
* 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<Message | undefined> {
|
||||||
|
// 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();
|
const rows = await db.insert(messages).values(data).returning();
|
||||||
return rows[0]!;
|
return rows[0]!;
|
||||||
},
|
},
|
||||||
|
|||||||
Reference in New Issue
Block a user