feat(apps): ad-hoc redaction for apps logs (#40096)

Co-authored-by: Diego Sampaio <chinello@gmail.com>
pull/39944/merge
Douglas Gubert 3 weeks ago committed by GitHub
parent f4dfb8ddc2
commit 95a82f72dd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
  1. 5
      .changeset/grumpy-ligers-drum.md
  2. 5
      .changeset/smart-chicken-repair.md
  3. 5
      .changeset/tender-spies-give.md
  4. 3
      apps/meteor/app/api/server/middlewares/logger.ts
  5. 3
      apps/meteor/app/apps/server/bridges/http.ts
  6. 17
      apps/meteor/app/apps/server/bridges/persistence.ts
  7. 42
      apps/meteor/ee/server/apps/lib/redactor.ts
  8. 3
      apps/meteor/ee/server/apps/orchestrator.js
  9. 6
      apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts
  10. 2
      apps/meteor/package.json
  11. 12
      packages/logger/src/getPino.ts
  12. 10
      packages/logger/src/index.ts
  13. 9
      packages/server-fetch/src/index.ts
  14. 53
      packages/tools/src/censorUrl.spec.ts
  15. 45
      packages/tools/src/censorUrl.ts
  16. 1
      packages/tools/src/index.ts
  17. 16
      yarn.lock

@ -0,0 +1,5 @@
---
'@rocket.chat/server-fetch': minor
---
Introduces redaction of potentially sensitive data when logging request URLs

@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': minor
---
Introduces redaction of potentially sensitive data in logs related to apps-engine

@ -0,0 +1,5 @@
---
'@rocket.chat/tools': minor
---
Adds new function for censoring URL components in logs

@ -1,4 +1,5 @@
import type { Logger } from '@rocket.chat/logger';
import { censorUrl } from '@rocket.chat/tools';
import type { MiddlewareHandler } from 'hono';
import { getRestPayload } from '../../../../server/lib/logger/logPayloads';
@ -11,7 +12,7 @@ export const loggerMiddleware =
const log = logger.logger.child(
{
method: c.req.method,
url: c.req.url,
url: censorUrl(c.req.url),
userId: c.req.header('x-user-id'),
userAgent: c.req.header('user-agent'),
length: c.req.header('content-length'),

@ -3,6 +3,7 @@ import type { IHttpResponse } from '@rocket.chat/apps-engine/definition/accessor
import type { IHttpBridgeRequestInfo } from '@rocket.chat/apps-engine/server/bridges';
import { HttpBridge } from '@rocket.chat/apps-engine/server/bridges/HttpBridge';
import { serverFetch as fetch, type ExtendedFetchOptions } from '@rocket.chat/server-fetch';
import { censorUrl } from '@rocket.chat/tools';
import { settings } from '../../../settings/server';
@ -72,7 +73,7 @@ export class AppHttpBridge extends HttpBridge {
// end comptability with old HTTP.call API
this.orch.debugLog(`The App ${info.appId} is requesting from the outter webs:`, info);
this.orch.debugLog({ msg: `The App ${info.appId} is requesting from the outter webs:`, info: { ...info, url: censorUrl(info.url) } });
const shouldIgnoreSsrf = request.ssrfValidation !== true;
const fetchOptions: ExtendedFetchOptions = {

@ -15,7 +15,7 @@ export class AppPersistenceBridge extends PersistenceBridge {
}
protected async create(data: object, appId: string): Promise<string> {
this.orch.debugLog(`The App ${appId} is storing a new object in their persistence.`, data);
this.orch.debugLog(`The App ${appId} is storing a new object in their persistence.`);
if (typeof data !== 'object') {
throw new Error('Attempted to store an invalid data type, it must be an object.');
@ -28,11 +28,10 @@ export class AppPersistenceBridge extends PersistenceBridge {
}
protected async createWithAssociations(data: object, associations: Array<RocketChatAssociationRecord>, appId: string): Promise<string> {
this.orch.debugLog(
`The App ${appId} is storing a new object in their persistence that is associated with some models.`,
data,
this.orch.debugLog({
msg: `The App ${appId} is storing a new object in their persistence that is associated with some models.`,
associations,
);
});
if (typeof data !== 'object') {
throw new Error('Attempted to store an invalid data type, it must be an object.');
@ -53,7 +52,7 @@ export class AppPersistenceBridge extends PersistenceBridge {
}
protected async readByAssociations(associations: Array<RocketChatAssociationRecord>, appId: string): Promise<Array<object>> {
this.orch.debugLog(`The App ${appId} is searching for records that are associated with the following:`, associations);
this.orch.debugLog({ msg: `The App ${appId} is searching for records that are associated with the following:`, associations });
const records = await this.orch
.getPersistenceModel()
@ -84,7 +83,7 @@ export class AppPersistenceBridge extends PersistenceBridge {
associations: Array<RocketChatAssociationRecord>,
appId: string,
): Promise<Array<object> | undefined> {
this.orch.debugLog(`The App ${appId} is removing records with the following associations:`, associations);
this.orch.debugLog({ msg: `The App ${appId} is removing records with the following associations:`, associations });
const query = {
appId,
@ -105,7 +104,7 @@ export class AppPersistenceBridge extends PersistenceBridge {
}
protected async update(id: string, data: object, _upsert: boolean, appId: string): Promise<string> {
this.orch.debugLog(`The App ${appId} is updating the record "${id}" to:`, data);
this.orch.debugLog(`The App ${appId} is updating the record "${id}"`);
if (typeof data !== 'object') {
throw new Error('Attempted to store an invalid data type, it must be an object.');
@ -120,7 +119,7 @@ export class AppPersistenceBridge extends PersistenceBridge {
upsert = true,
appId: string,
): Promise<string> {
this.orch.debugLog(`The App ${appId} is updating the record with association to data as follows:`, associations, data);
this.orch.debugLog({ msg: `The App ${appId} is updating the record with association to data as follows:`, associations });
if (typeof data !== 'object') {
throw new Error('Attempted to store an invalid data type, it must be an object.');

@ -0,0 +1,42 @@
import fastRedact from 'fast-redact';
const requestFields = [
'headers.Cookie',
'headers.cookie',
'headers["x-auth-token"]',
'headers["X-Auth-Token"]',
'headers.auth',
'headers.Auth',
'headers.authorization',
'headers.Authorization',
'headers.access_token',
'content.password',
'content.pass',
'data.password',
'data.pass',
];
const entityFields = ['password', 'pass', 'customFields.*', '_unmappedProperties_'];
const roomFields = ['customFields.*', '_unmappedProperties_', ...entityFields.map((field) => `creator.${field}`)];
export const redactionFieldPaths = [
// Incoming requests to the Apps API endpoints
...requestFields,
...entityFields.map((field) => `user.${field}`),
'query.access_token',
'query.query', // The deprecated `query` search param
// Outgoing requests from the Apps to the outter webs
...requestFields.map((field) => `request.${field}`),
`request.query`, // `query` here is a string, so we have to redact it all
// Slashcommands
...roomFields.map((field) => `params[0].room.${field}`),
...entityFields.map((field) => `params[0].sender.${field}`),
];
export const redact = fastRedact({
paths: redactionFieldPaths,
censor: '[Redacted]',
serialize: false,
strict: false,
});

@ -10,6 +10,7 @@ import { AppLogs, Apps as AppsModel, AppsPersistence, Statistics } from '@rocket
import { Meteor } from 'meteor/meteor';
import { AppServerNotifier, AppsRestApi, AppUIKitInteractionApi } from './communication';
import { redactionFieldPaths } from './lib/redactor';
import { MarketplaceAPIClient } from './marketplace/MarketplaceAPIClient';
import { isTesting } from './marketplace/isTesting';
import { AppRealLogStorage, AppRealStorage, ConfigurableAppSourceStorage } from './storage';
@ -44,7 +45,7 @@ export class AppServerOrchestrator {
return;
}
this._rocketchatLogger = new Logger('Rocket.Chat Apps');
this._rocketchatLogger = new Logger('Rocket.Chat Apps', { redact: redactionFieldPaths });
this._model = AppsModel;
this._logModel = AppLogs;

@ -4,6 +4,8 @@ import { AppLogStorage } from '@rocket.chat/apps-engine/server/storage';
import { InstanceStatus } from '@rocket.chat/instance-status';
import type { AppLogs } from '@rocket.chat/models';
import { redact } from '../lib/redactor';
export class AppRealLogStorage extends AppLogStorage {
constructor(private db: typeof AppLogs) {
super('mongodb');
@ -41,6 +43,10 @@ export class AppRealLogStorage extends AppLogStorage {
async storeEntries(logEntry: ILoggerStorageEntry): Promise<ILoggerStorageEntry> {
logEntry.instanceId = InstanceStatus.id();
logEntry.entries.forEach((entry) => {
entry.args.forEach(redact);
});
const id = (await this.db.insertOne(logEntry)).insertedId;
return this.db.findOneById(id);

@ -204,6 +204,7 @@
"expiry-map": "^2.0.0",
"express": "^4.21.2",
"express-rate-limit": "^5.5.1",
"fast-redact": "^3.5.0",
"fastq": "^1.17.1",
"fflate": "^0.8.2",
"file-type": "^16.5.4",
@ -353,6 +354,7 @@
"@types/ejson": "^2.2.2",
"@types/express": "^4.17.25",
"@types/express-rate-limit": "^5.1.3",
"@types/fast-redact": "^3",
"@types/google-libphonenumber": "^7.4.30",
"@types/gravatar": "^1.8.6",
"@types/he": "^1.2.3",

@ -1,4 +1,4 @@
import { pino } from 'pino';
import { pino, type ChildLoggerOptions } from 'pino';
const infoLevel = process.env.LESS_INFO_LOGS ? 20 : 35;
@ -25,6 +25,12 @@ const mainPino = pino({
export type MainLogger = typeof mainPino;
export function getPino(name: string, level = 'warn'): MainLogger {
return mainPino.child({ name }, { level }) as MainLogger;
export type LoggerOptions = Pick<ChildLoggerOptions<keyof MainLogger['customLevels']>, 'level' | 'redact'>;
const defaultOptions: LoggerOptions = {
level: 'warn',
};
export function getPino(name: string, options: LoggerOptions = {}): MainLogger {
return mainPino.child({ name }, { ...defaultOptions, ...options });
}

@ -1,4 +1,4 @@
import { getPino, type MainLogger } from './getPino';
import { getPino, type LoggerOptions, type MainLogger } from './getPino';
import type { LogLevelSetting } from './logLevel';
import { logLevel } from './logLevel';
@ -27,16 +27,16 @@ logLevel.once('changed', (level: LogLevelSetting) => {
export class Logger {
readonly logger: MainLogger;
constructor(loggerLabel: string) {
this.logger = getPino(loggerLabel, defaultLevel);
constructor(loggerLabel: string, options: LoggerOptions = {}) {
this.logger = getPino(loggerLabel, { level: defaultLevel, ...options });
logLevel.on('changed', (level: LogLevelSetting) => {
this.logger.level = getLevel(level);
});
}
section(name: string): MainLogger {
const child = this.logger.child({ section: name }) as MainLogger;
section(name: string, options: LoggerOptions = {}): MainLogger {
const child = this.logger.child({ section: name }, { ...options });
logLevel.on('changed', (level: LogLevelSetting) => {
child.level = getLevel(level);

@ -2,6 +2,7 @@ import http from 'http';
import https from 'https';
import { Logger } from '@rocket.chat/logger';
import { censorUrl } from '@rocket.chat/tools';
import { AbortController } from 'abort-controller';
import { HttpProxyAgent } from 'http-proxy-agent';
import { HttpsProxyAgent } from 'https-proxy-agent';
@ -21,7 +22,7 @@ function getFetchAgent<U extends string>(
allowSelfSignedCerts?: boolean,
originalHostname?: string,
): http.Agent | https.Agent | null | HttpsProxyAgent<U> | HttpProxyAgent<U> {
const isHttps = /^https/.test(url);
const isHttps = url.startsWith('https');
const proxy = getProxyForUrl(url);
if (proxy) {
@ -67,7 +68,7 @@ async function getFetchAgentWithValidation<U extends string>(
if (!ignoreSsrfValidation) {
const ssrfResult = await checkForSsrfWithIp(url, allowList);
if (!ssrfResult.allowed) {
logger.error({ msg: 'SSRF validation failed for URL', url });
logger.error({ msg: 'SSRF validation failed for URL', url: censorUrl(url) });
throw new Error('error-ssrf-validation-failed');
}
@ -82,7 +83,7 @@ async function getFetchAgentWithValidation<U extends string>(
}
}
} else {
logger.debug({ msg: 'Request not using SSRF validation', url: pinnedUrl });
logger.debug({ msg: 'Request not using SSRF validation', url: censorUrl(pinnedUrl) });
}
return { agent: getFetchAgent(pinnedUrl, allowSelfSignedCerts, originalHostname), pinnedUrl, originalHostname, resolvedIp };
@ -108,7 +109,7 @@ function followRedirect(response: fetch.Response, redirectCount = 0) {
throw new Error('error-too-many-redirects');
}
logger.debug({ msg: 'Following redirect', redirectCount, location, status: response.status });
logger.debug({ msg: 'Following redirect', redirectCount, location: censorUrl(location), status: response.status });
return location;
}

@ -0,0 +1,53 @@
import { censorUrl } from './censorUrl';
describe('censorUrl', () => {
it('returns the original value when URL parsing fails', () => {
const input = 'not-a-url';
expect(censorUrl(input)).toBe(input);
});
it('returns relative URLs unchanged when no base is provided', () => {
const input = '/path/to/resource?query=secret&access_token=token';
expect(censorUrl(input)).toBe(input);
});
it('does not change URLs without sensitive parts', () => {
expect(censorUrl('https://example.com/path?foo=bar')).toBe('https://example.com/path?foo=bar');
});
it('redacts username and password from auth section', () => {
expect(censorUrl('https://user:password@example.com/path')).toBe('https://*Redacted*:*Redacted*@example.com/path');
});
it('redacts only username when password is not present', () => {
expect(censorUrl('https://user@example.com/path')).toBe('https://*Redacted*@example.com/path');
});
it('redacts query and access_token search params', () => {
expect(censorUrl('https://example.com/path?query=secret&access_token=token&foo=bar')).toBe(
'https://example.com/path?query=*Redacted*&access_token=*Redacted*&foo=bar',
);
});
it('redacts access_token even when query is absent', () => {
expect(censorUrl('https://example.com/path?access_token=token&foo=bar')).toBe(
'https://example.com/path?access_token=*Redacted*&foo=bar',
);
});
it('accepts URL objects as input', () => {
expect(censorUrl(new URL('https://user:password@example.com/path?query=secret'))).toBe(
'https://*Redacted*:*Redacted*@example.com/path?query=*Redacted*',
);
});
it('does not modify the original URL object', () => {
const input = new URL('https://user:password@example.com/path?query=secret&access_token=token');
const originalValue = input.toString();
expect(censorUrl(input)).toBe('https://*Redacted*:*Redacted*@example.com/path?query=*Redacted*&access_token=*Redacted*');
expect(input.toString()).toBe(originalValue);
});
});

@ -0,0 +1,45 @@
/**
* Redacts sensitive information from a URL string.
*
* This function parses a URL and replaces potentially sensitive data with placeholder text.
* It redacts the following URL components:
* - Username in the authentication section
* - Password in the authentication section
* - Query parameters named: 'query', 'access_token'
*
* Note: We use `*Redacted*` instead of `[Redacted]` for legibility, as `[` and `]` would be encoded by toString()
*
* @param url - The URL string to be censored
* @returns The URL string with sensitive information redacted, or the original URL if parsing fails
*
* @example
* ```ts
* censorUrl('https://user:password@example.com/path?query=secret&access_token=token');
* // Returns: 'https://*Redacted*:*Redacted*@example.com/path?query=*Redacted*&access_token=*Redacted*'
* ```
*/
export function censorUrl(url: string | URL): string {
try {
const parsedUrl = new URL(url);
if (parsedUrl.username) {
parsedUrl.username = '*Redacted*';
}
if (parsedUrl.password) {
parsedUrl.password = '*Redacted*';
}
if (parsedUrl.searchParams.has('query')) {
parsedUrl.searchParams.set('query', '*Redacted*');
}
if (parsedUrl.searchParams.has('access_token')) {
parsedUrl.searchParams.set('access_token', '*Redacted*');
}
return parsedUrl.toString();
} catch {
return url.toString();
}
}

@ -1,3 +1,4 @@
export * from './censorUrl';
export * from './convertSubObjectsIntoPaths';
export * from './convertPathsIntoSubObjects';
export * from './getObjectKeys';

@ -9912,6 +9912,7 @@ __metadata:
"@types/ejson": "npm:^2.2.2"
"@types/express": "npm:^4.17.25"
"@types/express-rate-limit": "npm:^5.1.3"
"@types/fast-redact": "npm:^3"
"@types/google-libphonenumber": "npm:^7.4.30"
"@types/gravatar": "npm:^1.8.6"
"@types/he": "npm:^1.2.3"
@ -10018,6 +10019,7 @@ __metadata:
express: "npm:^4.21.2"
express-rate-limit: "npm:^5.5.1"
fast-glob: "npm:^3.3.3"
fast-redact: "npm:^3.5.0"
fastq: "npm:^1.17.1"
fflate: "npm:^0.8.2"
file-type: "npm:^16.5.4"
@ -14167,6 +14169,13 @@ __metadata:
languageName: node
linkType: hard
"@types/fast-redact@npm:^3":
version: 3.0.4
resolution: "@types/fast-redact@npm:3.0.4"
checksum: 10/d130c7c2d322e761c65ec8ecd821bf56e132672cd9de1703a53a65e051d2f43af6062040d3f0afdd48b364e1e4b29b858a748517ba9a6a7f1dab12ce7000103a
languageName: node
linkType: hard
"@types/fibers@npm:^3.1.4":
version: 3.1.4
resolution: "@types/fibers@npm:3.1.4"
@ -22305,6 +22314,13 @@ __metadata:
languageName: node
linkType: hard
"fast-redact@npm:^3.5.0":
version: 3.5.0
resolution: "fast-redact@npm:3.5.0"
checksum: 10/24b27e2023bd5a62f908d97a753b1adb8d89206b260f97727728e00b693197dea2fc2aa3711147a385d0ec6e713569fd533df37a4ef947e08cb65af3019c7ad5
languageName: node
linkType: hard
"fast-safe-stringify@npm:^2.1.1":
version: 2.1.1
resolution: "fast-safe-stringify@npm:2.1.1"

Loading…
Cancel
Save