Security/error response pii guard (#924)

* feat: vault lifecycle audit timeline endpoint (#723)

* security: sanitize PII and internals from error responses (#879)

---------

Co-authored-by: whitezaddy <austinihueze@gmail.com>

Author: Tekprecious

docs/error-contract.md

@@ -19,40 +19,48 @@ All API errors return a consistent JSON envelope with the following structure:
 
 ### Field Descriptions
 
-| Field | Type | Required | Description |
-|-------|------|----------|-------------|
-| `code` | string | Yes | Machine-readable error code for programmatic handling |
-| `message` | string | Yes | Human-readable error description |
-| `details` | object | No | Additional context (e.g., validation field errors). Only present for validation errors. |
-| `requestId` | string | No | Echoed from `x-request-id` header for request correlation |
+| Field       | Type   | Required | Description                                                                             |
+| ----------- | ------ | -------- | --------------------------------------------------------------------------------------- |
+| `code`      | string | Yes      | Machine-readable error code for programmatic handling                                   |
+| `message`   | string | Yes      | Human-readable error description                                                        |
+| `details`   | object | No       | Additional context (e.g., validation field errors). Only present for validation errors. |
+| `requestId` | string | No       | Echoed from `x-request-id` header for request correlation                               |
 
 ## Error Codes
 
 The following stable error codes are used across the API:
 
 ### 400 Bad Request
+
 - `BAD_REQUEST` - Generic bad request (malformed syntax, invalid parameters)
 - `VALIDATION_ERROR` - Request validation failed (includes field-level details)
 
 ### 401 Unauthorized
+
 - `UNAUTHORIZED` - Authentication required or failed
 
 ### 403 Forbidden
+
 - `FORBIDDEN` - Authenticated but not authorized for this resource
 
 ### 404 Not Found
+
 - `NOT_FOUND` - Resource does not exist
 
 ### 409 Conflict
+
 - `CONFLICT` - Resource conflict (e.g., duplicate entry)
 
 ### 422 Unprocessable Entity
+
 - `UNPROCESSABLE` - Business logic violation (e.g., cannot delete last admin)
 
 ### 429 Too Many Requests
+
 - `RATE_LIMITED` - Rate limit exceeded
 
 ### 500 Internal Server Error
+
 - `INTERNAL_ERROR` - Unexpected server error (safe message, no stack traces)
 
 ## Example Error Responses
@@ -171,16 +179,26 @@ Examples:
 
 ## Security Considerations
 
+### PII and Internal Detail Sanitization
+
+In production environments (`NODE_ENV=production`), all error responses are automatically sanitized to prevent the leakage of sensitive information. This includes:
+
+- **Internal Error Details**: For `500 Internal Server Error` responses, the original error message and stack trace are logged internally but are **never** included in the JSON response body. The client receives a generic "Internal server error" message.
+- **PII Redaction**: For validation errors (HTTP 400) that might echo back parts of the request payload in the `details` field, any values matching the PII taxonomy (e.g., email addresses, wallet addresses, sensitive keys) are automatically redacted.
+- **Correlation ID**: The `requestId` is always preserved, allowing for secure error correlation between the client and server-side logs.
+
+In non-production environments, error messages may contain more detail to aid in debugging.
+
 1. **No Stack Traces**: Stack traces are never exposed in production error responses
 2. **No Secrets**: Error messages never include tokens, API keys, database credentials, or raw SQL
 3. **Safe Messages**: Internal errors return generic "Internal server error" messages to prevent information leakage
-4. **Structured Logging**: Errors are logged server-side with full context for debugging
 
 ## Client Integration Guidelines
 
 ### Handling Errors
 
 Clients should:
+
 1. Check the HTTP status code first
 2. Use the `code` field for programmatic error handling (not the message)
 3. Display the `message` field to users
@@ -196,40 +214,40 @@ interface ApiError {
     message: string;
     details?: Record<string, unknown>;
     requestId?: string;
-  }
+  };
 }
 
 async function apiCall(): Promise<void> {
-  const response = await fetch('/api/resource', {
+  const response = await fetch("/api/resource", {
     headers: {
-      'x-request-id': generateRequestId() // For traceability
-    }
+      "x-request-id": generateRequestId(), // For traceability
+    },
   });
-  
+
   if (!response.ok) {
     const error: ApiError = await response.json();
-    
+
     // Handle by code, not message
     switch (error.error.code) {
-      case 'VALIDATION_ERROR':
+      case "VALIDATION_ERROR":
         // Show field-level errors
         showValidationErrors(error.error.details);
         break;
-      case 'UNAUTHORIZED':
+      case "UNAUTHORIZED":
         // Redirect to login
         redirectToLogin();
         break;
-      case 'NOT_FOUND':
+      case "NOT_FOUND":
         // Show 404 page
         showNotFound();
         break;
       default:
         // Generic error display
         showError(error.error.message);
     }
-    
+
     // Log requestId for support
-    console.error('Request ID:', error.error.requestId);
+    console.error("Request ID:", error.error.requestId);
   }
 }
 ```
@@ -239,28 +257,28 @@ async function apiCall(): Promise<void> {
 Route handlers use `AppError` factory methods for consistent errors:
 
 ```typescript
-import { AppError } from '../middleware/errorHandler.js';
+import { AppError } from "../middleware/errorHandler.js";
 
 // Validation error with details
-return next(AppError.validation('Invalid input', { field: 'email' }));
+return next(AppError.validation("Invalid input", { field: "email" }));
 
 // Simple bad request
-return next(AppError.badRequest('Missing required field'));
+return next(AppError.badRequest("Missing required field"));
 
 // Authentication required
-return next(AppError.unauthorized('Invalid token'));
+return next(AppError.unauthorized("Invalid token"));
 
 // Permission denied
-return next(AppError.forbidden('Admin access required'));
+return next(AppError.forbidden("Admin access required"));
 
 // Resource not found
-return next(AppError.notFound('User not found'));
+return next(AppError.notFound("User not found"));
 
 // Conflict
-return next(AppError.conflict('Email already registered'));
+return next(AppError.conflict("Email already registered"));
 
 // Business logic violation
-return next(AppError.unprocessable('Cannot delete last admin'));
+return next(AppError.unprocessable("Cannot delete last admin"));
 
 // Internal error (rarely used directly)
 return next(AppError.internal());
@@ -269,6 +287,7 @@ return next(AppError.internal());
 ## Testing
 
 Error responses are thoroughly tested in `src/tests/errorHandler.test.ts` with >95% coverage for:
+
 - All AppError factory methods
 - Error envelope structure validation
 - requestId echo behavior

src/middleware/errorHandler.piiGuard.test.ts

@@ -0,0 +1,126 @@
+import { describe, it, expect, beforeEach, afterEach } from '@jest/globals'
+import request from 'supertest'
+import express, { type Request, type Response, type NextFunction } from 'express'
+import { AppError, errorHandler } from '../middleware/errorHandler.js'
+
+function buildApp(thrower: (req: Request, res: Response, next: NextFunction) => void) {
+  const app = express()
+  app.use(express.json())
+  app.get('/test', thrower)
+  app.use(errorHandler)
+  return app
+}
+
+describe('errorHandler PII Guard', () => {
+  let originalNodeEnv: string | undefined
+
+  beforeEach(() => {
+    originalNodeEnv = process.env.NODE_ENV
+  })
+
+  afterEach(() => {
+    process.env.NODE_ENV = originalNodeEnv
+  })
+
+  describe('in production mode (NODE_ENV=production)', () => {
+    beforeEach(() => {
+      process.env.NODE_ENV = 'production'
+    })
+
+    it('redacts PII from AppError details', async () => {
+      const app = buildApp((_req, _res, next) => {
+        next(AppError.validation('Invalid data', {
+          email: 'leaked-user@example.com',
+          creator: 'GA7QYNF7SOWQ3GLR2BGMZEHXAVIRZA4KVWLTJJFC7MGXUA74P7UJVSGZ',
+          safeField: 'is-ok'
+        }))
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(400)
+      const details = res.body.error.details
+     expect(details.email).toMatch(/^[a-f0-9]{8}$/);
+      expect(details.creator).toMatch(/^[a-f0-9]{8}$/);
+      expect(details.safeField).toBe('is-ok')
+    })
+
+    it('strips internal details from generic Error messages', async () => {
+      const app = buildApp((_req, _res, next) => {
+        next(new Error('FATAL: connection to "db-prod-internal" failed for user "admin"'))
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(500)
+      expect(res.body.error.message).toBe('Internal server error')
+      expect(res.body.error.message).not.toContain('db-prod-internal')
+    })
+
+    it('strips stack traces from generic Error messages', async () => {
+      const app = buildApp((_req, _res, next) => {
+        const err = new Error('Something broke')
+        err.stack = 'Error: Something broke\n    at /app/src/services/critical.js:123:45'
+        next(err)
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(500)
+      expect(res.body.error.message).toBe('Internal server error')
+      expect(res.body.error.message).not.toContain('critical.js')
+    })
+
+    it('preserves correlation ID while sanitizing', async () => {
+      const app = buildApp((_req, _res, next) => {
+        next(new Error('Internal failure'))
+      })
+
+      const res = await request(app).get('/test').set('x-request-id', 'trace-me-123')
+      expect(res.status).toBe(500)
+      expect(res.body.error.message).toBe('Internal server error')
+      expect(res.body.error.requestId).toBe('trace-me-123')
+    })
+  })
+
+  describe('in development mode (NODE_ENV!=production)', () => {
+    beforeEach(() => {
+      process.env.NODE_ENV = 'development'
+    })
+
+    it('does NOT redact PII from AppError details', async () => {
+      const app = buildApp((_req, _res, next) => {
+        next(AppError.validation('Invalid data', {
+          email: 'dev-user@example.com',
+          creator: 'GDEVADDRESSXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX'
+        }))
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(400)
+      const details = res.body.error.details
+      expect(details.email).toBe('dev-user@example.com')
+      expect(details.creator).toBe('GDEVADDRESSXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX')
+    })
+
+    it('preserves the original message for generic Errors', async () => {
+      const app = buildApp((_req, _res, next) => {
+        next(new Error('SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique constraint "users_email_key"'))
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(500)
+      expect(res.body.error.message).toContain('duplicate key value')
+    })
+
+    it('preserves stack traces in the original message for generic Errors', async () => {
+      const app = buildApp((_req, _res, next) => {
+        const err = new Error('Dev mode error')
+        err.stack = 'Error: Dev mode error\n    at /app/src/dev.js:10:20'
+        next(err)
+      })
+
+      const res = await request(app).get('/test')
+      expect(res.status).toBe(500)
+      // The default behavior is to use the message, not the stack, but we ensure it's not the generic prod message.
+      expect(res.body.error.message).toBe('Dev mode error')
+    })
+  })
+})

src/middleware/errorHandler.ts

@@ -1,4 +1,5 @@
 import type { NextFunction, Request, Response } from 'express'
+import { sanitizePrivacyPayload } from '../utils/privacy.js'
 
 // ─── Error Codes ─────────────────────────────────────────────────────────────
 // Machine-readable codes clients can branch on without parsing message strings.
@@ -166,6 +167,13 @@ export const errorHandler = (
   // PII is not logged: we only record method, path, and a sanitised message.
   const requestId = (req.headers['x-request-id'] as string | undefined) ?? undefined
 
+  // Determine if we are in a production environment
+  const isProduction = process.env.NODE_ENV === 'production'
+
+  // Sanitize any echoed PII from error details in production
+  const sanitizeDetails = (details: unknown) =>
+    isProduction ? sanitizePrivacyPayload(details) : details
+
   // Sanitize and convert express body-parser size limit errors
   if (err && typeof err === 'object' && 'status' in err && err.status === 413 && 'type' in err && (err as any).type === 'entity.too.large') {
     err = new AppError(413, ErrorCode.PAYLOAD_TOO_LARGE, 'Payload too large')
@@ -191,7 +199,7 @@ export const errorHandler = (
       error: {
         code: err.code,
         message: err.message,
-        ...(err.details !== undefined && { details: err.details }),
+        ...(err.details !== undefined && { details: sanitizeDetails(err.details) }),
         ...(requestId && { requestId }),
       },
     }
@@ -201,7 +209,10 @@ export const errorHandler = (
   }
 
   // Unknown / unexpected errors – never leak internals to the client.
-  const message = err instanceof Error ? err.message : 'Internal server error'
+  // In production, always use a generic message. In dev, show the real error.
+  const message = isProduction
+    ? 'Internal server error'
+    : err instanceof Error ? err.message : 'Internal server error'
 
   console.error(
     JSON.stringify({
@@ -221,7 +232,7 @@ export const errorHandler = (
   const body: ErrorResponse = {
     error: {
       code: ErrorCode.INTERNAL_ERROR,
-      message: 'Internal server error',
+      message,
       ...(requestId && { requestId }),
     },
   }