Mitchwork

PRODUCTION_READINESS_AUDIT.md

@@ -0,0 +1,248 @@
+# TitanBot Production Readiness Audit
+
+**Date:** 2026-02-25  
+**Scope:** Full repository scan (`src`, `config`, `events`, `handlers`, `services`, `utils`, startup/runtime scripts)  
+**Focus Areas:** Production standards, PostgreSQL ("PQL") security posture, logging consistency, error handling, operational security, and database architecture maturity.
+
+---
+
+## Executive Summary
+
+TitanBot is **close to production-ready** and demonstrates strong architectural direction (centralized logging utility, robust PostgreSQL wrapper, graceful startup/shutdown, and structured error handling patterns). However, there are several **high-priority security and standardization gaps** that should be closed before enterprise-grade production operation.
+
+### Overall Rating
+- **Current Readiness:** **7.1 / 10** (Good, with important hardening work remaining)
+- **Recommendation:** **Conditional Go** for controlled production; complete P0 items before broad rollout.
+
+### Top Strengths
+- PostgreSQL-first architecture with pooling, health checks, retries, indexes, and audit tables.
+- Parameterized SQL usage in the majority of data operations.
+- Central logger (`winston` + rotate files + exception/rejection handlers).
+- Centralized interaction error system with categorized user/system error paths.
+- Dependency audit currently clean (`npm audit`: 0 known vulnerabilities).
+
+### Top Risks
+- Unsafe dynamic code execution path present (`new Function`) in calculator flows.
+- Inconsistent logging standards: mixed `logger.*` and `console.*` usage.
+- Weak database TLS setting (`rejectUnauthorized: false` when SSL enabled).
+- Hardcoded external API key fallback exists in movie command.
+- No CI workflow found enforcing tests/security/quality gates.
+
+---
+
+## Methodology
+
+This report is based on:
+- Static scan of source tree for security/logging/error/database patterns.
+- Review of core runtime/config modules and key services.
+- Query pattern inspection (parameterized vs interpolated SQL).
+- Dependency risk check (`npm audit --json`).
+- Operational-readiness signals (health/readiness endpoints, migration scripts, policy docs, workflow presence).
+
+---
+
+## Scorecard by Domain
+
+| Domain | Score | Status | Summary |
+|---|---:|---|---|
+| Runtime Architecture | 8.0 | Good | Clean modular layout, service separation, and startup sequencing |
+| PostgreSQL / Data Layer | 7.5 | Good | Strong schema/index setup; TLS and migration governance need hardening |
+| PQL/SQL Security | 7.0 | Moderate | Parameterized queries mostly strong; dynamic SQL surfaces need guardrails |
+| Logging Consistency | 6.0 | Moderate Risk | Central logger exists, but substantial `console.*` drift remains |
+| Error Handling | 8.0 | Good | Centralized error categorization and interaction-safe response handling |
+| Secrets & Config Security | 5.8 | Moderate Risk | Env-based model is good; hardcoded API key fallback is a security anti-pattern |
+| Operations / Delivery | 5.5 | Moderate Risk | Health/readiness present, but no CI workflows detected |
+| Dependency Security | 9.0 | Strong | Current advisory scan clean |
+
+---
+
+## Findings (Pros, Cons, Improvements)
+
+## 1) PostgreSQL / PQL Security & Database Standards
+
+### Pros
+- PostgreSQL wrapper includes:
+  - Connection pooling and retry/backoff.
+  - `statement_timeout` in production.
+  - Automatic table/index creation.
+  - Audit-oriented schema (`verification_audit`) and timestamp triggers.
+- Most SQL operations use bind parameters (`$1`, `$2`, ...), reducing injection risk.
+- Production protection exists in `src/services/database.js`: fallback storage is refused in production.
+
+### Cons / Risks
+- SSL config uses `rejectUnauthorized: false` when `POSTGRES_SSL=true`, which weakens TLS trust verification.
+- Some SQL statements interpolate identifiers (table/trigger names) dynamically; currently fed by internal constants, but still a sensitive surface.
+- Two database access layers coexist (`src/utils/database.js` and `src/services/database.js`), increasing behavior divergence risk.
+- No explicit migration history enforcement in standard runtime path (migration tool exists but workflow governance is unclear).
+
+### Improvements
+- **P0:** Enforce strict TLS in production (`rejectUnauthorized: true` + CA bundle support).
+- **P1:** Add identifier allowlist/assertion utility for dynamic SQL identifiers.
+- **P1:** Consolidate to a single database abstraction or formally define boundaries between the two layers.
+- **P1:** Add migration ledger/check gate at startup (or CI) to prevent schema drift.
+
+---
+
+## 2) Logging Consistency & Observability
+
+### Pros
+- Strong central logger (`winston`) with:
+  - Rotating files for combined/error logs.
+  - Exception/rejection handlers.
+  - Structured metadata support.
+- Logging service provides event taxonomy and guild-level filtering.
+- Good startup/shutdown and degraded-mode logging visibility.
+
+### Cons / Risks
+- Logging standards are inconsistent across repo:
+  - Approx. **644** `logger.*` calls (good adoption)
+  - Approx. **60** `console.*` calls (standard drift)
+- `console.*` usage bypasses central formatting, correlation, and retention behavior.
+- No request/interaction correlation ID propagated consistently across services.
+
+### Improvements
+- **P0:** Replace direct `console.*` with centralized logger wrapper.
+- **P1:** Introduce correlation IDs for command interactions and propagate through service calls.
+- **P1:** Define logging schema contract (`event`, `guildId`, `userId`, `command`, `errorCode`, `traceId`).
+- **P2:** Add log quality checks (lint rule or CI grep guard) to block new `console.*` usage.
+
+---
+
+## 3) Error Handling & Resilience
+
+### Pros
+- Centralized `errorHandler` supports:
+  - Typed error categories.
+  - User-safe messaging.
+  - System-vs-user error separation.
+  - Safe interaction reply/edit fallback logic.
+- `interactionCreate` catches handler-level failures and provides fallback user response.
+- Startup catches fatal failures and exits in a controlled way.
+
+### Cons / Risks
+- Error shape still varies by module (some throw typed errors, others raw errors).
+- Some paths still report through console rather than central error flow.
+- Limited evidence of automated negative-path testing for error contracts.
+
+### Improvements
+- **P1:** Require typed application errors in service boundary layers.
+- **P1:** Add error code registry and map to remediation hints.
+- **P2:** Add tests for critical error scenarios (DB unavailable, Discord API failures, expired interactions).
+
+---
+
+## 4) Security Posture (Application + Secrets)
+
+### Pros
+- Security policy file exists with disclosure workflow and hardening guidance.
+- Permission checks/audits are present in command helpers.
+- Input sanitization helpers and schema normalization (`zod`) are in use.
+- Dependency scan currently reports zero known vulnerabilities.
+
+### Cons / Risks
+- **High-risk:** Dynamic execution present via `new Function` in calculator-related code paths.
+- **High-risk:** Hardcoded TMDB API key fallback in movie search command.
+- Sanitization utility is partial and may not be uniformly applied in all user-input flows.
+- No CI workflow found for automated SAST/dependency/license/security policy enforcement.
+
+### Improvements
+- **P0:** Remove `new Function`; replace with strict expression parser/evaluator (whitelist-based AST parser).
+- **P0:** Remove hardcoded API key fallback and fail closed when env var is missing.
+- **P1:** Expand schema-based validation (`zod`) for all command payloads and config mutations.
+- **P1:** Add secret scanning and dependency policy checks in CI.
+- **P2:** Add abuse protections for high-risk endpoints/commands (cooldown + anomaly logging).
+
+---
+
+## 5) Operations, Reliability, and Production Standards
+
+### Pros
+- Health (`/health`) and readiness (`/ready`) endpoints exist.
+- Startup sequence and graceful shutdown behavior are implemented.
+- Cron job registration and periodic maintenance tasks are structured.
+
+### Cons / Risks
+- No GitHub Actions workflows detected in repository (build/test/security gates absent).
+- No explicit SLO/SLI definitions or alert threshold policy found.
+- Backup/restore drills and DR evidence are not represented in repo artifacts.
+
+### Improvements
+- **P0:** Add CI workflows: lint, syntax checks, unit tests, dependency audit, secret scan.
+- **P1:** Define minimum release gates (branch protection + required checks).
+- **P1:** Add operational runbook docs for incident, rollback, and DB restore validation.
+- **P2:** Add service-level metrics and alerting playbook (error rate, latency, DB connectivity, command failure rate).
+
+---
+
+## Priority Remediation Plan
+
+### P0 (Do Before Broad Production Rollout)
+1. Replace `new Function` evaluator with safe math parser.
+2. Remove hardcoded TMDB API key fallback.
+3. Enforce strict PostgreSQL TLS verification in production.
+4. Eliminate `console.*` in runtime/service layers (route through logger).
+5. Add basic CI workflow with mandatory security and quality checks.
+
+### P1 (Next 2–4 Weeks)
+1. Consolidate database access abstractions and codify single source of truth.
+2. Add correlation IDs and standardized structured log fields.
+3. Expand schema validation coverage for commands/config writes.
+4. Add migration governance (version checks and controlled rollout).
+
+### P2 (Maturity / Scale)
+1. Add SLO/SLI monitoring with alerts.
+2. Add backup/restore automation and periodic drill evidence.
+3. Add policy-as-code checks for security controls and code quality drift.
+
+---
+
+## Evidence Snapshot
+
+- Dependency audit: **0 vulnerabilities** (prod deps: 149).
+- Logging pattern scan:
+  - `logger.*`: ~644 occurrences
+  - `console.*`: ~60 occurrences
+- Dynamic execution scan:
+  - `new Function`: detected in calculator-related modules.
+- Security-sensitive config scan:
+  - PostgreSQL SSL currently allows `rejectUnauthorized: false` when enabled.
+- Repo operations scan:
+  - No `.github/workflows/*` files detected.
+
+---
+
+## Final Verdict
+
+TitanBot is **architecturally solid and operationally promising**, but it is **not yet at full industry-standard production hardening** due to specific P0 security and governance gaps.
+
+If P0 actions are completed, the project moves from **Conditional Production** to **Production Ready (Standard)**. If P1 actions are then completed, it reaches **Production Ready (Mature)** for larger-scale deployments.
+
+---
+
+## Simple Improvement List (Copy/Paste)
+
+Use this checklist to track everything that should be improved:
+
+1. Turn on strict PostgreSQL TLS checks in production (require trusted cert validation).
+2. Add a safe allowlist check for any dynamic SQL identifiers (like table/trigger names).
+3. Merge the two database access layers into one standard approach (or clearly define strict boundaries).
+4. Add migration version checks in startup/CI so schema drift is blocked.
+5. Replace all `console.*` logs with the central logger.
+6. Add a correlation/trace ID for each interaction and pass it through service calls.
+7. Standardize log fields (`event`, `guildId`, `userId`, `command`, `errorCode`, `traceId`).
+8. Add a lint/CI rule to block new `console.*` usage.
+9. Enforce typed application errors at service boundaries.
+10. Create an error code registry with remediation hints.
+11. Add tests for major failure paths (DB down, Discord API failure, expired interaction).
+12. Remove `new Function` and use a safe whitelist-based math parser.
+13. Remove hardcoded TMDB key fallback; fail safely if env key is missing.
+14. Expand `zod` validation to all command inputs and config writes.
+15. Add CI secret scanning and dependency policy checks.
+16. Add abuse protections on risky commands (cooldowns + anomaly logging).
+17. Create CI workflows for lint, syntax checks, tests, dependency audit, and secret scan.
+18. Add branch protection and required checks as release gates.
+19. Add runbooks for incident response, rollback, and DB restore steps.
+20. Add service metrics + alert playbooks (error rate, latency, DB connectivity, command failure rate).
+21. Define SLO/SLI targets and alert thresholds.
+22. Add backup/restore automation and run regular restore drills.
+23. Add policy-as-code checks to prevent security/quality drift.

src/commands/Birthday/modules/birthday_list.js

@@ -1,6 +1,7 @@
 import { MessageFlags } from 'discord.js';
 import { createEmbed, errorEmbed, successEmbed } from '../../../utils/embeds.js';
 import { getAllBirthdays } from '../../../services/birthdayService.js';
+import { deleteBirthday } from '../../../utils/database.js';
 import { logger } from '../../../utils/logger.js';
 import { handleInteractionError } from '../../../utils/errorHandler.js';
 
@@ -30,22 +31,52 @@ export default {
                 color: 'info'
             });
 
-            let birthdayList = `**${sortedBirthdays.length} birthdays in ${interaction.guild.name}**\n\n`;
-            sortedBirthdays.forEach((birthday, index) => {
-                const member = interaction.guild.members.cache.get(birthday.userId);
-                const userName = member ? member.user.username : `User ${birthday.userId}`;
-                birthdayList += `${index + 1}. **${userName}** - ${birthday.monthName} ${birthday.day}\n`;
-            });
+            // Batch fetch to verify which users are still in the guild
+            const userIds = sortedBirthdays.map(b => b.userId);
+            const fetchedMembers = await interaction.guild.members.fetch({ user: userIds }).catch(() => null);
+
+            let birthdayList = '';
+            let displayIndex = 0;
+            const staleUserIds = [];
+
+            for (const birthday of sortedBirthdays) {
+                if (fetchedMembers && !fetchedMembers.has(birthday.userId)) {
+                    staleUserIds.push(birthday.userId);
+                    continue;
+                }
+                displayIndex++;
+                birthdayList += `${displayIndex}. <@${birthday.userId}> - ${birthday.monthName} ${birthday.day}\n`;
+            }
+
+            // Clean up birthday entries for members who left the server
+            if (fetchedMembers && staleUserIds.length > 0) {
+                for (const userId of staleUserIds) {
+                    deleteBirthday(client, guildId, userId).catch(() => null);
+                }
+            }
+
+            if (displayIndex === 0) {
+                return await InteractionHelper.safeEditReply(interaction, {
+                    embeds: [createEmbed({
+                        title: '❌ No Birthdays',
+                        description: 'No birthdays have been set by current server members.',
+                        color: 'error'
+                    })]
+                });
+            }
+
+            birthdayList = `**${displayIndex} birthday${displayIndex !== 1 ? 's' : ''} in ${interaction.guild.name}**\n\n` + birthdayList;
 
-            embed.setDescription(birthdayList || "No birthdays found");
-            embed.setFooter({ text: `Total: ${sortedBirthdays.length} birthdays` });
+            embed.setDescription(birthdayList);
+            embed.setFooter({ text: `Total: ${displayIndex} birthday${displayIndex !== 1 ? 's' : ''}` });
 
             await InteractionHelper.safeEditReply(interaction, { embeds: [embed] });
 
             logger.info('Birthday list retrieved successfully', {
                 userId: interaction.user.id,
                 guildId,
-                birthdayCount: sortedBirthdays.length,
+                birthdayCount: displayIndex,
+                staleRemoved: staleUserIds.length,
                 commandName: 'birthday_list'
             });
         } catch (error) {

src/commands/Birthday/modules/next_birthdays.js

@@ -1,6 +1,7 @@
 import { MessageFlags } from 'discord.js';
 import { createEmbed, errorEmbed, successEmbed } from '../../../utils/embeds.js';
 import { getUpcomingBirthdays } from '../../../services/birthdayService.js';
+import { deleteBirthday } from '../../../utils/database.js';
 import { logger } from '../../../utils/logger.js';
 import { handleInteractionError } from '../../../utils/errorHandler.js';
 
@@ -31,11 +32,15 @@ export default {
                 color: 'info'
             });
 
-            for (let i = 0; i < next5.length; i++) {
-                const birthday = next5[i];
+            let displayIndex = 0;
+            for (const birthday of next5) {
                 const member = await interaction.guild.members.fetch(birthday.userId).catch(() => null);
-                const userName = member ? member.user.username : `User ${birthday.userId}`;
-
+                if (!member) {
+                    deleteBirthday(client, interaction.guildId, birthday.userId).catch(() => null);
+                    continue;
+                }
+                displayIndex++;
+
                 let timeUntil = '';
                 if (birthday.daysUntil === 0) {
                     timeUntil = '🎉 **Today!**';
@@ -46,12 +51,24 @@ export default {
                 }
 
                 embed.addFields({
-                    name: `${i + 1}. ${userName}`,
-                    value: `📅 **Date:** ${birthday.monthName} ${birthday.day}\n⏰ **Time:** ${timeUntil}`,
+                    name: `${displayIndex}. ${member.displayName}`,
+                    value: `<@${birthday.userId}>\n📅 **Date:** ${birthday.monthName} ${birthday.day}\n⏰ **Time:** ${timeUntil}`,
                     inline: false
                 });
             }
 
+            if (displayIndex === 0) {
+                return await InteractionHelper.safeEditReply(interaction, {
+                    embeds: [
+                        createEmbed({
+                            title: '❌ No Upcoming Birthdays',
+                            description: 'No upcoming birthdays found for current server members.',
+                            color: 'error'
+                        })
+                    ]
+                });
+            }
+
             embed.setFooter({
                 text: 'Use /birthday set to add your birthday!',
                 iconURL: interaction.guild.iconURL()
@@ -62,7 +79,7 @@ export default {
             logger.info('Next birthdays retrieved successfully', {
                 userId: interaction.user.id,
                 guildId: interaction.guildId,
-                upcomingCount: next5.length,
+                upcomingCount: displayIndex,
                 commandName: 'next_birthdays'
             });
         } catch (error) {