🛡️ Sentinel: Fix mass assignment and wildcard enumeration vulnerabilities#539
Conversation
…ties - Fix mass assignment in `lib/actions/calendar.ts` by explicitly listing mutable columns in `saveNote`. - Fix wildcard enumeration in `lib/actions/users.ts` by escaping `ILIKE` special characters and enforcing length limits in `searchUsers`. - Add Sentinel Security Journal with critical learnings. 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.
|
|
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
✨ 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 |
There was a problem hiding this comment.
The security direction is correct, but searchUsers currently makes abusive/invalid queries hard to observe by silently returning [], and it should normalize input (trim) before validation/escaping. The new raw sql ILIKE ... ESCAPE clause should explicitly decide how to treat NULL emails (current behavior excludes them). In saveNote, the allowlist is good but should be centralized to avoid drift and accidental policy changes.
Additional notes (3)
-
Security |
lib/actions/users.ts:188-188
The length limit is a good start, but returning[]for overlong queries makes abusive traffic indistinguishable from “no matches” and can hide client bugs. Consider failing fast with a clear error (or at least logging a structured warning) so you can observe and rate-limit misuse. Also,queryshould be normalized (e.g.,trim()) before checks/escaping, otherwise inputs like' % 'can still behave unexpectedly and consume work. -
Maintainability |
lib/actions/users.ts:3-8
Using raw SQL here is justified for theESCAPEclause, but you should ensure consistent behavior withNULLemails.ILIKEagainstNULLyieldsNULL(notfalse), which effectively excludes those rows; that’s probably fine, but if the UI expects users withNULLemail to be searchable or shown, you’ll need explicit handling. Also, sinceilikeis no longer used, consider dropping it from the import to avoid confusion about which path is active. -
Maintainability |
lib/actions/calendar.ts:73-73
The explicit allowlist for mutable fields fixes mass assignment, but this creates a second risk: it’s easy for future schema changes to accidentally omit newly-intended editable fields (leading to “silent no-op” updates) or to accidentally add sensitive fields later. Consider centralizing the allowlist in a helper (e.g.,pickCalendarNoteMutableFields(noteData)) used by both insert/update so the policy is defined in one place and can be unit-tested.
Summary of changes
Summary
This PR hardens two server actions against common injection/authorization pitfalls:
-
Mass-assignment mitigation in
lib/actions/calendar.ts- Replaced spreading of client-supplied
noteDatain Drizzle.set()/.values()with an explicit allowlist of mutable columns (content,date,chatId,locationTags,userTags,mapFeatureId) and explicitupdatedAthandling.
- Replaced spreading of client-supplied
-
Wildcard-enumeration mitigation in
lib/actions/users.ts- Added
sqlusage to implement anILIKE ... ESCAPE '\\'clause. - Escaped special pattern characters (
%,_,\) and added a 100-char max length guard, returning[]for empty/too-long queries.
- Added
-
Documentation
- Added a Sentinel security journal entry at
.jules/sentinel.mddocumenting the two vulnerabilities and the remediation patterns.
- Added a Sentinel security journal entry at
This PR addresses two security vulnerabilities identified in the server actions:
Mass Assignment Prevention: In
lib/actions/calendar.ts, thesaveNoteaction used the spread operator on client-supplied data when updating or inserting into the database. This could allow a malicious user to overwrite protected fields likeuserId. I refactored the code to explicitly pick only the allowed mutable columns.Wildcard Enumeration Hardening: In
lib/actions/users.ts, thesearchUsersaction allowed unsanitized input in anILIKEquery. This enabled users to enumerate all email addresses by searching for%. I added a 100-character length limit, implemented escaping for special SQL characters (%,_,\), and used an explicitESCAPEclause in the query.Verified with
bun run lint,bun run build, and manual code inspection.PR created automatically by Jules for task 8454964759189557742 started by @ngoiyaeric