Skip to content

🛡️ Sentinel: [HIGH] Fix IDOR vulnerability in getChatMessages #531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ngoiyaeric wants to merge 1 commit into
base: main
Choose a base branch
from
+15 −12
Open

🛡️ Sentinel: [HIGH] Fix IDOR vulnerability in getChatMessages #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
## 2026-02-18 - [IDOR in Chat Message Retrieval]
**Vulnerability:** The `getChatMessages` server action in `lib/actions/chat.ts` was fetching messages by `chatId` without verifying if the requesting user was the owner of the chat or if the chat was public.
**Learning:** High-level server actions were relying on low-level database utilities that lacked authorization logic, assuming callers would perform checks. This led to an IDOR vulnerability where anyone could read any chat's messages if they knew the `chatId`.
**Prevention:** Always perform authorization checks in high-level server actions (the entry points for client calls) using the current user's ID from the session and verifying ownership or visibility of the target resource.
Comment on lines +1 to +4
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Two markdownlint violations: first-line heading level (MD041) and missing blank line after heading (MD022)

Line 1 uses an H2 (##) but MD041 requires the first line to be an H1 (#). MD022 also requires a blank line between the heading and the following content block.

📝 Proposed fix 
-## 2026-02-18 - [IDOR in Chat Message Retrieval]
-**Vulnerability:** The `getChatMessages` server action in `lib/actions/chat.ts` was fetching messages by `chatId` without verifying if the requesting user was the owner of the chat or if the chat was public.
+# Security Sentinel Log
+
+## 2026-02-18 - [IDOR in Chat Message Retrieval]
+
+**Vulnerability:** The `getChatMessages` server action in `lib/actions/chat.ts` was fetching messages by `chatId` without verifying if the requesting user was the owner of the chat or if the chat was public.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
## 2026-02-18 - [IDOR in Chat Message Retrieval]
**Vulnerability:** The `getChatMessages` server action in `lib/actions/chat.ts` was fetching messages by `chatId` without verifying if the requesting user was the owner of the chat or if the chat was public.
**Learning:** High-level server actions were relying on low-level database utilities that lacked authorization logic, assuming callers would perform checks. This led to an IDOR vulnerability where anyone could read any chat's messages if they knew the `chatId`.
**Prevention:** Always perform authorization checks in high-level server actions (the entry points for client calls) using the current user's ID from the session and verifying ownership or visibility of the target resource.
# Security Sentinel Log
## 2026-02-18 - [IDOR in Chat Message Retrieval]
**Vulnerability:** The `getChatMessages` server action in `lib/actions/chat.ts` was fetching messages by `chatId` without verifying if the requesting user was the owner of the chat or if the chat was public.
**Learning:** High-level server actions were relying on low-level database utilities that lacked authorization logic, assuming callers would perform checks. This led to an IDOR vulnerability where anyone could read any chat's messages if they knew the `chatId`.
**Prevention:** Always perform authorization checks in high-level server actions (the entry points for client calls) using the current user's ID from the session and verifying ownership or visibility of the target resource.
🧰 Tools
🪛 LanguageTool

[style] ~2-~2: You can shorten this phrase to avoid wordiness.
Context: ...ithout verifying if the requesting user was the owner of the chat or if the chat was public. **L...

(BE_THE_MAKER_OF_WORDINESS)

🪛 markdownlint-cli2 (0.21.0)

[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

[warning] 1-1: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.jules/sentinel.md around lines 1 - 4, Change the first-line heading in
.jules/sentinel.md from "## 2026-02-18 - [IDOR in Chat Message Retrieval]" to an
H1 by using a single leading "#" and insert a blank line immediately after that
heading so the content that follows is separated; update the exact heading text
"2026-02-18 - [IDOR in Chat Message Retrieval]" to the H1 form and ensure there
is one empty line before the paragraph starting with "**Vulnerability:**".

12 changes: 11 additions & 1 deletion lib/actions/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,24 @@ export async function getChat(id: string, userId: string): Promise<DrizzleChat |
}

/**
* Retrieves all messages for a specific chat.
* Retrieves all messages for a specific chat, ensuring authorization.
*/
export async function getChatMessages(chatId: string): Promise<DrizzleMessage[]> {
if (!chatId) {
console.warn('getChatMessages called without chatId');
return [];
}

const userId = await getCurrentUserIdOnServer();
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

getCurrentUserIdOnServer() is outside the try-catch, breaking the function's error-containment contract

If getCurrentUserIdOnServer() throws (e.g., session store unavailable, cookie parsing error), the exception propagates unhandled to the caller. The function signature contracts Promise<DrizzleMessage[]>, implying callers always get an array — never an unhandled rejection. All other error paths inside the try-catch return [], so this gap is inconsistent.

🛡️ Proposed fix — pull `getCurrentUserIdOnServer()` inside the `try` block 
 export async function getChatMessages(chatId: string): Promise<DrizzleMessage[]> {
   if (!chatId) {
     console.warn('getChatMessages called without chatId');
     return [];
   }
-
-  const userId = await getCurrentUserIdOnServer();
-
+  let userId: string | null | undefined;
   try {
+    userId = await getCurrentUserIdOnServer();
     // Verify user has access to this chat (either as owner or if it's public)
     const chat = await dbGetChat(chatId, userId || '');
     if (!chat) {
       console.warn(`Unauthorized access attempt to messages for chat ${chatId} by user ${userId}`);
       return [];
     }

     return dbGetMessagesByChatId(chatId);
   } catch (error) {
     console.error(`Error fetching messages for chat ${chatId} in getChatMessages:`, error);
     return [];
   }
 }

Also applies to: 63-63

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@lib/actions/chat.ts` at line 61, Move the call to getCurrentUserIdOnServer()
inside the existing try block so any exception it throws is caught and the
function still resolves to an empty array; specifically, in the function that
returns Promise<DrizzleMessage[]> wrap the getCurrentUserIdOnServer() invocation
within the try { ... } before using userId (rather than calling it above the
try), and on catch return [] as the other error paths do.


try {
// Verify user has access to this chat (either as owner or if it's public)
const chat = await dbGetChat(chatId, userId || '');
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Implicit coupling between userId || '' and dbGetChat's internal null-guard

dbGetChat uses !userId to route to the public-only path, which catches '' as falsy. If dbGetChat ever tightens its guard to userId === null, passing '' would silently fall through to the authenticated SQL branch with an empty userId, potentially causing incorrect query behaviour. A conditional call makes the intent explicit and decouples the call-site from dbGetChat's internals.

Additionally, the access check (dbGetChat) and the data fetch (dbGetMessagesByChatId) are not atomic. A chat's visibility could change between the two calls (TOCTOU). The window is negligible in practice, but worth noting.

♻️ Proposed refactor — explicit conditional call 
-    const chat = await dbGetChat(chatId, userId || '');
+    const chat = userId
+      ? await dbGetChat(chatId, userId)
+      : await dbGetChat(chatId, '');

Also applies to: 71-71

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@lib/actions/chat.ts` at line 65, The call-site is implicitly coupling to
dbGetChat's falsy check by passing userId || ''; change to an explicit
conditional: call dbGetChat(chatId) when userId == null/undefined and call
dbGetChat(chatId, userId) when a userId exists (replace the two occurrences
around dbGetChat usage). Also eliminate the TOCTOU between dbGetChat and
dbGetMessagesByChatId by either performing both reads inside a single DB
transaction or by introducing and calling a single helper like
dbGetChatAndMessages(chatId, userId) (or dbGetChatWithMessages) that validates
access and returns messages atomically; update callers to use that new atomic
method instead of separate dbGetChat and dbGetMessagesByChatId calls.

if (!chat) {
Comment on lines +61 to +66
Copy link

@charliecreates charliecreates bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing userId || '' into dbGetChat is a subtle contract: it assumes dbGetChat treats empty string as unauthenticated and still allows access to public chats. If dbGetChat was originally designed for authenticated access only, this could accidentally block legitimate public access (or worse, accidentally allow access if it treats '' as a wildcard in some SQL condition).

This is security-sensitive—make the contract explicit by passing null/undefined (if supported) or by using a dedicated dbGetPublicChat(chatId)/dbGetChatForViewer(chatId, viewerId) function.

Suggestion

Make the unauthenticated/public-access path explicit. Options:

  1. Update the DB helper signature to accept userId?: string and handle undefined explicitly.
  2. Add a dedicated helper:
const chat = userId
  ? await dbGetChat(chatId, userId)
  : await dbGetPublicChat(chatId)

This reduces ambiguity and prevents future regressions in dbGetChat.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing one of these approaches.

console.warn(`Unauthorized access attempt to messages for chat ${chatId} by user ${userId}`);
return [];
}
Comment on lines +64 to +69
Copy link

@charliecreates charliecreates bot Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging chatId and userId for unauthorized attempts can leak identifiers into logs (which often have broader access/retention than application data). This is especially risky if userId is considered sensitive or if chatId is guessable and appears frequently.

Prefer structured logging with redaction/hashing, or omit IDs and rely on request correlation IDs / auth context already present in server logs.

Suggestion

Redact or hash identifiers in the warning log (or log only one of them). For example:

console.warn('Unauthorized access attempt to chat messages', {
  chatId: chatId.slice(0, 8),
  hasUser: Boolean(userId),
})

Or rely on request-scoped correlation IDs if your logging stack supports it.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.


return dbGetMessagesByChatId(chatId);
} catch (error) {
console.error(`Error fetching messages for chat ${chatId} in getChatMessages:`, error);
Expand Down
11 changes: 0 additions & 11 deletions server.log
View file
Open in desktop

This file was deleted.