[pull] master from unlock-protocol:master

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

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()
-      ),
     })
   )
 }

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<string, unknown>)[propName]
+  if (
+    (propName === 'url' || propName === 'originalUrl') &&
+    typeof value === 'string'
+  ) {
+    return redactUrl(value)
+  }
+  if (propName === 'query' && value && typeof value === 'object') {
+    const query = { ...(value as Record<string, unknown>) }
+    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,
+}