fix!: create inviteToken for invites and remove it from listInvites #38290
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API_Server as API Server
participant Invites_DB as Invites DB
Client->>API_Server: POST /findOrCreateInvite (roomId)
activate API_Server
API_Server->>Invites_DB: find existing invite by roomId
alt invite not found
API_Server->>API_Server: generate _id and inviteToken (crypto.randomUUID)
API_Server->>Invites_DB: insert invite with inviteToken
else invite found
API_Server->>Invites_DB: ensureInviteToken(_id) -> token (generate if missing)
API_Server->>Invites_DB: update invite with token if needed
end
API_Server->>API_Server: build invite URL using inviteToken
API_Server-->>Client: return invite object including inviteToken and url
deactivate API_Server
Client->>API_Server: POST /validateInviteToken (inviteToken)
activate API_Server
API_Server->>Invites_DB: findOneByInviteToken(inviteToken)
Invites_DB-->>API_Server: invite data / null
API_Server-->>Client: return validation result
deactivate API_Server
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)
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 |
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/24 00:36 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Sat, 24 Jan 2026 00:36:14 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38290 +/- ##
===========================================
+ Coverage 70.74% 70.75% +0.01%
===========================================
Files 3159 3159
Lines 109384 109384
Branches 19676 19682 +6
===========================================
+ Hits 77383 77398 +15
+ Misses 29963 29958 -5
+ Partials 2038 2028 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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)
apps/meteor/app/invites/server/functions/validateInviteToken.ts (1)
6-21: Backwards-compat break for legacy invite links.With token-only lookup, existing invite URLs based on
_idwill fail after deploy unless a migration/backfill updates tokens (and URLs) ahead of time or you add a legacy fallback lookup. Please confirm intended behavior and, if legacy links should keep working, add a compatibility path (e.g., fallback to_idand optionally backfill inviteToken/url).apps/meteor/app/invites/server/functions/listInvites.ts (1)
1-25: Add explicit crypto import for consistency with sibling module.The sibling file
findOrCreateInvite.tsin the same module importscrypto from 'node:crypto'. For consistency, add the import here as well:import type { IInvite } from '@rocket.chat/core-typings'; +import crypto from 'node:crypto'; import { Invites } from '@rocket.chat/models'; import { Meteor } from 'meteor/meteor';
🤖 Fix all issues with AI agents
In `@apps/meteor/app/invites/server/functions/findOrCreateInvite.ts`:
- Around line 93-103: The legacy invite token race is fixed by atomically
setting the token with a conditional findOneAndUpdate instead of
Invites.updateOne: when you detect an existing invite without inviteToken,
generate a token and call Invites.findOneAndUpdate({ _id: existing._id, $or: [{
inviteToken: { $exists: false } }, { inviteToken: null }, { inviteToken: "" }]
}, { $set: { inviteToken } }, { returnDocument: "after" }); then set existing to
the returned document (or, if null, re-fetch the invite) and compute
existing.url = getInviteUrl(existing) before returning; replace the current
updateOne + local assignment with this atomic pattern so the response token
always matches DB state.
🧹 Nitpick comments (3)
apps/meteor/app/invites/server/functions/listInvites.ts (1)
18-33: Batch legacy-token backfill and drop inline comments.Consider batching updates to avoid
no-await-in-loop, and remove inline comments to keep the implementation clean. This also speeds up token backfill when many invites exist.As per coding guidelines, avoid code comments in the implementation.♻️ Possible refactor (apply after fixing crypto import)
- // Ensure all invites have inviteToken (for legacy invites that might not have it) - for (const invite of invites) { - const inviteWithToken = invite as IInvite & { inviteToken?: string }; - if (!inviteWithToken.inviteToken) { - const inviteToken = randomUUID(); - // eslint-disable-next-line no-await-in-loop - await Invites.updateOne({ _id: invite._id }, { $set: { inviteToken } }); - inviteWithToken.inviteToken = inviteToken; - } - } - - // Remove inviteToken from the response - return invites.map((invite) => { + const invitesNeedingToken = invites.filter((invite) => !(invite as IInvite & { inviteToken?: string }).inviteToken); + await Promise.all( + invitesNeedingToken.map(async (invite) => { + const inviteToken = randomUUID(); + await Invites.updateOne({ _id: invite._id }, { $set: { inviteToken } }); + (invite as IInvite & { inviteToken: string }).inviteToken = inviteToken; + }), + ); + + return invites.map((invite) => {apps/meteor/tests/e2e/saml.spec.ts (1)
142-143: Consider renaming variable for clarity.The variable
inviteIdnow holds theinviteTokenvalue rather than the_id. While functionally correct (invite URLs should use the token), the naming is misleading.Consider renaming to better reflect its purpose:
♻️ Suggested variable rename
- const { inviteToken } = await inviteResponse.json(); - inviteId = inviteToken; + const { inviteToken: token } = await inviteResponse.json(); + inviteId = token;Or rename the variable declaration at line 100:
- let inviteId: string; + let inviteToken: string;And update all usages of
inviteIdtoinviteTokenthroughout the file (lines 445, 448, 459, 462, 485, 488, 508, 511).apps/meteor/tests/end-to-end/api/invites.ts (1)
104-112: Stabilize listInvites assertion by matching on ID (avoid order dependence).The list order isn’t guaranteed; picking index
0can make the test flaky if other invites exist. Consider locating the invite by_idbefore asserting oninviteToken.♻️ Proposed adjustment
- expect(res.body[0]).to.have.property('_id', testInviteID); - expect(res.body[0]).to.not.have.property('inviteToken'); + const invite = res.body.find((i: IInvite) => i._id === testInviteID); + expect(invite).to.exist; + expect(invite).to.not.have.property('inviteToken');
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
pierre-lehnen-rc
left a comment
pierre-lehnen-rc
left a comment
There was a problem hiding this comment.
This change breaks compatibility with all existing Invite URLs, which IMO would already be a bad idea in a major version, and is even more so in a minor one.
What would be a better alternative in your opinion to ensure security at the same time that we don't break compatibility? |
|
It wouldn't have the same level of security, but one option would be to update the old invites setting their old ID as the inviteToken (and perhaps changing their ID to a different value); That way the old URLs would still work and would not be leaked anymore. Then in addition we could perhaps make the invite list show an indicator on old invites suggesting they should be replaced. |
|
BTW the old token length (6) was a product decision. It was shorter in order to make it easier for users to type them out if needed (though I don't know if we have any workflow in which the code is typed manually). |
I see. I'll talk to Product about it and take a look into doing it this way as well. |
Got it. I'll talk to product about this one as well, but I don't think we have workflows where the code is typed manually either. |
julio-rocketchat
changed the title
|
@pierre-lehnen-rc talked to Milton about this one and he agreed to wait for 9.0.0 to do this fix. |
@julio-rocketchat is this really a thing? I don't think so by the looking at the code.. if that's not true does it means there is no vulnerability at all? |
I’ve created a few private rooms with the 
With these, since Then, I used it to get access to the private rooms with the user 
|
|
@julio-rocketchat I assume then you have created a new role with |
@sampaiodiego for the PoC I gave the I've thought about creating a new |
Proposed changes (including videos or screenshots)
Users who have the
create-invite-linkspermission are also able to access/admin/invitesand request all invites from thelistInvitesAPI endpoint. This means that users with such a permission are able to get invites for rooms they are not part of and they can use such invites to get arbitrary access to these rooms.This happens because the invite ID works as a token and can be used in an invite link. Notwithstanding, the invite ID/token is too short and may be susceptible to brute-force attempts.
This PR:
inviteTokenand separates it from_id.inviteTokenin thelistInvitesresponse nor in the UI._id's randomness and entropy withconst _id = crypto.randomBytes(8).toString('hex');.inviteTokenin a cryptographically secure manner by usingconst inviteToken = crypto.randomUUID();.Issue(s)
VLN-38
Steps to test or reproduce
N/A
Further comments
N/A
Summary by CodeRabbit
API Changes
UI Updates
Tests
✏️ Tip: You can customize this high-level summary in your review settings.