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

.changeset/grumpy-ligers-drum.md

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

.changeset/smart-chicken-repair.md

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

.changeset/tender-spies-give.md

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

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

@@ -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'),

apps/meteor/app/apps/server/bridges/http.ts

@@ -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 = {

apps/meteor/app/apps/server/bridges/persistence.ts

@@ -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.');

apps/meteor/ee/server/apps/lib/redactor.ts

@@ -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,
+});

apps/meteor/ee/server/apps/orchestrator.js

@@ -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;

apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts

@@ -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);

apps/meteor/package.json

@@ -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",

packages/logger/src/getPino.ts

@@ -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 });
 }

packages/logger/src/index.ts

@@ -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);

packages/server-fetch/src/index.ts

@@ -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;
 }
 

packages/tools/src/censorUrl.spec.ts

@@ -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);
+	});
+});

packages/tools/src/censorUrl.ts

@@ -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();
+	}
+}