🛡️ Sentinel: Fix IDOR vulnerabilities and enhance search security#526
🛡️ Sentinel: Fix IDOR vulnerabilities and enhance search security#526ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
This commit addresses several security issues in the chat and user actions: - Fixed IDOR vulnerabilities in `lib/actions/chat.ts` by ensuring all server actions (getChats, getChat, getChatMessages, saveChat, clearChats, updateDrawingContext, saveSystemPrompt, getSystemPrompt) use the authenticated session's userId instead of trusting client-provided arguments. - Added authorization checks to `getChatMessages` and `updateDrawingContext` to verify chat ownership/access. - Enhanced `searchUsers` in `lib/actions/users.ts` with input sanitization, escaping SQL wildcard characters (%, _, \) and enforcing a 255-character length limit to prevent wildcard enumeration and resource exhaustion. - Documented these security findings and preventions in `.jules/sentinel.md`. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughSecurity vulnerabilities are addressed by implementing session-based authentication to prevent IDOR attacks and adding input sanitization in search queries. Chat functions now retrieve userId from secure sessions instead of trusting passed parameters. A new documentation file documents these vulnerability patterns and mitigations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/actions/chat.ts (1)
97-117:⚠️ Potential issue | 🔴 Critical
redirect()will be caught by thecatchblock, preventing the actual redirect.In Next.js,
redirect()works by throwing a specialNEXT_REDIRECTerror. Because it's called inside thetryblock (Line 112), thecatchon Line 113 will intercept it and return{ error: 'Failed to clear chat history' }instead of redirecting.Move
redirect()outside thetry/catch, or re-throw the redirect error.🐛 Proposed fix
try { const success = await dbClearHistory(currentUserId) if (!success) { return { error: 'Failed to clear chats from database.' } } revalidatePath('/') - redirect('/') } catch (error) { console.error('Error clearing chats from DB:', error) return { error: 'Failed to clear chat history' } } + redirect('/')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/actions/chat.ts` around lines 97 - 117, The try/catch around dbClearHistory is catching Next.js's special redirect throw; update clearChats to avoid swallowing that: either move revalidatePath('/') and redirect('/') outside the try/catch (call them after a successful dbClearHistory) or detect and re-throw the NEXT_REDIRECT in the catch (e.g., if ((error as any)?.name === 'NEXT_REDIRECT') throw error). Ensure dbClearHistory success handling remains (returning an error object on failure) and keep references to dbClearHistory, revalidatePath, and redirect unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.jules/sentinel.mdlib/actions/chat.tslib/actions/users.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-10T08:45:03.719Z
Learnt from: ngoiyaeric
Repo: QueueLab/QCX PR: 0
File: :0-0
Timestamp: 2026-02-10T08:45:03.719Z
Learning: The app/api/chats/route.ts endpoint correctly returns { chats: [] } for unauthenticated users instead of a 401 error. This is the intended behavior for better UX and should not be changed to return 401.
Applied to files:
lib/actions/chat.ts
🧬 Code graph analysis (1)
lib/actions/chat.ts (7)
lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)lib/db/schema.ts (2)
chats(30-40)users(22-28)lib/actions/chat-db.ts (2)
getChat(22-40)saveChat(83-126)lib/db/index.ts (1)
db(25-25)components/sidebar/chat-history-client.tsx (1)
ChatHistoryClientProps(26-28)components/history-list.tsx (1)
userId(11-13)app/actions.tsx (1)
state(598-659)
🪛 markdownlint-cli2 (0.20.0)
.jules/sentinel.md
[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)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (4)
lib/actions/users.ts (1)
193-210: LGTM — input sanitization correctly prevents wildcard enumeration.The single-pass regex properly escapes
\,%, and_without double-escaping issues, and the 255-char limit is a reasonable guard against resource exhaustion.lib/actions/chat.ts (3)
23-41: Good IDOR fix — session-based auth with graceful fallback for unauthenticated users.Returns
[]for unauthenticated users, consistent with the intended UX pattern. Based on learnings, the app correctly returns empty arrays for unauthenticated users instead of 401 errors.
163-168: Good defense-in-depth: ownership check on write path.
dbGetChatallows access to public chats, but the additionalchat.userId !== userIdcheck correctly restricts mutations to the chat owner only. This is the right pattern — read access ≠ write access.
67-91: LGTM — proper authorization check before returning messages.Verifying chat access via
dbGetChat(which checks ownership or public visibility) before returning messages is the correct approach to prevent IDOR on the messages endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.jules/sentinel.md:
- Around line 1-9: Add a top-level title line (e.g., "# Sentinel Vulnerability
Log") at the top of the file and ensure there is a blank line before each
secondary heading such as "## 2025-05-14 - [IDOR in Server Actions]" and "##
2025-05-14 - [Wildcard Enumeration in SQL]" to satisfy markdownlint rules MD041
and MD022; update the file so headings have surrounding blank lines and the
document begins with a single H1 heading.
In `@lib/actions/chat.ts`:
- Line 27: Add explicit `@deprecated` JSDoc tags to the unused userId parameters
on the functions getChats, getChat, clearChats, saveChat, saveSystemPrompt, and
getSystemPrompt to make it clear they are ignored; update each function's
declaration comment to include something like "@deprecated userId parameter is
ignored — getCurrentUserIdOnServer() is used for IDOR protection" and ensure the
JSDoc is placed immediately above the function so IDEs and linters surface the
deprecation; keep the parameter in place for backward compatibility but mark it
deprecated so callers know it has no effect and can be removed in a follow-up.
---
Outside diff comments:
In `@lib/actions/chat.ts`:
- Around line 97-117: The try/catch around dbClearHistory is catching Next.js's
special redirect throw; update clearChats to avoid swallowing that: either move
revalidatePath('/') and redirect('/') outside the try/catch (call them after a
successful dbClearHistory) or detect and re-throw the NEXT_REDIRECT in the catch
(e.g., if ((error as any)?.name === 'NEXT_REDIRECT') throw error). Ensure
dbClearHistory success handling remains (returning an error object on failure)
and keep references to dbClearHistory, revalidatePath, and redirect unchanged.
| ## 2025-05-14 - [IDOR in Server Actions] | ||
| **Vulnerability:** Several server actions in `lib/actions/chat.ts` accepted a `userId` parameter from the client and used it directly for database queries without verification, allowing any authenticated user to read or modify data belonging to other users. | ||
| **Learning:** Next.js Server Actions are public API endpoints. Parameters passed from the client cannot be trusted for authorization. | ||
| **Prevention:** Always retrieve the `userId` from a secure session (e.g., via `getCurrentUserIdOnServer()`) inside the server action and ignore or verify any `userId` passed as an argument. | ||
|
|
||
| ## 2025-05-14 - [Wildcard Enumeration in SQL] | ||
| **Vulnerability:** The `searchUsers` function used unescaped user input in an `ilike` query, allowing users to enumerate the entire user table using wildcard characters like `%` or `_`. | ||
| **Learning:** Wildcard characters in `LIKE`/`ILIKE` queries can be abused to bypass filters or extract data if not properly escaped. | ||
| **Prevention:** Always escape `%`, `_`, and `\` in user input before using it in a `LIKE`/`ILIKE` query. Also, enforce length limits on search queries. |
There was a problem hiding this comment.
Minor markdown lint issues flagged by markdownlint.
The file is missing a top-level heading (#) and blank lines around ## headings, as flagged by static analysis (MD041, MD022). Consider adding a title and blank lines for proper formatting.
📝 Proposed fix
+# Sentinel Security Notes
+
## 2025-05-14 - [IDOR in Server Actions]
+
**Vulnerability:** Several server actions in `lib/actions/chat.ts` accepted a `userId` parameter from the client and used it directly for database queries without verification, allowing any authenticated user to read or modify data belonging to other users.
**Learning:** Next.js Server Actions are public API endpoints. Parameters passed from the client cannot be trusted for authorization.
**Prevention:** Always retrieve the `userId` from a secure session (e.g., via `getCurrentUserIdOnServer()`) inside the server action and ignore or verify any `userId` passed as an argument.
## 2025-05-14 - [Wildcard Enumeration in SQL]
+
**Vulnerability:** The `searchUsers` function used unescaped user input in an `ilike` query, allowing users to enumerate the entire user table using wildcard characters like `%` or `_`.
**Learning:** Wildcard characters in `LIKE`/`ILIKE` queries can be abused to bypass filters or extract data if not properly escaped.
**Prevention:** Always escape `%`, `_`, and `\` in user input before using it in a `LIKE`/`ILIKE` query. Also, enforce length limits on search queries.📝 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.
| ## 2025-05-14 - [IDOR in Server Actions] | |
| **Vulnerability:** Several server actions in `lib/actions/chat.ts` accepted a `userId` parameter from the client and used it directly for database queries without verification, allowing any authenticated user to read or modify data belonging to other users. | |
| **Learning:** Next.js Server Actions are public API endpoints. Parameters passed from the client cannot be trusted for authorization. | |
| **Prevention:** Always retrieve the `userId` from a secure session (e.g., via `getCurrentUserIdOnServer()`) inside the server action and ignore or verify any `userId` passed as an argument. | |
| ## 2025-05-14 - [Wildcard Enumeration in SQL] | |
| **Vulnerability:** The `searchUsers` function used unescaped user input in an `ilike` query, allowing users to enumerate the entire user table using wildcard characters like `%` or `_`. | |
| **Learning:** Wildcard characters in `LIKE`/`ILIKE` queries can be abused to bypass filters or extract data if not properly escaped. | |
| **Prevention:** Always escape `%`, `_`, and `\` in user input before using it in a `LIKE`/`ILIKE` query. Also, enforce length limits on search queries. | |
| # Sentinel Security Notes | |
| ## 2025-05-14 - [IDOR in Server Actions] | |
| **Vulnerability:** Several server actions in `lib/actions/chat.ts` accepted a `userId` parameter from the client and used it directly for database queries without verification, allowing any authenticated user to read or modify data belonging to other users. | |
| **Learning:** Next.js Server Actions are public API endpoints. Parameters passed from the client cannot be trusted for authorization. | |
| **Prevention:** Always retrieve the `userId` from a secure session (e.g., via `getCurrentUserIdOnServer()`) inside the server action and ignore or verify any `userId` passed as an argument. | |
| ## 2025-05-14 - [Wildcard Enumeration in SQL] | |
| **Vulnerability:** The `searchUsers` function used unescaped user input in an `ilike` query, allowing users to enumerate the entire user table using wildcard characters like `%` or `_`. | |
| **Learning:** Wildcard characters in `LIKE`/`ILIKE` queries can be abused to bypass filters or extract data if not properly escaped. | |
| **Prevention:** Always escape `%`, `_`, and `\` in user input before using it in a `LIKE`/`ILIKE` query. Also, enforce length limits on search queries. |
🧰 Tools
🪛 markdownlint-cli2 (0.20.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)
[warning] 6-6: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.jules/sentinel.md around lines 1 - 9, Add a top-level title line (e.g., "#
Sentinel Vulnerability Log") at the top of the file and ensure there is a blank
line before each secondary heading such as "## 2025-05-14 - [IDOR in Server
Actions]" and "## 2025-05-14 - [Wildcard Enumeration in SQL]" to satisfy
markdownlint rules MD041 and MD022; update the file so headings have surrounding
blank lines and the document begins with a single H1 heading.
| * Retrieves chats for the authenticated user. | ||
| * Protected against IDOR by using session-based userId. | ||
| */ | ||
| export async function getChats(userId?: string | null): Promise<DrizzleChat[]> { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the lib/actions/chat.ts file to see the function signatures and implementations
cat -n lib/actions/chat.ts | head -250Repository: QueueLab/QCX
Length of output: 8977
🏁 Script executed:
# Search for all call sites of these functions in the codebase
rg -n 'getChats\(|getChat\(|clearChats\(|saveChat\(|saveSystemPrompt\(|getSystemPrompt\(' --type=ts --type=tsx -C 2 -g '!lib/actions/chat.ts'Repository: QueueLab/QCX
Length of output: 83
🏁 Script executed:
# Also check the specific file mentions in the review: app/actions.tsx line 658 and components/history-list.tsx
cat -n app/actions.tsx | sed -n '650,670p'Repository: QueueLab/QCX
Length of output: 686
🏁 Script executed:
cat -n components/history-list.tsx | head -50Repository: QueueLab/QCX
Length of output: 1900
Consider deprecating unused userId parameters or removing them in a follow-up.
The functions getChats, getChat, clearChats, saveChat, saveSystemPrompt, and getSystemPrompt accept userId parameters that are now completely ignored—they use getCurrentUserIdOnServer() instead for IDOR protection. Some already include comments acknowledging they're "kept for backward compatibility but ignored." However, call sites like app/actions.tsx (line 658) and components/history-list.tsx (line 12) still pass values, creating confusion about whether the parameters matter.
Add explicit @deprecated JSDoc tags to these parameters or remove them entirely in a follow-up refactor to eliminate this confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/actions/chat.ts` at line 27, Add explicit `@deprecated` JSDoc tags to the
unused userId parameters on the functions getChats, getChat, clearChats,
saveChat, saveSystemPrompt, and getSystemPrompt to make it clear they are
ignored; update each function's declaration comment to include something like
"@deprecated userId parameter is ignored — getCurrentUserIdOnServer() is used
for IDOR protection" and ensure the JSDoc is placed immediately above the
function so IDEs and linters surface the deprecation; keep the parameter in
place for backward compatibility but mark it deprecated so callers know it has
no effect and can be removed in a follow-up.
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security improvements are directionally correct, but a couple of changes still leave room for regressions. In getChatMessages, authorization is decoupled from message retrieval, which can reintroduce IDOR if dbGetMessagesByChatId isn’t also scoped. In searchUsers, escaping wildcards should be paired with an explicit ESCAPE clause (or equivalent ORM support) to make the protection reliable. Also consider adding payload limits/validation in updateDrawingContext to prevent oversized writes.
Additional notes (1)
- Security |
lib/actions/chat.ts:160-172
updateDrawingContextusesany[]/anyforcontextData. Even with server-side ownership checks, accepting arbitrary-shaped payloads and blindlyJSON.stringify-ing can lead to unexpectedly large writes (DoS) and makes it hard to validate/limit content. Given this PR’s security theme, you should at minimum enforce size limits (e.g., max number of features / max serialized byte length) and ideally validate a concrete schema before persisting.
Summary of changes
Summary of changes
Security hardening (IDOR)
- Refactored multiple chat-related server actions in
lib/actions/chat.tsto derive the acting user fromgetCurrentUserIdOnServer()rather than trusting a client-supplieduserId. - Added access/ownership checks before returning chat messages and before writing drawing-context messages.
- Kept some
userIdparameters in function signatures for backward compatibility, but they are now ignored.
Search query sanitization
- Updated
searchUsersinlib/actions/users.tsto:- enforce a
255-character limit, - escape SQL wildcard characters (
%,_,\) prior to building anILIKEpattern, - continue limiting results to
10.
- enforce a
Documentation
- Added
.jules/sentinel.mddocumenting the IDOR and SQL wildcard enumeration findings and preventative guidance.
| export async function getChat(id: string, userId: string): Promise<DrizzleChat | null> { | ||
| if (!userId) { | ||
| console.warn('getChat called without userId.') | ||
| const currentUserId = await getCurrentUserIdOnServer(); | ||
| if (!currentUserId) { | ||
| console.warn('getChat: User not authenticated'); | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const chat = await dbGetChat(id, userId) | ||
| const chat = await dbGetChat(id, currentUserId) |
There was a problem hiding this comment.
getChat still accepts a userId argument but ignores it. Unlike clearChats/saveSystemPrompt/getSystemPrompt, there’s no comment indicating the parameter is deprecated/ignored. This creates an easy footgun for callers who think they’re scoping access by passing a specific userId (and it also makes the API inconsistent within this file).
Suggestion
Make the intent explicit and reduce misuse risk by either (a) removing the userId parameter (preferred if you can do a small breaking change) or (b) marking it as deprecated/ignored and renaming to _userId.
Example (non-breaking):
// TODO: remove `userId` parameter in next major; ignored in favor of session userId
export async function getChat(id: string, _userId: string): Promise<DrizzleChat | null> {
const currentUserId = await getCurrentUserIdOnServer();
if (!currentUserId) return null;
return dbGetChat(id, currentUserId);
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| export async function getChatMessages(chatId: string): Promise<DrizzleMessage[]> { | ||
| if (!chatId) { | ||
| console.warn('getChatMessages called without chatId'); | ||
| return []; | ||
| } | ||
|
|
||
| const userId = await getCurrentUserIdOnServer(); | ||
| if (!userId) { | ||
| console.warn('getChatMessages: User not authenticated'); | ||
| return []; | ||
| } | ||
|
|
||
| try { | ||
| // Verify access (ownership or public visibility) | ||
| const chat = await dbGetChat(chatId, userId); | ||
| if (!chat) { | ||
| console.warn(`getChatMessages: User ${userId} does not have access to chat ${chatId}`); | ||
| return []; | ||
| } | ||
| return dbGetMessagesByChatId(chatId); | ||
| } catch (error) { |
There was a problem hiding this comment.
getChatMessages performs an authorization query (dbGetChat) and then fetches messages (dbGetMessagesByChatId) in a separate call. If dbGetMessagesByChatId does not also enforce ownership/visibility, this becomes a TOCTOU-style gap where authorization and data retrieval are decoupled (and it’s also an extra round-trip). Since the stated goal is to prevent IDOR, prefer retrieving messages via a single query that joins/filters by chatId and userId (or a visibility rule) at the DB layer.
Suggestion
Refactor to a single DB method that enforces authorization while fetching messages, e.g. dbGetMessagesForUserChat(chatId, userId) or a join-based query. This reduces the chance of future regressions where one layer forgets the check.
Example shape:
// chat-db.ts
export async function dbGetMessagesForUserChat(chatId: string, userId: string) {
// SELECT m.* FROM messages m
// JOIN chats c ON c.id = m.chatId
// WHERE m.chatId = ? AND c.userId = ?
}
// action
return dbGetMessagesForUserChat(chatId, userId);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| */ | ||
| export async function searchUsers(query: string) { | ||
| noStore(); | ||
| if (!query) return []; | ||
|
|
||
| // Enforce input length limit to mitigate DoS/resource exhaustion | ||
| const sanitizedQuery = query.slice(0, 255); | ||
|
|
||
| const userId = await getCurrentUserIdOnServer(); | ||
| if (!userId) { | ||
| throw new Error('Unauthorized'); | ||
| } | ||
|
|
||
| try { | ||
| // Escape special characters (%, _, \) to prevent wildcard enumeration | ||
| const escapedQuery = sanitizedQuery.replace(/[%_\\]/g, '\\$&'); | ||
|
|
||
| const result = await db.select({ | ||
| id: users.id, | ||
| email: users.email, | ||
| }) | ||
| .from(users) | ||
| .where(ilike(users.email, `%${query}%`)) | ||
| .where(ilike(users.email, `%${escapedQuery}%`)) | ||
| .limit(10); |
There was a problem hiding this comment.
Escaping %, _, and \ is a good start, but ILIKE wildcard escaping is only correct if the generated SQL includes an explicit ESCAPE '\\' (or if your DB/ORM guarantees backslash escaping semantics). Without ESCAPE, some databases/configurations treat \ as a normal character, and the wildcard may still be interpreted, weakening the intended protection.
Given this is explicitly fixing a high-severity enumeration issue, make the escape semantics unambiguous at the query level.
Suggestion
Ensure the query uses an explicit escape clause (or use an ORM helper that generates one). If Drizzle doesn’t support ESCAPE via ilike directly, consider using sql to express it.
Example (Postgres):
import { sql } from 'drizzle-orm';
const escapedQuery = sanitizedQuery.replace(/[\\%_]/g, '\\$&');
const pattern = `%${escapedQuery}%`;
.where(sql`${users.email} ILIKE ${pattern} ESCAPE '\\'`)Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this.
2 participants
🛡️ Sentinel Security Update:
read_fileand ensured type safety and build stability withbun run lintandbun run build.PR created automatically by Jules for task 9092276581003351957 started by @ngoiyaeric
Summary by CodeRabbit
Documentation
Security