From 94c4c625d9147f23cadacc950d0f6c9fbed88de7 Mon Sep 17 00:00:00 2001 From: Pip Build Date: Sun, 1 Mar 2026 23:26:17 -0500 Subject: [PATCH 1/3] feat: server config backup and restore (#129) - Export all config sections to timestamped JSON files (secrets redacted) - Import config from JSON payload (skips [REDACTED] values) - Scheduled automatic backups every 24h with retention pruning - Backup history/versioning: list, download, restore by ID - Path-traversal protection on backup ID inputs - API routes: GET /backups, POST /backups, GET /export, POST /import, GET /:id/download, POST /:id/restore, POST /prune - Closes #129 --- src/api/index.js | 4 + src/api/routes/backup.js | 422 +++++++++++++++++++++++++++++++ src/index.js | 3 + src/modules/backup.js | 428 ++++++++++++++++++++++++++++++++ tests/api/routes/backup.test.js | 306 +++++++++++++++++++++++ tests/modules/backup.test.js | 309 +++++++++++++++++++++++ 6 files changed, 1472 insertions(+) create mode 100644 src/api/routes/backup.js create mode 100644 src/modules/backup.js create mode 100644 tests/api/routes/backup.test.js create mode 100644 tests/modules/backup.test.js diff --git a/src/api/index.js b/src/api/index.js index 0a69c678d..db839b59a 100644 --- a/src/api/index.js +++ b/src/api/index.js @@ -9,6 +9,7 @@ import { requireAuth } from './middleware/auth.js'; import aiFeedbackRouter from './routes/ai-feedback.js'; import auditLogRouter from './routes/auditLog.js'; import authRouter from './routes/auth.js'; +import backupRouter from './routes/backup.js'; import communityRouter from './routes/community.js'; import configRouter from './routes/config.js'; import conversationsRouter from './routes/conversations.js'; @@ -61,4 +62,7 @@ router.use('/guilds', requireAuth(), auditLogRouter); // Webhook routes — require API secret or OAuth2 JWT (endpoint further restricts to api-secret) router.use('/webhooks', requireAuth(), webhooksRouter); +// Backup routes — require API secret or OAuth2 JWT +router.use('/backups', requireAuth(), auditLogMiddleware(), backupRouter); + export default router; diff --git a/src/api/routes/backup.js b/src/api/routes/backup.js new file mode 100644 index 000000000..b3733e424 --- /dev/null +++ b/src/api/routes/backup.js @@ -0,0 +1,422 @@ +/** + * Backup Routes + * Endpoints for config export, import, backup creation, listing, restore, and deletion. + * + * All routes require global admin access (API secret or bot-owner OAuth). + * + * @see https://github.com/VolvoxLLC/volvox-bot/issues/129 + */ + +import { Router } from 'express'; +import { warn } from '../../logger.js'; +import { + createBackup, + exportConfig, + importConfig, + listBackups, + pruneBackups, + readBackup, + restoreBackup, + validateImportPayload, +} from '../../modules/backup.js'; +import { getConfig } from '../../modules/config.js'; +import { getBotOwnerIds } from '../../utils/permissions.js'; + +const router = Router(); + +/** + * Middleware — restricts access to API-secret callers or bot-owner OAuth users. + * Mirrors the requireGlobalAdmin guard used by the config routes. + */ +function requireGlobalAdmin(req, res, next) { + if (req.authMethod === 'api-secret') return next(); + + if (req.authMethod === 'oauth') { + const botOwners = getBotOwnerIds(getConfig()); + if (botOwners.includes(req.user?.userId)) return next(); + + return res.status(403).json({ error: 'Backup access requires bot owner permissions' }); + } + + warn('Unknown authMethod in backup route', { authMethod: req.authMethod, path: req.path }); + return res.status(401).json({ error: 'Unauthorized' }); +} + +/** + * @openapi + * /backups/export: + * get: + * tags: + * - Backup + * summary: Export current config as JSON + * description: > + * Download the current server configuration as a JSON file. + * Sensitive fields (API keys, tokens) are redacted. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * responses: + * "200": + * description: Config exported successfully + * content: + * application/json: + * schema: + * type: object + * properties: + * config: + * type: object + * exportedAt: + * type: string + * format: date-time + * version: + * type: integer + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + */ +router.get('/export', requireGlobalAdmin, (_req, res) => { + const payload = exportConfig(); + const filename = `config-export-${new Date().toISOString().replace(/[:.]/g, '-')}.json`; + + res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); + res.setHeader('Content-Type', 'application/json'); + res.json(payload); +}); + +/** + * @openapi + * /backups/import: + * post: + * tags: + * - Backup + * summary: Import config from a JSON payload + * description: > + * Apply configuration from a previously exported JSON payload. + * Redacted values (REDACTED placeholder) are skipped to preserve live secrets. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * requestBody: + * required: true + * content: + * application/json: + * schema: + * type: object + * required: [config] + * properties: + * config: + * type: object + * exportedAt: + * type: string + * version: + * type: integer + * responses: + * "200": + * description: Import completed + * content: + * application/json: + * schema: + * type: object + * properties: + * applied: + * type: array + * items: + * type: string + * skipped: + * type: array + * items: + * type: string + * failed: + * type: array + * items: + * type: object + * "400": + * description: Invalid import payload + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + */ +router.post('/import', requireGlobalAdmin, async (req, res) => { + const payload = req.body; + + const validationErrors = validateImportPayload(payload); + if (validationErrors.length > 0) { + return res.status(400).json({ error: 'Invalid import payload', details: validationErrors }); + } + + try { + const result = await importConfig(payload); + return res.json(result); + } catch (err) { + return res.status(500).json({ error: 'Import failed', details: err.message }); + } +}); + +/** + * @openapi + * /backups: + * get: + * tags: + * - Backup + * summary: List available backups + * description: > + * Returns metadata for all stored config backups, sorted newest first. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * responses: + * "200": + * description: List of backups + * content: + * application/json: + * schema: + * type: array + * items: + * type: object + * properties: + * id: + * type: string + * filename: + * type: string + * createdAt: + * type: string + * format: date-time + * size: + * type: integer + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + */ +router.get('/', requireGlobalAdmin, (_req, res) => { + const backups = listBackups(); + res.json(backups); +}); + +/** + * @openapi + * /backups: + * post: + * tags: + * - Backup + * summary: Create a manual backup + * description: > + * Triggers an immediate backup of the current config. + * Returns the backup metadata (id, size, createdAt). + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * responses: + * "201": + * description: Backup created + * content: + * application/json: + * schema: + * type: object + * properties: + * id: + * type: string + * size: + * type: integer + * createdAt: + * type: string + * format: date-time + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + * "500": + * $ref: "#/components/responses/ServerError" + */ +router.post('/', requireGlobalAdmin, (_req, res) => { + try { + const meta = createBackup(); + return res.status(201).json({ id: meta.id, size: meta.size, createdAt: meta.createdAt }); + } catch (err) { + return res.status(500).json({ error: 'Failed to create backup', details: err.message }); + } +}); + +/** + * @openapi + * /backups/{id}/download: + * get: + * tags: + * - Backup + * summary: Download a specific backup file + * description: > + * Stream a backup JSON file for download by its ID. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * parameters: + * - name: id + * in: path + * required: true + * schema: + * type: string + * description: Backup ID (filename without .json extension) + * responses: + * "200": + * description: Backup JSON file + * content: + * application/json: + * schema: + * type: object + * "400": + * description: Invalid backup ID + * "404": + * description: Backup not found + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + */ +router.get('/:id/download', requireGlobalAdmin, (req, res) => { + const { id } = req.params; + + try { + const payload = readBackup(id); + const filename = `${id}.json`; + res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); + res.setHeader('Content-Type', 'application/json'); + return res.json(payload); + } catch (err) { + const status = err.message.includes('not found') + ? 404 + : err.message.includes('Invalid') + ? 400 + : 500; + return res.status(status).json({ error: err.message }); + } +}); + +/** + * @openapi + * /backups/{id}/restore: + * post: + * tags: + * - Backup + * summary: Restore config from a backup + * description: > + * Restore the live configuration from a stored backup. + * Redacted sensitive values are skipped to preserve live secrets. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * parameters: + * - name: id + * in: path + * required: true + * schema: + * type: string + * description: Backup ID to restore + * responses: + * "200": + * description: Restore completed + * content: + * application/json: + * schema: + * type: object + * properties: + * applied: + * type: array + * items: + * type: string + * skipped: + * type: array + * items: + * type: string + * failed: + * type: array + * items: + * type: object + * "400": + * description: Invalid backup ID or backup format + * "404": + * description: Backup not found + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + * "500": + * $ref: "#/components/responses/ServerError" + */ +router.post('/:id/restore', requireGlobalAdmin, async (req, res) => { + const { id } = req.params; + + try { + const result = await restoreBackup(id); + return res.json(result); + } catch (err) { + const status = err.message.includes('not found') + ? 404 + : err.message.includes('Invalid') + ? 400 + : 500; + return res.status(status).json({ error: err.message }); + } +}); + +/** + * @openapi + * /backups/prune: + * post: + * tags: + * - Backup + * summary: Prune old backups + * description: > + * Delete backups that exceed the retention policy. + * Default: keep last 7 daily + 4 weekly backups. + * Restricted to API-secret callers or bot-owner OAuth users. + * security: + * - ApiKeyAuth: [] + * - BearerAuth: [] + * requestBody: + * content: + * application/json: + * schema: + * type: object + * properties: + * daily: + * type: integer + * description: Number of daily backups to keep + * default: 7 + * weekly: + * type: integer + * description: Number of weekly backups to keep + * default: 4 + * responses: + * "200": + * description: Pruning completed + * content: + * application/json: + * schema: + * type: object + * properties: + * deleted: + * type: array + * items: + * type: string + * count: + * type: integer + * "401": + * $ref: "#/components/responses/Unauthorized" + * "403": + * $ref: "#/components/responses/Forbidden" + */ +router.post('/prune', requireGlobalAdmin, (req, res) => { + const retention = req.body ?? {}; + const deleted = pruneBackups(retention); + return res.json({ deleted, count: deleted.length }); +}); + +export default router; diff --git a/src/index.js b/src/index.js index 22197c30c..6a1c7b2f1 100644 --- a/src/index.js +++ b/src/index.js @@ -43,6 +43,7 @@ import { startConversationCleanup, stopConversationCleanup, } from './modules/ai.js'; +import { startScheduledBackups, stopScheduledBackups } from './modules/backup.js'; import { getConfig, loadConfig } from './modules/config.js'; import { registerEventHandlers } from './modules/events.js'; import { startGithubFeed, stopGithubFeed } from './modules/githubFeed.js'; @@ -277,6 +278,7 @@ async function gracefulShutdown(signal) { stopTempbanScheduler(); stopScheduler(); stopGithubFeed(); + stopScheduledBackups(); // 1.5. Stop API server (drain in-flight HTTP requests before closing DB) try { @@ -467,6 +469,7 @@ async function startup() { startTempbanScheduler(client); startScheduler(client); startGithubFeed(client); + startScheduledBackups(); } // Load commands and login diff --git a/src/modules/backup.js b/src/modules/backup.js new file mode 100644 index 000000000..3cdf80c5f --- /dev/null +++ b/src/modules/backup.js @@ -0,0 +1,428 @@ +/** + * Backup Module + * Handles server configuration export, import, scheduled backups, and backup history. + * + * @see https://github.com/VolvoxLLC/volvox-bot/issues/129 + */ + +import { + existsSync, + mkdirSync, + readdirSync, + readFileSync, + statSync, + unlinkSync, + writeFileSync, +} from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { flattenToLeafPaths } from '../api/routes/config.js'; +import { SAFE_CONFIG_KEYS, SENSITIVE_FIELDS } from '../api/utils/configAllowlist.js'; +import { info, error as logError, warn } from '../logger.js'; +import { getConfig, setConfigValue } from './config.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +/** Default backup directory (data/backups relative to project root) */ +const DEFAULT_BACKUP_DIR = path.join(__dirname, '..', '..', 'data', 'backups'); + +/** Backup file naming pattern */ +const BACKUP_FILENAME_PATTERN = /^backup-(\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3})-\d{4}\.json$/; + +/** Default retention: keep last 7 daily + 4 weekly backups */ +const DEFAULT_RETENTION = { daily: 7, weekly: 4 }; + +/** Monotonic counter used to disambiguate same-millisecond backups */ +let backupSeq = 0; + +/** Interval handle for scheduled backups */ +let scheduledBackupInterval = null; + +/** + * Get or create the backup directory. + * + * @param {string} [dir] - Override backup directory path + * @returns {string} The backup directory path + */ +export function getBackupDir(dir) { + const backupDir = dir ?? DEFAULT_BACKUP_DIR; + if (!existsSync(backupDir)) { + mkdirSync(backupDir, { recursive: true }); + } + return backupDir; +} + +/** + * Sanitize config by replacing sensitive field values with a redaction placeholder. + * Exported configs can then be shared without leaking secrets. + * + * @param {Object} config - Config object to sanitize + * @returns {Object} Sanitized deep-clone of config + */ +export function sanitizeConfig(config) { + const sanitized = structuredClone(config); + + for (const dotPath of SENSITIVE_FIELDS) { + const parts = dotPath.split('.'); + let obj = sanitized; + for (let i = 0; i < parts.length - 1; i++) { + if (obj == null || typeof obj !== 'object') break; + obj = obj[parts[i]]; + } + const lastKey = parts[parts.length - 1]; + if (obj != null && typeof obj === 'object' && lastKey in obj) { + obj[lastKey] = '[REDACTED]'; + } + } + + return sanitized; +} + +/** + * Export current configuration as a JSON-serialisable payload. + * Only exports sections listed in SAFE_CONFIG_KEYS. + * Sensitive fields are redacted. + * + * @returns {{config: Object, exportedAt: string, version: number}} Export payload + */ +export function exportConfig() { + const config = getConfig(); + const exported = {}; + + for (const key of SAFE_CONFIG_KEYS) { + if (key in config) { + exported[key] = config[key]; + } + } + + return { + config: sanitizeConfig(exported), + exportedAt: new Date().toISOString(), + version: 1, + }; +} + +/** + * Validate an import payload structure. + * + * @param {unknown} payload - Parsed JSON payload from an import file + * @returns {string[]} Array of validation error messages (empty if valid) + */ +export function validateImportPayload(payload) { + if (typeof payload !== 'object' || payload === null || Array.isArray(payload)) { + return ['Import payload must be a JSON object']; + } + + if (!('config' in payload)) { + return ['Import payload must have a "config" key']; + } + + const { config } = payload; + if (typeof config !== 'object' || config === null || Array.isArray(config)) { + return ['"config" must be a JSON object']; + } + + const errors = []; + for (const key of Object.keys(config)) { + if (!SAFE_CONFIG_KEYS.has(key)) { + errors.push(`"${key}" is not a writable config section`); + } + } + + return errors; +} + +/** + * Import configuration from an export payload. + * Applies all non-redacted values to the live config. + * + * @param {Object} payload - Export payload (must pass validateImportPayload) + * @returns {Promise<{applied: string[], skipped: string[], failed: Array<{path: string, error: string}>}>} + */ +export async function importConfig(payload) { + const { config } = payload; + + const applied = []; + const skipped = []; + const failed = []; + + for (const [section, sectionValue] of Object.entries(config)) { + if (!SAFE_CONFIG_KEYS.has(section)) continue; + + const paths = flattenToLeafPaths(sectionValue, section); + for (const [dotPath, value] of paths) { + // Skip redacted placeholders — don't overwrite real secrets with placeholder text + if (value === '[REDACTED]') { + skipped.push(dotPath); + continue; + } + + try { + await setConfigValue(dotPath, value); + applied.push(dotPath); + } catch (err) { + failed.push({ path: dotPath, error: err.message }); + } + } + } + + return { applied, skipped, failed }; +} + +/** + * Generate a backup filename for the given date. + * + * @param {Date} [date] - Date to use (defaults to now) + * @returns {string} Filename like "backup-2026-03-01T12-00-00.json" + */ +function makeBackupFilename(date = new Date()) { + // Include milliseconds for precision; append an incrementing sequence to guarantee uniqueness + // within the same millisecond (e.g. rapid test runs or burst backup triggers). + const iso = date.toISOString().replace(/:/g, '-').replace(/Z$/, '').replace(/\./, '-'); + const seq = String(backupSeq++).padStart(4, '0'); + return `backup-${iso}-${seq}.json`; +} + +/** + * Create a timestamped backup of the current config in the backup directory. + * + * @param {string} [backupDir] - Override backup directory + * @returns {{id: string, path: string, size: number, createdAt: string}} Backup metadata + */ +export function createBackup(backupDir) { + const dir = getBackupDir(backupDir); + const now = new Date(); + const filename = makeBackupFilename(now); + const filePath = path.join(dir, filename); + + const payload = exportConfig(); + const json = JSON.stringify(payload, null, 2); + + writeFileSync(filePath, json, 'utf8'); + + const { size } = statSync(filePath); + const id = filename.replace('.json', ''); + + info('Config backup created', { id, path: filePath, size }); + + return { + id, + path: filePath, + size, + createdAt: now.toISOString(), + }; +} + +/** + * Parse backup metadata from a filename and stat the file. + * + * @param {string} filename - Backup filename + * @param {string} dir - Directory containing the backup file + * @returns {{id: string, filename: string, createdAt: string, size: number} | null} + */ +function parseBackupMeta(filename, dir) { + const match = BACKUP_FILENAME_PATTERN.exec(filename); + if (!match) return null; + + const filePath = path.join(dir, filename); + let size = 0; + try { + size = statSync(filePath).size; + } catch { + return null; + } + + // Convert "2026-03-01T12-00-00-000" → "2026-03-01T12:00:00.000Z" + const isoStr = match[1].replace(/T(\d{2})-(\d{2})-(\d{2})-(\d{3})$/, 'T$1:$2:$3.$4Z'); + + return { + id: filename.replace('.json', ''), + filename, + createdAt: isoStr, + size, + }; +} + +/** + * List all available backups, sorted newest first. + * + * @param {string} [backupDir] - Override backup directory + * @returns {Array<{id: string, filename: string, createdAt: string, size: number}>} + */ +export function listBackups(backupDir) { + const dir = getBackupDir(backupDir); + + let files; + try { + files = readdirSync(dir); + } catch { + return []; + } + + const backups = files + .map((filename) => parseBackupMeta(filename, dir)) + .filter(Boolean) + .sort((a, b) => new Date(b.createdAt) - new Date(a.createdAt)); + + return backups; +} + +/** + * Read and parse a backup file by ID. + * + * @param {string} id - Backup ID (filename without .json) + * @param {string} [backupDir] - Override backup directory + * @returns {Object} Parsed backup payload + * @throws {Error} If backup file not found or invalid + */ +export function readBackup(id, backupDir) { + // Prevent path traversal attacks + if (id.includes('/') || id.includes('..') || id.includes('\\')) { + throw new Error('Invalid backup ID'); + } + + const dir = getBackupDir(backupDir); + const filename = `${id}.json`; + const filePath = path.join(dir, filename); + + if (!existsSync(filePath)) { + throw new Error(`Backup not found: ${id}`); + } + + const raw = readFileSync(filePath, 'utf8'); + try { + return JSON.parse(raw); + } catch { + throw new Error(`Backup file is corrupted: ${id}`); + } +} + +/** + * Restore configuration from a backup. + * + * @param {string} id - Backup ID to restore from + * @param {string} [backupDir] - Override backup directory + * @returns {Promise<{applied: string[], skipped: string[], failed: Array<{path: string, error: string}>}>} + * @throws {Error} If backup not found or invalid + */ +export async function restoreBackup(id, backupDir) { + const payload = readBackup(id, backupDir); + + const validationErrors = validateImportPayload(payload); + if (validationErrors.length > 0) { + throw new Error(`Invalid backup format: ${validationErrors.join(', ')}`); + } + + info('Restoring config from backup', { id }); + const result = await importConfig(payload); + info('Config restored from backup', { + id, + applied: result.applied.length, + failed: result.failed.length, + }); + + return result; +} + +/** + * Prune old backups according to retention policy. + * Always keeps the `daily` most recent backups. + * Additionally keeps one backup per week for the last `weekly` weeks. + * + * @param {{daily?: number, weekly?: number}} [retention] - Retention counts + * @param {string} [backupDir] - Override backup directory + * @returns {string[]} IDs of deleted backups + */ +export function pruneBackups(retention, backupDir) { + const { daily = DEFAULT_RETENTION.daily, weekly = DEFAULT_RETENTION.weekly } = retention ?? {}; + const dir = getBackupDir(backupDir); + const all = listBackups(dir); + + if (all.length === 0) return []; + + // Always keep the `daily` most recent backups + const toKeep = new Set(all.slice(0, daily).map((b) => b.id)); + + // Keep one representative backup per week for `weekly` weeks + const now = new Date(); + for (let week = 0; week < weekly; week++) { + const weekStart = new Date(now); + weekStart.setDate(weekStart.getDate() - (week + 1) * 7); + const weekEnd = new Date(now); + weekEnd.setDate(weekEnd.getDate() - week * 7); + + const weekBackup = all.find((b) => { + const ts = new Date(b.createdAt); + return ts >= weekStart && ts < weekEnd; + }); + + if (weekBackup) { + toKeep.add(weekBackup.id); + } + } + + const deleted = []; + for (const backup of all) { + if (!toKeep.has(backup.id)) { + try { + unlinkSync(path.join(dir, backup.filename)); + deleted.push(backup.id); + info('Pruned old backup', { id: backup.id }); + } catch (err) { + logError('Failed to prune backup', { id: backup.id, error: err.message }); + } + } + } + + return deleted; +} + +/** + * Start the scheduled backup job. + * Creates a backup at the given interval (default: every 24 hours) + * and prunes old backups after each run. + * + * @param {{intervalMs?: number, retention?: {daily?: number, weekly?: number}, backupDir?: string}} [opts] + * @returns {void} + */ +export function startScheduledBackups(opts = {}) { + const { + intervalMs = 24 * 60 * 60 * 1000, // 24 hours + retention, + backupDir, + } = opts; + + if (scheduledBackupInterval) { + warn('Scheduled backups already running — skipping duplicate start'); + return; + } + + info('Starting scheduled config backups', { intervalMs }); + + scheduledBackupInterval = setInterval(() => { + try { + createBackup(backupDir); + pruneBackups(retention, backupDir); + } catch (err) { + logError('Scheduled backup failed', { error: err.message }); + } + }, intervalMs); + + // Prevent the interval from keeping the process alive unnecessarily (e.g. in tests) + if (typeof scheduledBackupInterval.unref === 'function') { + scheduledBackupInterval.unref(); + } +} + +/** + * Stop the scheduled backup job. + * + * @returns {void} + */ +export function stopScheduledBackups() { + if (scheduledBackupInterval) { + clearInterval(scheduledBackupInterval); + scheduledBackupInterval = null; + info('Stopped scheduled config backups'); + } +} diff --git a/tests/api/routes/backup.test.js b/tests/api/routes/backup.test.js new file mode 100644 index 000000000..430b97536 --- /dev/null +++ b/tests/api/routes/backup.test.js @@ -0,0 +1,306 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import jwt from 'jsonwebtoken'; +import request from 'supertest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// --- Mocks --- + +vi.mock('../../../src/logger.js', () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); + +vi.mock('../../../src/modules/config.js', () => ({ + getConfig: vi.fn().mockReturnValue({ + ai: { enabled: true, model: 'claude-3' }, + welcome: { enabled: false }, + spam: { enabled: true }, + moderation: { enabled: true }, + triage: { enabled: true, classifyApiKey: 'sk-secret', respondApiKey: 'sk-resp' }, + permissions: { botOwners: ['owner-user-id'] }, + }), + setConfigValue: vi.fn().mockResolvedValue({}), +})); + +vi.mock('../../../src/api/utils/configAllowlist.js', () => ({ + SAFE_CONFIG_KEYS: new Set(['ai', 'welcome', 'spam', 'moderation', 'triage']), + SENSITIVE_FIELDS: new Set(['triage.classifyApiKey', 'triage.respondApiKey']), + READABLE_CONFIG_KEYS: ['ai', 'welcome', 'spam', 'moderation', 'triage', 'logging'], + maskSensitiveFields: vi.fn((c) => c), + stripMaskedWrites: vi.fn((w) => w), + isMasked: vi.fn(() => false), + MASK: '••••••••', +})); + +vi.mock('../../../src/api/utils/validateWebhookUrl.js', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, validateDnsResolution: vi.fn().mockResolvedValue(true) }; +}); + +// Mock backup module so tests don't touch the filesystem via default paths +let mockBackups = []; + +vi.mock('../../../src/modules/backup.js', () => ({ + exportConfig: vi.fn(() => ({ + config: { ai: { enabled: true } }, + exportedAt: new Date().toISOString(), + version: 1, + })), + importConfig: vi.fn().mockResolvedValue({ applied: ['ai.enabled'], skipped: [], failed: [] }), + validateImportPayload: vi.fn((p) => { + if (!p || typeof p !== 'object' || !('config' in p)) + return ['Import payload must have a "config" key']; + return []; + }), + listBackups: vi.fn(() => mockBackups), + createBackup: vi.fn(() => ({ + id: 'backup-2026-03-01T12-00-00', + size: 1024, + createdAt: new Date().toISOString(), + })), + readBackup: vi.fn((id) => { + if (id === 'not-found') throw new Error('Backup not found: not-found'); + if (id === 'bad-id-traversal') throw new Error('Invalid backup ID'); + return { config: { ai: { enabled: true } }, exportedAt: new Date().toISOString(), version: 1 }; + }), + restoreBackup: vi.fn().mockResolvedValue({ applied: ['ai.enabled'], skipped: [], failed: [] }), + pruneBackups: vi.fn(() => ['backup-old-1', 'backup-old-2']), + startScheduledBackups: vi.fn(), + stopScheduledBackups: vi.fn(), + sanitizeConfig: vi.fn((c) => c), + getBackupDir: vi.fn(() => '/tmp/mock-backups'), +})); + +import { _resetSecretCache } from '../../../src/api/middleware/verifyJwt.js'; +import { createApp } from '../../../src/api/server.js'; +import { guildCache } from '../../../src/api/utils/discordApi.js'; +import { sessionStore } from '../../../src/api/utils/sessionStore.js'; + +describe('backup routes', () => { + let app; + const SECRET = 'test-backup-secret'; + + beforeEach(() => { + vi.stubEnv('BOT_API_SECRET', SECRET); + mockBackups = []; + + const client = { + guilds: { cache: new Map() }, + ws: { status: 0, ping: 42 }, + user: { tag: 'Bot#1234' }, + }; + app = createApp(client, null); + }); + + afterEach(() => { + sessionStore.clear(); + guildCache.clear(); + _resetSecretCache(); + vi.clearAllMocks(); + vi.unstubAllEnvs(); + vi.restoreAllMocks(); + }); + + function createOwnerToken(secret = 'jwt-test-secret', userId = 'owner-user-id') { + sessionStore.set(userId, 'discord-access-token'); + return jwt.sign({ userId, username: 'owner' }, secret, { algorithm: 'HS256' }); + } + + function createNonOwnerToken(secret = 'jwt-test-secret', userId = 'normal-user') { + sessionStore.set(userId, 'discord-access-token'); + return jwt.sign({ userId, username: 'nobody' }, secret, { algorithm: 'HS256' }); + } + + // --- Auth checks --- + + describe('auth', () => { + it('rejects unauthenticated requests', async () => { + const res = await request(app).get('/api/v1/backups'); + expect(res.status).toBe(401); + }); + + it('rejects non-owner OAuth users', async () => { + vi.stubEnv('SESSION_SECRET', 'jwt-test-secret'); + const token = createNonOwnerToken(); + const res = await request(app).get('/api/v1/backups').set('Authorization', `Bearer ${token}`); + expect(res.status).toBe(403); + }); + + it('allows api-secret', async () => { + const res = await request(app).get('/api/v1/backups').set('x-api-secret', SECRET); + expect(res.status).toBe(200); + }); + + it('allows bot-owner OAuth', async () => { + vi.stubEnv('SESSION_SECRET', 'jwt-test-secret'); + const token = createOwnerToken(); + const res = await request(app).get('/api/v1/backups').set('Authorization', `Bearer ${token}`); + expect(res.status).toBe(200); + }); + }); + + // --- GET /backups --- + + describe('GET /backups', () => { + it('returns empty array when no backups', async () => { + const res = await request(app).get('/api/v1/backups').set('x-api-secret', SECRET); + expect(res.status).toBe(200); + expect(res.body).toEqual([]); + }); + + it('returns list of backups', async () => { + mockBackups = [ + { + id: 'backup-2026-03-01T12-00-00', + filename: 'backup-2026-03-01T12-00-00.json', + createdAt: '2026-03-01T12:00:00Z', + size: 512, + }, + ]; + const res = await request(app).get('/api/v1/backups').set('x-api-secret', SECRET); + expect(res.status).toBe(200); + expect(res.body.length).toBe(1); + expect(res.body[0].id).toBe('backup-2026-03-01T12-00-00'); + }); + }); + + // --- POST /backups --- + + describe('POST /backups', () => { + it('creates a backup and returns 201', async () => { + const res = await request(app).post('/api/v1/backups').set('x-api-secret', SECRET); + expect(res.status).toBe(201); + expect(res.body.id).toBe('backup-2026-03-01T12-00-00'); + expect(res.body.size).toBe(1024); + }); + + it('returns 500 on backup creation failure', async () => { + const { createBackup } = await import('../../../src/modules/backup.js'); + createBackup.mockImplementationOnce(() => { + throw new Error('disk full'); + }); + + const res = await request(app).post('/api/v1/backups').set('x-api-secret', SECRET); + expect(res.status).toBe(500); + expect(res.body.error).toContain('Failed to create backup'); + }); + }); + + // --- GET /backups/export --- + + describe('GET /backups/export', () => { + it('returns config JSON with content-disposition attachment', async () => { + const res = await request(app).get('/api/v1/backups/export').set('x-api-secret', SECRET); + expect(res.status).toBe(200); + expect(res.headers['content-disposition']).toMatch(/attachment/); + expect(res.body).toHaveProperty('config'); + expect(res.body).toHaveProperty('exportedAt'); + expect(res.body.version).toBe(1); + }); + }); + + // --- POST /backups/import --- + + describe('POST /backups/import', () => { + it('imports a valid payload', async () => { + const res = await request(app) + .post('/api/v1/backups/import') + .set('x-api-secret', SECRET) + .send({ config: { ai: { enabled: false } } }); + expect(res.status).toBe(200); + expect(res.body).toHaveProperty('applied'); + }); + + it('rejects invalid payload', async () => { + const { validateImportPayload } = await import('../../../src/modules/backup.js'); + validateImportPayload.mockReturnValueOnce(['Import payload must have a "config" key']); + + const res = await request(app) + .post('/api/v1/backups/import') + .set('x-api-secret', SECRET) + .send({}); + expect(res.status).toBe(400); + expect(res.body.error).toContain('Invalid import payload'); + }); + }); + + // --- GET /backups/:id/download --- + + describe('GET /backups/:id/download', () => { + it('downloads a specific backup', async () => { + const res = await request(app) + .get('/api/v1/backups/backup-2026-03-01T12-00-00/download') + .set('x-api-secret', SECRET); + expect(res.status).toBe(200); + expect(res.headers['content-disposition']).toMatch(/attachment/); + expect(res.body).toHaveProperty('config'); + }); + + it('returns 404 for unknown backup', async () => { + const res = await request(app) + .get('/api/v1/backups/not-found/download') + .set('x-api-secret', SECRET); + expect(res.status).toBe(404); + }); + + it('returns 400 for invalid (path-traversal) backup id', async () => { + const { readBackup } = await import('../../../src/modules/backup.js'); + readBackup.mockImplementationOnce(() => { + throw new Error('Invalid backup ID'); + }); + const res = await request(app) + .get('/api/v1/backups/bad-id-traversal/download') + .set('x-api-secret', SECRET); + expect(res.status).toBe(400); + }); + }); + + // --- POST /backups/:id/restore --- + + describe('POST /backups/:id/restore', () => { + it('restores from a valid backup', async () => { + const res = await request(app) + .post('/api/v1/backups/backup-2026-03-01T12-00-00/restore') + .set('x-api-secret', SECRET); + expect(res.status).toBe(200); + expect(res.body).toHaveProperty('applied'); + }); + + it('returns 404 for unknown backup', async () => { + const { restoreBackup } = await import('../../../src/modules/backup.js'); + restoreBackup.mockRejectedValueOnce(new Error('Backup not found: not-found')); + const res = await request(app) + .post('/api/v1/backups/not-found/restore') + .set('x-api-secret', SECRET); + expect(res.status).toBe(404); + }); + }); + + // --- POST /backups/prune --- + + describe('POST /backups/prune', () => { + it('prunes with default retention', async () => { + const res = await request(app) + .post('/api/v1/backups/prune') + .set('x-api-secret', SECRET) + .send({}); + expect(res.status).toBe(200); + expect(res.body.deleted).toEqual(['backup-old-1', 'backup-old-2']); + expect(res.body.count).toBe(2); + }); + + it('prunes with custom retention', async () => { + const { pruneBackups } = await import('../../../src/modules/backup.js'); + pruneBackups.mockReturnValueOnce([]); + const res = await request(app) + .post('/api/v1/backups/prune') + .set('x-api-secret', SECRET) + .send({ daily: 10, weekly: 8 }); + expect(res.status).toBe(200); + expect(res.body.count).toBe(0); + }); + }); +}); diff --git a/tests/modules/backup.test.js b/tests/modules/backup.test.js new file mode 100644 index 000000000..cbcdfcecc --- /dev/null +++ b/tests/modules/backup.test.js @@ -0,0 +1,309 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +// --- Mocks --- + +vi.mock('../../src/logger.js', () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), +})); + +vi.mock('../../src/modules/config.js', () => ({ + getConfig: vi.fn().mockReturnValue({ + ai: { enabled: true, model: 'claude-3' }, + welcome: { enabled: false, channelId: 'ch1' }, + spam: { enabled: true }, + moderation: { enabled: true }, + triage: { + enabled: true, + classifyApiKey: 'sk-real-secret', + respondApiKey: 'sk-respond-secret', + }, + }), + setConfigValue: vi.fn().mockResolvedValue({}), +})); + +vi.mock('../../src/api/utils/configAllowlist.js', () => ({ + SAFE_CONFIG_KEYS: new Set(['ai', 'welcome', 'spam', 'moderation', 'triage']), + SENSITIVE_FIELDS: new Set(['triage.classifyApiKey', 'triage.respondApiKey']), +})); + +vi.mock('../../src/api/routes/config.js', () => ({ + flattenToLeafPaths: (obj, prefix) => { + const results = []; + for (const [key, value] of Object.entries(obj)) { + const dotPath = `${prefix}.${key}`; + if (typeof value === 'object' && value !== null && !Array.isArray(value)) { + // Recursively flatten + for (const [k2, v2] of Object.entries(value)) { + results.push([`${dotPath}.${k2}`, v2]); + } + } else { + results.push([dotPath, value]); + } + } + return results; + }, +})); + +import { + createBackup, + exportConfig, + getBackupDir, + importConfig, + listBackups, + pruneBackups, + readBackup, + restoreBackup, + sanitizeConfig, + startScheduledBackups, + stopScheduledBackups, + validateImportPayload, +} from '../../src/modules/backup.js'; + +// --- Helpers --- + +let tmpDir; + +beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), 'backup-test-')); + vi.clearAllMocks(); +}); + +afterEach(() => { + stopScheduledBackups(); + try { + rmSync(tmpDir, { recursive: true, force: true }); + } catch { + /* ignore */ + } +}); + +// --- sanitizeConfig --- + +describe('sanitizeConfig', () => { + it('replaces sensitive fields with [REDACTED]', () => { + const config = { + triage: { enabled: true, classifyApiKey: 'sk-real-key', respondApiKey: 'sk-resp' }, + ai: { model: 'claude' }, + }; + const result = sanitizeConfig(config); + expect(result.triage.classifyApiKey).toBe('[REDACTED]'); + expect(result.triage.respondApiKey).toBe('[REDACTED]'); + expect(result.triage.enabled).toBe(true); + expect(result.ai.model).toBe('claude'); + }); + + it('does not mutate the original config', () => { + const config = { triage: { classifyApiKey: 'real', respondApiKey: 'real2' } }; + sanitizeConfig(config); + expect(config.triage.classifyApiKey).toBe('real'); + }); + + it('handles missing sensitive paths gracefully', () => { + const config = { ai: { enabled: true } }; + expect(() => sanitizeConfig(config)).not.toThrow(); + }); +}); + +// --- exportConfig --- + +describe('exportConfig', () => { + it('returns only SAFE_CONFIG_KEYS sections', () => { + const { config } = exportConfig(); + expect(Object.keys(config)).toEqual( + expect.arrayContaining(['ai', 'welcome', 'spam', 'moderation', 'triage']), + ); + }); + + it('redacts sensitive fields', () => { + const { config } = exportConfig(); + expect(config.triage.classifyApiKey).toBe('[REDACTED]'); + }); + + it('includes exportedAt and version', () => { + const payload = exportConfig(); + expect(payload.exportedAt).toMatch(/^\d{4}-/); + expect(payload.version).toBe(1); + }); +}); + +// --- validateImportPayload --- + +describe('validateImportPayload', () => { + it('rejects null', () => { + expect(validateImportPayload(null)).toContain('Import payload must be a JSON object'); + }); + + it('rejects array', () => { + expect(validateImportPayload([])).toContain('Import payload must be a JSON object'); + }); + + it('rejects payload without config key', () => { + expect(validateImportPayload({})).toContain('Import payload must have a "config" key'); + }); + + it('rejects non-object config', () => { + expect(validateImportPayload({ config: 'bad' })).toContain('"config" must be a JSON object'); + }); + + it('rejects unknown config section keys', () => { + const errors = validateImportPayload({ config: { unknown_section: {} } }); + expect(errors.some((e) => e.includes('"unknown_section"'))).toBe(true); + }); + + it('accepts valid payload', () => { + const errors = validateImportPayload({ config: { ai: { enabled: true } } }); + expect(errors).toHaveLength(0); + }); +}); + +// --- importConfig --- + +describe('importConfig', () => { + it('applies non-redacted values', async () => { + const { setConfigValue } = await import('../../src/modules/config.js'); + const payload = { config: { ai: { enabled: false } } }; + const result = await importConfig(payload); + expect(result.applied.length).toBeGreaterThan(0); + expect(setConfigValue).toHaveBeenCalled(); + }); + + it('skips [REDACTED] values', async () => { + const payload = { config: { triage: { classifyApiKey: '[REDACTED]' } } }; + const result = await importConfig(payload); + expect(result.skipped).toContain('triage.classifyApiKey'); + expect(result.applied).not.toContain('triage.classifyApiKey'); + }); + + it('reports failed writes', async () => { + const { setConfigValue } = await import('../../src/modules/config.js'); + setConfigValue.mockRejectedValueOnce(new Error('DB error')); + const payload = { config: { ai: { enabled: true } } }; + const result = await importConfig(payload); + expect(result.failed.length).toBeGreaterThan(0); + expect(result.failed[0].error).toContain('DB error'); + }); +}); + +// --- createBackup / listBackups --- + +describe('createBackup and listBackups', () => { + it('creates a backup file', () => { + const meta = createBackup(tmpDir); + expect(meta.id).toMatch(/^backup-/); + expect(meta.size).toBeGreaterThan(0); + expect(meta.createdAt).toMatch(/^\d{4}-/); + }); + + it('lists created backups sorted newest first', async () => { + createBackup(tmpDir); + await new Promise((r) => setTimeout(r, 10)); // ensure different timestamps (at least ms apart) + createBackup(tmpDir); + const backups = listBackups(tmpDir); + expect(backups.length).toBe(2); + expect(new Date(backups[0].createdAt) >= new Date(backups[1].createdAt)).toBe(true); + }); + + it('returns empty array when no backups', () => { + const emptyDir = mkdtempSync(join(tmpdir(), 'empty-backup-')); + try { + expect(listBackups(emptyDir)).toEqual([]); + } finally { + rmSync(emptyDir, { recursive: true }); + } + }); +}); + +// --- readBackup --- + +describe('readBackup', () => { + it('reads a valid backup by id', () => { + const meta = createBackup(tmpDir); + const payload = readBackup(meta.id, tmpDir); + expect(payload).toHaveProperty('config'); + expect(payload).toHaveProperty('exportedAt'); + }); + + it('throws for unknown id', () => { + expect(() => readBackup('backup-9999-01-01T00-00-00', tmpDir)).toThrow('not found'); + }); + + it('throws for path-traversal attempts', () => { + expect(() => readBackup('../etc/passwd', tmpDir)).toThrow('Invalid backup ID'); + expect(() => readBackup('..\\windows\\system32', tmpDir)).toThrow('Invalid backup ID'); + }); + + it('throws for corrupted backup', () => { + const badFile = join(tmpDir, 'backup-2020-01-01T00-00-00.json'); + writeFileSync(badFile, 'not json', 'utf8'); + expect(() => readBackup('backup-2020-01-01T00-00-00', tmpDir)).toThrow('corrupted'); + }); +}); + +// --- restoreBackup --- + +describe('restoreBackup', () => { + it('restores config from a valid backup', async () => { + const meta = createBackup(tmpDir); + const result = await restoreBackup(meta.id, tmpDir); + expect(result).toHaveProperty('applied'); + expect(result).toHaveProperty('skipped'); + expect(result).toHaveProperty('failed'); + }); + + it('throws for invalid backup format', () => { + const badFile = join(tmpDir, 'backup-2020-01-01T00-00-00.json'); + writeFileSync(badFile, JSON.stringify({ bad: 'payload' }), 'utf8'); + expect(restoreBackup('backup-2020-01-01T00-00-00', tmpDir)).rejects.toThrow( + 'Invalid backup format', + ); + }); +}); + +// --- pruneBackups --- + +describe('pruneBackups', () => { + it('keeps the N most recent backups', () => { + for (let i = 0; i < 5; i++) { + createBackup(tmpDir); + } + const deleted = pruneBackups({ daily: 3, weekly: 0 }, tmpDir); + expect(deleted.length).toBe(2); + expect(listBackups(tmpDir).length).toBe(3); + }); + + it('keeps zero backups when daily=0 and weekly=0', () => { + createBackup(tmpDir); + createBackup(tmpDir); + const deleted = pruneBackups({ daily: 0, weekly: 0 }, tmpDir); + expect(deleted.length).toBe(2); + expect(listBackups(tmpDir).length).toBe(0); + }); + + it('returns empty array when no backups exist', () => { + expect(pruneBackups({}, tmpDir)).toEqual([]); + }); +}); + +// --- startScheduledBackups / stopScheduledBackups --- + +describe('scheduled backups', () => { + it('starts without throwing', () => { + expect(() => startScheduledBackups({ intervalMs: 999999, backupDir: tmpDir })).not.toThrow(); + }); + + it('logs a warning on duplicate start', () => { + startScheduledBackups({ intervalMs: 999999, backupDir: tmpDir }); + // A second start call should not throw (guard prevents double-start) + expect(() => startScheduledBackups({ intervalMs: 999999, backupDir: tmpDir })).not.toThrow(); + }); + + it('stops cleanly', () => { + startScheduledBackups({ intervalMs: 999999, backupDir: tmpDir }); + expect(() => stopScheduledBackups()).not.toThrow(); + }); +}); From 61a2dca1f51cb363d6c5555a90fb02ad0540f9d6 Mon Sep 17 00:00:00 2001 From: Bill Chirico Date: Mon, 2 Mar 2026 06:42:26 -0500 Subject: [PATCH 2/3] fix: review feedback for backup feature --- src/api/middleware/requireGlobalAdmin.js | 51 ++++++ src/api/routes/backup.js | 203 +++++++++++++---------- src/api/routes/config.js | 58 +------ src/modules/backup.js | 17 +- src/utils/flattenToLeafPaths.js | 25 +++ tests/api/routes/backup.test.js | 3 - tests/modules/backup.test.js | 5 +- 7 files changed, 213 insertions(+), 149 deletions(-) create mode 100644 src/api/middleware/requireGlobalAdmin.js create mode 100644 src/utils/flattenToLeafPaths.js diff --git a/src/api/middleware/requireGlobalAdmin.js b/src/api/middleware/requireGlobalAdmin.js new file mode 100644 index 000000000..6f5b75b40 --- /dev/null +++ b/src/api/middleware/requireGlobalAdmin.js @@ -0,0 +1,51 @@ +import { getConfig } from '../../modules/config.js'; +import { getBotOwnerIds } from '../../utils/permissions.js'; +import { warn } from '../../logger.js'; + +/** + * Middleware: restrict to API-secret callers or bot-owner OAuth users. + */ +export function requireGlobalAdmin(forResource, req, res, next) { + console.error('[DEBUG] requireGlobalAdmin called'); + console.error('[DEBUG] arguments.length:', arguments.length); + console.error('[DEBUG] typeof forResource:', typeof forResource); + + // Support both requireGlobalAdmin(req, res, next) and requireGlobalAdmin('Resource', req, res, next) + if (arguments.length === 3) { + console.error('[DEBUG] 3-arg case: shifting parameters'); + // Called as requireGlobalAdmin(req, res, next) + // Parameters are shifted: forResource=req, req=res, res=next, next=undefined + next = res; // res parameter is actually the next function + res = req; // req parameter is actually the res object + req = forResource; // forResource is the actual req object + forResource = 'Global admin access'; + } else { + forResource = forResource || 'Global admin access'; + } + + console.error('[DEBUG] After shift - authMethod:', req.authMethod, 'userId:', req.user?.userId); + + if (req.authMethod === 'api-secret') { + console.error('[DEBUG] api-secret - calling next()'); + return next(); + } + + if (req.authMethod === 'oauth') { + const config = getConfig(); + const botOwners = getBotOwnerIds(config); + console.error('[DEBUG] oauth - botOwners:', botOwners, 'userId:', req.user?.userId); + if (botOwners.includes(req.user?.userId)) { + console.error('[DEBUG] oauth owner - calling next()'); + return next(); + } + console.error('[DEBUG] oauth non-owner - returning 403'); + return res.status(403).json({ error: `${forResource} requires bot owner permissions` }); + } + + console.error('[DEBUG] unknown authMethod - returning 401'); + warn('Unknown authMethod in global admin check', { + authMethod: req.authMethod, + path: req.path, + }); + return res.status(401).json({ error: 'Unauthorized' }); +} diff --git a/src/api/routes/backup.js b/src/api/routes/backup.js index b3733e424..cd2e23917 100644 --- a/src/api/routes/backup.js +++ b/src/api/routes/backup.js @@ -8,7 +8,6 @@ */ import { Router } from 'express'; -import { warn } from '../../logger.js'; import { createBackup, exportConfig, @@ -19,29 +18,10 @@ import { restoreBackup, validateImportPayload, } from '../../modules/backup.js'; -import { getConfig } from '../../modules/config.js'; -import { getBotOwnerIds } from '../../utils/permissions.js'; +import { requireGlobalAdmin } from '../middleware/requireGlobalAdmin.js'; const router = Router(); -/** - * Middleware — restricts access to API-secret callers or bot-owner OAuth users. - * Mirrors the requireGlobalAdmin guard used by the config routes. - */ -function requireGlobalAdmin(req, res, next) { - if (req.authMethod === 'api-secret') return next(); - - if (req.authMethod === 'oauth') { - const botOwners = getBotOwnerIds(getConfig()); - if (botOwners.includes(req.user?.userId)) return next(); - - return res.status(403).json({ error: 'Backup access requires bot owner permissions' }); - } - - warn('Unknown authMethod in backup route', { authMethod: req.authMethod, path: req.path }); - return res.status(401).json({ error: 'Unauthorized' }); -} - /** * @openapi * /backups/export: @@ -76,14 +56,18 @@ function requireGlobalAdmin(req, res, next) { * "403": * $ref: "#/components/responses/Forbidden" */ -router.get('/export', requireGlobalAdmin, (_req, res) => { - const payload = exportConfig(); - const filename = `config-export-${new Date().toISOString().replace(/[:.]/g, '-')}.json`; +router.get( + '/export', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + (_req, res) => { + const payload = exportConfig(); + const filename = `config-export-${new Date().toISOString().replace(/[:.]/g, '-')}.json`; - res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); - res.setHeader('Content-Type', 'application/json'); - res.json(payload); -}); + res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); + res.setHeader('Content-Type', 'application/json'); + res.json(payload); + }, +); /** * @openapi @@ -140,21 +124,25 @@ router.get('/export', requireGlobalAdmin, (_req, res) => { * "403": * $ref: "#/components/responses/Forbidden" */ -router.post('/import', requireGlobalAdmin, async (req, res) => { - const payload = req.body; +router.post( + '/import', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + async (req, res) => { + const payload = req.body; - const validationErrors = validateImportPayload(payload); - if (validationErrors.length > 0) { - return res.status(400).json({ error: 'Invalid import payload', details: validationErrors }); - } + const validationErrors = validateImportPayload(payload); + if (validationErrors.length > 0) { + return res.status(400).json({ error: 'Invalid import payload', details: validationErrors }); + } - try { - const result = await importConfig(payload); - return res.json(result); - } catch (err) { - return res.status(500).json({ error: 'Import failed', details: err.message }); - } -}); + try { + const result = await importConfig(payload); + return res.json(result); + } catch (err) { + return res.status(500).json({ error: 'Import failed', details: err.message }); + } + }, +); /** * @openapi @@ -193,10 +181,14 @@ router.post('/import', requireGlobalAdmin, async (req, res) => { * "403": * $ref: "#/components/responses/Forbidden" */ -router.get('/', requireGlobalAdmin, (_req, res) => { - const backups = listBackups(); - res.json(backups); -}); +router.get( + '/', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + (_req, res) => { + const backups = listBackups(); + res.json(backups); + }, +); /** * @openapi @@ -234,14 +226,18 @@ router.get('/', requireGlobalAdmin, (_req, res) => { * "500": * $ref: "#/components/responses/ServerError" */ -router.post('/', requireGlobalAdmin, (_req, res) => { - try { - const meta = createBackup(); - return res.status(201).json({ id: meta.id, size: meta.size, createdAt: meta.createdAt }); - } catch (err) { - return res.status(500).json({ error: 'Failed to create backup', details: err.message }); - } -}); +router.post( + '/', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + (_req, res) => { + try { + const meta = createBackup(); + return res.status(201).json({ id: meta.id, size: meta.size, createdAt: meta.createdAt }); + } catch (err) { + return res.status(500).json({ error: 'Failed to create backup', details: err.message }); + } + }, +); /** * @openapi @@ -279,24 +275,28 @@ router.post('/', requireGlobalAdmin, (_req, res) => { * "403": * $ref: "#/components/responses/Forbidden" */ -router.get('/:id/download', requireGlobalAdmin, (req, res) => { - const { id } = req.params; +router.get( + '/:id/download', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + (req, res) => { + const { id } = req.params; - try { - const payload = readBackup(id); - const filename = `${id}.json`; - res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); - res.setHeader('Content-Type', 'application/json'); - return res.json(payload); - } catch (err) { - const status = err.message.includes('not found') - ? 404 - : err.message.includes('Invalid') - ? 400 - : 500; - return res.status(status).json({ error: err.message }); - } -}); + try { + const payload = readBackup(id); + const filename = `${id}.json`; + res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); + res.setHeader('Content-Type', 'application/json'); + return res.json(payload); + } catch (err) { + const status = err.message.includes('not found') + ? 404 + : err.message.includes('Invalid') + ? 400 + : 500; + return res.status(status).json({ error: err.message }); + } + }, +); /** * @openapi @@ -350,21 +350,25 @@ router.get('/:id/download', requireGlobalAdmin, (req, res) => { * "500": * $ref: "#/components/responses/ServerError" */ -router.post('/:id/restore', requireGlobalAdmin, async (req, res) => { - const { id } = req.params; +router.post( + '/:id/restore', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + async (req, res) => { + const { id } = req.params; - try { - const result = await restoreBackup(id); - return res.json(result); - } catch (err) { - const status = err.message.includes('not found') - ? 404 - : err.message.includes('Invalid') - ? 400 - : 500; - return res.status(status).json({ error: err.message }); - } -}); + try { + const result = await restoreBackup(id); + return res.json(result); + } catch (err) { + const status = err.message.includes('not found') + ? 404 + : err.message.includes('Invalid') + ? 400 + : 500; + return res.status(status).json({ error: err.message }); + } + }, +); /** * @openapi @@ -413,10 +417,31 @@ router.post('/:id/restore', requireGlobalAdmin, async (req, res) => { * "403": * $ref: "#/components/responses/Forbidden" */ -router.post('/prune', requireGlobalAdmin, (req, res) => { - const retention = req.body ?? {}; - const deleted = pruneBackups(retention); - return res.json({ deleted, count: deleted.length }); -}); +router.post( + '/prune', + (req, res, next) => requireGlobalAdmin('Backup access', req, res, next), + (req, res) => { + const retention = req.body ?? {}; + const errors = []; + + if (retention.daily !== undefined) { + if (!Number.isInteger(retention.daily) || retention.daily < 0) { + errors.push('daily must be a non-negative integer'); + } + } + if (retention.weekly !== undefined) { + if (!Number.isInteger(retention.weekly) || retention.weekly < 0) { + errors.push('weekly must be a non-negative integer'); + } + } + + if (errors.length > 0) { + return res.status(400).json({ error: 'Invalid prune options', details: errors }); + } + + const deleted = pruneBackups(retention); + return res.json({ deleted, count: deleted.length }); + }, +); export default router; diff --git a/src/api/routes/config.js b/src/api/routes/config.js index 15021f9db..4590542e5 100644 --- a/src/api/routes/config.js +++ b/src/api/routes/config.js @@ -6,7 +6,7 @@ import { Router } from 'express'; import { error, info, warn } from '../../logger.js'; import { getConfig, setConfigValue } from '../../modules/config.js'; -import { getBotOwnerIds } from '../../utils/permissions.js'; +import { flattenToLeafPaths } from '../../utils/flattenToLeafPaths.js'; import { maskSensitiveFields, READABLE_CONFIG_KEYS, @@ -16,6 +16,11 @@ import { import { CONFIG_SCHEMA, validateValue } from '../utils/configValidation.js'; import { fireAndForgetWebhook } from '../utils/webhook.js'; +// Re-export flattenToLeafPaths for backward compatibility +export { flattenToLeafPaths }; + +import { requireGlobalAdmin } from '../middleware/requireGlobalAdmin.js'; + // Re-export validateSingleValue so existing callers that import it from this // module continue to work without changes. export { validateSingleValue } from '../utils/configValidation.js'; @@ -53,53 +58,6 @@ export function validateConfigSchema(config) { return errors; } -/** Keys that must be skipped during object traversal to prevent prototype pollution. */ -const DANGEROUS_KEYS = new Set(['__proto__', 'constructor', 'prototype']); - -/** - * Flattens a nested object into dot-notated leaf path/value pairs, using the provided prefix as the root path. - * @param {Object} obj - The object to flatten. - * @param {string} prefix - The starting dot-notated prefix (for example, "section"). - * @returns {Array<[string, any]>} An array of [path, value] pairs where path is the dot-notated key and value is the leaf value. Arrays and primitive values are treated as leaves; dangerous keys ('__proto__', 'constructor', 'prototype') are skipped. - */ -export function flattenToLeafPaths(obj, prefix) { - const results = []; - - for (const [key, value] of Object.entries(obj)) { - if (DANGEROUS_KEYS.has(key)) continue; - const path = `${prefix}.${key}`; - - if (typeof value === 'object' && value !== null && !Array.isArray(value)) { - results.push(...flattenToLeafPaths(value, path)); - } else { - results.push([path, value]); - } - } - - return results; -} - -/** - * Middleware: restrict to API-secret callers or bot-owner OAuth users. - * Global config changes affect all guilds, so only trusted callers are allowed. - */ -function requireGlobalAdmin(req, res, next) { - if (req.authMethod === 'api-secret') return next(); - - if (req.authMethod === 'oauth') { - const botOwners = getBotOwnerIds(getConfig()); - if (botOwners.includes(req.user?.userId)) return next(); - - return res.status(403).json({ error: 'Global config access requires bot owner permissions' }); - } - - warn('Unknown authMethod in global config check', { - authMethod: req.authMethod, - path: req.path, - }); - return res.status(401).json({ error: 'Unauthorized' }); -} - /** * @openapi * /config: @@ -124,7 +82,9 @@ function requireGlobalAdmin(req, res, next) { * "403": * $ref: "#/components/responses/Forbidden" */ -router.get('/', requireGlobalAdmin, (_req, res) => { +router.get("/", requireGlobalAdmin, (req, res) => { + const fs = require("fs"); + try { fs.writeFileSync("/tmp/config-route.log", "Route handler called at " + Date.now()); } catch(e) {} const config = getConfig(); const safeConfig = {}; diff --git a/src/modules/backup.js b/src/modules/backup.js index 3cdf80c5f..981aae200 100644 --- a/src/modules/backup.js +++ b/src/modules/backup.js @@ -14,11 +14,14 @@ import { unlinkSync, writeFileSync, } from 'node:fs'; + +// TODO: Consider switching to fs.promises for async operations to improve performance +// and avoid blocking the event loop with synchronous file system operations. import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { flattenToLeafPaths } from '../api/routes/config.js'; import { SAFE_CONFIG_KEYS, SENSITIVE_FIELDS } from '../api/utils/configAllowlist.js'; import { info, error as logError, warn } from '../logger.js'; +import { flattenToLeafPaths } from '../utils/flattenToLeafPaths.js'; import { getConfig, setConfigValue } from './config.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -123,9 +126,11 @@ export function validateImportPayload(payload) { } const errors = []; - for (const key of Object.keys(config)) { + for (const [key, value] of Object.entries(config)) { if (!SAFE_CONFIG_KEYS.has(key)) { errors.push(`"${key}" is not a writable config section`); + } else if (typeof value !== 'object' || value === null) { + errors.push(`"${key}" must be an object or array`); } } @@ -173,7 +178,7 @@ export async function importConfig(payload) { * Generate a backup filename for the given date. * * @param {Date} [date] - Date to use (defaults to now) - * @returns {string} Filename like "backup-2026-03-01T12-00-00.json" + * @returns {string} Filename like "backup-2026-03-01T12-00-00-000-0000.json" (includes milliseconds and sequence suffix) */ function makeBackupFilename(date = new Date()) { // Include milliseconds for precision; append an incrementing sequence to guarantee uniqueness @@ -276,8 +281,10 @@ export function listBackups(backupDir) { * @throws {Error} If backup file not found or invalid */ export function readBackup(id, backupDir) { - // Prevent path traversal attacks - if (id.includes('/') || id.includes('..') || id.includes('\\')) { + // Validate ID against strict pattern: backup-YYYY-MM-DDTHH-mm-ss-SSS-NNNN + const BACKUP_ID_PATTERN = + /^backup-[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}-[0-9]{2}-[0-9]{2}-[0-9]{3}-[0-9]{4}$/; + if (!BACKUP_ID_PATTERN.test(id)) { throw new Error('Invalid backup ID'); } diff --git a/src/utils/flattenToLeafPaths.js b/src/utils/flattenToLeafPaths.js new file mode 100644 index 000000000..a223dfd9f --- /dev/null +++ b/src/utils/flattenToLeafPaths.js @@ -0,0 +1,25 @@ +/** Keys that must be skipped during object traversal to prevent prototype pollution. */ +const DANGEROUS_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + +/** + * Flattens a nested object into dot-notated leaf path/value pairs, using the provided prefix as the root path. + * @param {Object} obj - The object to flatten. + * @param {string} prefix - The starting dot-notated prefix (for example, "section"). + * @returns {Array<[string, any]>} An array of [path, value] pairs where path is the dot-notated key and value is the leaf value. Arrays and primitive values are treated as leaves; dangerous keys ('__proto__', 'constructor', 'prototype') are skipped. + */ +export function flattenToLeafPaths(obj, prefix) { + const results = []; + + for (const [key, value] of Object.entries(obj)) { + if (DANGEROUS_KEYS.has(key)) continue; + const path = `${prefix}.${key}`; + + if (typeof value === 'object' && value !== null && !Array.isArray(value)) { + results.push(...flattenToLeafPaths(value, path)); + } else { + results.push([path, value]); + } + } + + return results; +} diff --git a/tests/api/routes/backup.test.js b/tests/api/routes/backup.test.js index 430b97536..185a221fa 100644 --- a/tests/api/routes/backup.test.js +++ b/tests/api/routes/backup.test.js @@ -1,6 +1,3 @@ -import { mkdtempSync, rmSync } from 'node:fs'; -import { tmpdir } from 'node:os'; -import { join } from 'node:path'; import jwt from 'jsonwebtoken'; import request from 'supertest'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; diff --git a/tests/modules/backup.test.js b/tests/modules/backup.test.js index cbcdfcecc..7280d206d 100644 --- a/tests/modules/backup.test.js +++ b/tests/modules/backup.test.js @@ -1,4 +1,4 @@ -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -31,7 +31,7 @@ vi.mock('../../src/api/utils/configAllowlist.js', () => ({ SENSITIVE_FIELDS: new Set(['triage.classifyApiKey', 'triage.respondApiKey']), })); -vi.mock('../../src/api/routes/config.js', () => ({ +vi.mock('../../src/utils/flattenToLeafPaths.js', () => ({ flattenToLeafPaths: (obj, prefix) => { const results = []; for (const [key, value] of Object.entries(obj)) { @@ -52,7 +52,6 @@ vi.mock('../../src/api/routes/config.js', () => ({ import { createBackup, exportConfig, - getBackupDir, importConfig, listBackups, pruneBackups, From 1ccd7125e5b8fe85e296f88a019ec1a7dc444f3a Mon Sep 17 00:00:00 2001 From: Bill Chirico Date: Mon, 2 Mar 2026 06:49:32 -0500 Subject: [PATCH 3/3] fix: resolve merge conflicts and remove debug code --- node_modules | 1 - src/api/middleware/requireGlobalAdmin.js | 18 +++--------------- src/api/routes/config.js | 4 +--- src/index.js | 1 + 4 files changed, 5 insertions(+), 19 deletions(-) delete mode 120000 node_modules diff --git a/node_modules b/node_modules deleted file mode 120000 index 87273d9fd..000000000 --- a/node_modules +++ /dev/null @@ -1 +0,0 @@ -/home/bill/volvox-bot/node_modules \ No newline at end of file diff --git a/src/api/middleware/requireGlobalAdmin.js b/src/api/middleware/requireGlobalAdmin.js index 6f5b75b40..ff7c545ee 100644 --- a/src/api/middleware/requireGlobalAdmin.js +++ b/src/api/middleware/requireGlobalAdmin.js @@ -1,48 +1,36 @@ +import { warn } from '../../logger.js'; import { getConfig } from '../../modules/config.js'; import { getBotOwnerIds } from '../../utils/permissions.js'; -import { warn } from '../../logger.js'; /** * Middleware: restrict to API-secret callers or bot-owner OAuth users. */ export function requireGlobalAdmin(forResource, req, res, next) { - console.error('[DEBUG] requireGlobalAdmin called'); - console.error('[DEBUG] arguments.length:', arguments.length); - console.error('[DEBUG] typeof forResource:', typeof forResource); - // Support both requireGlobalAdmin(req, res, next) and requireGlobalAdmin('Resource', req, res, next) if (arguments.length === 3) { - console.error('[DEBUG] 3-arg case: shifting parameters'); // Called as requireGlobalAdmin(req, res, next) // Parameters are shifted: forResource=req, req=res, res=next, next=undefined - next = res; // res parameter is actually the next function - res = req; // req parameter is actually the res object + next = res; // res parameter is actually the next function + res = req; // req parameter is actually the res object req = forResource; // forResource is the actual req object forResource = 'Global admin access'; } else { forResource = forResource || 'Global admin access'; } - console.error('[DEBUG] After shift - authMethod:', req.authMethod, 'userId:', req.user?.userId); - if (req.authMethod === 'api-secret') { - console.error('[DEBUG] api-secret - calling next()'); return next(); } if (req.authMethod === 'oauth') { const config = getConfig(); const botOwners = getBotOwnerIds(config); - console.error('[DEBUG] oauth - botOwners:', botOwners, 'userId:', req.user?.userId); if (botOwners.includes(req.user?.userId)) { - console.error('[DEBUG] oauth owner - calling next()'); return next(); } - console.error('[DEBUG] oauth non-owner - returning 403'); return res.status(403).json({ error: `${forResource} requires bot owner permissions` }); } - console.error('[DEBUG] unknown authMethod - returning 401'); warn('Unknown authMethod in global admin check', { authMethod: req.authMethod, path: req.path, diff --git a/src/api/routes/config.js b/src/api/routes/config.js index 4590542e5..639eb7893 100644 --- a/src/api/routes/config.js +++ b/src/api/routes/config.js @@ -82,9 +82,7 @@ export function validateConfigSchema(config) { * "403": * $ref: "#/components/responses/Forbidden" */ -router.get("/", requireGlobalAdmin, (req, res) => { - const fs = require("fs"); - try { fs.writeFileSync("/tmp/config-route.log", "Route handler called at " + Date.now()); } catch(e) {} +router.get('/', requireGlobalAdmin, (req, res) => { const config = getConfig(); const safeConfig = {}; diff --git a/src/index.js b/src/index.js index ae819b454..12d6cb933 100644 --- a/src/index.js +++ b/src/index.js @@ -520,6 +520,7 @@ async function startup() { startGithubFeed(client); startScheduledBackups(); startVoiceFlush(); + } // Load commands and login await loadCommands(); await client.login(token);