fix: eliminate TOCTOU race in getOrCreateChatChannel - #847
Conversation
The function had a classic check-then-insert pattern: first SELECT to see if a channel exists, then INSERT if not found. Two concurrent requests could both pass the SELECT and create duplicate channels. The schema already defines a unique index on (mentor_id, mentee_id). Fix: invert the order — try INSERT first with onConflictDoNothing(), then SELECT only if the insert conflicts (duplicate). This eliminates the TOCTOU window entirely by letting the database enforce uniqueness atomically. Closes Coder-s-OG-s#844
|
@ionfwsrijan is attempting to deploy a commit to the codersogs-3057's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@codersogs-code @Soumya-codr @Ayush-Patel-56 @jakharmonika364 Please review this |
|
CI failure, please fix it |
|
@jakharmonika364 @Soumya-codr Please review now |
jakharmonika364
left a comment
jakharmonika364
left a comment
There was a problem hiding this comment.
Fix is correct - verified the unique index (chat_channels_mentor_mentee_uniq) already exists in schema, so onConflictDoNothing() has a real constraint to target, and the insert-first/select-on-conflict ordering genuinely closes the TOCTOU window rather than narrowing it.
One gap before merge: all three getOrCreateChatChannel tests either error out early or exercise the conflict-fallback branch - none covers the primary case, a fresh insert that succeeds with no conflict. That's the path every non-duplicate call takes and it's untested through the new .onConflictDoNothing().returning() chain. Can you add a test asserting newChannel truthy -> immediate ok({ channelId }) return?
jakharmonika364
added
Needs author reply
Adds the missing primary-case test (insert succeeds, immediate ok({ channelId })
return, no conflict fallback select). Also mocks the insert chain in the
conflict-fallback test, which previously hit a TypeError on undefined values.
|
@jakharmonika364 Please review now. I've made the changes |
jakharmonika364
added
level:advanced
2 participants
Description
The
getOrCreateChatChannelfunction insrc/app/actions/chat.tsuses a classic check-then-insert pattern: first SELECT to see if a channel exists for(mentorId, menteeId), then INSERT if none found. Two concurrent requests can both read "no existing channel" and both insert, creating duplicate channels.The schema already defines a
uniqueIndexon(mentor_id, mentee_id), so the database would reject the second insert, but the application doesn't handle the conflict gracefully.Changes
.onConflictDoNothing(), then SELECT only on conflict (when the insert returns nothing)Closes #844