Conversation
JustinTan-1
requested review from
Lujarios,
Yemyam,
burtonjong,
jason-duong4509,
promatty,
tanvimahal,
themaxboucher and
wesleylui
as code owners
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced a driver-specific trips query with the shared Changes
Sequence DiagramsequenceDiagram
actor Driver
participant Dashboard
participant BookingsAPI
participant SurveyButtons
participant AuthClient
Driver->>Dashboard: Open dashboard
Dashboard->>BookingsAPI: call bookings.getAll(startDate,endDate)
BookingsAPI->>BookingsAPI: authorize (role === "admin" || role === "driver")
BookingsAPI-->>Dashboard: return bookings
Dashboard->>SurveyButtons: mount
SurveyButtons->>AuthClient: useSession()
AuthClient-->>SurveyButtons: session data
SurveyButtons->>SurveyButtons: (optionally) filter bookings by session.user.id
SurveyButtons-->>Driver: render surveys
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/_components/drivercomponents/driver-dashboard.tsx (1)
21-34:⚠️ Potential issue | 🟠 MajorDrivers will see all bookings, not just their own.
Unlike
surveyButtons.tsxwhich filtersrawBookingsbysession?.user?.id, this component assigns all results fromgetAlldirectly todriverTripswithout filtering. Drivers will see every booking in the system on their calendar.At minimum, apply the same client-side filter used in
surveyButtons.tsx. However, the proper fix is server-side filtering in thegetAllprocedure (see comment onbookings.ts).Proposed client-side filter (interim fix)
+ import { authClient } from "@/lib/auth-client"; export const DriverDashboard = () => { + const { data: session } = authClient.useSession(); const [dateJSON, setDateJSON] = useState<CalendarDates>({ startDate: "", endDate: "", }); let driverTrips = [{}] as Booking[]; const tripQuery = api.bookings.getAll.useQuery( { startDate: dateJSON.startDate, endDate: dateJSON.endDate, }, { enabled: dateJSON.startDate !== "" && dateJSON.endDate !== "" }, ); if (tripQuery.data) { - driverTrips = tripQuery.data as Booking[]; + driverTrips = (tripQuery.data as Booking[]).filter( + (b) => b.driverId === session?.user?.id + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/_components/drivercomponents/driver-dashboard.tsx` around lines 21 - 34, The component currently assigns all results from api.bookings.getAll.useQuery directly to driverTrips, exposing every booking to drivers; update the assignment to filter tripQuery.data by the current user id (same logic used in surveyButtons.tsx that filters rawBookings by session?.user?.id) so driverTrips only includes bookings where booking.driverId (or the appropriate booking user id field) === session?.user?.id; locate the tripQuery and driverTrips variables in driver-dashboard.tsx and apply the client-side filter as an interim fix until you implement server-side filtering in the getAll procedure.src/server/api/routers/bookings.ts (1)
151-157:⚠️ Potential issue | 🔴 CriticalSecurity issue: Drivers can access all bookings without server-side filtering.
Allowing
role === "driver"into the admin branch grants drivers access to ALL bookings in the system. ThegetDriverTripprocedure (lines 279-336) correctly filters byeq(bookings.driverId, user.id), but this change bypasses that protection.Drivers should only see bookings assigned to them. Apply server-side filtering for the driver role.
Proposed fix: Add server-side filtering for drivers
- if (role === "admin" || role === "driver") { + if (role === "admin") { return ctx.db .select() .from(bookings) .where(and(...conditions)) .orderBy(desc(bookings.createdAt)); } + if (role === "driver") { + return ctx.db + .select() + .from(bookings) + .where(and(eq(bookings.driverId, userId), ...conditions)) + .orderBy(desc(bookings.createdAt)); + } + return ctx.db🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/api/routers/bookings.ts` around lines 151 - 157, The current branch in bookings.ts that returns all bookings when role === "admin" || role === "driver" lets drivers see every booking; change the logic so admin continues to get unfiltered results but drivers get an added server-side filter eq(bookings.driverId, user.id). In the bookings router code block (the if checking role === "admin" || role === "driver"), split the branch into admin and driver cases: for admin return the existing query, for driver add and(eq(bookings.driverId, user.id), ...conditions) (use ctx.session.user.id or user.id as in this file) so drivers only receive bookings assigned to them; mirror the same server-side restriction used by getDriverTrip to locate the correct filtering approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/_components/drivercomponents/surveyButtons/surveyButtons.tsx`:
- Around line 30-37: The client is calling api.bookings.getAll.useQuery and then
filtering rawBookings by session?.user?.id (bookings variable), which leaks all
bookings to the client; move the primary authorization into the server-side
getAll procedure so it only returns bookings for the authenticated driver, e.g.
check ctx.session.user.id inside the getAll resolver and apply a query filter
(or require a driverId param validated against ctx.session) before fetching from
the DB; keep the client-side filter in surveyButtons.tsx as defense-in-depth but
ensure getAll enforces the authorization so no other drivers' bookings are sent
to the client.
---
Outside diff comments:
In `@src/app/_components/drivercomponents/driver-dashboard.tsx`:
- Around line 21-34: The component currently assigns all results from
api.bookings.getAll.useQuery directly to driverTrips, exposing every booking to
drivers; update the assignment to filter tripQuery.data by the current user id
(same logic used in surveyButtons.tsx that filters rawBookings by
session?.user?.id) so driverTrips only includes bookings where booking.driverId
(or the appropriate booking user id field) === session?.user?.id; locate the
tripQuery and driverTrips variables in driver-dashboard.tsx and apply the
client-side filter as an interim fix until you implement server-side filtering
in the getAll procedure.
In `@src/server/api/routers/bookings.ts`:
- Around line 151-157: The current branch in bookings.ts that returns all
bookings when role === "admin" || role === "driver" lets drivers see every
booking; change the logic so admin continues to get unfiltered results but
drivers get an added server-side filter eq(bookings.driverId, user.id). In the
bookings router code block (the if checking role === "admin" || role ===
"driver"), split the branch into admin and driver cases: for admin return the
existing query, for driver add and(eq(bookings.driverId, user.id),
...conditions) (use ctx.session.user.id or user.id as in this file) so drivers
only receive bookings assigned to them; mirror the same server-side restriction
used by getDriverTrip to locate the correct filtering approach.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa081775-64a1-45e3-b345-af5d3bef8acf
📒 Files selected for processing (3)
src/app/_components/drivercomponents/driver-dashboard.tsxsrc/app/_components/drivercomponents/surveyButtons/surveyButtons.tsxsrc/server/api/routers/bookings.ts
| const { data: rawBookings = [] } = api.bookings.getAll.useQuery( | ||
| { surveyCompleted: false }, | ||
| { | ||
| initialData: initialBookings, | ||
| refetchInterval: 60000, | ||
| }, | ||
| ); | ||
| const bookings = rawBookings.filter((b) => b.driverId === session?.user?.id); |
There was a problem hiding this comment.
Client-side filtering is insufficient for authorization.
While filtering by session?.user?.id prevents displaying other drivers' bookings in the UI, all booking data is still fetched and sent to the client. A driver could inspect network traffic or call the API directly to see all bookings.
Authorization should be enforced server-side in the getAll procedure. This client-side filter can remain as defense-in-depth but should not be the primary access control mechanism.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/_components/drivercomponents/surveyButtons/surveyButtons.tsx` around
lines 30 - 37, The client is calling api.bookings.getAll.useQuery and then
filtering rawBookings by session?.user?.id (bookings variable), which leaks all
bookings to the client; move the primary authorization into the server-side
getAll procedure so it only returns bookings for the authenticated driver, e.g.
check ctx.session.user.id inside the getAll resolver and apply a query filter
(or require a driverId param validated against ctx.session) before fetching from
the DB; keep the client-side filter in surveyButtons.tsx as defense-in-depth but
ensure getAll enforces the authorization so no other drivers' bookings are sent
to the client.
| refetchInterval: 60000, | ||
| }, | ||
| ); | ||
| const bookings = rawBookings.filter((b) => b.driverId === session?.user?.id); |
There was a problem hiding this comment.
Is this intended...? If we want the driver to see all bookings then why are we filtering here?
2 participants
Changed getAll api endpoint to get all bookings if role is driver. Changed query in driver-dashboard.tsx to getAll instead of getDriverTrip.
Summary by CodeRabbit