feat!: Undeprecate /v1/users.createToken endpoint
#36570
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 |
🦋 Changeset detectedLatest commit: 03ec248 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36570 +/- ##
===========================================
- Coverage 67.74% 66.92% -0.82%
===========================================
Files 3457 3471 +14
Lines 113708 115054 +1346
Branches 20903 21311 +408
===========================================
- Hits 77026 77005 -21
- Misses 34547 35987 +1440
+ Partials 2135 2062 -73
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
WalkthroughReplaces the previous users.createToken flow with a secret-gated implementation: the REST endpoint and server DDP method now require a provided Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant REST as "REST /v1/users.createToken"
participant Env as "Env (CREATE_TOKENS_FOR_USERS_SECRET)"
participant Service as "Token Service / Method"
participant DB as "Users DB"
Client->>REST: POST { userId, secret }
REST->>Env: read CREATE_TOKENS_FOR_USERS_SECRET
alt secret matches
REST->>Service: generateAccessToken(userId, secret)
Service->>DB: insert login token
Service->>DB: ensureLoginTokensLimit(uid) (tokens default to [])
Service-->>REST: { userId, authToken }
REST-->>Client: 200 { data: { userId, authToken }, success: true }
else secret mismatch
REST-->>Client: 400 / MeteorError (not authorized)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (6)📚 Learning: 2025-09-15T06:21:00.139ZApplied to files:
📚 Learning: 2025-09-15T06:21:00.139ZApplied to files:
📚 Learning: 2025-11-04T16:49:19.107ZApplied to files:
📚 Learning: 2025-10-07T15:08:37.419ZApplied to files:
📚 Learning: 2025-11-19T12:32:29.696ZApplied to files:
📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
🧬 Code graph analysis (2)apps/meteor/app/api/server/v1/users.ts (2)
apps/meteor/app/lib/server/methods/createToken.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
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 |
sampaiodiego
force-pushed
the
undeprecate-create-tokens-endpoint
branch
from
a52f2d3 to
ae7d1f9
Compare
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/methods/createToken.ts (1)
11-11: Fix the secret argument flow increateToken.
generateAccessTokennow requires(userId, secret), butcreateTokenstill calls it with the old order. As a result, the second argument is the requested user ID (from clients still calling with a user ID), so the secret comparison fails and the method always throwserror-not-authorized. Please update the method signature (and its declaration) to accept the secret explicitly and forward it correctly.- createToken(userId: string): { userId: string; authToken: string }; + createToken(secret: string): { userId: string; authToken: string }; @@ -Meteor.methods<ServerMethods>({ - async createToken(userId) { +Meteor.methods<ServerMethods>({ + async createToken(secret) { methodDeprecationLogger.method('createToken', '8.0.0', '/v1/users.createToken'); const callee = Meteor.userId(); if (!callee) { throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'createToken' }); } - return generateAccessToken(callee, userId); + return generateAccessToken(callee, secret); }, });Also applies to: 34-43
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/users.ts (1)
781-799: RequireauthTokenin the success schema.Line 789 only marks
userIdas required. Because this schema feedsExtractRoutesFromAPI, the generated typings (and the runtime validator) will treatauthTokenas optional even though the route always returns it. That weakens the contract and would let a regression slip through unnoticed. Please addauthTokento therequiredarray.- required: ['userId'], + required: ['userId', 'authToken'],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/afraid-parents-bake.md(1 hunks)apps/meteor/app/api/server/v1/users.ts(4 hunks)apps/meteor/app/lib/server/methods/createToken.ts(2 hunks)apps/meteor/server/services/user/service.ts(1 hunks)packages/rest-typings/src/v1/users.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/users.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/app/api/server/v1/users.ts (5)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)apps/meteor/app/lib/server/methods/createToken.ts (1)
generateAccessToken(17-31)packages/rest-typings/src/v1/users.ts (1)
UsersEndpoints(131-389)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
apps/meteor/app/lib/server/methods/createToken.ts (1)
packages/core-services/src/index.ts (1)
MeteorError(56-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/users.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/users.ts
🧠 Learnings (2)
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the permission model distinguishes between token pass-through and true impersonation: `server-endpoints.call` is required for all endpoint access, while `server-endpoints.impersonate` is only required when `info.user.id` is provided without `info.user.token` (lines 48-53), meaning the bridge needs to mint a token. When both user ID and token are provided, it's considered legitimate credential usage, not impersonation.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the impersonation permission (`server-endpoints.impersonate`) is enforced at lines 48-49 specifically when `info.user` is provided without `info.user.token`, which represents the impersonation scenario where a token needs to be minted. Token pass-through (when both user ID and token are provided) doesn't require the impersonate permission since the app already possesses a valid token.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/users.ts (4)
apps/meteor/app/api/server/index.ts (1)
API(55-55)packages/rest-typings/src/v1/Ajv.ts (1)
ajv(24-24)apps/meteor/app/lib/server/methods/createToken.ts (1)
generateAccessToken(17-31)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/app/api/server/v1/users.ts (3)
21-21: LGTM: Imports support the new validated endpoint.The
ajvimport enables runtime validation for the endpoint, andExtractRoutesFromAPIallows proper type extraction for REST typings augmentation.Also applies to: 73-73
819-826: LGTM: Token generation logic is correct.The implementation properly:
- Validates the user via
getUserFromParams- Delegates secret validation and token generation to
generateAccessToken- Returns the structured response
Error handling for invalid secrets is properly delegated to the
generateAccessTokenfunction, which throws aMeteorErrorwhen the secret doesn't match.
1488-1493: LGTM: Type exports and module augmentation are correctly implemented.The pattern properly:
- Extracts route types from the endpoint definition using
ExtractRoutesFromAPI- Augments the REST typings module to expose the endpoint signatures
- Maintains type safety for API consumers
| const usersEndpoints = API.v1.post( | ||
| 'users.createToken', | ||
| { authRequired: true, deprecationVersion: '8.0.0' }, | ||
| { | ||
| async post() { | ||
| const user = await getUserFromParams(this.bodyParams); | ||
| authRequired: true, | ||
| body: ajv.compile<{ userId: string; secret: string }>({ | ||
| type: 'object', | ||
| properties: { | ||
| userId: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| }, | ||
| secret: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| }, | ||
| }, | ||
| required: ['userId', 'secret'], | ||
| additionalProperties: false, | ||
| }), | ||
| response: { | ||
| 200: ajv.compile<{ data: { userId: string; authToken: string } }>({ | ||
| type: 'object', | ||
| properties: { | ||
| data: { | ||
| type: 'object', | ||
| properties: { | ||
| userId: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| }, | ||
| authToken: { | ||
| type: 'string', | ||
| minLength: 1, | ||
| }, | ||
| }, | ||
| required: ['userId'], | ||
| additionalProperties: false, | ||
| }, | ||
| success: { | ||
| type: 'boolean', | ||
| enum: [true], | ||
| }, | ||
| }, | ||
| required: ['data', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 400: ajv.compile({ | ||
| type: 'object', | ||
| properties: { | ||
| success: { type: 'boolean', enum: [false] }, | ||
| error: { type: 'string' }, | ||
| errorType: { type: 'string' }, | ||
| }, | ||
| required: ['success'], | ||
| additionalProperties: false, | ||
| }), | ||
| }, | ||
| }, | ||
| async function action() { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the users.ts file around the endpoint definition
head -n 850 apps/meteor/app/api/server/v1/users.ts | tail -n 100Repository: RocketChat/Rocket.Chat
Length of output: 2135
🏁 Script executed:
#!/bin/bash
# First, check file size and line count
wc -l apps/meteor/app/api/server/v1/users.ts
# Read the specific section with the endpoint definition
sed -n '761,819p' apps/meteor/app/api/server/v1/users.tsRepository: RocketChat/Rocket.Chat
Length of output: 1331
🏁 Script executed:
#!/bin/bash
# Search for hasPermission checks in the file
rg -n -A 5 "hasPermission.*create.*token" apps/meteor/app/api/server/v1/users.ts
# Also search for the createToken endpoint handler
rg -n -A 30 "users\.createToken" apps/meteor/app/api/server/v1/users.ts | head -60
# Check for rate limiting patterns
rg -n "rateLimiterOptions" apps/meteor/app/api/server/v1/users.ts
# Check for audit logging
rg -n "Audit\." apps/meteor/app/api/server/v1/users.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 854
🏁 Script executed:
#!/bin/bash
# Get more lines to see the complete endpoint
sed -n '810,850p' apps/meteor/app/api/server/v1/users.ts
# Find the generateAccessToken function
rg -n -B 5 -A 20 "generateAccessToken" apps/meteor/app/api/server/v1/users.ts | head -50
# Check if this endpoint has rateLimiterOptions by reading larger range
sed -n '760,825p' apps/meteor/app/api/server/v1/users.ts
# Search for all endpoints near createToken to understand pattern
rg -n "API\.v1\.(post|get|put|delete)" apps/meteor/app/api/server/v1/users.ts | grep -A 2 -B 2 "createToken"Repository: RocketChat/Rocket.Chat
Length of output: 4551
🏁 Script executed:
#!/bin/bash
# Let's look at the imports to understand what getUserFromParams and generateAccessToken do
head -100 apps/meteor/app/api/server/v1/users.ts
# Also check the actual location of generateAccessToken function
rg -n "^(export )?(async )?function generateAccessToken|^const generateAccessToken" apps/meteor/app/api/server/v1/users.tsRepository: RocketChat/Rocket.Chat
Length of output: 5662
🏁 Script executed:
#!/bin/bash
# Search for imports and helper functions
rg -n "generateAccessToken|getUserFromParams" apps/meteor/app/api/server/v1/users.ts
# Also find where the endpoint definition closes and look for rateLimiterOptions
sed -n '760,830p' apps/meteor/app/api/server/v1/users.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 2869
🏁 Script executed:
#!/bin/bash
# Find and read the generateAccessToken function
find apps/meteor -name "createToken.ts" -o -name "createToken.js" | head -5
# Also search more broadly
rg -l "generateAccessToken" apps/meteor --type=ts | head -5Repository: RocketChat/Rocket.Chat
Length of output: 204
🏁 Script executed:
#!/bin/bash
# Read the createToken module
cat apps/meteor/app/lib/server/methods/createToken.ts
# Also search for hasPermission checks related to token creation
rg -n "hasPermission.*token" apps/meteor --type=ts -i | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1981
🏁 Script executed:
#!/bin/bash
# Let's check if there's a getUserFromParams implementation that might have permission checks
cat apps/meteor/app/api/helpers/getUserFromParams.ts
# And check for rateLimiterOptions in the entire API file for pattern understanding
rg -B 5 -A 5 "rateLimiterOptions" apps/meteor/app/api/server/v1/users.ts | head -40Repository: RocketChat/Rocket.Chat
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Check if createToken endpoint has rateLimiterOptions
sed -n '761,827p' apps/meteor/app/api/server/v1/users.ts | grep -A 2 -B 2 "rateLimiterOptions\|numRequestsAllowed"
# Search for any rateLimiterOptions in context of createToken
rg -B 20 "users\.createToken" apps/meteor/app/api/server/v1/users.ts | grep -A 15 "rateLimiterOptions\|numRequestsAllowed"
# Check all endpoints to see if createToken has rate limiting in its options object
sed -n '761,830p' apps/meteor/app/api/server/v1/users.tsRepository: RocketChat/Rocket.Chat
Length of output: 1532
🏁 Script executed:
#!/bin/bash
# Search for audit logging patterns related to token creation
rg -n "Audit\.|audit" apps/meteor/app/lib/server/methods/createToken.ts
# Check for any logging in generateAccessToken
rg -n "logger\|log\|audit" apps/meteor/app/lib/server/methods/createToken.ts
# Look for UserChangedAuditStore which was imported
rg -n "UserChangedAuditStore" apps/meteor/app/api/server/v1/users.ts | head -10
# Check getUserFromParams implementation
find apps/meteor -path "*/api/helpers/*" -name "*.ts" | xargs ls -laRepository: RocketChat/Rocket.Chat
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Find getUserFromParams helper
find apps/meteor -name "getUserFromParams*" -o -name "*getUserFromParams*" | grep -E "\.(ts|js)$"
# Search for it in the api directory
rg -l "export.*getUserFromParams" apps/meteor/app/api --type=tsRepository: RocketChat/Rocket.Chat
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Check the full implementation details
rg -B 5 -A 30 "export.*function getUserFromParams|export const getUserFromParams" apps/meteor/app/api --type=ts
# Also check what other sensitive endpoints do for comparison
rg -B 10 "users\.generatePersonalAccessToken\|users\.setUserStatus\|users\.create" apps/meteor/app/api/server/v1/users.ts | grep -A 10 -B 10 "hasPermission\|rateLimiter"Repository: RocketChat/Rocket.Chat
Length of output: 3370
🏁 Script executed:
#!/bin/bash
# Look at how generatePersonalAccessToken endpoint is defined for comparison
rg -B 5 -A 30 "users\.generatePersonalAccessToken" apps/meteor/app/api/server/v1/users.ts | head -60
# Also look at how users.create handles permissions
rg -B 5 -A 30 "users\.create" apps/meteor/app/api/server/v1/users.ts | head -60Repository: RocketChat/Rocket.Chat
Length of output: 2507
🏁 Script executed:
#!/bin/bash
# Look specifically at the endpoint options for createToken
sed -n '761,768p' apps/meteor/app/api/server/v1/users.ts
# Check if there's a validateParams or permission check option we might have missed
sed -n '761,820p' apps/meteor/app/api/server/v1/users.ts | grep -i "permission\|validate\|rateLimiter"
# Compare structure with a known secure endpoint
rg -B 2 -A 15 "authRequired: true," apps/meteor/app/api/server/v1/users.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2854
🏁 Script executed:
#!/bin/bash
# Check the deprecation status and any related PRs or changes
rg -n "CREATE_TOKENS_FOR_USERS\|deprecated\|8.0.0" apps/meteor/app/lib/server/methods/createToken.ts
# Search for any permission checks that might be in the endpoint definition itself
sed -n '761,827p' apps/meteor/app/api/server/v1/users.ts | cat -n
# Check if there are any authorization middleware or decorators
rg -B 10 "async function action" apps/meteor/app/api/server/v1/users.ts | grep -A 10 "createToken"Repository: RocketChat/Rocket.Chat
Length of output: 1934
🏁 Script executed:
#!/bin/bash
# Check PR context or related changes
git log --oneline --all --grep="createToken\|users.createToken" 2>/dev/null | head -20
# Look for any recent changes to the endpoint
git log --oneline -p apps/meteor/app/api/server/v1/users.ts 2>/dev/null | grep -A 20 "users.createToken" | head -40
# Check if there are permission checks in any middleware
rg -n "hasPermission" apps/meteor/app/api/server/v1/users.ts | grep -i "token\|create"Repository: RocketChat/Rocket.Chat
Length of output: 718
Endpoint lacks permission checks, rate limiting, and audit logging.
The endpoint documentation indicates it should require the user-generate-access-token permission, but the implementation contains only authRequired: true with no explicit permission check. The sole protection mechanism is the CREATE_TOKENS_FOR_USERS_SECRET environment variable validation in generateAccessToken(), meaning any authenticated user with access to the secret can create tokens for any other user.
The endpoint lacks:
- Permission checks: No
hasPermissionAsync()call foruser-generate-access-tokenor equivalent - Rate limiting: No
rateLimiterOptionsdefined (unlike authentication endpoints such asusers.register) - Audit logging: No
UserChangedAuditStoreor token creation audit records
Documented security issue: users with token creation capability can create tokens for administrators and use them for unauthorized actions. Consider implementing role-based permission checks, rate limiting, and audit logging for this sensitive endpoint before the 8.0.0 deprecation removal.
| minLength: 1, | ||
| }, | ||
| }, | ||
| required: ['userId'], |
There was a problem hiding this comment.
Fix the response schema: authToken should be required.
The required array only includes userId, but generateAccessToken always returns both userId and authToken (as seen in the relevant code snippet). The response schema should reflect this.
Apply this diff:
- required: ['userId'],
+ required: ['userId', 'authToken'],📝 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.
| required: ['userId'], | |
| required: ['userId', 'authToken'], |
🤖 Prompt for AI Agents
In apps/meteor/app/api/server/v1/users.ts around line 796, the JSON response
schema's required array only lists 'userId' but the function always returns both
'userId' and 'authToken'; update the schema so the required array includes both
'userId' and 'authToken' (e.g. required: ['userId','authToken']) to accurately
reflect the response shape.
📦 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/15 22:28", "11/16 01:28", "11/17 23:50", "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 17:50", "12/17 19:27 (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]
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]
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]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 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]
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]
line "queue-worker-service" [0.14, 0.14, 0.14, 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]
line "rocketchat" [0.36, 0.36, 0.35, 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.35]
Statistics (last 23 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 17 Dec 2025 19:27:52 GMT |
dionisio-bot
bot
added
the
stat: ready to merge
ggazzo
force-pushed
the
undeprecate-create-tokens-endpoint
branch
from
c91a367 to
4224d8a
Compare
dionisio-bot
bot
removed
the
stat: ready to merge
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)
apps/meteor/app/lib/server/methods/createToken.ts (1)
13-27: Add timing-safe secret comparison and explicit user validation.The REST API endpoint validates user existence via
getUserFromParams()before calling this function, but the function itself lacks internal validation. For defense in depth, add an explicit check. Additionally, the secret comparison should use timing-safe comparison to prevent timing attacks:export async function generateAccessToken(userId: string, secret: string) { if (!secret || !CREATE_TOKENS_FOR_USERS_SECRET || !crypto.timingSafeEqual(Buffer.from(secret), Buffer.from(CREATE_TOKENS_FOR_USERS_SECRET))) { throw new MeteorError('error-not-authorized', 'Not authorized'); } + const user = await User.findOneById(userId); + if (!user) { + throw new MeteorError('error-invalid-user', 'User not found'); + } const token = Accounts._generateStampedLoginToken(); Accounts._insertLoginToken(userId, token); await User.ensureLoginTokensLimit(userId); return { userId, authToken: token.token, }; }The timing-safe comparison is critical to prevent attackers from using timing analysis to guess the secret. User validation should be explicit at the function level to prevent issues if this is called from other contexts in the future.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/afraid-parents-bake.md(1 hunks)apps/meteor/app/api/server/v1/users.ts(4 hunks)apps/meteor/app/lib/server/methods/createToken.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/api/server/v1/users.ts
- .changeset/afraid-parents-bake.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/methods/createToken.ts
🧠 Learnings (4)
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the permission model distinguishes between token pass-through and true impersonation: `server-endpoints.call` is required for all endpoint access, while `server-endpoints.impersonate` is only required when `info.user.id` is provided without `info.user.token` (lines 48-53), meaning the bridge needs to mint a token. When both user ID and token are provided, it's considered legitimate credential usage, not impersonation.
Applied to files:
apps/meteor/app/lib/server/methods/createToken.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/lib/server/methods/createToken.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/methods/createToken.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the impersonation permission (`server-endpoints.impersonate`) is enforced at lines 48-49 specifically when `info.user` is provided without `info.user.token`, which represents the impersonation scenario where a token needs to be minted. Token pass-through (when both user ID and token are provided) doesn't require the impersonate permission since the app already possesses a valid token.
Applied to files:
apps/meteor/app/lib/server/methods/createToken.ts
🔇 Additional comments (2)
apps/meteor/app/lib/server/methods/createToken.ts (2)
1-3: LGTM!The imports are appropriate for the token generation functionality.
4-9: LGTM!The TypeScript augmentation properly declares the
createTokenmethod signature for the public API. The signature difference between this declaration (taking onlyuserId) and thegenerateAccessTokenimplementation (takinguserIdandsecret) is expected, as the actual method registration likely supplies the secret internally.
| } | ||
| } | ||
|
|
||
| const { CREATE_TOKENS_FOR_USERS_SECRET } = process.env; |
There was a problem hiding this comment.
Critical: Validate that the secret environment variable is defined and non-empty.
If CREATE_TOKENS_FOR_USERS_SECRET is not set in the environment, it will be undefined. This creates a critical security vulnerability where the validation on line 14 could be bypassed if an attacker passes undefined as the secret parameter (since undefined !== undefined is false).
Apply this diff to add defensive validation:
-const { CREATE_TOKENS_FOR_USERS_SECRET } = process.env;
+const { CREATE_TOKENS_FOR_USERS_SECRET } = process.env;
+
+if (!CREATE_TOKENS_FOR_USERS_SECRET || CREATE_TOKENS_FOR_USERS_SECRET.trim() === '') {
+ throw new Error('CREATE_TOKENS_FOR_USERS_SECRET environment variable must be set and non-empty');
+}📝 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 { CREATE_TOKENS_FOR_USERS_SECRET } = process.env; | |
| const { CREATE_TOKENS_FOR_USERS_SECRET } = process.env; | |
| if (!CREATE_TOKENS_FOR_USERS_SECRET || CREATE_TOKENS_FOR_USERS_SECRET.trim() === '') { | |
| throw new Error('CREATE_TOKENS_FOR_USERS_SECRET environment variable must be set and non-empty'); | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/app/lib/server/methods/createToken.ts around line 11, the code
reads the CREATE_TOKENS_FOR_USERS_SECRET env var but doesn't validate it; add a
defensive check that ensures process.env.CREATE_TOKENS_FOR_USERS_SECRET is a
defined, non-empty string (e.g. typeof value === 'string' && value.trim().length
> 0) and if not, throw an Error (or fail fast) so the server refuses to start or
the module fails to load; this prevents an attacker from passing undefined and
bypassing the secret check and ensures later comparisons are against a
guaranteed string.
| if (secret !== CREATE_TOKENS_FOR_USERS_SECRET) { | ||
| throw new MeteorError('error-not-authorized', 'Not authorized'); |
There was a problem hiding this comment.
Critical: Use constant-time comparison for secret validation.
The direct string comparison (!==) is vulnerable to timing attacks, which could allow an attacker to gradually guess the secret by measuring response times. Additionally, if CREATE_TOKENS_FOR_USERS_SECRET is undefined, this check can be bypassed.
Apply this diff to use a constant-time comparison:
+import crypto from 'crypto';
+
export async function generateAccessToken(userId: string, secret: string) {
- if (secret !== CREATE_TOKENS_FOR_USERS_SECRET) {
+ if (!secret || !CREATE_TOKENS_FOR_USERS_SECRET || !crypto.timingSafeEqual(Buffer.from(secret), Buffer.from(CREATE_TOKENS_FOR_USERS_SECRET))) {
throw new MeteorError('error-not-authorized', 'Not authorized');
}Note: This assumes both strings are the same length. For production use, consider using a dedicated constant-time comparison library or padding the strings to the same length before comparison.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/app/lib/server/methods/createToken.ts around lines 14-15, the
direct string comparison of the provided secret to
CREATE_TOKENS_FOR_USERS_SECRET must be replaced with a constant-time comparison
and a guard against undefined to prevent timing attacks and bypasses; convert
both values to Buffers (or Uint8Array), ensure both are defined and have equal
byte length, and then use crypto.timingSafeEqual to compare them, throwing the
same MeteorError when the check fails; if lengths differ or either value is
undefined, treat as non-equal and throw the MeteorError.
/v1/users.createToken endpoint/v1/users.createToken endpoint
ggazzo
force-pushed
the
release-8.0.0
branch
from
d92c161 to
5ace0ee
Compare
ggazzo
force-pushed
the
release-8.0.0
branch
from
5ace0ee to
76cf16f
Compare
ggazzo
force-pushed
the
undeprecate-create-tokens-endpoint
branch
from
4224d8a to
03ec248
Compare
https://rocketchat.atlassian.net/browse/ARCH-1749
https://rocketchat.atlassian.net/browse/ARCH-1800
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Chores
Misc
✏️ Tip: You can customize this high-level summary in your review settings.