Open
Conversation
Add PATCH /api/groups/:groupId/members/:userId/role endpoint that allows admins to grant admin privileges and owners to revoke admin or transfer ownership. Uses existing GroupService.updateGroup for persistence.
Add dropdown menu per member in the signing status sidebar allowing admins to grant admin privileges and owners to revoke admin or transfer ownership. Controls are only visible to users with appropriate permissions.
Contributor
📝 WalkthroughWalkthroughAdds backend endpoint and controller to update group member roles (admin/member/owner) with permission and ownership-transfer logic; frontend exposes per-member role-management UI in CharterSigningStatus and passes user context to call the new API and refresh status. Changes
Sequence DiagramsequenceDiagram
participant User as Admin/Owner User
participant UI as CharterSigningStatus UI
participant API as Express Route
participant Controller as GroupController
participant DB as Group Database
User->>UI: select role action for member
UI->>API: PATCH /api/groups/:groupId/members/:userId/role
API->>Controller: updateMemberRole(req)
Controller->>Controller: validate auth, membership, permission, role
Controller->>DB: update admins / owner fields
DB-->>Controller: updated group
Controller-->>API: 200 + updated group
API-->>UI: response
UI->>UI: show toast & refresh status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platforms/group-charter-manager/api/src/index.ts (1)
160-160: Consider consistent naming:membersvsparticipants.The existing routes at lines 158-159 use
/participants/in the path, while this new route uses/members/. For API consistency, consider using/participants/:userId/roleto match the established pattern.♻️ Suggested path for consistency
-app.patch("/api/groups/:groupId/members/:userId/role", authGuard, groupController.updateMemberRole.bind(groupController)); +app.patch("/api/groups/:groupId/participants/:userId/role", authGuard, groupController.updateMemberRole.bind(groupController));Note: If you apply this change, also update the client-side API call in
charter-signing-status.tsxline 72.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/group-charter-manager/api/src/index.ts` at line 160, Change the route path to use "participants" for consistency: replace app.patch("/api/groups/:groupId/members/:userId/role", authGuard, groupController.updateMemberRole.bind(groupController)) with a path using "/api/groups/:groupId/participants/:userId/role" and keep the same authGuard and handler (groupController.updateMemberRole.bind(groupController)); also update the corresponding client call in charter-signing-status.tsx (around line 72) to use the new "/participants/:userId/role" path so client and server remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@platforms/group-charter-manager/api/src/index.ts`:
- Line 160: Change the route path to use "participants" for consistency: replace
app.patch("/api/groups/:groupId/members/:userId/role", authGuard,
groupController.updateMemberRole.bind(groupController)) with a path using
"/api/groups/:groupId/participants/:userId/role" and keep the same authGuard and
handler (groupController.updateMemberRole.bind(groupController)); also update
the corresponding client call in charter-signing-status.tsx (around line 72) to
use the new "/participants/:userId/role" path so client and server remain
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e89cf55-bfe4-4fbb-85d8-490863035803
📒 Files selected for processing (4)
platforms/group-charter-manager/api/src/controllers/GroupController.tsplatforms/group-charter-manager/api/src/index.tsplatforms/group-charter-manager/client/src/app/charter/[id]/page.tsxplatforms/group-charter-manager/client/src/components/charter-signing-status.tsx
- Remove admin button now visible to all admins, not just owners - Backend allows admins to revoke admin privileges from other admins - Members list renders on group page even when no charter exists - Hide signing status indicators when no charter is present
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
platforms/group-charter-manager/client/src/app/charter/[id]/page.tsx (1)
422-428: Props integration looks correct; minor readability suggestion.The user context props are correctly wired to enable role management in the sidebar.
One minor improvement for line 426: the fallback
user?.id || ''is a bit awkward since it checks if an empty string is in the admins array when user is undefined. A slightly cleaner pattern:- currentUserIsAdmin={group.admins?.includes(user?.id || '') || false} + currentUserIsAdmin={Boolean(user?.id && group.admins?.includes(user.id))}This is functionally equivalent but more explicit about the intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/group-charter-manager/client/src/app/charter/`[id]/page.tsx around lines 422 - 428, The currentUserIsAdmin prop uses the awkward fallback user?.id || '' which causes an empty-string lookup in group.admins when user is undefined; update the CharterSigningStatus prop so it only calls group.admins.includes when a user exists — e.g. compute currentUserIsAdmin with a safe check like using Boolean(user && group.admins?.includes(user.id)) or a ternary (user ? group.admins?.includes(user.id) ?? false : false) so the includes call never receives an empty string and the prop reliably resolves to true/false.platforms/group-charter-manager/api/src/controllers/GroupController.ts (1)
380-380: Consider removingas anytype assertions.The
as anycasts on lines 380, 387, and 398 bypass type checking. If theGroupService.updateGroupmethod's signature expectsPartial<Group>, the types foradminsandownershould already match. If there's a type mismatch, consider fixing the underlying type definitions rather than usingas any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/group-charter-manager/api/src/controllers/GroupController.ts` at line 380, The call in GroupController to this.groupService.updateGroup is using unsafe `as any` casts for the payload (e.g. `{ admins: newAdmins } as any` and similar for `owner`); remove these casts and make the payload match the expected type (likely `Partial<Group>`). Ensure `newAdmins` and any `owner` values have the correct types (or map/validate them into the proper shape) or adjust the GroupService.updateGroup signature to accept the actual payload type; alternatively use a typed helper/DTO to construct `{ admins: newAdmins }` as `Partial<Group>` or perform an explicit type-safe transform before calling updateGroup. Update references to `GroupController`, `groupService.updateGroup`, `newAdmins`, and `owner` accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/group-charter-manager/api/src/controllers/GroupController.ts`:
- Around line 381-387: The code currently allows any admin to demote other
admins (removing targetUserId from currentAdmins via
this.groupService.updateGroup), but the test plan requires only the owner can
remove admins; enforce that by adding an owner-only authorization check before
modifying admins: verify the requester's id (e.g., req.user.id or the
controller's existing requestingUserId variable used elsewhere) equals
group.owner and return 403 if not; then proceed to compute newAdmins
(currentAdmins.filter...) and call this.groupService.updateGroup(groupId, {
admins: newAdmins }). Ensure the check is placed in the branch handling role ===
"member" before mutating admins.
In
`@platforms/group-charter-manager/client/src/components/charter-signing-status.tsx`:
- Around line 166-194: Render the DropdownMenu only when there are actionable
items: add a guard (or helper like hasAvailableActions) that checks
canManageRoles, participant.id !== currentUserId, and at least one of the three
action conditions (participant not admin and not owner -> Make Admin;
participant is admin and not owner -> Remove Admin; participant not owner and
currentUserIsOwner -> Transfer Ownership), then wrap the existing DropdownMenu
(and its trigger) with that guard so the three-dot button never opens an empty
menu; reference the participant, currentUserId, canManageRoles,
currentUserIsOwner, handleRoleChange and DropdownMenu elements when applying the
change.
---
Nitpick comments:
In `@platforms/group-charter-manager/api/src/controllers/GroupController.ts`:
- Line 380: The call in GroupController to this.groupService.updateGroup is
using unsafe `as any` casts for the payload (e.g. `{ admins: newAdmins } as any`
and similar for `owner`); remove these casts and make the payload match the
expected type (likely `Partial<Group>`). Ensure `newAdmins` and any `owner`
values have the correct types (or map/validate them into the proper shape) or
adjust the GroupService.updateGroup signature to accept the actual payload type;
alternatively use a typed helper/DTO to construct `{ admins: newAdmins }` as
`Partial<Group>` or perform an explicit type-safe transform before calling
updateGroup. Update references to `GroupController`, `groupService.updateGroup`,
`newAdmins`, and `owner` accordingly.
In `@platforms/group-charter-manager/client/src/app/charter/`[id]/page.tsx:
- Around line 422-428: The currentUserIsAdmin prop uses the awkward fallback
user?.id || '' which causes an empty-string lookup in group.admins when user is
undefined; update the CharterSigningStatus prop so it only calls
group.admins.includes when a user exists — e.g. compute currentUserIsAdmin with
a safe check like using Boolean(user && group.admins?.includes(user.id)) or a
ternary (user ? group.admins?.includes(user.id) ?? false : false) so the
includes call never receives an empty string and the prop reliably resolves to
true/false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0f660d1-577e-41b0-b579-ed5b7f67f4d0
📒 Files selected for processing (3)
platforms/group-charter-manager/api/src/controllers/GroupController.tsplatforms/group-charter-manager/client/src/app/charter/[id]/page.tsxplatforms/group-charter-manager/client/src/components/charter-signing-status.tsx
Comment on lines
+381
to
+387
| } else if (role === "member") { | ||
| // Remove admin - admins and owners can do this | ||
| if (targetUserId === group.owner) { | ||
| return res.status(400).json({ error: "Cannot demote the owner" }); | ||
| } | ||
| const newAdmins = currentAdmins.filter(id => id !== targetUserId); | ||
| await this.groupService.updateGroup(groupId, { admins: newAdmins } as any); |
Contributor
There was a problem hiding this comment.
Authorization behavior contradicts stated test plan.
The code allows any admin to remove admin privileges from other users. However, the PR objectives state: "Admin: grant admin; verify cannot remove admin from others or transfer ownership (checked)."
The commit message indicates this is intentional ("allow admins to revoke admin privileges"), but this contradicts the test plan verification. Please clarify the intended authorization model:
- If admins should not be able to demote other admins, add an owner-only check here.
- If admins can demote other admins, update the PR test plan to reflect this.
🔒 Option 1: Restrict to owner-only
} else if (role === "member") {
// Remove admin - admins and owners can do this
+ if (!isOwner) {
+ return res.status(403).json({ error: "Only the owner can remove admin privileges" });
+ }
if (targetUserId === group.owner) {
return res.status(400).json({ error: "Cannot demote the owner" });
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/group-charter-manager/api/src/controllers/GroupController.ts`
around lines 381 - 387, The code currently allows any admin to demote other
admins (removing targetUserId from currentAdmins via
this.groupService.updateGroup), but the test plan requires only the owner can
remove admins; enforce that by adding an owner-only authorization check before
modifying admins: verify the requester's id (e.g., req.user.id or the
controller's existing requestingUserId variable used elsewhere) equals
group.owner and return 403 if not; then proceed to compute newAdmins
(currentAdmins.filter...) and call this.groupService.updateGroup(groupId, {
admins: newAdmins }). Ensure the check is placed in the branch handling role ===
"member" before mutating admins.
Comment on lines
+166
to
+194
| {canManageRoles && participant.id !== currentUserId && ( | ||
| <DropdownMenu> | ||
| <DropdownMenuTrigger asChild> | ||
| <Button variant="ghost" size="icon" className="h-7 w-7"> | ||
| <MoreVertical className="h-4 w-4" /> | ||
| </Button> | ||
| </DropdownMenuTrigger> | ||
| <DropdownMenuContent align="end"> | ||
| {!participant.isAdmin && !participant.isOwner && ( | ||
| <DropdownMenuItem onClick={() => handleRoleChange(participant.id, "admin")}> | ||
| <Shield className="mr-2 h-4 w-4" /> | ||
| Make Admin | ||
| </DropdownMenuItem> | ||
| )} | ||
| {participant.isAdmin && !participant.isOwner && ( | ||
| <DropdownMenuItem onClick={() => handleRoleChange(participant.id, "member")}> | ||
| <ShieldOff className="mr-2 h-4 w-4" /> | ||
| Remove Admin | ||
| </DropdownMenuItem> | ||
| )} | ||
| {!participant.isOwner && currentUserIsOwner && ( | ||
| <DropdownMenuItem className="text-amber-600" onClick={() => handleRoleChange(participant.id, "owner")}> | ||
| <Crown className="mr-2 h-4 w-4" /> | ||
| Transfer Ownership | ||
| </DropdownMenuItem> | ||
| )} | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| )} |
Contributor
There was a problem hiding this comment.
Empty dropdown menu can render when admin views the owner.
When an admin (non-owner) views this component, the dropdown will render for the owner participant, but all menu items will be hidden:
- "Make Admin" hidden (owner is already an admin)
- "Remove Admin" hidden (cannot remove admin from owner)
- "Transfer Ownership" hidden (only owner can transfer)
This results in clicking the three-dot button and seeing an empty menu.
🐛 Proposed fix: only render dropdown if actions are available
- {canManageRoles && participant.id !== currentUserId && (
+ {canManageRoles && participant.id !== currentUserId && (
+ // Only show dropdown if there are available actions
+ (!participant.isAdmin && !participant.isOwner) || // can make admin
+ (participant.isAdmin && !participant.isOwner) || // can remove admin
+ (!participant.isOwner && currentUserIsOwner) // can transfer ownership
+ ) && (
<DropdownMenu>Or extract to a helper:
const hasAvailableActions = (participant: Participant) => {
if (participant.id === currentUserId) return false;
if (!canManageRoles) return false;
// Can make admin
if (!participant.isAdmin && !participant.isOwner) return true;
// Can remove admin (admin but not owner)
if (participant.isAdmin && !participant.isOwner) return true;
// Can transfer ownership (current user is owner, target is not owner)
if (!participant.isOwner && currentUserIsOwner) return true;
return false;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platforms/group-charter-manager/client/src/components/charter-signing-status.tsx`
around lines 166 - 194, Render the DropdownMenu only when there are actionable
items: add a guard (or helper like hasAvailableActions) that checks
canManageRoles, participant.id !== currentUserId, and at least one of the three
action conditions (participant not admin and not owner -> Make Admin;
participant is admin and not owner -> Remove Admin; participant not owner and
currentUserIsOwner -> Transfer Ownership), then wrap the existing DropdownMenu
(and its trigger) with that guard so the three-dot button never opens an empty
menu; reference the participant, currentUserId, canManageRoles,
currentUserIsOwner, handleRoleChange and DropdownMenu elements when applying the
change.
Instead of blindly rejecting chat webhooks when the ID is locked, compare updatedAt timestamps and accept the update if the incoming data is more recent than what we have locally.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
platforms/blabsy/api/src/controllers/WebhookController.ts (1)
100-100: Include meaningful error message when mapping is not found.Throwing an empty error makes debugging difficult. Include the
schemaIdin the error message to help identify which webhook failed.Suggested fix
- if (!mapping) throw new Error(); + if (!mapping) throw new Error(`No mapping found for schemaId: ${schemaId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platforms/blabsy/api/src/controllers/WebhookController.ts` at line 100, The current check "if (!mapping) throw new Error();" throws an empty error which is unhelpful; update the WebhookController code that resolves mapping (the variable "mapping" and the incoming "schemaId") to throw a descriptive Error that includes the schemaId and context (e.g., "Mapping not found for schemaId: <schemaId>" or similar) so failures clearly indicate which webhook/schema failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platforms/blabsy/api/src/controllers/WebhookController.ts`:
- Around line 103-116: The chat path currently skips lock checks causing a race
before updateChatIfNewer; change the flow in WebhookController.ts so chats
acquire the same global lock (use adapter.lockedIds and call
adapter.addToLockedIds(id)) before performing the timestamp comparison, or
replace the timestamp-check + write with an atomic Firestore transaction inside
updateChatIfNewer so the read-compare-write is done atomically; ensure you
release the lock after the update or let the transaction handle concurrency if
you choose the transaction approach.
- Around line 263-287: The updateChatIfNewer function has a TOCTOU race: it
reads the document, compares timestamps, then updates outside an atomic context;
change it to use a Firestore transaction (use this.db.runTransaction) that reads
the doc inside the transaction, computes existingUpdatedAt and incomingUpdatedAt
using mapChatData(data, Timestamp.now()) (or compute mappedData and use
mappedData.updatedAt inside the transaction), and only performs
docRef.update(mappedData) within the transaction when incomingUpdatedAt >=
existingUpdatedAt; remove or move adapter.addToLockedIds so it is not relied on
for atomicity (either call it after a successful transaction commit or handle
locking inside the transaction logic) and ensure errors are handled/logged when
the transaction aborts.
- Around line 273-282: The timestamp check is using Timestamp.now() via
mapChatData, so incomingUpdatedAt is wrong; change the comparison to use the
webhook payload's original timestamp (data.updatedAt) instead of
mappedData.updatedAt/Timestamp.now(). Locate the comparison around
existingUpdatedAt/incomingUpdatedAt and compute incomingUpdatedAt from the
incoming payload (e.g., parse data.updatedAt or pass it through mapChatData
before you overwrite it), or modify mapChatData to accept and preserve the
incoming timestamp so that incomingUpdatedAt reflects the webhook's timestamp
rather than current time.
---
Nitpick comments:
In `@platforms/blabsy/api/src/controllers/WebhookController.ts`:
- Line 100: The current check "if (!mapping) throw new Error();" throws an empty
error which is unhelpful; update the WebhookController code that resolves
mapping (the variable "mapping" and the incoming "schemaId") to throw a
descriptive Error that includes the schemaId and context (e.g., "Mapping not
found for schemaId: <schemaId>" or similar) so failures clearly indicate which
webhook/schema failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62049c50-8be8-4722-a0fa-4f451b094f49
📒 Files selected for processing (1)
platforms/blabsy/api/src/controllers/WebhookController.ts
Comment on lines
+103
to
+116
| // For chats, skip the lock check and use timestamp comparison instead | ||
| const isChatData = tableName === "chats"; | ||
|
|
||
| if (!isChatData && adapter.lockedIds.includes(id)) { | ||
| console.log(`Webhook skipped - ID ${id} already locked`); | ||
| return res.status(200).json({ success: true, skipped: true }); | ||
| } | ||
|
|
||
| console.log(`Processing webhook for ID: ${id}`); | ||
|
|
||
| // Lock the global ID immediately to prevent duplicates | ||
| adapter.addToLockedIds(id); | ||
|
|
||
| const mapping = Object.values(adapter.mapping).find( | ||
| (m) => m.schemaId === schemaId, | ||
| ); | ||
| if (!mapping) throw new Error(); | ||
| const tableName = mapping.tableName + "s"; | ||
| // Lock the global ID immediately to prevent duplicates (non-chat) | ||
| if (!isChatData) { | ||
| adapter.addToLockedIds(id); | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition window for concurrent chat webhooks.
By skipping the lock check for chats entirely (line 106), multiple concurrent webhooks for the same chat ID can all proceed to updateChatIfNewer simultaneously. The lock is only acquired at line 284 (after the timestamp check), leaving a race window where:
- Webhook A reads existing doc, passes timestamp check
- Webhook B reads same doc (before A writes), also passes timestamp check
- Both A and B update the document
Since the timestamp comparison uses Timestamp.now() (see related comment on updateChatIfNewer), this race is more likely to cause issues. Consider acquiring the lock earlier in the chat path, or using a Firestore transaction for atomic read-compare-write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 103 -
116, The chat path currently skips lock checks causing a race before
updateChatIfNewer; change the flow in WebhookController.ts so chats acquire the
same global lock (use adapter.lockedIds and call adapter.addToLockedIds(id))
before performing the timestamp comparison, or replace the timestamp-check +
write with an atomic Firestore transaction inside updateChatIfNewer so the
read-compare-write is done atomically; ensure you release the lock after the
update or let the transaction handle concurrency if you choose the transaction
approach.
Comment on lines
+263
to
+287
| private async updateChatIfNewer(localId: string, data: any) { | ||
| const docRef = this.db.collection("chats").doc(localId); | ||
| const docSnapshot = await docRef.get(); | ||
|
|
||
| if (!docSnapshot.exists) { | ||
| console.warn(`Chat document '${localId}' does not exist. Skipping update.`); | ||
| return; | ||
| } | ||
|
|
||
| const existing = docSnapshot.data(); | ||
| const mappedData = this.mapChatData(data, Timestamp.now()); | ||
|
|
||
| // Compare updatedAt timestamps - accept if incoming is more recent | ||
| const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0; | ||
| const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0; | ||
|
|
||
| if (incomingUpdatedAt < existingUpdatedAt) { | ||
| console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`); | ||
| return; | ||
| } | ||
|
|
||
| adapter.addToLockedIds(localId); | ||
| await docRef.update(mappedData); | ||
| console.log(`Chat ${localId} updated via webhook (timestamp check passed)`); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using a Firestore transaction for atomic read-compare-write.
The current implementation has a TOCTOU (time-of-check to time-of-use) race condition: the document is read, the timestamp is compared, and then updated in separate operations. Two concurrent webhooks can both pass the timestamp check before either writes.
A Firestore transaction would make this atomic:
Proposed fix using Firestore transaction
private async updateChatIfNewer(localId: string, data: any) {
const docRef = this.db.collection("chats").doc(localId);
- const docSnapshot = await docRef.get();
-
- if (!docSnapshot.exists) {
- console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
- return;
- }
-
- const existing = docSnapshot.data();
- const mappedData = this.mapChatData(data, Timestamp.now());
-
- // Compare updatedAt timestamps - accept if incoming is more recent
- const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
- const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;
-
- if (incomingUpdatedAt < existingUpdatedAt) {
- console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
- return;
- }
-
- adapter.addToLockedIds(localId);
- await docRef.update(mappedData);
- console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
+ const incomingUpdatedAt = data.updatedAt
+ ? new Date(data.updatedAt).getTime()
+ : Date.now();
+
+ await this.db.runTransaction(async (transaction) => {
+ const docSnapshot = await transaction.get(docRef);
+
+ if (!docSnapshot.exists) {
+ console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
+ return;
+ }
+
+ const existing = docSnapshot.data();
+ const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
+
+ if (incomingUpdatedAt < existingUpdatedAt) {
+ console.log(`Chat ${localId} webhook skipped - local data is more recent`);
+ return;
+ }
+
+ const mappedData = this.mapChatData(data, Timestamp.now());
+ transaction.update(docRef, mappedData);
+ console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
+ });
+
+ adapter.addToLockedIds(localId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 263 -
287, The updateChatIfNewer function has a TOCTOU race: it reads the document,
compares timestamps, then updates outside an atomic context; change it to use a
Firestore transaction (use this.db.runTransaction) that reads the doc inside the
transaction, computes existingUpdatedAt and incomingUpdatedAt using
mapChatData(data, Timestamp.now()) (or compute mappedData and use
mappedData.updatedAt inside the transaction), and only performs
docRef.update(mappedData) within the transaction when incomingUpdatedAt >=
existingUpdatedAt; remove or move adapter.addToLockedIds so it is not relied on
for atomicity (either call it after a successful transaction commit or handle
locking inside the transaction logic) and ensure errors are handled/logged when
the transaction aborts.
Comment on lines
+273
to
+282
| const mappedData = this.mapChatData(data, Timestamp.now()); | ||
|
|
||
| // Compare updatedAt timestamps - accept if incoming is more recent | ||
| const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0; | ||
| const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0; | ||
|
|
||
| if (incomingUpdatedAt < existingUpdatedAt) { | ||
| console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Timestamp comparison uses current time instead of incoming webhook's timestamp.
The comparison is ineffective because mappedData.updatedAt is set to Timestamp.now() (via mapChatData at line 410), not the actual timestamp from the incoming webhook data. This means:
incomingUpdatedAt= current wall-clock time (always)existingUpdatedAt= when the document was last written
Since Timestamp.now() will almost always be greater than the existing updatedAt, out-of-order webhooks will still overwrite newer data with older data.
If the intent is to skip stale webhooks, you need to compare the original data.updatedAt from the webhook payload, not the mapped result.
Proposed fix: Compare using incoming webhook's timestamp
private async updateChatIfNewer(localId: string, data: any) {
const docRef = this.db.collection("chats").doc(localId);
const docSnapshot = await docRef.get();
if (!docSnapshot.exists) {
console.warn(`Chat document '${localId}' does not exist. Skipping update.`);
return;
}
const existing = docSnapshot.data();
- const mappedData = this.mapChatData(data, Timestamp.now());
-
- // Compare updatedAt timestamps - accept if incoming is more recent
const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0;
- const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0;
+
+ // Use the actual incoming webhook's updatedAt for comparison
+ const incomingUpdatedAt = data.updatedAt
+ ? new Date(data.updatedAt).getTime()
+ : Date.now();
if (incomingUpdatedAt < existingUpdatedAt) {
console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`);
return;
}
+ const mappedData = this.mapChatData(data, Timestamp.now());
adapter.addToLockedIds(localId);
await docRef.update(mappedData);
console.log(`Chat ${localId} updated via webhook (timestamp check passed)`);
}📝 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.
Suggested change
| const mappedData = this.mapChatData(data, Timestamp.now()); | |
| // Compare updatedAt timestamps - accept if incoming is more recent | |
| const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0; | |
| const incomingUpdatedAt = mappedData.updatedAt?.toMillis?.() ?? 0; | |
| if (incomingUpdatedAt < existingUpdatedAt) { | |
| console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`); | |
| return; | |
| } | |
| const existingUpdatedAt = existing?.updatedAt?.toMillis?.() ?? 0; | |
| // Use the actual incoming webhook's updatedAt for comparison | |
| const incomingUpdatedAt = data.updatedAt | |
| ? new Date(data.updatedAt).getTime() | |
| : Date.now(); | |
| if (incomingUpdatedAt < existingUpdatedAt) { | |
| console.log(`Chat ${localId} webhook skipped - local data is more recent (local: ${existingUpdatedAt}, incoming: ${incomingUpdatedAt})`); | |
| return; | |
| } | |
| const mappedData = this.mapChatData(data, Timestamp.now()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platforms/blabsy/api/src/controllers/WebhookController.ts` around lines 273 -
282, The timestamp check is using Timestamp.now() via mapChatData, so
incomingUpdatedAt is wrong; change the comparison to use the webhook payload's
original timestamp (data.updatedAt) instead of
mappedData.updatedAt/Timestamp.now(). Locate the comparison around
existingUpdatedAt/incomingUpdatedAt and compute incomingUpdatedAt from the
incoming payload (e.g., parse data.updatedAt or pass it through mapChatData
before you overwrite it), or modify mapChatData to accept and preserve the
incoming timestamp so that incomingUpdatedAt reflects the webhook's timestamp
rather than current time.
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.
Summary
PATCH /api/groups/:groupId/members/:userId/roleendpoint for managing admin/owner roles with proper authorization checksTest plan
Summary by CodeRabbit