Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
313 changes: 234 additions & 79 deletions apps/meteor/app/api/server/v1/push.ts
Original file line number Diff line number Diff line change
@@ -1,107 +1,247 @@
import type { IAppsTokens } from '@rocket.chat/core-typings';
import { Messages, AppsTokens, Users, Rooms, Settings } from '@rocket.chat/models';
import { Random } from '@rocket.chat/random';
import { ajv, validateBadRequestErrorResponse, validateUnauthorizedErrorResponse } from '@rocket.chat/rest-typings';
import { Match, check } from 'meteor/check';
import { Push } from '@rocket.chat/core-services';
import type { IPushToken } from '@rocket.chat/core-typings';
import { Messages, PushToken, Users, Rooms, Settings } from '@rocket.chat/models';
import {
ajv,
isPushGetProps,
validateNotFoundErrorResponse,
validateBadRequestErrorResponse,
validateUnauthorizedErrorResponse,
validateForbiddenErrorResponse,
} from '@rocket.chat/rest-typings';
import type { JSONSchemaType } from 'ajv';
import { Meteor } from 'meteor/meteor';

import { executePushTest } from '../../../../server/lib/pushConfig';
import { canAccessRoomAsync } from '../../../authorization/server/functions/canAccessRoom';
import { pushUpdate } from '../../../push/server/methods';
import PushNotification from '../../../push-notifications/server/lib/PushNotification';
import { settings } from '../../../settings/server';
import type { ExtractRoutesFromAPI } from '../ApiClass';
import { API } from '../api';
import type { SuccessResult } from '../definition';

API.v1.addRoute(
'push.token',
{ authRequired: true },
{
async post() {
const { id, type, value, appName } = this.bodyParams;
type PushTokenPOST = {
id?: string;
type: 'apn' | 'gcm';
value: string;
appName: string;
};

if (id && typeof id !== 'string') {
throw new Meteor.Error('error-id-param-not-valid', 'The required "id" body param is invalid.');
}
const PushTokenPOSTSchema: JSONSchemaType<PushTokenPOST> = {
type: 'object',
properties: {
id: {
type: 'string',
nullable: true,
},
type: {
type: 'string',
enum: ['apn', 'gcm'],
},
value: {
type: 'string',
minLength: 1,
},
appName: {
type: 'string',
minLength: 1,
},
},
required: ['type', 'value', 'appName'],
additionalProperties: false,
};

const deviceId = id || Random.id();
export const isPushTokenPOSTProps = ajv.compile<PushTokenPOST>(PushTokenPOSTSchema);

if (!type || (type !== 'apn' && type !== 'gcm')) {
throw new Meteor.Error('error-type-param-not-valid', 'The required "type" body param is missing or invalid.');
}
type PushTokenDELETE = {
token: string;
};

if (!value || typeof value !== 'string') {
throw new Meteor.Error('error-token-param-not-valid', 'The required "value" body param is missing or invalid.');
}
const PushTokenDELETESchema: JSONSchemaType<PushTokenDELETE> = {
type: 'object',
properties: {
token: {
type: 'string',
minLength: 1,
},
},
required: ['token'],
additionalProperties: false,
};

if (!appName || typeof appName !== 'string') {
throw new Meteor.Error('error-appName-param-not-valid', 'The required "appName" body param is missing or invalid.');
}
export const isPushTokenDELETEProps = ajv.compile<PushTokenDELETE>(PushTokenDELETESchema);

const authToken = this.request.headers.get('x-auth-token');
if (!authToken) {
type PushTokenResult = Pick<IPushToken, '_id' | 'token' | 'appName' | 'userId' | 'enabled' | 'createdAt' | '_updatedAt'>;

/**
* Pick only the attributes we actually want to return on the endpoint, ensuring nothing from older schemas get mixed in
*/
function cleanTokenResult(result: Omit<IPushToken, 'authToken'>): PushTokenResult {
const { _id, token, appName, userId, enabled, createdAt, _updatedAt } = result;

return {
_id,
token,
appName,
userId,
enabled,
createdAt,
_updatedAt,
};
Comment on lines +85 to +97
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

This narrows the existing push.token response contract.

packages/rest-typings/src/v1/push.ts:52-55 still exposes POST /v1/push.token as { result: IPushToken }, but this handler now validates and returns only PushTokenResult (dropping authToken and any other fields outside that pick). That makes the migration behavior-breaking for callers compiled against the old shape. Please either preserve the legacy payload here or update the shared push typings and consumers in the same PR.

Based on learnings, Rocket.Chat OpenAPI migration PRs under apps/meteor/app/api/server/v1 should avoid introducing logic changes and keep behavior intact.

Also applies to: 98-147, 172-172

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/meteor/app/api/server/v1/push.ts` around lines 79 - 90, The handler
narrows the POST /v1/push.token response to PushTokenResult, breaking the
existing contract which expects IPushToken; revert to returning the full
IPushToken (including authToken and any other fields) from cleanTokenResult or
stop using that narrow type so callers compiled against IPushToken still receive
the legacy shape, and ensure other similar places (the other clean/response uses
around the same block) are updated the same way; specifically modify
cleanTokenResult to return the original IPushToken object (or return a merged
object that includes authToken) instead of only the fields picked into
PushTokenResult, or alternatively update the shared rest-typings (IPushToken ->
new shape) across consumers in the same PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior is pre-existing and not introduced by this migration. The upstream already had cleanTokenResult stripping authToken (a security-sensitive field) before returning the response. The PushTokenResult pick type and cleanTokenResult function were in the codebase before this PR — this migration only makes the response schema explicit about what's actually returned, aligning the declared contract with the actual behavior.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

I need to verify whether cleanTokenResult and PushTokenResult existed on the develop branch before this PR.


🏁 Script executed:

#!/bin/bash
# Check if cleanTokenResult or PushTokenResult exist on the develop branch
git show origin/develop:apps/meteor/app/api/server/v1/push.ts 2>/dev/null || echo "FILE_NOT_FOUND_ON_DEVELOP"

Length of output: 8145


@smirk-dev, you are correct, and I apologize for the incorrect review comment. After checking the develop branch directly, I can confirm that cleanTokenResult, PushTokenResult, isPushTokenPOSTProps, isPushTokenDELETEProps, and the chained push.token POST/DELETE endpoints are all already present on develop. The diff was comparing against an older baseline, making it appear as if those were newly introduced by this PR.

The actual new work in this PR is limited to migrating push.get and push.info to the chained pattern — the response contract for push.token was already established in a prior landing. My comment was mistaken and does not apply.


✏️ Learnings added
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.

Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.

Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.

Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.

Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.

Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.

Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.

Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.

}

const pushEndpoints = API.v1
.post(
'push.token',
{
response: {
200: ajv.compile<SuccessResult<{ result: PushTokenResult }>['body']>({
additionalProperties: false,
type: 'object',
properties: {
success: {
type: 'boolean',
description: 'Indicates if the request was successful.',
},
result: {
type: 'object',
description: 'The updated token data for this device',
properties: {
_id: {
type: 'string',
},
token: {
type: 'object',
properties: {
apn: {
type: 'string',
},
gcm: {
type: 'string',
},
},
required: [],
additionalProperties: false,
},
appName: {
type: 'string',
},
userId: {
type: 'string',
nullable: true,
},
enabled: {
type: 'boolean',
},
createdAt: {
type: 'string',
},
_updatedAt: {
type: 'string',
},
},
additionalProperties: false,
},
},
required: ['success', 'result'],
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
body: isPushTokenPOSTProps,
authRequired: true,
},
async function action() {
const { id, type, value, appName } = this.bodyParams;

const rawToken = this.request.headers.get('x-auth-token');
if (!rawToken) {
throw new Meteor.Error('error-authToken-param-not-valid', 'The required "authToken" header param is missing or invalid.');
}
const authToken = Accounts._hashLoginToken(rawToken);

const result = await pushUpdate({
id: deviceId,
token: { [type]: value } as IAppsTokens['token'],
const result = await Push.registerPushToken({
...(id && { _id: id }),
token: { [type]: value } as IPushToken['token'],
authToken,
appName,
userId: this.userId,
});

return API.v1.success({ result });
return API.v1.success({ result: cleanTokenResult(result) });
},
)
.delete(
'push.token',
{
response: {
200: ajv.compile<void>({
additionalProperties: false,
type: 'object',
properties: {
success: {
type: 'boolean',
},
},
required: ['success'],
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
404: validateNotFoundErrorResponse,
},
body: isPushTokenDELETEProps,
authRequired: true,
},
async delete() {
async function action() {
const { token } = this.bodyParams;

if (!token || typeof token !== 'string') {
throw new Meteor.Error('error-token-param-not-valid', 'The required "token" body param is missing or invalid.');
}

const affectedRecords = (
await AppsTokens.deleteMany({
$or: [
{
'token.apn': token,
},
{
'token.gcm': token,
},
],
userId: this.userId,
})
).deletedCount;
const removeResult = await PushToken.removeAllByTokenStringAndUserId(token, this.userId);

if (affectedRecords === 0) {
if (removeResult.deletedCount === 0) {
return API.v1.notFound();
}

return API.v1.success();
},
},
);

API.v1.addRoute(
'push.get',
{ authRequired: true },
{
async get() {
const params = this.queryParams;
check(
params,
Match.ObjectIncluding({
id: String,
)
.get(
'push.get',
{
authRequired: true,
query: isPushGetProps,
response: {
200: ajv.compile<{ data: { message: object; notification: object }; success: true }>({
type: 'object',
properties: {
data: {
type: 'object',
properties: {
message: { type: 'object', additionalProperties: true },
notification: { type: 'object', additionalProperties: true },
},
required: ['message', 'notification'],
additionalProperties: false,
},
success: { type: 'boolean', enum: [true] },
},
required: ['data', 'success'],
additionalProperties: false,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We avoid using additionalProperties: true because it weakens validation. Instead, we use the $ref tags from Typia or explicitly define the properties if they are simple. In this specific case, I think you should use the Typia reference props

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the push.test response schema — the type annotation is now SuccessResult<{ tokensCount: number }>['body'] (consistent with the push.token pattern), and the schema is tightened to additionalProperties: false throughout. For push.get, since the response wraps IMessage and IPushNotificationConfig — both complex core types , I've used $ref: '#/components/schemas/IMessage' and $ref: '#/components/schemas/IPushNotificationConfig' as Typia reference properties rather than inlining the full object shapes. does that work?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about this issue? why still unsolve

}),
);
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
},
},
async function action() {
const { id } = this.queryParams;

const receiver = await Users.findOneById(this.userId);
if (!receiver) {
throw new Error('error-user-not-found');
}

const message = await Messages.findOneById(params.id);
const message = await Messages.findOneById(id);
if (!message) {
throw new Error('error-message-not-found');
}
Expand All @@ -119,25 +259,36 @@ API.v1.addRoute(

return API.v1.success({ data });
},
},
);

API.v1.addRoute(
'push.info',
{ authRequired: true },
{
async get() {
)
.get(
'push.info',
{
authRequired: true,
response: {
200: ajv.compile<{ pushGatewayEnabled: boolean; defaultPushGateway: boolean; success: true }>({
type: 'object',
properties: {
pushGatewayEnabled: { type: 'boolean' },
defaultPushGateway: { type: 'boolean' },
success: { type: 'boolean', enum: [true] },
},
required: ['pushGatewayEnabled', 'defaultPushGateway', 'success'],
additionalProperties: false,
}),
401: validateUnauthorizedErrorResponse,
},
},
async function action() {
const defaultGateway = (await Settings.findOneById('Push_gateway', { projection: { packageValue: 1 } }))?.packageValue;
const defaultPushGateway = settings.get('Push_gateway') === defaultGateway;
return API.v1.success({
pushGatewayEnabled: settings.get('Push_enable'),
pushGatewayEnabled: settings.get<boolean>('Push_enable'),
defaultPushGateway,
});
},
},
);
);

const pushEndpoints = API.v1.post(
const pushTestEndpoints = API.v1.post(
'push.test',
{
authRequired: true,
Expand Down Expand Up @@ -177,9 +328,13 @@ const pushEndpoints = API.v1.post(
},
);

export type PushEndpoints = ExtractRoutesFromAPI<typeof pushEndpoints>;
type PushTestEndpoints = ExtractRoutesFromAPI<typeof pushTestEndpoints>;

type PushTokenEndpoints = ExtractRoutesFromAPI<typeof pushEndpoints>;

type PushAllEndpoints = PushTestEndpoints & PushTokenEndpoints;

declare module '@rocket.chat/rest-typings' {
// eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface
interface Endpoints extends PushEndpoints {}
interface Endpoints extends PushAllEndpoints {}
}
2 changes: 2 additions & 0 deletions packages/rest-typings/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -257,5 +257,7 @@ export * from './v1/cloud';
export * from './v1/banners';
export * from './default';

export * from './v1/push';

// Export the ajv instance for use in other packages
export * from './v1/Ajv';