Skip to content

chore: api http router improvements #38227

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

Open
d-gubert wants to merge 6 commits into
base: chore/improve-livechat-customfield-endpoint
Choose a base branch
from
Open

chore: api http router improvements #38227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e8e1db7
fix: schema definition POSTLivechatSaveCustomFieldsSchema
d-gubert Jan 20, 2026
6e8ff92
refactor: parseBodyParams usage and types
d-gubert Jan 16, 2026
1f578b7
refactor: defer payload parsing to getRestPayload
d-gubert Jan 16, 2026
85fde0c
feat: add redaction to logger middleware
d-gubert Jan 16, 2026
9068fca
fix: validatorFn mutates its parameter, pass clone instead
d-gubert Jan 17, 2026
20ecbe8
revert: cloning data shoves real problems under the rug
d-gubert Jan 20, 2026
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
39 changes: 16 additions & 23 deletions apps/meteor/app/api/server/middlewares/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,22 @@ 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: ['password'],
},
Comment on lines +23 to +25
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the logger.ts file around the specified lines
cat -n apps/meteor/app/api/server/middlewares/logger.ts | head -40

Repository: RocketChat/Rocket.Chat

Length of output: 1182

🏁 Script executed:

# Also check if getRestPayload function shows what fields are in the payload
fd -t f "getRestPayload" --exec rg -l "getRestPayload" {} \;

Repository: RocketChat/Rocket.Chat

Length of output: 48

🌐 Web query:

pino logger redact option nested paths syntax documentation

💡 Result:

In Pino, the redact option takes an array (or { paths, censor, remove }) where each entry in paths is a string path written in JavaScript dot/bracket notation, with support for arrays and * wildcards. [2][1]

Basic nested paths (dot / bracket)

  • Dot notation: a.b.c [1]
  • Bracket notation (useful for keys with dashes/spaces/case): a["b-c"].d [1]
  • Paths can even start with bracket notation: ["a-b"].c [1]

Arrays

  • Index: users[0].password (redact first element’s password) [3]
  • “All elements”: stuff.thats[*].secret [1]

Wildcards (*)

  • Redact all keys under an object: a.b.* [1]
  • Redact a field for all items: users.*.password [4]

Example

const logger = require('pino')({
  redact: {
    paths: [
      'user.password',
      'headers["x-api-key"]',
      'stuff.thats[*].secret',
      'users.*.password',
      'a.b.*'
    ],
    censor: '[Redacted]',  // or a function
    remove: false          // true removes keys instead of replacing values
  }
})

Path syntax is shared with Pino’s underlying redaction engine (fast-redact). [1][3]

Sources: Pino redaction docs (Path Syntax + examples) [1], Pino redact API option docs [2], fast-redact path syntax details [3], @pinojs/redact path/wildcard examples [4].

🏁 Script executed:

# Find and examine getRestPayload implementation
fd -t f "logPayloads" --exec cat -n {} \;

Repository: RocketChat/Rocket.Chat

Length of output: 1639

🏁 Script executed:

# Also search for any test files or usage of getRestPayload to understand the payload structure
rg "getRestPayload" -A 5 -B 2 apps/meteor/app/api/server

Repository: RocketChat/Rocket.Chat

Length of output: 1328

Use nested path to properly redact passwords: redact: ['payload.password']

The payload is wrapped in a payload key, so top-level redact: ['password'] won't catch nested passwords. Use ['payload.password'] instead to redact passwords from the JSON body.

🤖 Prompt for AI Agents 
In `@apps/meteor/app/api/server/middlewares/logger.ts` around lines 23 - 25, The
logger middleware currently uses redact: ['password'] which won't match nested
fields inside the request body; update the redact configuration to target the
nested payload key (use redact: ['payload.password']) in the logger/middleware
configuration so that any password inside the payload object is properly
redacted; locate the redact property in the logger middleware file
(apps/meteor/app/api/server/middlewares/logger.ts) and replace the array entry
'password' with 'payload.password'.

);

await next();

Expand Down
18 changes: 7 additions & 11 deletions apps/meteor/app/api/server/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,17 @@ import type { TypedOptions } from './definition';
type HonoContext = Context<{
Bindings: { incoming: IncomingMessage };
Variables: {
'remoteAddress': string;
'bodyParams-override'?: Record<string, any>;
remoteAddress: string;
bodyParams: Record<string, unknown>;
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;
Expand All @@ -37,19 +38,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 = await this.parseBodyParams<{ bodyParamsOverride: Record<string, any> }>({
request: req,
extra: { bodyParamsOverride: c.var['bodyParams-override'] || {} },
});

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'),
request,
path: req.path,
response: res,
Expand Down
2 changes: 0 additions & 2 deletions apps/meteor/app/integrations/server/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,14 +403,12 @@ const middleware = async (c: Context, next: Next): Promise<void> => {

if (body.payload) {
// need to compose the full payload in this weird way because body-parser thought it was a form
c.set('bodyParams-override', JSON.parse(body.payload));
return next();
}
incomingLogger.debug({
msg: 'Body received as application/x-www-form-urlencoded without the "payload" key, parsed as string',
content,
});
c.set('bodyParams-override', JSON.parse(content));
} catch (e: any) {
c.body(JSON.stringify({ success: false, error: e.message }), 400);
}
Expand Down
4 changes: 0 additions & 4 deletions apps/meteor/app/livechat/server/api/v1/customField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
19 changes: 15 additions & 4 deletions apps/meteor/server/lib/logger/logPayloads.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -23,7 +25,16 @@ 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]' };
}

return {
payload: await request.raw
.clone()
.json()
.catch(() => null),
};
};
59 changes: 23 additions & 36 deletions packages/http-router/src/Router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand All @@ -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 });

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -439,11 +430,7 @@ export class Router<
return router;
}

getHonoRouter(): Hono<{
Variables: {
remoteAddress: string;
};
}> {
getHonoRouter(): InnerRouter {
return this.innerRouter;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/model-typings/src/models/ILivechatCustomFieldModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export interface ILivechatCustomFieldModel extends IBaseModel<ILivechatCustomFie
options?: FindOptions<ILivechatCustomField>,
): FindCursor<ILivechatCustomField>;
createOrUpdateCustomField(
_id: string,
_id: string | null,
field: string,
label: ILivechatCustomField['label'],
scope: ILivechatCustomField['scope'],
Expand Down
2 changes: 1 addition & 1 deletion packages/models/src/models/LivechatCustomField.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export class LivechatCustomFieldRaw extends BaseRaw<ILivechatCustomField> implem
}

async createOrUpdateCustomField(
_id: string,
_id: string | null,
field: string,
label: ILivechatCustomField['label'],
scope: ILivechatCustomField['scope'],
Expand Down
5 changes: 4 additions & 1 deletion packages/rest-typings/src/v1/omnichannel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4445,12 +4445,15 @@ const POSTLivechatSaveCustomFieldsSchema = {
properties: {
customFieldId: {
type: 'string',
pattern: '^[0-9a-zA-Z_-]+$',
nullable: true,
},
customFieldData: {
type: 'object',
properties: {
field: {
type: 'string',
pattern: '^[0-9a-zA-Z_-]+$',
},
label: {
type: 'string',
Expand Down Expand Up @@ -4497,7 +4500,7 @@ const POSTLivechatSaveCustomFieldsSchema = {
};

export const isPOSTLivechatSaveCustomFieldsParams = ajv.compile<{
customFieldId: string;
customFieldId: string | null;
customFieldData: Omit<ILivechatCustomField, '_id' | '_updatedAt'> & { field: string };
}>(POSTLivechatSaveCustomFieldsSchema);

Expand Down
Loading