-
Notifications
You must be signed in to change notification settings - Fork 13k
feat!: Undeprecate /v1/users.createToken endpoint
#36570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@rocket.chat/rest-typings': minor | ||
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| REST endpoint `/v1/users.createToken` is not deprecated anymore. It now requires a `secret` parameter to generate a token for a user. This change is part of the effort to enhance security by ensuring that tokens are generated with an additional layer of validation. The `secret` parameter is validated against a new environment variable `CREATE_TOKENS_FOR_USERS_SECRET`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import { | |
| isUsersSetPreferencesParamsPOST, | ||
| isUsersCheckUsernameAvailabilityParamsGET, | ||
| isUsersSendConfirmationEmailParamsPOST, | ||
| ajv, | ||
| } from '@rocket.chat/rest-typings'; | ||
| import { getLoginExpirationInMs, wrapExceptions } from '@rocket.chat/tools'; | ||
| import { Accounts } from 'meteor/accounts-base'; | ||
|
|
@@ -69,6 +70,7 @@ import { deleteUserOwnAccount } from '../../../lib/server/methods/deleteUserOwnA | |
| import { settings } from '../../../settings/server'; | ||
| import { isSMTPConfigured } from '../../../utils/server/functions/isSMTPConfigured'; | ||
| import { getURL } from '../../../utils/server/getURL'; | ||
| import type { ExtractRoutesFromAPI } from '../ApiClass'; | ||
| import { API } from '../api'; | ||
| import { getPaginationItems } from '../helpers/getPaginationItems'; | ||
| import { getUserFromParams } from '../helpers/getUserFromParams'; | ||
|
|
@@ -756,17 +758,70 @@ API.v1.addRoute( | |
| }, | ||
| ); | ||
|
|
||
| API.v1.addRoute( | ||
| 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() { | ||
|
Comment on lines
+761
to
+819
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 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 The endpoint lacks:
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. |
||
| const user = await getUserFromParams(this.bodyParams); | ||
|
|
||
| const data = await generateAccessToken(this.userId, user._id); | ||
| const data = await generateAccessToken(user._id, this.bodyParams.secret); | ||
|
|
||
| return data ? API.v1.success({ data }) : API.v1.forbidden(); | ||
| }, | ||
| return API.v1.success({ data }); | ||
| }, | ||
| ); | ||
|
|
||
|
|
@@ -1429,3 +1484,10 @@ settings.watch<number>('Rate_Limiter_Limit_RegisterUser', (value) => { | |
|
|
||
| API.v1.updateRateLimiterDictionaryForRoute(userRegisterRoute, value); | ||
| }); | ||
|
|
||
| type UsersEndpoints = ExtractRoutesFromAPI<typeof usersEndpoints>; | ||
|
|
||
| declare module '@rocket.chat/rest-typings' { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface | ||
| interface Endpoints extends UsersEndpoints {} | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,14 +1,18 @@ | ||||||||||||||
| import { User } from '@rocket.chat/core-services'; | ||||||||||||||
| import { MeteorError, User } from '@rocket.chat/core-services'; | ||||||||||||||
| import { Accounts } from 'meteor/accounts-base'; | ||||||||||||||
|
|
||||||||||||||
| import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission'; | ||||||||||||||
| declare module '@rocket.chat/ddp-client' { | ||||||||||||||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||||||||||||||
| interface ServerMethods { | ||||||||||||||
| createToken(userId: string): { userId: string; authToken: string }; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const { CREATE_TOKENS_FOR_USERS_SECRET } = process.env; | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Validate that the secret environment variable is defined and non-empty. If 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| export async function generateAccessToken(callee: string, userId: string) { | ||||||||||||||
| if ( | ||||||||||||||
| !['yes', 'true'].includes(String(process.env.CREATE_TOKENS_FOR_USERS)) || | ||||||||||||||
| (callee !== userId && !(await hasPermissionAsync(callee, 'user-generate-access-token'))) | ||||||||||||||
| ) { | ||||||||||||||
| throw new Meteor.Error('error-not-authorized', 'Not authorized'); | ||||||||||||||
| export async function generateAccessToken(userId: string, secret: string) { | ||||||||||||||
| if (secret !== CREATE_TOKENS_FOR_USERS_SECRET) { | ||||||||||||||
| throw new MeteorError('error-not-authorized', 'Not authorized'); | ||||||||||||||
|
Comment on lines
+14
to
+15
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: Use constant-time comparison for secret validation. The direct string comparison ( 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.
🤖 Prompt for AI Agents |
||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| const token = Accounts._generateStampedLoginToken(); | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the response schema:
authTokenshould be required.The
requiredarray only includesuserId, butgenerateAccessTokenalways returns bothuserIdandauthToken(as seen in the relevant code snippet). The response schema should reflect this.Apply this diff:
📝 Committable suggestion
🤖 Prompt for AI Agents