diff --git a/locksmith/__tests__/utils/requestLogger.test.ts b/locksmith/__tests__/utils/requestLogger.test.ts new file mode 100644 index 00000000000..7533f71cb23 --- /dev/null +++ b/locksmith/__tests__/utils/requestLogger.test.ts @@ -0,0 +1,101 @@ +// ABOUTME: Ensures the HTTP request/error loggers never write credentials +// ABOUTME: (session tokens, cookies, API keys) into the log stream. +import express from 'express' +import expressWinston from 'express-winston' +import request from 'supertest' +import { Writable } from 'stream' +import winston from 'winston' +import { describe, expect, it } from 'vitest' +import { + requestLoggerOptions, + errorLoggerOptions, +} from '../../src/utils/requestLogger' + +// Collects every log line written through winston as a string. +class CaptureTransport extends winston.transports.Stream { + entries: string[] = [] + constructor() { + const entries: string[] = [] + super({ + stream: new Writable({ + write(chunk, _encoding, callback) { + entries.push(chunk.toString()) + callback() + }, + }), + format: winston.format.json(), + }) + this.entries = entries + } +} + +const SECRETS = ['session-token-abc', 'cookie-value-xyz', 'api-key-123'] + +const buildApp = (capture: CaptureTransport, fail = false) => { + const app = express() + app.use( + expressWinston.logger({ + ...requestLoggerOptions, + transports: [capture], + }) + ) + app.get('/ping', (_req, res, next) => { + if (fail) { + next(new Error('boom')) + return + } + res.send('pong') + }) + app.use( + expressWinston.errorLogger({ + ...errorLoggerOptions, + transports: [capture], + }) + ) + // Explicit terminal handler so the test does not depend on Express's + // default error handling to end the response. + app.use( + ( + _err: Error, + _req: express.Request, + res: express.Response, + _next: express.NextFunction + ) => { + res.status(500).send('error') + } + ) + return app +} + +const send = (app: express.Express) => + request(app) + .get('/ping?api-key=api-key-123&chain=8453') + .set('authorization', 'Bearer session-token-abc') + .set('cookie', 'session=cookie-value-xyz') + .set('user-agent', 'test-agent') + +describe('request logger redaction', () => { + it('logs requests without credentials but keeps useful metadata', async () => { + const capture = new CaptureTransport() + await send(buildApp(capture)).expect(200) + + expect(capture.entries).toHaveLength(1) + const entry = capture.entries[0] + for (const secret of SECRETS) { + expect(entry).not.toContain(secret) + } + expect(entry).toContain('test-agent') + expect(entry).toContain('"chain":"8453"') + }) + + it('logs errors without credentials', { timeout: 10_000 }, async () => { + const capture = new CaptureTransport() + await send(buildApp(capture, true)).expect(500) + + const errorEntry = capture.entries.find((e) => e.includes('boom')) + expect(errorEntry).toBeDefined() + for (const secret of SECRETS) { + expect(errorEntry).not.toContain(secret) + } + }) +}) diff --git a/locksmith/src/app.ts b/locksmith/src/app.ts index a99de87a5de..dbbb813fd1d 100644 --- a/locksmith/src/app.ts +++ b/locksmith/src/app.ts @@ -7,6 +7,7 @@ import * as Sentry from '@sentry/node' import cookieParser from 'cookie-parser' import router from './routes' import { errorHandler } from './utils/middlewares/error' +import { requestLoggerOptions, errorLoggerOptions } from './utils/requestLogger' import timeout from 'connect-timeout' import config from './config/config' import logger from './logger' @@ -37,15 +38,8 @@ app.use(express.json({ limit: '5mb' })) if ('test' !== process.env?.NODE_ENV) { app.use( expressWinston.logger({ + ...requestLoggerOptions, transports: logger.transports, - format: winston.format.combine( - winston.format.colorize(), - winston.format.json() - ), - meta: true, // optional: control whether you want to log the meta data about the request (default to true) - msg: 'HTTP {{req.method}} {{req.url}}', // optional: customize the default logging message. E.g. "{{res.statusCode}} {{req.method}} {{res.responseTime}}ms {{req.url}}" - expressFormat: true, // Use the default Express/morgan request formatting. Enabling this will override any msg if true. Will only output colors with colorize set to true - colorize: false, // Color the text and status code, using the Express/morgan color palette (text: gray, status: default green, 3XX cyan, 4XX yellow, 5XX red). }) ) } @@ -59,11 +53,8 @@ Sentry.setupExpressErrorHandler(app) if ('test' !== process.env?.NODE_ENV) { app.use( expressWinston.errorLogger({ + ...errorLoggerOptions, transports: [new winston.transports.Console()], - format: winston.format.combine( - winston.format.colorize(), - winston.format.json() - ), }) ) } diff --git a/locksmith/src/utils/requestLogger.ts b/locksmith/src/utils/requestLogger.ts new file mode 100644 index 00000000000..03061f0de6e --- /dev/null +++ b/locksmith/src/utils/requestLogger.ts @@ -0,0 +1,68 @@ +// ABOUTME: Shared express-winston options for HTTP request and error logging. +// ABOUTME: Strips credentials (session tokens, cookies, API keys) from log meta. +import winston from 'winston' +import type { Request } from 'express' + +// Headers that carry credentials and must never reach the log stream. +const headerBlacklist = ['authorization', 'cookie'] + +// Query parameters that carry credentials. +const secretQueryParams = ['api-key'] + +// Replaces the value of secret query parameters in a request URL. +const redactUrl = (url: string) => { + const parsed = new URL(url, 'http://localhost') + let changed = false + for (const param of secretQueryParams) { + if (parsed.searchParams.has(param)) { + parsed.searchParams.set(param, '[REDACTED]') + changed = true + } + } + return changed ? `${parsed.pathname}${parsed.search}` : url +} + +// express-winston calls this for every whitelisted request property; the +// return value is what gets logged. +const requestFilter = (req: Request, propName: string) => { + const value = (req as unknown as Record)[propName] + if ( + (propName === 'url' || propName === 'originalUrl') && + typeof value === 'string' + ) { + return redactUrl(value) + } + if (propName === 'query' && value && typeof value === 'object') { + const query = { ...(value as Record) } + for (const param of secretQueryParams) { + if (param in query) { + query[param] = '[REDACTED]' + } + } + return query + } + return value +} + +const format = winston.format.combine( + winston.format.colorize(), + winston.format.json() +) + +export const requestLoggerOptions = { + format, + meta: true, + // Message built from req.path rather than req.url so that query-string + // credentials never appear in the log line either. + msg: '{{req.method}} {{req.path}} {{res.statusCode}} {{res.responseTime}}ms', + expressFormat: false, + colorize: false, + headerBlacklist, + requestFilter, +} + +export const errorLoggerOptions = { + format, + headerBlacklist, + requestFilter, +}