diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 00000000..6f4ddad1 --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,11 @@ +# Sentinel Security Journal - Critical Learnings + +## 2025-05-14 - [IDOR in Server Actions] +**Vulnerability:** Server actions in `lib/actions/chat.ts` were accepting `userId` as an argument from the client, allowing attackers to access or modify other users' chats by providing their IDs. Additionally, `getChatMessages` lacked any authorization check, returning all messages for any given `chatId`. +**Learning:** Next.js server actions (exported functions in files with `'use server'`) are public endpoints. Trusting client-side parameters for user identification is a major security risk. Authorization must be enforced at the entry point of every server action. +**Prevention:** Always retrieve user identity server-side from secure sessions/cookies using utilities like `getCurrentUserIdOnServer()`. For resource access (like messages), verify ownership or public visibility using a database-level check (e.g., `dbGetChat`) before performing the operation. + +## 2025-05-14 - [Database UUID Type Safety] +**Vulnerability:** Passing a non-UUID string like `'anonymous'` to a PostgreSQL `uuid` field in a query can cause a database error. +**Learning:** When handling unauthenticated users in database queries, ensure that either a valid UUID is used or the logic explicitly handles the absence of a user ID before hitting the database query that expects a UUID. +**Prevention:** Refactor database utility functions to accept `string | null` and handle the `null` case early to avoid invalid type syntax errors in SQL. diff --git a/app/actions.tsx b/app/actions.tsx index 50e985bf..2f686137 100644 --- a/app/actions.tsx +++ b/app/actions.tsx @@ -397,8 +397,7 @@ async function submit(formData?: FormData, skip?: boolean) { } as CoreMessage) } - const userId = 'anonymous' - const currentSystemPrompt = (await getSystemPrompt(userId)) || '' + const currentSystemPrompt = (await getSystemPrompt()) || '' const mapProvider = formData?.get('mapProvider') as 'mapbox' | 'google' async function processEvents() { @@ -655,7 +654,7 @@ export const AI = createAI({ title, messages: updatedMessages } - await saveChat(chat, actualUserId) + await saveChat(chat) } }) diff --git a/app/search/[id]/page.tsx b/app/search/[id]/page.tsx index 8db74186..b3960e62 100644 --- a/app/search/[id]/page.tsx +++ b/app/search/[id]/page.tsx @@ -15,10 +15,8 @@ export interface SearchPageProps { export async function generateMetadata({ params }: SearchPageProps) { const { id } = await params; // Keep as is for now - // TODO: Metadata generation might need authenticated user if chats are private - // For now, assuming getChat can be called or it handles anon access for metadata appropriately - const userId = await getCurrentUserIdOnServer(); // Attempt to get user for metadata - const chat = await getChat(id, userId || 'anonymous'); // Pass userId or 'anonymous' if none + // getChat will handle authenticated or anonymous access via session + const chat = await getChat(id); return { title: chat?.title?.toString().slice(0, 50) || 'Search', }; @@ -34,14 +32,14 @@ export default async function SearchPage({ params }: SearchPageProps) { redirect('/'); } - const chat = await getChat(id, userId); + const chat = await getChat(id); if (!chat) { // If chat doesn't exist or user doesn't have access (handled by getChat) notFound(); } - // Fetch messages for the chat + // Fetch messages for the chat (verified by getChatMessages) const dbMessages: DrizzleMessage[] = await getChatMessages(chat.id); // Transform DrizzleMessages to AIMessages diff --git a/components/history-list.tsx b/components/history-list.tsx index 5b67c538..a0ced7d3 100644 --- a/components/history-list.tsx +++ b/components/history-list.tsx @@ -5,16 +5,16 @@ import { getChats } from '@/lib/actions/chat'; import type { Chat as DrizzleChat } from '@/lib/actions/chat-db'; type HistoryListProps = { - userId?: string; + // userId is no longer used here as getChats() retrieves it from the session }; -const loadChats = cache(async (userId?: string): Promise => { - return await getChats(userId); +const loadChats = cache(async (): Promise => { + return await getChats(); }); -export async function HistoryList({ userId }: HistoryListProps) { +export async function HistoryList({}: HistoryListProps) { try { - const chats = await loadChats(userId); + const chats = await loadChats(); if (!chats) { return ( diff --git a/components/settings/components/settings.tsx b/components/settings/components/settings.tsx index 7a526e18..515d63f4 100644 --- a/components/settings/components/settings.tsx +++ b/components/settings/components/settings.tsx @@ -86,7 +86,7 @@ export function Settings({ initialTab = "system-prompt" }: SettingsProps) { if (!userId || authLoading) return; const [existingPrompt, selectedModel] = await Promise.all([ - getSystemPrompt(userId), + getSystemPrompt(), getSelectedModel(), ]); @@ -119,7 +119,7 @@ export function Settings({ initialTab = "system-prompt" }: SettingsProps) { try { // Save the system prompt and selected model const [promptSaveResult, modelSaveResult] = await Promise.all([ - saveSystemPrompt(userId, data.systemPrompt), + saveSystemPrompt(data.systemPrompt), saveSelectedModel(data.selectedModel), ]); diff --git a/lib/actions/chat-db.ts b/lib/actions/chat-db.ts index e15ea4ad..19aa70ef 100644 --- a/lib/actions/chat-db.ts +++ b/lib/actions/chat-db.ts @@ -19,9 +19,9 @@ export type NewMessage = typeof messages.$inferInsert; * @param userId - The ID of the user requesting the chat. * @returns The chat object if found and accessible, otherwise null. */ -export async function getChat(id: string, userId: string): Promise { - if (!userId) { - console.warn('getChat called without userId'); +export async function getChat(id: string, userId: string | null): Promise { + if (!userId || userId === 'anonymous') { + console.warn('getChat called without valid userId'); // Potentially allow fetching public chats if userId is null for anonymous users const result = await db.select().from(chats).where(and(eq(chats.id, id), eq(chats.visibility, 'public'))).limit(1); return result[0] || null; diff --git a/lib/actions/chat.ts b/lib/actions/chat.ts index f36f2cf6..509402c0 100644 --- a/lib/actions/chat.ts +++ b/lib/actions/chat.ts @@ -20,9 +20,14 @@ import { users } from '@/lib/db/schema' import { eq } from 'drizzle-orm' import { getCurrentUserIdOnServer } from '@/lib/auth/get-current-user' -export async function getChats(userId?: string | null): Promise { +/** + * Retrieves chats for the current authenticated user. + * Protected against IDOR by retrieving userId from session. + */ +export async function getChats(): Promise { + const userId = await getCurrentUserIdOnServer() if (!userId) { - console.warn('getChats called without userId, returning empty array.') + console.warn('getChats: No authenticated user found.') return [] } @@ -35,13 +40,15 @@ export async function getChats(userId?: string | null): Promise { } } -export async function getChat(id: string, userId: string): Promise { - if (!userId) { - console.warn('getChat called without userId.') - return null; - } +/** + * Retrieves a specific chat, ensuring the current user has access. + * Protected against IDOR by retrieving userId from session. + */ +export async function getChat(id: string): Promise { + const userId = await getCurrentUserIdOnServer() try { - const chat = await dbGetChat(id, userId) + // dbGetChat verifies ownership or public visibility + const chat = await dbGetChat(id, userId || 'anonymous') return chat } catch (error) { console.error(`Error fetching chat ${id} from DB:`, error) @@ -50,14 +57,25 @@ export async function getChat(id: string, userId: string): Promise { if (!chatId) { console.warn('getChatMessages called without chatId'); return []; } + + const userId = await getCurrentUserIdOnServer(); + try { + // Verify user has access to this chat before returning messages + const chat = await dbGetChat(chatId, userId || 'anonymous'); + if (!chat) { + console.warn(`getChatMessages: Unauthorized access attempt to chat ${chatId}`); + return []; + } + return dbGetMessagesByChatId(chatId); } catch (error) { console.error(`Error fetching messages for chat ${chatId} in getChatMessages:`, error); @@ -65,17 +83,18 @@ export async function getChatMessages(chatId: string): Promise } } -export async function clearChats( - userId?: string | null -): Promise<{ error?: string } | void> { - const currentUserId = userId || (await getCurrentUserIdOnServer()) - if (!currentUserId) { - console.error('clearChats: No user ID provided or found.') - return { error: 'User ID is required to clear chats' } +/** + * Clears chat history for the current authenticated user. + */ +export async function clearChats(): Promise<{ error?: string } | void> { + const userId = await getCurrentUserIdOnServer() + if (!userId) { + console.error('clearChats: No authenticated user found.') + return { error: 'Authentication required to clear chats' } } try { - const success = await dbClearHistory(currentUserId) + const success = await dbClearHistory(userId) if (!success) { return { error: 'Failed to clear chats from database.' } } @@ -87,16 +106,19 @@ export async function clearChats( } } -export async function saveChat(chat: OldChatType, userId: string): Promise { - if (!userId && !chat.userId) { - console.error('saveChat: userId is required either as a parameter or in chat object.') - return null; +/** + * Saves a chat for the current authenticated user. + */ +export async function saveChat(chat: OldChatType): Promise { + const userId = await getCurrentUserIdOnServer() + if (!userId) { + console.error('saveChat: User not authenticated.') + return null } - const effectiveUserId = userId || chat.userId; const newChatData: DbNewChat = { id: chat.id, - userId: effectiveUserId, + userId: userId, title: chat.title || 'Untitled Chat', createdAt: chat.createdAt ? new Date(chat.createdAt) : new Date(), visibility: 'private', @@ -105,7 +127,7 @@ export async function saveChat(chat: OldChatType, userId: string): Promise[] = chat.messages.map(msg => ({ id: msg.id, - userId: effectiveUserId, + userId: userId, role: msg.role, content: typeof msg.content === 'object' ? JSON.stringify(msg.content) : msg.content, createdAt: msg.createdAt ? new Date(msg.createdAt) : new Date(), @@ -120,6 +142,9 @@ export async function saveChat(chat: OldChatType, userId: string): Promise { - if (!userId) return { error: 'User ID is required' } + const userId = await getCurrentUserIdOnServer() + if (!userId) return { error: 'Authentication required' } if (!prompt) return { error: 'Prompt is required' } try { @@ -166,9 +194,11 @@ export async function saveSystemPrompt( } } -export async function getSystemPrompt( - userId: string -): Promise { +/** + * Retrieves the system prompt for the current authenticated user. + */ +export async function getSystemPrompt(): Promise { + const userId = await getCurrentUserIdOnServer() if (!userId) return null try {