chore: api http router improvements (#38227)

Author: d-gubert

apps/meteor/app/api/server/middlewares/logger.ts

@@ -8,29 +8,24 @@ export const loggerMiddleware =
 	async (c, next) => {
 		const startTime = Date.now();
 
-		let payload = {};
-
-		// We don't want to consume the request body stream for multipart requests
-		if (!c.req.header('content-type')?.includes('multipart/form-data')) {
-			try {
-				payload = await c.req.raw.clone().json();
-				// eslint-disable-next-line no-empty
-			} catch {}
-		} else {
-			payload = '[multipart/form-data]';
-		}
-
-		const log = logger.logger.child({
-			method: c.req.method,
-			url: c.req.url,
-			userId: c.req.header('x-user-id'),
-			userAgent: c.req.header('user-agent'),
-			length: c.req.header('content-length'),
-			host: c.req.header('host'),
-			referer: c.req.header('referer'),
-			remoteIP: c.get('remoteAddress'),
-			...(['POST', 'PUT', 'PATCH', 'DELETE'].includes(c.req.method) && getRestPayload(payload)),
-		});
+		const log = logger.logger.child(
+			{
+				method: c.req.method,
+				url: c.req.url,
+				userId: c.req.header('x-user-id'),
+				userAgent: c.req.header('user-agent'),
+				length: c.req.header('content-length'),
+				host: c.req.header('host'),
+				referer: c.req.header('referer'),
+				remoteIP: c.get('remoteAddress'),
+				...(['POST', 'PUT', 'PATCH', 'DELETE'].includes(c.req.method) && (await getRestPayload(c.req))),
+			},
+			{
+				redact: [
+					'payload.password', // Potentially logged by v1/login
+				],
+			},
+		);
 
 		await next();
 

apps/meteor/app/api/server/router.ts

@@ -10,15 +10,17 @@ type HonoContext = Context<{
 	Bindings: { incoming: IncomingMessage };
 	Variables: {
 		'remoteAddress': string;
-		'bodyParams-override'?: Record<string, any>;
+		'bodyParams': Record<string, unknown>;
+		'bodyParams-override': Record<string, unknown> | undefined;
+		'queryParams': Record<string, unknown>;
 	};
 }>;
 
 export type APIActionContext = {
 	requestIp: string;
 	urlParams: Record<string, string>;
-	queryParams: Record<string, any>;
-	bodyParams: Record<string, any>;
+	queryParams: Record<string, unknown>;
+	bodyParams: Record<string, unknown>;
 	request: Request;
 	path: string;
 	response: any;
@@ -37,16 +39,14 @@ export class RocketChatAPIRouter<
 	protected override convertActionToHandler(action: APIActionHandler): (c: HonoContext) => Promise<ResponseSchema<TypedOptions>> {
 		return async (c: HonoContext): Promise<ResponseSchema<TypedOptions>> => {
 			const { req, res } = c;
-			const queryParams = this.parseQueryParams(req);
-			const bodyParams = c.get('bodyParams-override') ?? (await this.parseBodyParams({ request: req }));
 
 			const request = req.raw.clone();
 
 			const context: APIActionContext = {
 				requestIp: c.get('remoteAddress'),
 				urlParams: req.param(),
-				queryParams,
-				bodyParams,
+				queryParams: c.get('queryParams'),
+				bodyParams: c.get('bodyParams-override') || c.get('bodyParams'),
 				request,
 				path: req.path,
 				response: res,

apps/meteor/app/livechat/server/api/v1/customField.ts

@@ -111,10 +111,6 @@ const livechatCustomFieldsEndpoints = API.v1
 		async function action() {
 			const { customFieldId, customFieldData } = this.bodyParams;
 
-			if (!/^[0-9a-zA-Z-_]+$/.test(customFieldId)) {
-				return API.v1.failure('Invalid custom field name. Use only letters, numbers, hyphens and underscores.');
-			}
-
 			if (customFieldId) {
 				const customField = await LivechatCustomField.findOneById(customFieldId);
 				if (!customField) {

apps/meteor/server/lib/logger/logPayloads.ts

@@ -1,3 +1,5 @@
+import type { HonoRequest } from 'hono';
+
 import { omit } from '../../../lib/utils/omit';
 
 const { LOG_METHOD_PAYLOAD = 'false', LOG_REST_PAYLOAD = 'false', LOG_REST_METHOD_PAYLOADS = 'false' } = process.env;
@@ -23,7 +25,17 @@ export const getMethodArgs =
 
 export const getRestPayload =
 	LOG_REST_PAYLOAD === 'false' && LOG_REST_METHOD_PAYLOADS === 'false'
-		? (): null => null
-		: (payload: unknown): { payload: unknown } => ({
-				payload,
-			});
+		? (): Promise<null> => Promise.resolve(null)
+		: async (request: HonoRequest): Promise<{ payload: unknown } | null> => {
+				if (request.header('content-type')?.includes('multipart/form-data')) {
+					return { payload: '[multipart/form-data]' };
+				}
+
+				try {
+					return {
+						payload: await request.raw.clone().json(),
+					};
+				} catch {
+					return { payload: null };
+				}
+			};

packages/http-router/src/Router.ts

@@ -78,18 +78,22 @@ export abstract class AbstractRouter<TActionCallback = (c: Context) => Promise<R
 	protected abstract convertActionToHandler(action: TActionCallback): (c: Context) => Promise<ResponseSchema<TypedOptions>>;
 }
 
+type InnerRouter = Hono<{
+	Variables: {
+		remoteAddress: string;
+		bodyParams: Record<string, unknown>;
+		queryParams: Record<string, unknown>;
+	};
+}>;
+
 export class Router<
 	TBasePath extends string,
 	TOperations extends {
 		[x: string]: unknown;
 	} = NonNullable<unknown>,
 	TActionCallback = (c: Context) => Promise<ResponseSchema<TypedOptions>>,
 > extends AbstractRouter<TActionCallback> {
-	protected innerRouter: Hono<{
-		Variables: {
-			remoteAddress: string;
-		};
-	}>;
+	protected innerRouter: InnerRouter;
 
 	constructor(readonly base: TBasePath) {
 		super();
@@ -150,39 +154,24 @@ export class Router<
 		};
 	}
 
-	protected async parseBodyParams<T extends Record<string, any>>({ request }: { request: HonoRequest; extra?: T }) {
+	protected async parseBodyParams({ request }: { request: HonoRequest }): Promise<NonNullable<unknown>> {
 		try {
-			let parsedBody = {};
-			const contentType = request.header('content-type');
+			const contentType = request.header('content-type') || '';
 
-			if (contentType?.includes('multipart/form-data')) {
-				// Don't parse multipart here, routes handle it manually via UploadService.parse()
-				// since multipart/form-data is only used for file uploads
-				return parsedBody;
+			if (contentType.includes('application/json')) {
+				return await request.raw.clone().json();
 			}
 
-			if (contentType?.includes('application/json')) {
-				parsedBody = await request.raw.clone().json();
-			} else if (contentType?.includes('application/x-www-form-urlencoded')) {
+			if (contentType.includes('application/x-www-form-urlencoded')) {
 				const req = await request.raw.clone().formData();
-				parsedBody = Object.fromEntries(req.entries());
-			} else {
-				parsedBody = await request.raw.clone().text();
-			}
-			// This is necessary to keep the compatibility with the previous version, otherwise the bodyParams will be an empty string when no content-type is sent
-			if (parsedBody === '') {
-				return {};
+				return Object.fromEntries(req.entries());
 			}
 
-			if (Array.isArray(parsedBody)) {
-				return parsedBody;
-			}
-
-			return { ...parsedBody };
-			// eslint-disable-next-line no-empty
-		} catch {}
-
-		return {};
+			return {};
+		} catch {
+			// No problem if there is error, just means the endpoint is going to have to parse the body itself if necessary
+			return {};
+		}
 	}
 
 	protected parseQueryParams(request: HonoRequest) {
@@ -204,6 +193,7 @@ export class Router<
 			let queryParams: Record<string, any>;
 			try {
 				queryParams = this.parseQueryParams(req);
+				c.set('queryParams', queryParams);
 			} catch (e) {
 				logger.warn({ msg: 'Error parsing query params for request', path: req.path, err: e });
 
@@ -233,6 +223,7 @@ export class Router<
 			}
 
 			const bodyParams = await this.parseBodyParams({ request: req });
+			c.set('bodyParams', bodyParams);
 
 			if (options.body) {
 				const validatorFn = options.body;
@@ -439,11 +430,7 @@ export class Router<
 		return router;
 	}
 
-	getHonoRouter(): Hono<{
-		Variables: {
-			remoteAddress: string;
-		};
-	}> {
+	getHonoRouter(): InnerRouter {
 		return this.innerRouter;
 	}
 }