🛡️ Sentinel: [HIGH] Fix IDOR and insecure server actions#534
🛡️ Sentinel: [HIGH] Fix IDOR and insecure server actions#534ngoiyaeric wants to merge 1 commit intomainfrom
Conversation
This commit secures the chat-related server actions by: 1. Retrieving `userId` from the session server-side using `getCurrentUserIdOnServer()` instead of accepting it as a client-side argument. 2. Adding authorization checks to `getChatMessages` to ensure users can only see messages from chats they have access to. 3. Updating all call sites to match the new secure function signatures. 4. Enhancing `dbGetChat` to safely handle anonymous access without causing UUID syntax errors. These changes prevent Insecure Direct Object Reference (IDOR) vulnerabilities where an attacker could otherwise access or modify other users' chat data. 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. |
There was a problem hiding this comment.
The PR meaningfully improves security by removing client-controlled userId parameters and adding access checks, but it still uses the magic 'anonymous' sentinel despite updating DB helpers to accept null, which dilutes the new contract. The biggest remaining concern is saveChat potentially allowing cross-user overwrites if dbSaveChat upserts/updates by chat.id without scoping to userId; ownership verification before save would close that gap. Consider also making unauthorized access in getChatMessages explicit rather than indistinguishable from an empty chat.
Additional notes (4)
- Maintainability |
lib/actions/chat.ts:47-54
getChat()currently callsdbGetChat(id, userId || 'anonymous'), butdbGetChatwas explicitly updated to acceptstring | nulland treat unauthenticated access safely. Passing the sentinel string'anonymous'reintroduces the "magic value" pattern and is easy to replicate elsewhere, undermining the goal of avoiding invalid UUIDs by design.
This also creates inconsistent semantics: sometimes unauthenticated is represented as null, sometimes as 'anonymous' (and you now need both checks, as seen in chat-db.ts).
- Security |
lib/actions/chat.ts:63-63
The access check is good, but it usesuserId || 'anonymous'. SincedbGetChatnow acceptsstring | null, the safer/cleaner contract is to passuserIddirectly (ornull) rather than a magic string.
Also, returning an empty array for unauthorized access makes it hard for callers to distinguish "no messages" vs "not allowed". If this is meant to be a hard authorization boundary, consider returning a structured error (or throwing) so the UI/route can respond appropriately (e.g., notFound() vs silent empty view).
- Security |
lib/actions/chat.ts:112-112
saveChatnow always persists the chat under the current session user, ignoring anychat.userIdprovided by the caller. That's the correct direction for IDOR prevention, but there’s a subtle correctness risk: if callers can pass achat.idthat already exists for another user,dbSaveChatmay update/overwrite depending on its upsert semantics.
Because the diff doesn’t show dbSaveChat, it’s unclear whether it enforces (id,userId) scoping. If id is globally unique and updates are keyed only by id, an attacker could potentially overwrite another user’s chat by guessing/obtaining the id even though you set userId on the new record.
This is especially important because server actions are public endpoints (as noted in the journal).
- Maintainability |
lib/actions/chat-db.ts:22-27
Inchat-db.ts,getChat(id, userId)now treats bothnulland'anonymous'as anonymous. Given the stated prevention guidance (acceptstring | nulland handlenull), continuing to support'anonymous'encourages other call sites to keep using the sentinel.
If you standardize on null for anonymous, you can remove 'anonymous' handling here to prevent regressions.
Summary of changes
What changed
Security hardening (IDOR)
- Refactored chat-related server actions in
lib/actions/chat.tsto stop acceptinguserIdfrom the client and instead derive it from the session viagetCurrentUserIdOnServer(). - Added an authorization gate in
getChatMessages(chatId)by callingdbGetChat(chatId, userId || 'anonymous')before returning messages.
DB access behavior
- Updated
lib/actions/chat-db.ts#getChatto acceptuserId: string | nulland to treatnull/'anonymous'as unauthenticated, falling back to public-only chat retrieval.
Call-site updates
- Updated frontend/server call sites to match new signatures:
getSystemPrompt()is now parameterless.saveChat(chat)no longer takes an explicituserId.getChat(id)no longer takes an explicituserId.getChats()no longer takesuserId.
Documentation
- Added
.jules/sentinel.mddocumenting the IDOR finding and the UUID/anonymous user caveat.
|
|
🛡️ Sentinel: [HIGH] Fix IDOR and insecure server actions in
lib/actions/chat.ts.🚨 Severity: HIGH
💡 Vulnerability: Insecure Direct Object Reference (IDOR)
🎯 Impact: Attackers could potentially view, modify, or delete chats belonging to other users by providing their user IDs or chat IDs to server actions.
🔧 Fix:
getChats,getChat,clearChats,saveChat,saveSystemPrompt, andgetSystemPromptto use the authenticated session'suserId.getChatMessages.✅ Verification:
bun run buildpassed, confirming type consistency across all call sites.bun run lintpassed.PR created automatically by Jules for task 17445475223123787931 started by @ngoiyaeric