🛡️ Sentinel: Fix IDOR and SQL Wildcard Enumeration vulnerabilities#524
🛡️ Sentinel: Fix IDOR and SQL Wildcard Enumeration vulnerabilities#524ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
…ties Summary of changes: 1. Fix IDOR in `getChatMessages`: Added authentication and authorization checks to ensure users can only access their own or public chats. 2. Fix IDOR in `updateDrawingContext`: Added ownership check to ensure only the chat owner can add drawing context messages. 3. Fix SQL Wildcard Enumeration in `searchUsers`: Escaped special PostgreSQL characters (% , _, \) and added a length limit to the search query. 4. Added security learnings to `.jules/sentinel.md`. These changes improve the security posture of the application by protecting private user data and preventing information disclosure. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
👋 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. |
|
|
WalkthroughThe changes add security hardening across multiple server actions by implementing authentication enforcement, authorization checks, and SQL injection prevention. Documentation is added detailing IDOR vulnerabilities and secure server action patterns with input validation guidelines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
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)
170-187: 🧹 Nitpick | 🔵 TrivialPre-existing IDOR:
saveSystemPromptandgetSystemPromptacceptuserIdwithout authentication.While out of scope for this PR, these functions accept a caller-supplied
userIdand perform DB operations without verifying that the authenticated user matches. This is the same class of IDOR vulnerability being fixed ingetChatMessagesandupdateDrawingContext. The same pattern applies toclearChats(line 84), which uses the supplieduserIddirectly.Consider applying the same fix pattern (call
getCurrentUserIdOnServer()and ignore/reject mismatched caller-supplied IDs) in a follow-up.#!/bin/bash # Check how saveSystemPrompt and getSystemPrompt are called — are caller-supplied userIds verified upstream? rg -n --type=ts -C3 'saveSystemPrompt\|getSystemPrompt' -g '!node_modules' -g '!.next'
🤖 Fix all issues with AI agents
In @.jules/sentinel.md:
- Around line 1-4: Change the first line into a top-level heading by prefixing
it with "# " (e.g., "# 2026-02-16 - IDOR and Wildcard Enumeration in Server
Actions") and add blank lines immediately before and after each heading
(including the main heading and any "##" or "###" headings) so they are
surrounded by blank lines; also ensure the `<details>`/`<summary>` block is
separated from adjacent headings/content with blank lines to satisfy MD022 and
MD041.
In `@lib/actions/chat.ts`:
- Around line 60-72: The current behavior is inconsistent:
getCurrentUserIdOnServer() throws for unauthenticated but getChat(chatId,
userId) returns [] for unauthorized/DB errors; change to a consistent
return-of-empty-array policy by replacing the thrown Error from the
unauthenticated check with an early return of [] (instead of throw), keep the
existing try/catch around getChat, and ensure any logging (e.g., the
console.warn for unauthorized access) remains but does not change the return
contract; update callers/tests that assume an exception to expect
DrizzleMessage[] from this action.
In `@lib/actions/users.ts`:
- Around line 192-195: The input-length check currently throws an Error when
query.length > 255, causing inconsistent behavior because the empty-query path
returns [] earlier; update the logic in the same function that references query
(replace the throw new Error('Search query too long') branch) to return [] for
over-length queries instead of throwing so callers receive a uniform []
response, or alternatively add documentation/comments to the function explaining
that it will throw for too-long queries if you prefer keeping the throw (choose
one approach and apply it to the query length check).
📜 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 (3)
lib/actions/users.ts (3)
lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)lib/db/index.ts (1)
db(25-25)lib/db/schema.ts (1)
users(22-28)
lib/actions/chat.ts (4)
lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)lib/actions/chat-db.ts (2)
getChat(22-40)getMessagesByChatId(198-214)components/chat.tsx (1)
id(96-104)components/sidebar/chat-history-client.tsx (1)
ChatHistoryClientProps(26-28)
.jules/sentinel.md (1)
app/actions.tsx (2)
state(598-659)clearChat(550-559)
🪛 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)
🔇 Additional comments (2)
lib/actions/users.ts (1)
203-211: Wildcard escaping logic is correct and effectively mitigates enumeration.The regex
/[%_\\]/gcorrectly escapes all three PostgreSQL LIKE special characters in the right order (backslash first if encountered, then%and_).One robustness note: this relies on PostgreSQL's default behavior of treating
\as the LIKE escape character. If you ever need to make this explicit, you'd use a raw SQL fragment with anESCAPE '\'clause. For now, this is fine with standard PostgreSQL configurations.lib/actions/chat.ts (1)
144-157: Correct ownership enforcement for drawing context updates.The two-layer check is well designed:
getChatverifies view-access (owner or public), andchat.userId !== userIdfurther restricts mutation to the owner only. Moving the message construction (lines 151–157) after the authorization gate is the right fix — previously the payload was built before validation.LGTM.
| ## 2026-02-16 - IDOR and Wildcard Enumeration in Server Actions | ||
| **Vulnerability:** IDOR in `getChatMessages` and `updateDrawingContext` allowed unauthorized access/modification to chat data. `searchUsers` was susceptible to wildcard enumeration. | ||
| **Learning:** Exported server actions in Next.js ('use server') are public endpoints and must implement their own authentication and authorization checks, even if they are only intended for use in protected pages. | ||
| **Prevention:** Always retrieve the current user session within the server action and verify ownership/permissions before performing database operations. Escape special SQL characters in `LIKE`/`ILIKE` patterns from user input. |
There was a problem hiding this comment.
Fix markdown lint warnings (MD022, MD041).
Static analysis flags two issues: the first line should be a top-level heading (#), and headings need surrounding blank lines.
Proposed fix
-## 2026-02-16 - IDOR and Wildcard Enumeration in Server Actions
-**Vulnerability:** IDOR in `getChatMessages` and `updateDrawingContext` allowed unauthorized access/modification to chat data. `searchUsers` was susceptible to wildcard enumeration.
-**Learning:** Exported server actions in Next.js ('use server') are public endpoints and must implement their own authentication and authorization checks, even if they are only intended for use in protected pages.
-**Prevention:** Always retrieve the current user session within the server action and verify ownership/permissions before performing database operations. Escape special SQL characters in `LIKE`/`ILIKE` patterns from user input.
+# Sentinel Security Notes
+
+## 2026-02-16 - IDOR and Wildcard Enumeration in Server Actions
+
+**Vulnerability:** IDOR in `getChatMessages` and `updateDrawingContext` allowed unauthorized access/modification to chat data. `searchUsers` was susceptible to wildcard enumeration.
+
+**Learning:** Exported server actions in Next.js ('use server') are public endpoints and must implement their own authentication and authorization checks, even if they are only intended for use in protected pages.
+
+**Prevention:** Always retrieve the current user session within the server action and verify ownership/permissions before performing database operations. Escape special SQL characters in `LIKE`/`ILIKE` patterns from user input.📝 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.
| ## 2026-02-16 - IDOR and Wildcard Enumeration in Server Actions | |
| **Vulnerability:** IDOR in `getChatMessages` and `updateDrawingContext` allowed unauthorized access/modification to chat data. `searchUsers` was susceptible to wildcard enumeration. | |
| **Learning:** Exported server actions in Next.js ('use server') are public endpoints and must implement their own authentication and authorization checks, even if they are only intended for use in protected pages. | |
| **Prevention:** Always retrieve the current user session within the server action and verify ownership/permissions before performing database operations. Escape special SQL characters in `LIKE`/`ILIKE` patterns from user input. | |
| # Sentinel Security Notes | |
| ## 2026-02-16 - IDOR and Wildcard Enumeration in Server Actions | |
| **Vulnerability:** IDOR in `getChatMessages` and `updateDrawingContext` allowed unauthorized access/modification to chat data. `searchUsers` was susceptible to wildcard enumeration. | |
| **Learning:** Exported server actions in Next.js ('use server') are public endpoints and must implement their own authentication and authorization checks, even if they are only intended for use in protected pages. | |
| **Prevention:** Always retrieve the current user session within the server action and verify ownership/permissions before performing database operations. Escape special SQL characters in `LIKE`/`ILIKE` patterns from user input. |
🧰 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)
🤖 Prompt for AI Agents
In @.jules/sentinel.md around lines 1 - 4, Change the first line into a
top-level heading by prefixing it with "# " (e.g., "# 2026-02-16 - IDOR and
Wildcard Enumeration in Server Actions") and add blank lines immediately before
and after each heading (including the main heading and any "##" or "###"
headings) so they are surrounded by blank lines; also ensure the
`<details>`/`<summary>` block is separated from adjacent headings/content with
blank lines to satisfy MD022 and MD041.
|
|
||
| const userId = await getCurrentUserIdOnServer(); | ||
| if (!userId) { | ||
| throw new Error('Unauthorized: Authentication required'); | ||
| } | ||
|
|
||
| try { | ||
| // Check if the user has access to this chat | ||
| const chat = await getChat(chatId, userId); | ||
| if (!chat) { | ||
| console.warn(`Unauthorized access attempt to chat ${chatId} by user ${userId}`); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Good IDOR fix — but mixed error-handling contract may surprise callers.
The authentication and authorization checks correctly prevent unauthorized access to chat messages. However, the function has an inconsistent contract:
- Unauthenticated →
throw(line 63), which escapes thetry/catchat line 66. - Unauthorized (no chat access) → silently returns
[](line 71). - DB error → caught and returns
[](line 77).
A caller expecting DrizzleMessage[] will get an unhandled exception for unauthenticated requests but an empty array for everything else. Consider either returning [] consistently (aligning with the learned pattern that the chats API route returns empty for unauthenticated users) or throwing consistently.
Option A: return [] consistently (matches existing UX pattern)
const userId = await getCurrentUserIdOnServer();
if (!userId) {
- throw new Error('Unauthorized: Authentication required');
+ console.warn('getChatMessages: User not authenticated');
+ return [];
}Based on learnings, the app/api/chats/route.ts endpoint returns { chats: [] } for unauthenticated users instead of a 401 — a similar pattern here would keep the contract uniform.
🤖 Prompt for AI Agents
In `@lib/actions/chat.ts` around lines 60 - 72, The current behavior is
inconsistent: getCurrentUserIdOnServer() throws for unauthenticated but
getChat(chatId, userId) returns [] for unauthorized/DB errors; change to a
consistent return-of-empty-array policy by replacing the thrown Error from the
unauthenticated check with an early return of [] (instead of throw), keep the
existing try/catch around getChat, and ensure any logging (e.g., the
console.warn for unauthorized access) remains but does not change the return
contract; update callers/tests that assume an exception to expect
DrizzleMessage[] from this action.
| // Prevent DoS and long pattern matching | ||
| if (query.length > 255) { | ||
| throw new Error('Search query too long'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good addition of input length validation.
The length check correctly guards against excessively long patterns. One minor inconsistency: an empty query silently returns [] (line 190), but an over-length query throws. Consider returning [] here as well for a uniform caller experience, or document the throwing behavior so callers know to handle it.
🤖 Prompt for AI Agents
In `@lib/actions/users.ts` around lines 192 - 195, The input-length check
currently throws an Error when query.length > 255, causing inconsistent behavior
because the empty-query path returns [] earlier; update the logic in the same
function that references query (replace the throw new Error('Search query too
long') branch) to return [] for over-length queries instead of throwing so
callers receive a uniform [] response, or alternatively add
documentation/comments to the function explaining that it will throw for
too-long queries if you prefer keeping the throw (choose one approach and apply
it to the query length check).
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The main risk is in searchUsers: escaping wildcards may be ineffective unless the generated SQL includes ESCAPE '\\', which could leave wildcard enumeration partially unresolved. Additionally, getChatMessages mixes throw-vs-empty-array behavior for auth failures, making callers’ error handling inconsistent. Finally, the new authz warning logs include potentially sensitive identifiers; consider redaction to reduce exposure.
Additional notes (3)
- Maintainability |
lib/actions/chat.ts:57-76
getChatMessagescurrently returns an empty array on unauthorized access, which is indistinguishable from “chat exists but has no messages”. That’s good for reducing information disclosure, but the function throws for unauthenticated callers while silently returning[]for unauthorized callers. This inconsistency can lead to confusing client behavior and makes security posture harder to reason about across actions (some callers will needtry/catch, others won’t).
Given this is a server action used by UI code, decide on a single contract:
- either always throw on auth/authz failures, or
- always return a structured
{ error }result.
Mixing the two tends to create edge cases and can accidentally reintroduce side-channels depending on how the UI reacts to thrown vs non-thrown failures.
- Security |
lib/actions/chat.ts:140-161
updateDrawingContextreturns{ error: 'User not authenticated' }for unauthenticated users but returns{ error: 'Unauthorized: ...' }for non-owners. That’s fine, but you’re also logging thechatIdanduserIdin warning logs. If logs are accessible broadly (or shipped to third parties), this can leak sensitive identifiers.
Consider reducing identifier exposure in logs (e.g., omit userId, or hash/truncate), or log only at debug level with a request correlation ID.
- Maintainability |
lib/actions/users.ts:189-195
The new length limit throwsError('Search query too long'), while other failure paths return[](catch) or throwUnauthorized. This inconsistency is likely to bubble up as a 500 in some callers and a normal empty result in others. For user search, it’s generally better to return a deterministic client-friendly response (e.g.,[]) for invalid input, or a structured error shape, rather than throwing a generic error.
Summary of changes
Security hardening for server actions
- Added authorization checks to
getChatMessages(chatId)by fetching the current user viagetCurrentUserIdOnServer()and verifying access viagetChat(chatId, userId)before returning messages. - Restricted
updateDrawingContext(chatId, contextData)to chat owners only, moving message creation inside the authorization-guardedtryblock. - Mitigated wildcard enumeration in
searchUsers(query)by:- enforcing a 255 character max query length,
- escaping PostgreSQL pattern characters (
%,_,\) prior toilike(users.email, ...).
- Added a security learnings note in
.jules/sentinel.mddocumenting IDOR/wildcard enumeration prevention lessons.
2 participants
This PR addresses several security vulnerabilities in the application's server actions:
getChatMessages: Previously, any authenticated user could retrieve all messages for anychatIdby calling the server action directly. I added a check that verifies the user has access to the chat (either as owner or if it's public) before returning messages.updateDrawingContext: Authenticated users could add data messages to any chat. I added an ownership check to restrict this to the chat owner.searchUsers: Theilikesearch was susceptible to wildcard attacks (using%or_) which could allow a user to enumerate all user emails. I implemented escaping for these characters and added a 255-character length limit to the query.Verified with static checks (lint/tsc) and manual code review. e2e tests were not runnable in the environment but the logic is defensive and safe.
PR created automatically by Jules for task 13716273341324520366 started by @ngoiyaeric
Summary by CodeRabbit
Documentation
Bug Fixes