fix: isolate delivery OTP into dedicated delivery_otps table with RLS(#597)#618
Open
ionfwsrijan wants to merge 3 commits into
Open
fix: isolate delivery OTP into dedicated delivery_otps table with RLS(#597)#618ionfwsrijan wants to merge 3 commits into
ionfwsrijan wants to merge 3 commits into
Conversation
Contributor
|
🎉 Thank you for your contribution! Your pull request has been received and will be reviewed shortly. If you enjoy the project, please consider giving the repository a ⭐. You can also follow my GitHub profile to stay updated on future open-source projects. Thanks for being part of the community! 🚀 |
📝 WalkthroughWalkthroughDelivery OTP storage is moved from inline ChangesDelivery OTP Isolation
Client-Side Cleanup and Refactors
Sequence Diagram(s)sequenceDiagram
participant Driver
participant OrderRoutes
participant notificationService
participant delivery_otps
participant orders
rect rgba(100, 149, 237, 0.5)
Note over Driver,orders: In Transit milestone — OTP generation
Driver->>OrderRoutes: PUT /:id/milestones {milestone: "In Transit"}
OrderRoutes->>notificationService: getActiveDeliveryOtp(orderId)
notificationService->>delivery_otps: SELECT unverified, unexpired OTP
delivery_otps-->>notificationService: null (no active OTP)
OrderRoutes->>notificationService: expireDeliveryOtps(orderId)
OrderRoutes->>notificationService: storeDeliveryOtp(orderId, otp)
notificationService->>delivery_otps: INSERT otp with expires_at
OrderRoutes-->>Driver: 200 milestone updated
end
rect rgba(144, 238, 144, 0.5)
Note over Driver,orders: Delivery verification
Driver->>OrderRoutes: POST /:id/verify-delivery {otp}
OrderRoutes->>orders: SELECT id, escrow_status, driver_id
OrderRoutes->>notificationService: getActiveDeliveryOtp(orderId)
notificationService->>delivery_otps: SELECT latest unverified, unexpired
delivery_otps-->>notificationService: otpRecord
OrderRoutes->>notificationService: verifyDeliveryOtp(orderId)
notificationService->>delivery_otps: UPDATE verified=true, verified_at=now
OrderRoutes->>orders: UPDATE status (excluding terminal statuses)
OrderRoutes-->>Driver: 200 Delivery verified
end
rect rgba(255, 160, 122, 0.5)
Note over Driver,orders: Cancellation guard
Driver->>OrderRoutes: POST /:id/cancel
OrderRoutes->>delivery_otps: SELECT verified OTP for order
delivery_otps-->>OrderRoutes: verified record found
OrderRoutes-->>Driver: 409 Cannot cancel — delivery already verified
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
backend
![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)
backend/api/src/routes/orderRoutes.js (1)
736-736:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
responseOrderis undefined — this will throw a ReferenceError at runtime.The variable
responseOrderis referenced but never defined. Line 726 definesupdatedOrderfrom the database update. This appears to be a regression from removing the OTP-stripping logic.🐛 Proposed fix
- const response = { message: 'Milestone updated successfully.', order: responseOrder, milestone, status }; + const response = { message: 'Milestone updated successfully.', order: updatedOrder, milestone, status };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` at line 736, The response object on line 736 references the undefined variable `responseOrder`. Replace `responseOrder` with `updatedOrder` (the variable defined on line 726 that contains the result from the database update) in the response object construction to fix the ReferenceError.
🧹 Nitpick comments (1)
backend/api/src/routes/orderRoutes.js (1)
22-22: 💤 Low value
expireDeliveryOtpsis imported but never used.The function is imported but not called anywhere in this file. Either remove the unused import, or if the intent was to expire old OTPs before storing a new one in the milestone handler (lines 715-724), add the call there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/routes/orderRoutes.js` at line 22, The function `expireDeliveryOtps` is imported in the import statement at the top of the file but is never called anywhere in the file. Either remove `expireDeliveryOtps` from the import statement at the top of the file to clean up unused imports, or if the intent was to expire old OTPs before storing a new one, add a call to `expireDeliveryOtps` in the milestone handler section where `storeDeliveryOtp` is being invoked to ensure old OTPs are expired before new ones are created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/migration_add_delivery_otp.sql`:
- Around line 33-39: Remove the entire CREATE POLICY statement for
driver_select_delivery_otp (the policy that allows SELECT operations on
delivery_otps where order_id matches orders assigned to the current driver).
This policy exposes OTP verification secrets to drivers, which violates the
security requirement that drivers should have no access to OTP data. Delete the
complete policy block from the migration file.
- Line 8: The migration file stores OTP in plaintext by using a column named
`otp`, which poses a security risk since OTP secrets should never be stored in
recoverable form. Rename the `otp TEXT NOT NULL` column to `otp_hash TEXT NOT
NULL` in the migration_add_delivery_otp.sql file to properly indicate that this
column should store hashed OTP values rather than raw plaintext OTP. Ensure all
references to this column in the application code are updated to match the new
column name.
---
Outside diff comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Line 736: The response object on line 736 references the undefined variable
`responseOrder`. Replace `responseOrder` with `updatedOrder` (the variable
defined on line 726 that contains the result from the database update) in the
response object construction to fix the ReferenceError.
---
Nitpick comments:
In `@backend/api/src/routes/orderRoutes.js`:
- Line 22: The function `expireDeliveryOtps` is imported in the import statement
at the top of the file but is never called anywhere in the file. Either remove
`expireDeliveryOtps` from the import statement at the top of the file to clean
up unused imports, or if the intent was to expire old OTPs before storing a new
one, add a call to `expireDeliveryOtps` in the milestone handler section where
`storeDeliveryOtp` is being invoked to ensure old OTPs are expired before new
ones are created.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dd770585-2784-4394-9699-23b004be3b40
📒 Files selected for processing (3)
backend/api/src/routes/orderRoutes.jsbackend/api/src/services/notificationService.jsdocs/migration_add_delivery_otp.sql
ionfwsrijan
force-pushed
the
fix/issue-597-delivery-otp-isolation
branch
from
922b9f9 to
611c38b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/api/src/services/notificationService.js (1)
100-120: ⚡ Quick winUse
loggerinstead ofconsole.error/console.logfor consistency.The OTP lifecycle helpers use
console.errorandconsole.logwhile the rest of this file uses the importedlogger. This creates inconsistent log formatting and makes log aggregation harder.♻️ Suggested fix
export async function storeDeliveryOtp(orderId, otp, ttlMinutes = 15) { const expiresAt = new Date(Date.now() + ttlMinutes * 60 * 1000).toISOString(); const { data, error } = await supabase .from('delivery_otps') .insert({ order_id: orderId, otp, expires_at: expiresAt, }) .select('id') .single(); if (error) { - console.error('[NotificationService] Failed to store OTP:', error.message); + logger.error(`[NotificationService] Failed to store OTP: ${error.message}`); return null; } - console.log(`[NotificationService] OTP stored for order ${orderId}, expires at ${expiresAt}`); + logger.info(`[NotificationService] OTP stored for order ${orderId}, expires at ${expiresAt}`); return data; }Apply similar changes to
getActiveDeliveryOtp,verifyDeliveryOtp, andexpireDeliveryOtps.Also applies to: 128-145, 153-169, 177-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/services/notificationService.js` around lines 100 - 120, Replace all console.error and console.log calls with the logger instance that is already imported and used elsewhere in the file. In the storeDeliveryOtp function, replace the console.error call with logger.error when handling the error case, and replace the console.log call with an appropriate logger method (such as logger.info) for the success case. Apply the same pattern to the getActiveDeliveryOtp, verifyDeliveryOtp, and expireDeliveryOtps functions to ensure consistent logging throughout the OTP lifecycle helpers and maintain uniform log formatting across the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/src/services/notificationService.js`:
- Around line 201-214: The plaintext OTP is being persisted directly in the
notification body variable that gets inserted into the notifications table,
creating an unnecessary security vulnerability if the notifications table has
weaker RLS policies. Replace the body template string to remove the embedded OTP
value and instead use a generic reference message like "Your delivery OTP has
been generated. Open the app to view it." This way the actual OTP remains
isolated in the delivery_otps table and must be fetched through its own secured
access path.
In `@docs/migration_add_delivery_otp.sql`:
- Line 56: The comment on line 56 stating "no OTP changes needed" for the
complete_trip_tx RPC is misleading because the RPC does reference OTP fields
such as otp_verified being set to true. Since the otp_verified column is being
dropped in this migration, update the comment to accurately reflect that OTP
changes ARE required for complete_trip_tx, and either describe what those
changes are or remove the misleading statement entirely.
- Around line 42-49: The service role access control in the delivery_otps
policies is inconsistent with the project's standard pattern. Replace the
auth.role() function checks in both service_insert_delivery_otp and
service_update_delivery_otp policies with role-based grants using the TO
service_role clause instead. For the INSERT policy, add TO service_role and
change the WITH CHECK condition to (true). For the UPDATE policy, add TO
service_role and change both the USING and WITH CHECK conditions to (true). This
aligns with the pattern used for other tables like profiles, orders, and
driver_details.
- Around line 52-54: The migration_add_delivery_otp.sql is dropping the
otp_verified column from the orders table, but the complete_trip_tx RPC function
in migration_complete_trip_update.sql (at line 71) still contains a SET
otp_verified = true statement. Since migrations run alphabetically, this will
cause a runtime error when the function tries to reference a dropped column.
Remove the SET otp_verified = true line from the complete_trip_tx function in
migration_complete_trip_update.sql to align with the stated intention that the
function only uses orders.total_amount.
---
Nitpick comments:
In `@backend/api/src/services/notificationService.js`:
- Around line 100-120: Replace all console.error and console.log calls with the
logger instance that is already imported and used elsewhere in the file. In the
storeDeliveryOtp function, replace the console.error call with logger.error when
handling the error case, and replace the console.log call with an appropriate
logger method (such as logger.info) for the success case. Apply the same pattern
to the getActiveDeliveryOtp, verifyDeliveryOtp, and expireDeliveryOtps functions
to ensure consistent logging throughout the OTP lifecycle helpers and maintain
uniform log formatting across the service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48737a56-e257-453e-9424-b669735bfaa5
📒 Files selected for processing (3)
backend/api/src/routes/orderRoutes.jsbackend/api/src/services/notificationService.jsdocs/migration_add_delivery_otp.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/api/src/routes/orderRoutes.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (1)
backend/api/src/services/notificationService.js (1)
100-120: ⚡ Quick winUse
loggerinstead ofconsole.error/console.logfor consistency.The OTP lifecycle helpers use
console.errorandconsole.logwhile the rest of this file uses the importedlogger. This creates inconsistent log formatting and makes log aggregation harder.♻️ Suggested fix
export async function storeDeliveryOtp(orderId, otp, ttlMinutes = 15) { const expiresAt = new Date(Date.now() + ttlMinutes * 60 * 1000).toISOString(); const { data, error } = await supabase .from('delivery_otps') .insert({ order_id: orderId, otp, expires_at: expiresAt, }) .select('id') .single(); if (error) { - console.error('[NotificationService] Failed to store OTP:', error.message); + logger.error(`[NotificationService] Failed to store OTP: ${error.message}`); return null; } - console.log(`[NotificationService] OTP stored for order ${orderId}, expires at ${expiresAt}`); + logger.info(`[NotificationService] OTP stored for order ${orderId}, expires at ${expiresAt}`); return data; }Apply similar changes to
getActiveDeliveryOtp,verifyDeliveryOtp, andexpireDeliveryOtps.Also applies to: 128-145, 153-169, 177-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/services/notificationService.js` around lines 100 - 120, Replace all console.error and console.log calls with the logger instance that is already imported and used elsewhere in the file. In the storeDeliveryOtp function, replace the console.error call with logger.error when handling the error case, and replace the console.log call with an appropriate logger method (such as logger.info) for the success case. Apply the same pattern to the getActiveDeliveryOtp, verifyDeliveryOtp, and expireDeliveryOtps functions to ensure consistent logging throughout the OTP lifecycle helpers and maintain uniform log formatting across the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/api/src/services/notificationService.js`:
- Around line 201-214: The plaintext OTP is being persisted directly in the
notification body variable that gets inserted into the notifications table,
creating an unnecessary security vulnerability if the notifications table has
weaker RLS policies. Replace the body template string to remove the embedded OTP
value and instead use a generic reference message like "Your delivery OTP has
been generated. Open the app to view it." This way the actual OTP remains
isolated in the delivery_otps table and must be fetched through its own secured
access path.
In `@docs/migration_add_delivery_otp.sql`:
- Line 56: The comment on line 56 stating "no OTP changes needed" for the
complete_trip_tx RPC is misleading because the RPC does reference OTP fields
such as otp_verified being set to true. Since the otp_verified column is being
dropped in this migration, update the comment to accurately reflect that OTP
changes ARE required for complete_trip_tx, and either describe what those
changes are or remove the misleading statement entirely.
- Around line 42-49: The service role access control in the delivery_otps
policies is inconsistent with the project's standard pattern. Replace the
auth.role() function checks in both service_insert_delivery_otp and
service_update_delivery_otp policies with role-based grants using the TO
service_role clause instead. For the INSERT policy, add TO service_role and
change the WITH CHECK condition to (true). For the UPDATE policy, add TO
service_role and change both the USING and WITH CHECK conditions to (true). This
aligns with the pattern used for other tables like profiles, orders, and
driver_details.
- Around line 52-54: The migration_add_delivery_otp.sql is dropping the
otp_verified column from the orders table, but the complete_trip_tx RPC function
in migration_complete_trip_update.sql (at line 71) still contains a SET
otp_verified = true statement. Since migrations run alphabetically, this will
cause a runtime error when the function tries to reference a dropped column.
Remove the SET otp_verified = true line from the complete_trip_tx function in
migration_complete_trip_update.sql to align with the stated intention that the
function only uses orders.total_amount.
---
Nitpick comments:
In `@backend/api/src/services/notificationService.js`:
- Around line 100-120: Replace all console.error and console.log calls with the
logger instance that is already imported and used elsewhere in the file. In the
storeDeliveryOtp function, replace the console.error call with logger.error when
handling the error case, and replace the console.log call with an appropriate
logger method (such as logger.info) for the success case. Apply the same pattern
to the getActiveDeliveryOtp, verifyDeliveryOtp, and expireDeliveryOtps functions
to ensure consistent logging throughout the OTP lifecycle helpers and maintain
uniform log formatting across the service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48737a56-e257-453e-9424-b669735bfaa5
📒 Files selected for processing (3)
backend/api/src/routes/orderRoutes.jsbackend/api/src/services/notificationService.jsdocs/migration_add_delivery_otp.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/api/src/routes/orderRoutes.js
🛑 Comments failed to post (4)
backend/api/src/services/notificationService.js (1)
201-214:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaw OTP is persisted in the
notificationstable body.Line 202 embeds the plaintext OTP in the notification body that gets inserted into the
notificationstable. Ifnotificationshas weaker RLS thandelivery_otps, this creates an alternative access path to the OTP—undermining the isolation goal.Consider storing only a reference (e.g., "Your delivery OTP has been generated. Open the app to view it.") and letting the client fetch the OTP through the secured
delivery_otpspath.🔒 Suggested fix
const title = 'Delivery Verification OTP'; - const body = `Your delivery OTP for order ${orderDisplayId} is ${otp}. Share this with the driver only after verifying your cargo has arrived safely.`; + const body = `Your delivery OTP for order ${orderDisplayId} is ready. Open the app to view it. Share with the driver only after verifying your cargo has arrived safely.`;📝 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.const title = 'Delivery Verification OTP'; const body = `Your delivery OTP for order ${orderDisplayId} is ready. Open the app to view it. Share with the driver only after verifying your cargo has arrived safely.`; // 1. Database Notification Persistence (always attempted first) try { const { error } = await supabase .from('notifications') .insert({ user_id: customerId, title, body, notif_type: 'order_update', metadata: { order_display_id: orderDisplayId }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/src/services/notificationService.js` around lines 201 - 214, The plaintext OTP is being persisted directly in the notification body variable that gets inserted into the notifications table, creating an unnecessary security vulnerability if the notifications table has weaker RLS policies. Replace the body template string to remove the embedded OTP value and instead use a generic reference message like "Your delivery OTP has been generated. Open the app to view it." This way the actual OTP remains isolated in the delivery_otps table and must be fetched through its own secured access path.docs/migration_add_delivery_otp.sql (3)
42-49:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check if auth.role() is defined as a custom function or used elsewhere rg -n 'auth\.role\(\)' --type sql rg -n 'CREATE.*FUNCTION.*auth\.role' --type sqlRepository: KanishJebaMathewM/Truxify
Length of output: 315
🏁 Script executed:
# Search for other RLS policy definitions to understand patterns rg -n 'CREATE POLICY' --type sql -A 2 # Search for auth.jwt() usage for role checking rg -n 'auth\.jwt\(\)' --type sql # Search for current_setting usage rg -n 'current_setting' --type sqlRepository: KanishJebaMathewM/Truxify
Length of output: 17049
Use
TO service_rolerole grant instead ofauth.role() = 'service_role'.The delivery_otp policies use
auth.role() = 'service_role', which is inconsistent with how the rest of the project implements service role access. All other tables (profiles, orders, driver_details, load_offers, etc.) grant service role access usingFOR ALL TO service_role USING (true) WITH CHECK (true). Replace the non-standardauth.role()checks with role-based grants via theTOclause:Suggested change
CREATE POLICY service_insert_delivery_otp ON delivery_otps FOR INSERT TO service_role WITH CHECK (true); CREATE POLICY service_update_delivery_otp ON delivery_otps FOR UPDATE TO service_role USING (true) WITH CHECK (true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/migration_add_delivery_otp.sql` around lines 42 - 49, The service role access control in the delivery_otps policies is inconsistent with the project's standard pattern. Replace the auth.role() function checks in both service_insert_delivery_otp and service_update_delivery_otp policies with role-based grants using the TO service_role clause instead. For the INSERT policy, add TO service_role and change the WITH CHECK condition to (true). For the UPDATE policy, add TO service_role and change both the USING and WITH CHECK conditions to (true). This aligns with the pattern used for other tables like profiles, orders, and driver_details.
52-54:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find all SQL files that reference otp_verified to assess migration impact rg -n 'otp_verified' --type sqlRepository: KanishJebaMathewM/Truxify
Length of output: 414
🏁 Script executed:
cat -n docs/migration_complete_trip_update.sql | head -80Repository: KanishJebaMathewM/Truxify
Length of output: 3035
🏁 Script executed:
cat -n docs/migration_add_delivery_otp.sqlRepository: KanishJebaMathewM/Truxify
Length of output: 2425
🏁 Script executed:
git ls-files docs/*.sql | sortRepository: KanishJebaMathewM/Truxify
Length of output: 609
Dropping
otp_verifiedwill break existingcomplete_trip_txRPC.The migration
migration_complete_trip_update.sql:71still containsSET otp_verified = true, butmigration_add_delivery_otp.sql:53drops this column. If migrations run in alphabetical order (as typical), the column will be dropped before the function attempts to use it, causing SQL runtime errors.The comment in migration_add_delivery_otp.sql incorrectly states "Update complete_trip_tx RPC (no OTP changes needed — it uses orders.total_amount only)", but the actual function still references the column.
Either update
complete_trip_txto remove theSET otp_verified = trueline, or ensuremigration_complete_trip_update.sqlruns beforemigration_add_delivery_otp.sql.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/migration_add_delivery_otp.sql` around lines 52 - 54, The migration_add_delivery_otp.sql is dropping the otp_verified column from the orders table, but the complete_trip_tx RPC function in migration_complete_trip_update.sql (at line 71) still contains a SET otp_verified = true statement. Since migrations run alphabetically, this will cause a runtime error when the function tries to reference a dropped column. Remove the SET otp_verified = true line from the complete_trip_tx function in migration_complete_trip_update.sql to align with the stated intention that the function only uses orders.total_amount.
56-56:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComment claims no OTP changes needed, but
complete_trip_txdoes reference OTP fields.Line 56 states "no OTP changes needed" for
complete_trip_tx, but the context snippet shows it explicitly setsotp_verified = true. This comment is misleading given the column is being dropped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/migration_add_delivery_otp.sql` at line 56, The comment on line 56 stating "no OTP changes needed" for the complete_trip_tx RPC is misleading because the RPC does reference OTP fields such as otp_verified being set to true. Since the otp_verified column is being dropped in this migration, update the comment to accurately reflect that OTP changes ARE required for complete_trip_tx, and either describe what those changes are or remove the misleading statement entirely.
- Fix milestone handler using undefined responseOrder variable (responseOrder was removed with OTP-stripping code, use updatedOrder) - Add explicit verified: false to storeDeliveryOtp insert payload - Fix test pollution by resetting delivery_otps between OTP tests - Update 8 OTP integration tests to seed delivery_otps table - Add wallet addresses + escrow mock to two bid-acceptance tests (upstream wallet validation gate returns 422 when wallets missing)
- Replace undefined _client/_httpClient/_apiBaseUrl references in customer ProfileService.logout() with SupabaseService.client and _apiClient - Remove duplicate http import in driver profile_screen.dart - Remove unnecessary null check on RealtimePostgresChangesPayload.newRecord (non-nullable type)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/api/test/integration/orders.test.js (3)
1114-1117: ⚡ Quick winAdd assertion for verified_at field.
The test only validates that
verifiedis set totrue, but does not check thatverified_atis populated. According to theverifyDeliveryOtpimplementation (notificationService.js:151-156), both fields should be set upon successful verification.📋 Proposed enhancement
// OTP record should be marked as verified in isolated table const otpRecord = m.store.delivery_otps.find(o => o.order_id === 'order-1'); expect(otpRecord.verified).toBe(true); +expect(otpRecord.verified_at).toBeDefined(); +expect(new Date(otpRecord.verified_at).getTime()).toBeGreaterThan(Date.now() - 5000); // within last 5s🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/test/integration/orders.test.js` around lines 1114 - 1117, Add an assertion to verify that the verified_at field is populated when an OTP record is successfully verified. After the existing expect assertion for otpRecord.verified being true, add another assertion to check that otpRecord.verified_at is not null, undefined, or empty. This ensures the test validates both the verified flag and the timestamp field that the verifyDeliveryOtp implementation sets together according to the notificationService.js logic.
1005-1005: ⚡ Quick winConsider validating the expires_at timestamp range.
The test only checks that
expires_atis defined, but doesn't verify it's set to a future time within the expected 15-minute window. Based on thestoreDeliveryOtpimplementation (which setsttlMinutes = 15by default), consider adding an assertion like:const expiresAt = new Date(otpRecord.expires_at); const now = Date.now(); expect(expiresAt.getTime()).toBeGreaterThan(now); expect(expiresAt.getTime()).toBeLessThanOrEqual(now + 15 * 60 * 1000 + 1000); // +1s for clock skewThis ensures the expiry window is correctly computed and prevents regressions if the TTL logic changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/test/integration/orders.test.js` at line 1005, The current assertion for otpRecord.expires_at only verifies the field is defined but does not validate the actual timestamp value. Replace the toBeDefined check with additional assertions that verify the expires_at timestamp is a future time and falls within the expected 15-minute TTL window from the current time (accounting for small clock skew). This ensures the OTP expiry logic is working correctly and prevents regressions if the TTL calculation changes in the storeDeliveryOtp implementation.
1332-1338: ⚡ Quick winConsider more robust new OTP identification.
Line 1333 identifies the new OTP by filtering for records where
o.otp !== '123456'. While statistically unlikely with 6-digit OTPs, there's a theoretical chance the newly generated OTP could be '123456' again, causing the test to fail intermittently.Consider identifying the new OTP by
created_attimestamp or by excluding the known old OTP id:const newOtps = m.store.delivery_otps.filter(o => o.order_id === orderId && o.id !== 'otp-regen-old');This approach is deterministic and eliminates any chance of flakiness.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/api/test/integration/orders.test.js` around lines 1332 - 1338, The newOtps filter at line 1333 uses a value-based comparison (o.otp !== '123456') which is not deterministic and could fail if the newly generated OTP happens to be '123456' again. Replace this filtering logic with a more robust approach by either tracking and excluding the id of the previously known OTP, or by identifying the new OTP using the created_at timestamp to select the most recent entry. This will ensure the test is deterministic and eliminate any possibility of flakiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/customer/lib/services/profile_service.dart`:
- Around line 45-56: The backend logout request in the try block can stall
indefinitely, blocking the local sign-out that occurs in the finally block. Add
a timeout boundary to the _apiClient.post call so that if the backend logout
takes too long to respond, it fails fast and allows the
SupabaseService.client.auth.signOut() in the finally block to proceed without
unnecessary delays, preventing customers from being stuck waiting for the
backend to respond.
---
Nitpick comments:
In `@backend/api/test/integration/orders.test.js`:
- Around line 1114-1117: Add an assertion to verify that the verified_at field
is populated when an OTP record is successfully verified. After the existing
expect assertion for otpRecord.verified being true, add another assertion to
check that otpRecord.verified_at is not null, undefined, or empty. This ensures
the test validates both the verified flag and the timestamp field that the
verifyDeliveryOtp implementation sets together according to the
notificationService.js logic.
- Line 1005: The current assertion for otpRecord.expires_at only verifies the
field is defined but does not validate the actual timestamp value. Replace the
toBeDefined check with additional assertions that verify the expires_at
timestamp is a future time and falls within the expected 15-minute TTL window
from the current time (accounting for small clock skew). This ensures the OTP
expiry logic is working correctly and prevents regressions if the TTL
calculation changes in the storeDeliveryOtp implementation.
- Around line 1332-1338: The newOtps filter at line 1333 uses a value-based
comparison (o.otp !== '123456') which is not deterministic and could fail if the
newly generated OTP happens to be '123456' again. Replace this filtering logic
with a more robust approach by either tracking and excluding the id of the
previously known OTP, or by identifying the new OTP using the created_at
timestamp to select the most recent entry. This will ensure the test is
deterministic and eliminate any possibility of flakiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c824c34-98b9-4d47-8078-5f8d9b8b726f
📒 Files selected for processing (7)
apps/customer/lib/services/profile_service.dartapps/driver/lib/screens/profile_screen.dartapps/driver/lib/services/marketplace_repository.dartbackend/api/src/routes/orderRoutes.jsbackend/api/src/services/notificationService.jsbackend/api/test/integration/bids.test.jsbackend/api/test/integration/orders.test.js
💤 Files with no reviewable changes (1)
- apps/driver/lib/screens/profile_screen.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/api/src/services/notificationService.js
- backend/api/src/routes/orderRoutes.js
Contributor
Author
|
@KanishJebaMathewM Please review this and add labels |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Delivery OTPs were stored as columns on the
orderstable (delivery_otp,otp_verified,otp_generated_at). Any SELECT onorders— through broad RLS policies, a service-role bypass, or autogenerated APIs — exposed active and historical OTPs. Drivers could read past OTPs from completed deliveries, and a compromised service key could extract all OTPs in one query.Solution
delivery_otpstable with FK toorders,expires_at,verified,verified_at. Enables RLS: customers SELECT their own OTPs, drivers SELECT assigned orders' OTPs, service_role INSERT/UPDATE only. Drops old OTP columns fromorders.storeDeliveryOtp(),getActiveDeliveryOtp(),verifyDeliveryOtp(),expireDeliveryOtps()withexpires_at-based filteringstoreDeliveryOtp()instead of writing toorders. Verify-delivery reads fromdelivery_otpsviagetActiveDeliveryOtp(), callsverifyDeliveryOtp()on success. Cancel route checksdelivery_otpsfor verified OTP before allowing cancellation.Files changed
docs/migration_add_delivery_otp.sql— newdelivery_otpstable + RLS policiesbackend/api/src/services/notificationService.js— OTP persistence functionsbackend/api/src/routes/orderRoutes.js— OTP generation, verification, cancellationCloses #597
Summary by CodeRabbit