diff --git a/.changeset/grumpy-ligers-drum.md b/.changeset/grumpy-ligers-drum.md new file mode 100644 index 00000000000..43d7caa24e9 --- /dev/null +++ b/.changeset/grumpy-ligers-drum.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/server-fetch': minor +--- + +Introduces redaction of potentially sensitive data when logging request URLs diff --git a/.changeset/smart-chicken-repair.md b/.changeset/smart-chicken-repair.md new file mode 100644 index 00000000000..9c9b522563e --- /dev/null +++ b/.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 diff --git a/.changeset/tender-spies-give.md b/.changeset/tender-spies-give.md new file mode 100644 index 00000000000..e7f729f448c --- /dev/null +++ b/.changeset/tender-spies-give.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/tools': minor +--- + +Adds new function for censoring URL components in logs diff --git a/apps/meteor/app/api/server/middlewares/logger.ts b/apps/meteor/app/api/server/middlewares/logger.ts index 6c56de6cfce..17759687390 100644 --- a/apps/meteor/app/api/server/middlewares/logger.ts +++ b/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'), diff --git a/apps/meteor/app/apps/server/bridges/http.ts b/apps/meteor/app/apps/server/bridges/http.ts index aab0d56d301..cd297009c4a 100644 --- a/apps/meteor/app/apps/server/bridges/http.ts +++ b/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 = { diff --git a/apps/meteor/app/apps/server/bridges/persistence.ts b/apps/meteor/app/apps/server/bridges/persistence.ts index 857f6a561ed..e1dedd270ac 100644 --- a/apps/meteor/app/apps/server/bridges/persistence.ts +++ b/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 { - 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, appId: string): Promise { - 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, appId: string): Promise> { - 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, appId: string, ): Promise | 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 { - 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 { - 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.'); diff --git a/apps/meteor/ee/server/apps/lib/redactor.ts b/apps/meteor/ee/server/apps/lib/redactor.ts new file mode 100644 index 00000000000..c1249ac7f42 --- /dev/null +++ b/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, +}); diff --git a/apps/meteor/ee/server/apps/orchestrator.js b/apps/meteor/ee/server/apps/orchestrator.js index 7188695ac3f..919f695bedc 100644 --- a/apps/meteor/ee/server/apps/orchestrator.js +++ b/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; diff --git a/apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts b/apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts index 62f61699f2e..e86bafe7afe 100644 --- a/apps/meteor/ee/server/apps/storage/AppRealLogStorage.ts +++ b/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 { 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); diff --git a/apps/meteor/package.json b/apps/meteor/package.json index ede2b121c42..96a7d47676e 100644 --- a/apps/meteor/package.json +++ b/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", diff --git a/packages/logger/src/getPino.ts b/packages/logger/src/getPino.ts index c9727b08467..14cbacb53af 100644 --- a/packages/logger/src/getPino.ts +++ b/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, 'level' | 'redact'>; + +const defaultOptions: LoggerOptions = { + level: 'warn', +}; + +export function getPino(name: string, options: LoggerOptions = {}): MainLogger { + return mainPino.child({ name }, { ...defaultOptions, ...options }); } diff --git a/packages/logger/src/index.ts b/packages/logger/src/index.ts index b731cb09824..1e7b33489c6 100644 --- a/packages/logger/src/index.ts +++ b/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); diff --git a/packages/server-fetch/src/index.ts b/packages/server-fetch/src/index.ts index d147761ef1d..2355d2a1b47 100644 --- a/packages/server-fetch/src/index.ts +++ b/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( allowSelfSignedCerts?: boolean, originalHostname?: string, ): http.Agent | https.Agent | null | HttpsProxyAgent | HttpProxyAgent { - const isHttps = /^https/.test(url); + const isHttps = url.startsWith('https'); const proxy = getProxyForUrl(url); if (proxy) { @@ -67,7 +68,7 @@ async function getFetchAgentWithValidation( 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( } } } 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; } diff --git a/packages/tools/src/censorUrl.spec.ts b/packages/tools/src/censorUrl.spec.ts new file mode 100644 index 00000000000..0fe51046a58 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/tools/src/censorUrl.ts b/packages/tools/src/censorUrl.ts new file mode 100644 index 00000000000..0059e359f77 --- /dev/null +++ b/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(); + } +} diff --git a/packages/tools/src/index.ts b/packages/tools/src/index.ts index ca7406644c6..c8df164de85 100644 --- a/packages/tools/src/index.ts +++ b/packages/tools/src/index.ts @@ -1,3 +1,4 @@ +export * from './censorUrl'; export * from './convertSubObjectsIntoPaths'; export * from './convertPathsIntoSubObjects'; export * from './getObjectKeys'; diff --git a/yarn.lock b/yarn.lock index ba967915f20..605b1a56dbb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -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"