fix(#623): enforce identity verification on QR check-in to prevent sp…#646
Conversation
…eck-in to prevent spoofing
📝 WalkthroughWalkthroughAddresses issue ChangesFix
Sequence Diagram(s)sequenceDiagram
participant Scanner as QRScannerScreen
participant CF as verifyAndCheckIn
participant FS as Firestore
Scanner->>Scanner: handleBarCodeScanned(qrData)
alt user.uid !== scannedUserId
Scanner-->>Scanner: set error scanResult, return early
else UIDs match
Scanner->>CF: call({ qrUid, eventId })
CF->>CF: enforce auth.uid === qrUid
CF->>FS: check event, RSVP, existing attendance
alt already checked in
CF-->>Scanner: { success: true, alreadyCheckedIn: true }
else first check-in
CF->>FS: atomic transaction (attendance + reputation update)
CF-->>Scanner: { success: true, alreadyCheckedIn: false }
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/src/screens/QRScannerScreen.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. cloud-functions/src/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. cloud-functions/src/verifyAndCheckIn.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
|
Hi @roshankumar0036singh, I've implemented the fix for this issue and raised a PR. Please review when you get a chance! |
|
![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)
app/src/screens/QRScannerScreen.js (1)
259-275:⚠️ Potential issue | 🔴 CriticalFix line references: checkIns rules at 282–287 restrict direct writes; use
verifyAndCheckIncloud function instead.The identity check passes, but
submitCheckIncallscheckInAttendee/checkInParticipantwhich write directly to theevents/{eventId}/checkIns/{userId}subcollection. The Firestore rules at lines 282–287 restrict checkIn writes to admins, event owners, and clubs with event ownership only.For a regular attendee self-checking-in, this write will fail with
permission-denied. The client should call theverifyAndCheckIncloud function (which runs with admin privileges) instead of the direct-write service.🤖 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 `@app/src/screens/QRScannerScreen.js` around lines 259 - 275, The submitCheckIn function calls checkInAttendee or checkInParticipant which write directly to the events/{eventId}/checkIns/{userId} subcollection, but Firestore security rules restrict these writes to admins and event owners only. For a regular attendee self-checking-in, this direct write will fail with permission-denied errors. Replace the submitCheckIn call with a call to the verifyAndCheckIn cloud function instead, which runs with admin privileges and has the necessary permissions to perform the check-in operation. Update the call at the point where submitCheckIn is invoked to use verifyAndCheckIn with the appropriate parameters instead.
🧹 Nitpick comments (1)
firestore.rules (1)
238-256: 💤 Low valueRedundant
isClub()condition in write rules.Line 252 checks
isClub() && (isEventOwner(...) || isEventOwnerAfter(...)), but lines 253-254 also checkisEventOwner(...)andisEventOwnerAfter(...)independently. TheisClub()gate adds no additional restriction since event owners are permitted regardless of club status.The condition can be simplified to:
isAdmin() || isEventOwner(database, eventId) || isEventOwnerAfter(database, eventId).This is a minor clarity issue; the current rules are functionally correct.
🤖 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 `@firestore.rules` around lines 238 - 256, The allow create, update, delete rule in the attendance match block contains a redundant condition. The line checking `isClub() && (isEventOwner(database, eventId) || isEventOwnerAfter(database, eventId))` is unnecessary because the subsequent lines already independently check isEventOwner and isEventOwnerAfter, making the isClub() gate redundant. Simplify the condition by removing this redundant clause and consolidating the rules to: isAdmin() || isEventOwner(database, eventId) || isEventOwnerAfter(database, eventId).
🤖 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 `@cloud-functions/src/verifyAndCheckIn.ts`:
- Around line 94-113: The tx.update call on the userRef will fail if the user
document does not exist yet, which is possible for new users. Although the code
correctly handles reading non-existent user data by defaulting reputationPoints
to 0, the subsequent tx.update will throw an error if that document is missing.
Replace the tx.update(userRef, {...}) call with tx.set(userRef, {...}, { merge:
true }) to ensure the operation succeeds whether the user document exists or
not. This way, the reputationPoints and attendanceCount fields will be created
if the document is new, or updated if it already exists.
- Around line 77-88: The duplicate check on attendanceRef outside the
transaction creates a race condition where two concurrent requests can both pass
the existence check before either writes. Move the attendanceRef.get() call and
the existence check (attendanceSnap.exists) inside the runTransaction callback
so that the read and write are atomic together. This ensures that when multiple
concurrent check-in requests arrive, only one can successfully write the
attendance record, while the others will see the existing document and fail
gracefully.
---
Outside diff comments:
In `@app/src/screens/QRScannerScreen.js`:
- Around line 259-275: The submitCheckIn function calls checkInAttendee or
checkInParticipant which write directly to the
events/{eventId}/checkIns/{userId} subcollection, but Firestore security rules
restrict these writes to admins and event owners only. For a regular attendee
self-checking-in, this direct write will fail with permission-denied errors.
Replace the submitCheckIn call with a call to the verifyAndCheckIn cloud
function instead, which runs with admin privileges and has the necessary
permissions to perform the check-in operation. Update the call at the point
where submitCheckIn is invoked to use verifyAndCheckIn with the appropriate
parameters instead.
---
Nitpick comments:
In `@firestore.rules`:
- Around line 238-256: The allow create, update, delete rule in the attendance
match block contains a redundant condition. The line checking `isClub() &&
(isEventOwner(database, eventId) || isEventOwnerAfter(database, eventId))` is
unnecessary because the subsequent lines already independently check
isEventOwner and isEventOwnerAfter, making the isClub() gate redundant. Simplify
the condition by removing this redundant clause and consolidating the rules to:
isAdmin() || isEventOwner(database, eventId) || isEventOwnerAfter(database,
eventId).
🪄 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: ea1ce1e7-b0a4-4f7a-8ea7-b4abb19dea66
📒 Files selected for processing (4)
app/src/screens/QRScannerScreen.jscloud-functions/src/index.tscloud-functions/src/verifyAndCheckIn.tsfirestore.rules
| // ── 6. Prevent duplicate check-ins ──────────────────────────────────── | ||
| const attendanceRef = db | ||
| .collection('events') | ||
| .doc(eventId) | ||
| .collection('attendance') | ||
| .doc(callerUid); | ||
| const attendanceSnap = await attendanceRef.get(); | ||
|
|
||
| if (attendanceSnap.exists) { | ||
| // Already checked in – return success idempotently | ||
| return { success: true, alreadyCheckedIn: true }; | ||
| } |
There was a problem hiding this comment.
TOCTOU race: duplicate check is outside the transaction.
attendanceSnap is read at line 83, but the attendance write occurs inside runTransaction at line 101. Two concurrent check-in requests could both pass the existence check, then both enter the transaction. Since attendanceRef is not read inside the transaction, Firestore cannot detect the conflict—both writes may succeed, or one fails non-deterministically, and the reputation/attendance counters may double-increment.
Move the duplicate check inside the transaction to make it atomic.
🔧 Proposed fix
- const attendanceSnap = await attendanceRef.get();
-
- if (attendanceSnap.exists) {
- // Already checked in – return success idempotently
- return { success: true, alreadyCheckedIn: true };
- }
-
- // ── 7. Write attendance record & update reputation atomically ─────────
const userRef = db.collection('users').doc(callerUid);
const REPUTATION_POINTS = 10; // adjust to match your scoring schema
- await db.runTransaction(async tx => {
+ const result = await db.runTransaction(async tx => {
+ const attendanceSnap = await tx.get(attendanceRef);
+ if (attendanceSnap.exists) {
+ return { alreadyCheckedIn: true };
+ }
+
const userSnap = await tx.get(userRef);
...
+ return { alreadyCheckedIn: false };
});
- return { success: true, alreadyCheckedIn: false };
+ return { success: true, alreadyCheckedIn: result.alreadyCheckedIn };📝 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.
| // ── 6. Prevent duplicate check-ins ──────────────────────────────────── | |
| const attendanceRef = db | |
| .collection('events') | |
| .doc(eventId) | |
| .collection('attendance') | |
| .doc(callerUid); | |
| const attendanceSnap = await attendanceRef.get(); | |
| if (attendanceSnap.exists) { | |
| // Already checked in – return success idempotently | |
| return { success: true, alreadyCheckedIn: true }; | |
| } | |
| // ── 6. Prevent duplicate check-ins ──────────────────────────────────── | |
| const attendanceRef = db | |
| .collection('events') | |
| .doc(eventId) | |
| .collection('attendance') | |
| .doc(callerUid); | |
| // ── 7. Write attendance record & update reputation atomically ───────── | |
| const userRef = db.collection('users').doc(callerUid); | |
| const REPUTATION_POINTS = 10; // adjust to match your scoring schema | |
| const result = await db.runTransaction(async tx => { | |
| const attendanceSnap = await tx.get(attendanceRef); | |
| if (attendanceSnap.exists) { | |
| return { alreadyCheckedIn: true }; | |
| } | |
| const userSnap = await tx.get(userRef); | |
| // ... rest of transaction implementation | |
| return { alreadyCheckedIn: false }; | |
| }); | |
| return { success: true, alreadyCheckedIn: result.alreadyCheckedIn }; |
🤖 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 `@cloud-functions/src/verifyAndCheckIn.ts` around lines 77 - 88, The duplicate
check on attendanceRef outside the transaction creates a race condition where
two concurrent requests can both pass the existence check before either writes.
Move the attendanceRef.get() call and the existence check
(attendanceSnap.exists) inside the runTransaction callback so that the read and
write are atomic together. This ensures that when multiple concurrent check-in
requests arrive, only one can successfully write the attendance record, while
the others will see the existing document and fail gracefully.
| await db.runTransaction(async tx => { | ||
| const userSnap = await tx.get(userRef); | ||
| const currentPoints: number = userSnap.exists | ||
| ? (userSnap.data()?.reputationPoints ?? 0) | ||
| : 0; | ||
|
|
||
| // Mark attendance | ||
| tx.set(attendanceRef, { | ||
| uid: callerUid, | ||
| eventId, | ||
| checkedInAt: admin.firestore.FieldValue.serverTimestamp(), | ||
| verifiedByServer: true, // audit flag – distinguishes server-verified records | ||
| }); | ||
|
|
||
| // Increment reputation | ||
| tx.update(userRef, { | ||
| reputationPoints: currentPoints + REPUTATION_POINTS, | ||
| attendanceCount: admin.firestore.FieldValue.increment(1), | ||
| }); | ||
| }); |
There was a problem hiding this comment.
tx.update will fail if the user document does not exist.
Line 96 handles the case where userSnap does not exist for reading reputationPoints, but line 109 uses tx.update(userRef, ...). Firestore update throws if the document is missing. For new users who haven't written a profile yet, this will crash the transaction.
Use tx.set with { merge: true } to handle both existing and non-existing user documents.
🔧 Proposed fix
- // Increment reputation
- tx.update(userRef, {
- reputationPoints: currentPoints + REPUTATION_POINTS,
- attendanceCount: admin.firestore.FieldValue.increment(1),
- });
+ // Increment reputation (merge handles missing user docs)
+ tx.set(userRef, {
+ reputationPoints: currentPoints + REPUTATION_POINTS,
+ attendanceCount: admin.firestore.FieldValue.increment(1),
+ }, { merge: 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 `@cloud-functions/src/verifyAndCheckIn.ts` around lines 94 - 113, The tx.update
call on the userRef will fail if the user document does not exist yet, which is
possible for new users. Although the code correctly handles reading non-existent
user data by defaulting reputationPoints to 0, the subsequent tx.update will
throw an error if that document is missing. Replace the tx.update(userRef,
{...}) call with tx.set(userRef, {...}, { merge: true }) to ensure the operation
succeeds whether the user document exists or not. This way, the reputationPoints
and attendanceCount fields will be created if the document is new, or updated if
it already exists.
|
resolev the usggestiosn and conflict |
3 participants
Fixes #623
Problem
QRScannerScreen.jssubmitted check-ins using theuserIdfrom the scanned QR payload without verifying it matched the authenticated user'suid. Any authenticated user could scan someone else's QR code and create ghost attendance records, inflating leaderboard scores.Solution
handleBarCodeScanned: ifscannedUserId !== user.uid, check-in is rejected with a clear error message before any Firestore write occursverifyAndCheckIncloud function that enforces the same check server-side usingcontext.auth.uid(which cannot be spoofed by the client)attendancesubcollection infirestore.rules— all attendee check-ins must go through the cloud functionFiles Changed
app/src/screens/QRScannerScreen.js— added identity verification before check-in submissioncloud-functions/src/verifyAndCheckIn.ts— new cloud function for server-side uid enforcementcloud-functions/src/index.ts— exported the new cloud functionfirestore.rules— restricted attendance writes with fix commentContributing under GSSoC '26
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features