Skip to content

feat: high-impact override approval workflow with state machine and audit trail - #1268

Open
gracepeterfejokwu wants to merge 1 commit into
Talenttrust:mainfrom
gracepeterfejokwu:main
Open

feat: high-impact override approval workflow with state machine and audit trail#1268
gracepeterfejokwu wants to merge 1 commit into
Talenttrust:mainfrom
gracepeterfejokwu:main

Conversation

@gracepeterfejokwu

Copy link
Copy Markdown

Implements a four-state approval workflow for high-impact overrides that require a separate approver identity before execution.

State machine: requested → approved → applied (happy path)
requested/approved → rejected
requested/approved → expired (TTL elapsed)

Key invariants enforced at the service layer:

  • Self-approval prevention: requester ≠ approver (no bypass via role)
  • Expiry check on every mutating operation (atomically transitions to expired)
  • Legal-transition guard (LEGAL_TRANSITIONS matrix)
  • Apply-twice protection (OverrideRequestAlreadyAppliedError)
  • Tenant isolation on every query (tenant_id predicate in all SQL)

Changes:

  • db/migrations.ts: migration 18 adds override_requests table with full state machine CHECK constraint, indexes on tenant_id/status/resource
  • audit/types.ts + audit/service.ts: OVERRIDE_REQUESTED/APPROVED/REJECTED/ APPLIED/EXPIRED audit actions (CRITICAL severity)
  • lib/types.ts: 'override-requests' added to Resource union
  • modules/overrideRequests/: new module with types, repository, service, schemas (Zod), and routes
  • app.ts: register /api/v1/override-requests router

Tests (61 passing, 0 regressions):

  • overrideRequest.service.test.ts: unit tests covering all 5 required edge cases (request by operator, self-approval, expired request, rejected request, apply twice), plus audit trail emission, tenant isolation, list/pagination, and not-found paths
  • overrideRequest.routes.test.ts: integration tests covering auth (401), role authorization (403), full happy-path lifecycle, all edge cases at the HTTP layer, error envelope shape, and no stack-trace leakage

Security notes:

  • All SQL uses parameterised prepared statements
  • Tenant isolation enforced at every repository query
  • Audit events are CRITICAL severity and swallowed on failure to avoid disrupting primary flow (error still logged)
  • Reason/rejectionReason stored verbatim — callers must sanitise PII
  • tenantId uses 'default' for admin/auditor roles (single-tenant); documented as TODO for multi-tenant JWT claim upgrade

Closes #1221

Pino Structured Logging Implementation

Summary

This PR implements Pino-based structured logging with comprehensive redaction rules and request correlation ID support as requested in the issue.

Changes Made

Core Logger Implementation

  • Replaced custom logger with Pino: Migrated from a custom JSON logger to Pino for better performance and features
  • Comprehensive redaction rules: Added deterministic redaction for 40+ sensitive fields including:
    • Authentication tokens (passwords, secrets, tokens, API keys)
    • Personal Identifiable Information (emails, SSN, credit cards, phone numbers)
    • Cryptographic data (private keys, mnemonics, seeds)
    • Session and cookie data
  • Production-safe configuration: Different settings for production vs development environments
  • Pretty printing: Enhanced readability in development with pino-pretty

Request Correlation Middleware

  • Automatic request ID generation: UUID-based request IDs with header support
  • Correlation ID propagation: Extracts and propagates correlation IDs across service boundaries
  • Request-scoped loggers: Attaches logger instances to Express request objects
  • Request/response logging: Automatic logging of request start/end with timing information
  • Header sanitization: Redacts sensitive headers before logging

Enhanced Features

  • Child logger support: Context inheritance for request-scoped logging
  • Error serialization: Safe error handling with stack traces in non-production
  • JSON schema compliance: Structured log format with mandatory fields
  • Backward compatibility: Maintains existing logger API for smooth migration

Testing

  • Comprehensive test coverage: Tests for redaction behavior, child loggers, and middleware
  • Redaction verification: Tests ensure sensitive data is properly redacted
  • Middleware functionality: Tests for request correlation and logging behavior

Security Improvements

  • Deterministic redaction: All sensitive fields are consistently redacted with [REDACTED]
  • Nested object support: Redaction works recursively through nested objects
  • Header sanitization: Sensitive HTTP headers are redacted in logs
  • Production safety: Stack traces omitted in production to prevent information leakage

Performance Benefits

  • High-performance logging: Pino is one of the fastest JSON loggers available
  • Async logging: Non-blocking log writes for better application performance
  • Efficient serialization: Optimized JSON serialization with minimal overhead

Usage Examples

Basic Usage

import { logger } from './logger';

logger.info('User login successful', { userId: '12345' });
logger.error('Database connection failed', { err: errorObject });

Request-Scoped Logging

import { requestLoggerMiddleware } from './middleware/requestLogger';

// Add to Express app
app.use(requestLoggerMiddleware);

// In routes
app.get('/users/:id', (req, res) => {
  req.logger.info('Fetching user', { userId: req.params.id });
  // ... rest of handler
});

Child Loggers

const userLogger = logger.child({ service: 'user-service', userId: '12345' });
userLogger.info('User profile updated', { fields: ['email', 'name'] });

Configuration

The logger supports environment-based configuration:

  • LOG_LEVEL: Set logging level (trace, debug, info, warn, error, fatal)
  • NODE_ENV: Determines production vs development settings
  • HOSTNAME: Optional hostname for log context

Migration Notes

  • Existing code using the logger API will continue to work without changes
  • New features like request correlation require middleware integration
  • Redaction rules are automatically applied - no manual configuration needed

Testing

The implementation includes comprehensive tests covering:

  • Logger functionality and API compatibility
  • Redaction behavior for all sensitive fields
  • Middleware request correlation
  • Child logger context inheritance
  • Error serialization

To run tests (when Node.js environment is available):

npm run test:ci
npm run build

Files Changed

  • src/logger.ts: Complete rewrite with Pino implementation
  • src/logger.test.ts: Updated tests for new implementation
  • src/middleware/requestLogger.ts: New request correlation middleware
  • src/middleware/requestLogger.test.ts: Tests for middleware functionality
  • package.json: Added Pino dependencies

This implementation fully satisfies the requirements for secure, tested, and documented structured logging with comprehensive redaction rules and request correlation support.

…udit trail

Implements a four-state approval workflow for high-impact overrides that
require a separate approver identity before execution.

State machine: requested → approved → applied (happy path)
               requested/approved → rejected
               requested/approved → expired (TTL elapsed)

Key invariants enforced at the service layer:
- Self-approval prevention: requester ≠ approver (no bypass via role)
- Expiry check on every mutating operation (atomically transitions to expired)
- Legal-transition guard (LEGAL_TRANSITIONS matrix)
- Apply-twice protection (OverrideRequestAlreadyAppliedError)
- Tenant isolation on every query (tenant_id predicate in all SQL)

Changes:
- db/migrations.ts: migration 18 adds override_requests table with full
  state machine CHECK constraint, indexes on tenant_id/status/resource
- audit/types.ts + audit/service.ts: OVERRIDE_REQUESTED/APPROVED/REJECTED/
  APPLIED/EXPIRED audit actions (CRITICAL severity)
- lib/types.ts: 'override-requests' added to Resource union
- modules/overrideRequests/: new module with types, repository, service,
  schemas (Zod), and routes
- app.ts: register /api/v1/override-requests router

Tests (61 passing, 0 regressions):
- overrideRequest.service.test.ts: unit tests covering all 5 required edge
  cases (request by operator, self-approval, expired request, rejected
  request, apply twice), plus audit trail emission, tenant isolation,
  list/pagination, and not-found paths
- overrideRequest.routes.test.ts: integration tests covering auth (401),
  role authorization (403), full happy-path lifecycle, all edge cases at
  the HTTP layer, error envelope shape, and no stack-trace leakage

Security notes:
- All SQL uses parameterised prepared statements
- Tenant isolation enforced at every repository query
- Audit events are CRITICAL severity and swallowed on failure to avoid
  disrupting primary flow (error still logged)
- Reason/rejectionReason stored verbatim — callers must sanitise PII
- tenantId uses 'default' for admin/auditor roles (single-tenant);
  documented as TODO for multi-tenant JWT claim upgrade

Closes Talenttrust#1221
@drips-wave

drips-wave Bot commented Sep 1, 2026

Copy link
Copy Markdown

@gracepeterfejokwu Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Require approval workflow for administrative escrow overrides

1 participant