Skip to content

chore: add hono as router #35078

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.

Sign up for GitHub

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

Merged
ggazzo merged 20 commits into from
Apr 8, 2025
Merged

chore: add hono as router #35078

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4876e39
chore: add hono package
ggazzo Mar 26, 2025
2a32480
chore: create honoAdapter
ggazzo Mar 26, 2025
24bb56a
chore: convert router to use hono
ggazzo Mar 26, 2025
d674f39
fix api/apps
ggazzo Mar 13, 2025
c85f9a9
fix: fix breaking json parse
Mar 18, 2025
a875f77
fix: clone request at abstraction so its allowed to read body from re…
Mar 19, 2025
c3c7bd2
fix: fixing types and upload
Mar 19, 2025
377f5a6
fix: general fixes
Mar 20, 2025
427f3b9
fix: copy from headers type instead of raw object
Mar 21, 2025
9ba4601
fix: fix tests
Mar 21, 2025
904865e
fix: general fixes
Mar 24, 2025
fd7806b
chore: remoteAddressMiddleware
ggazzo Mar 25, 2025
f61cea1
chore: uikit interaction body parser
ggazzo Mar 26, 2025
6a1f080
chore: apps body parser
Mar 26, 2025
b874815
chore: saml body parser
Mar 27, 2025
9d6f82c
chore: oauth body parser
Mar 27, 2025
b1b67bf
chore: parse body for saml
Mar 28, 2025
f554f3c
review
ggazzo Apr 7, 2025
583e29c
Update apps/meteor/app/api/server/router.ts
ggazzo Apr 7, 2025
ff2508b
adapt to allow dynamic imports
ggazzo Apr 8, 2025
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
93 changes: 25 additions & 68 deletions apps/meteor/app/api/server/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import type { JoinPathPattern, Method } from '@rocket.chat/rest-typings';
import { ajv } from '@rocket.chat/rest-typings/src/v1/Ajv';
import { wrapExceptions } from '@rocket.chat/tools';
import type { ValidateFunction } from 'ajv';
import express from 'express';
import type { Request, Response } from 'express';
import type express from 'express';
import { Accounts } from 'meteor/accounts-base';
import { DDP } from 'meteor/ddp';
import { DDPCommon } from 'meteor/ddp-common';
Comment on lines 7 to 13
Copy link

@kody-ai kody-ai bot Apr 8, 2025

Choose a reason for hiding this comment

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

kody code-review Kody Rules medium

// Import the necessary types from the '@rocket.chat/rest-typings' package or define custom types if needed.
import type { Request, Response } from '@rocket.chat/rest-typings';

// Or, if using custom types:
// interface Request { /* ... */ }
// interface Response { /* ... */ }

The removal of the explicit 'express' import and the associated types 'Request' and 'Response' might lead to issues with type checking and code completion. While the 'WebApp' object might provide similar functionality, it's crucial to ensure type safety. Consider adding explicit types for request and response objects within the affected methods to maintain type correctness and avoid potential runtime errors.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

Expand Down Expand Up @@ -42,6 +41,7 @@ import { parseJsonQuery } from './helpers/parseJsonQuery';
import { cors } from './middlewares/cors';
import { loggerMiddleware } from './middlewares/logger';
import { metricsMiddleware } from './middlewares/metrics';
import { remoteAddressMiddleware } from './middlewares/remoteAddressMiddleware';
import { tracerSpanMiddleware } from './middlewares/tracer';
import type { Route } from './router';
import { Router } from './router';
Expand Down Expand Up @@ -105,34 +105,6 @@ const rateLimiterDictionary: Record<
}
> = {};

const getRequestIP = (req: Request): string | null => {
const socket = req.socket || (req.connection as any)?.socket;
const remoteAddress = String(
req.headers['x-real-ip'] || (typeof socket !== 'string' && (socket?.remoteAddress || req.connection?.remoteAddress || null)),
);
const forwardedFor = String(req.headers['x-forwarded-for']);

if (!socket) {
return remoteAddress || forwardedFor || null;
}

const httpForwardedCount = parseInt(String(process.env.HTTP_FORWARDED_COUNT)) || 0;
if (httpForwardedCount <= 0) {
return remoteAddress;
}

if (!forwardedFor || typeof forwardedFor.valueOf() !== 'string') {
return remoteAddress;
}

const forwardedForIPs = forwardedFor.trim().split(/\s*,\s*/);
if (httpForwardedCount > forwardedForIPs.length) {
return remoteAddress;
}

return forwardedForIPs[forwardedForIPs.length - httpForwardedCount];
};

const generateConnection = (
ipAddress: string,
httpHeaders: Record<string, any>,
Expand Down Expand Up @@ -220,7 +192,6 @@ export class APIClass<
services: 0,
inviteToken: 0,
};

this.router = new Router(`/${this.apiPath}`.replace(/\/$/, '').replaceAll('//', '/'));

if (useDefaultAuth) {
Expand Down Expand Up @@ -408,9 +379,12 @@ export class APIClass<
rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.increment(objectForRateLimitMatch);
const attemptResult = await rateLimiterDictionary[objectForRateLimitMatch.route].rateLimiter.check(objectForRateLimitMatch);
const timeToResetAttempsInSeconds = Math.ceil(attemptResult.timeToReset / 1000);
response.setHeader('X-RateLimit-Limit', rateLimiterDictionary[objectForRateLimitMatch.route].options.numRequestsAllowed ?? '');
response.setHeader('X-RateLimit-Remaining', attemptResult.numInvocationsLeft);
response.setHeader('X-RateLimit-Reset', new Date().getTime() + attemptResult.timeToReset);
response.headers.set(
'X-RateLimit-Limit',
String(rateLimiterDictionary[objectForRateLimitMatch.route].options.numRequestsAllowed ?? ''),
);
response.headers.set('X-RateLimit-Remaining', String(attemptResult.numInvocationsLeft));
response.headers.set('X-RateLimit-Reset', String(new Date().getTime() + attemptResult.timeToReset));

if (!attemptResult.allowed) {
throw new Meteor.Error(
Expand Down Expand Up @@ -494,8 +468,8 @@ export class APIClass<
if (options && (!('twoFactorRequired' in options) || !options.twoFactorRequired)) {
return;
}
const code = request.headers['x-2fa-code'] ? String(request.headers['x-2fa-code']) : undefined;
const method = request.headers['x-2fa-method'] ? String(request.headers['x-2fa-method']) : undefined;
const code = request.headers.get('x-2fa-code') ? String(request.headers.get('x-2fa-code')) : undefined;
const method = request.headers.get('x-2fa-method') ? String(request.headers.get('x-2fa-method')) : undefined;

await checkCodeForUser({
user: userId,
Expand Down Expand Up @@ -781,14 +755,12 @@ export class APIClass<
const api = this;
(operations[method as keyof Operations<TPathPattern, TOptions>] as Record<string, any>).action =
async function _internalRouteActionHandler() {
this.requestIp = getRequestIP(this.request)!;

if (options.authRequired || options.authOrAnonRequired) {
const user = await api.authenticatedRoute(this.request);
const user = await api.authenticatedRoute.call(this, this.request);
this.user = user!;
this.userId = String(this.request.headers['x-user-id']);
this.token = (this.request.headers['x-auth-token'] &&
Accounts._hashLoginToken(String(this.request.headers['x-auth-token'])))!;
this.userId = String(this.request.headers.get('x-user-id'));
const authToken = this.request.headers.get('x-auth-token');
this.token = (authToken && Accounts._hashLoginToken(String(authToken)))!;
}

if (!this.user && options.authRequired && !options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead')) {
Expand All @@ -806,7 +778,7 @@ export class APIClass<

const objectForRateLimitMatch = {
IPAddr: this.requestIp,
route: `/${api.apiPath}${this.request.route.path}${this.request.method.toLowerCase()}`,
route: `/${route}${this.request.method.toLowerCase()}`,
};

let result;
Expand Down Expand Up @@ -932,9 +904,11 @@ export class APIClass<
}

protected async authenticatedRoute(req: Request): Promise<IUser | null> {
const { 'x-user-id': userId } = req.headers;
const headers = Object.fromEntries(req.headers.entries());

const userToken = String(req.headers['x-auth-token']);
const { 'x-user-id': userId } = headers;

const userToken = String(headers['x-auth-token']);

if (userId && userToken) {
return Users.findOne(
Expand Down Expand Up @@ -973,7 +947,7 @@ export class APIClass<
return bodyParams;
}

const code = bodyCode || request.headers['x-2fa-code'];
const code = bodyCode || request.headers.get('x-2fa-code');

const auth: Record<string, any> = {
password,
Expand Down Expand Up @@ -1035,7 +1009,7 @@ export class APIClass<
const args = loginCompatibility(this.bodyParams, request);

const invocation = new DDPCommon.MethodInvocation({
connection: generateConnection(getRequestIP(request) || '', this.request.headers),
connection: generateConnection(this.requestIp || '', this.request.headers),
});

try {
Expand Down Expand Up @@ -1227,32 +1201,15 @@ settings.watch<number>('API_Enable_Rate_Limiter_Limit_Calls_Default', (value) =>
API.v1.reloadRoutesToRefreshRateLimiter();
});

Meteor.startup(() => {
(WebApp.connectHandlers as unknown as ReturnType<typeof express>).use(
export const startRestAPI = () => {
(WebApp.rawConnectHandlers as unknown as ReturnType<typeof express>).use(
API.api
.use((_req, res, next) => {
res.removeHeader('X-Powered-By');
next();
})
.use(remoteAddressMiddleware)
.use(cors(settings))
.use(loggerMiddleware(logger))
.use(metricsMiddleware(API.v1, settings, metrics.rocketchatRestApi))
.use(tracerSpanMiddleware)
.use(API.v1.router)
.use(API.default.router).router,
);
});

(WebApp.connectHandlers as unknown as ReturnType<typeof express>)
.use(
express.json({
limit: '50mb',
}),
)
.use(
express.urlencoded({
extended: true,
limit: '50mb',
}),
)
.use(express.query({}));
};
4 changes: 2 additions & 2 deletions apps/meteor/app/api/server/definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import type { IUser, LicenseModule } from '@rocket.chat/core-typings';
import type { Logger } from '@rocket.chat/logger';
import type { Method, MethodOf, OperationParams, OperationResult, PathPattern, UrlParams } from '@rocket.chat/rest-typings';
import type { ValidateFunction } from 'ajv';
import type { Request, Response } from 'express';

import type { ITwoFactorOptions } from '../../2fa/server/code';

Expand All @@ -12,7 +11,7 @@ export type RedirectStatusCodes = Exclude<Range<308>, Range<300>>;

export type AuthorizationStatusCodes = Exclude<Range<451>, Range<400>>;

export type ErrorStatusCodes = Exclude<Range<511>, Range<500>>;
export type ErrorStatusCodes = Exclude<Exclude<Range<511>, Range<500>>, 509>;
Copy link

@kody-ai kody-ai bot Apr 8, 2025

Choose a reason for hiding this comment

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

kody code-review Potential Issues high

export type ErrorStatusCodes = 500 | 510;

The ErrorStatusCodes type now excludes 509 from the range. This could potentially cause issues if 509 is a valid error code in the system. Consider documenting this exclusion or ensuring it's intentional.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​


export type SuccessResult<T, TStatusCode extends SuccessStatusCodes = 200> = {
statusCode: TStatusCode;
Expand Down Expand Up @@ -137,6 +136,7 @@ export type PartialThis = {
readonly response: Response;
readonly userId: string;
readonly bodyParams: Record<string, unknown>;
readonly path: string;
readonly queryParams: Record<string, string>;
readonly queryOperations?: string[];
readonly queryFields?: string[];
Expand Down
5 changes: 2 additions & 3 deletions apps/meteor/app/api/server/helpers/getLoggedInUser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
import type { IUser } from '@rocket.chat/core-typings';
import { Users } from '@rocket.chat/models';
import type { Request } from 'express';
import { Accounts } from 'meteor/accounts-base';

export async function getLoggedInUser(request: Request): Promise<Pick<IUser, '_id' | 'username'> | null> {
const token = request.headers['x-auth-token'];
const userId = request.headers['x-user-id'];
const token = request.headers.get('x-auth-token');
Copy link

@kody-ai kody-ai bot Apr 8, 2025

Choose a reason for hiding this comment

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

kody code-review Security high

const token = request.headers.get('x-auth-token')?.trim();
const userId = request.headers.get('x-user-id')?.trim();

Multiple instances of missing input validation for critical parameters, potentially leading to security vulnerabilities or runtime errors.

This issue appears in multiple locations:

  • apps/meteor/app/api/server/helpers/getLoggedInUser.ts: Lines 6-7
  • apps/meteor/app/integrations/server/api/api.js: Lines 243-243
  • apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts: Lines 248-250
  • apps/meteor/server/services/omnichannel-integrations/providers/voxtelesys.ts: Lines 165-167
    Please add validation checks for critical parameters to ensure they are not null, undefined, or malformed before processing.

Talk to Kody by mentioning @kody

Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.

​

​

const userId = request.headers.get('x-user-id');
if (!token || !userId || typeof token !== 'string' || typeof userId !== 'string') {
return null;
}
Expand Down
4 changes: 2 additions & 2 deletions apps/meteor/app/api/server/helpers/isWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { parse } from 'cookie';

export const isWidget = (headers: Record<string, any> = {}): boolean => {
const { rc_room_type: roomType, rc_is_widget: isWidget } = parse(headers.cookie || '');
export const isWidget = (headers: Headers): boolean => {
const { rc_room_type: roomType, rc_is_widget: isWidget } = parse(headers.get('cookie') || '');

const isLivechatRoom = roomType && roomType === 'l';
return !!(isLivechatRoom && isWidget === 't');
Expand Down
10 changes: 5 additions & 5 deletions apps/meteor/app/api/server/helpers/parseJsonQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
query: Record<string, unknown>;
}> {
const {
request: { path: route },
userId,
queryParams: params,
logger,
queryFields,
queryOperations,
response,
request: { route },
} = api;

let sort;
Expand Down Expand Up @@ -60,7 +60,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
let fields: Record<string, 0 | 1> | undefined;
if (params.fields && isUnsafeQueryParamsAllowed) {
try {
apiDeprecationLogger.parameter(route, 'fields', '8.0.0', response, messageGenerator);
apiDeprecationLogger.parameter(api.path, 'fields', '8.0.0', response, messageGenerator);
Copy link
Member

@KevLehman KevLehman Apr 7, 2025

Choose a reason for hiding this comment

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

[nitpick] if you're destructuring a ton of things from api, why not do it with this path and then use path instead of api.path?

fields = JSON.parse(params.fields) as Record<string, 0 | 1>;
Object.entries(fields).forEach(([key, value]) => {
if (value !== 1 && value !== 0) {
Expand Down Expand Up @@ -99,7 +99,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{

// Limit the fields by default
fields = Object.assign({}, fields, API.v1.defaultFieldsToExclude);
if (route.includes('/v1/users.')) {
if (api.path.includes('/v1/users.')) {
if (await hasPermissionAsync(userId, 'view-full-other-user-info')) {
fields = Object.assign(fields, API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser);
} else {
Expand All @@ -109,7 +109,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{

let query: Record<string, any> = {};
if (params.query && isUnsafeQueryParamsAllowed) {
apiDeprecationLogger.parameter(route, 'query', '8.0.0', response, messageGenerator);
apiDeprecationLogger.parameter(api.path, 'query', '8.0.0', response, messageGenerator);
try {
query = ejson.parse(params.query);
query = clean(query, pathAllowConf.def);
Expand All @@ -125,7 +125,7 @@ export async function parseJsonQuery(api: PartialThis): Promise<{
if (typeof query === 'object') {
let nonQueryableFields = Object.keys(API.v1.defaultFieldsToExclude);

if (route.includes('/v1/users.')) {
if (api.path.includes('/v1/users.')) {
if (await hasPermissionAsync(userId, 'view-full-other-user-info')) {
nonQueryableFields = nonQueryableFields.concat(Object.keys(API.v1.limitedUserFieldsToExcludeIfIsPrivilegedUser));
} else {
Expand Down
48 changes: 29 additions & 19 deletions apps/meteor/app/api/server/lib/getUploadFormData.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import { Readable } from 'stream';

import { expect } from 'chai';
import type { Request } from 'express';

import { getUploadFormData } from './getUploadFormData';

Expand All @@ -13,7 +10,7 @@ const createMockRequest = (
content: string | Buffer;
mimetype?: string;
},
): Readable & { headers: Record<string, string> } => {
): Request => {
const boundary = '----WebKitFormBoundary7MA4YWxkTrZu0gW';
const parts: string[] = [];

Expand All @@ -33,18 +30,31 @@ const createMockRequest = (

parts.push(`--${boundary}--`);

const mockRequest: any = new Readable({
read() {
this.push(Buffer.from(parts.join('\r\n')));
this.push(null);
},
});
const buffer = Buffer.from(parts.join('\r\n'));

mockRequest.headers = {
'content-type': `multipart/form-data; boundary=${boundary}`,
const mockRequest: any = {
headers: {
entries: () => [['content-type', `multipart/form-data; boundary=${boundary}`]],
},
blob: async () => ({
stream: () => {
let hasRead = false;
return {
getReader: () => ({
read: async () => {
if (!hasRead) {
hasRead = true;
return { value: buffer, done: false };
}
return { done: true };
},
}),
};
},
}),
};

return mockRequest as Readable & { headers: Record<string, string> };
return mockRequest as Request & { headers: Record<string, string> };
};

describe('getUploadFormData', () => {
Expand All @@ -59,7 +69,7 @@ describe('getUploadFormData', () => {
},
);

const result = await getUploadFormData({ request: mockRequest as Request }, { field: 'fileField' });
const result = await getUploadFormData({ request: mockRequest }, { field: 'fileField' });

expect(result).to.deep.include({
fieldname: 'fileField',
Expand All @@ -86,7 +96,7 @@ describe('getUploadFormData', () => {
},
);

const result = await getUploadFormData({ request: mockRequest as Request }, { field: 'fileField' });
const result = await getUploadFormData({ request: mockRequest }, { field: 'fileField' });

expect(result).to.deep.include({
fieldname: 'fileField',
Expand Down Expand Up @@ -114,7 +124,7 @@ describe('getUploadFormData', () => {
},
);

const result = await getUploadFormData({ request: mockRequest as Request }, { fileOptional: true });
const result = await getUploadFormData({ request: mockRequest }, { fileOptional: true });

expect(result).to.deep.include({
fieldname: 'fileField',
Expand All @@ -131,7 +141,7 @@ describe('getUploadFormData', () => {
const mockRequest = createMockRequest({ fieldName: 'fieldValue' });

try {
await getUploadFormData({ request: mockRequest as Request }, { fileOptional: false });
await getUploadFormData({ request: mockRequest }, { fileOptional: false });
throw new Error('Expected function to throw');
} catch (error) {
expect((error as Error).message).to.equal('[No file uploaded]');
Expand All @@ -141,7 +151,7 @@ describe('getUploadFormData', () => {
it('should return fields without errors when no file is uploaded but fileOptional is true', async () => {
const mockRequest = createMockRequest({ fieldName: 'fieldValue' }); // No file

const result = await getUploadFormData({ request: mockRequest as Request }, { fileOptional: true });
const result = await getUploadFormData({ request: mockRequest }, { fileOptional: true });

expect(result).to.deep.equal({
fields: { fieldName: 'fieldValue' },
Expand All @@ -167,7 +177,7 @@ describe('getUploadFormData', () => {

try {
await getUploadFormData(
{ request: mockRequest as Request },
{ request: mockRequest },
{ sizeLimit: 1024 * 1024 }, // 1 MB limit
);
throw new Error('Expected function to throw');
Expand Down
Loading
Loading