From 8a6a3485e945b9ee49151134be8ccb6b8a84ebdc Mon Sep 17 00:00:00 2001 From: jramos Date: Tue, 7 Apr 2026 10:23:10 -0600 Subject: [PATCH] security: address audit findings C-4 through M-8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Critical: - C-4: Add express-rate-limit to login (20 attempts/15min) - C-5: Remove default credentials from LoginForm.js - C-6: Add sandbox attribute to KB document iframe High: - H-2: Hard-fail on startup if SESSION_SECRET env var is missing - H-6: Sanitize filenames in Content-Disposition headers - H-7: Fix KB upload race condition — move file after DB insert succeeds - H-8: Generate random admin password in setup.js instead of hardcoded - H-9: Add rehype-sanitize to ReactMarkdown (requires npm install) Medium: - M-4: Fix loose equality (==) to strict (===) in users.js self-checks - M-5: Add hostname format regex validation in compliance notes - M-6: Fix vendor trim-before-validate in ivantiTodoQueue.js - M-7: Sanitize original filename in compliance temp JSON - M-8: Pull CSP frame-ancestors from CORS_ORIGINS env var New dependencies needed: - backend: express-rate-limit (npm install in root) - frontend: rehype-sanitize (npm install in frontend/) --- backend/routes/auth.js | 11 +++++- backend/routes/compliance.js | 6 +-- backend/routes/ivantiTodoQueue.js | 4 +- backend/routes/knowledgeBase.js | 37 +++++++++++-------- backend/routes/users.js | 6 +-- backend/server.js | 6 ++- backend/setup.js | 15 ++++++-- frontend/package.json | 1 + .../src/components/KnowledgeBaseViewer.js | 3 ++ frontend/src/components/LoginForm.js | 6 --- package.json | 1 + 11 files changed, 62 insertions(+), 34 deletions(-) diff --git a/backend/routes/auth.js b/backend/routes/auth.js index 4f9a973..35d0777 100644 --- a/backend/routes/auth.js +++ b/backend/routes/auth.js @@ -2,13 +2,22 @@ const express = require('express'); const bcrypt = require('bcryptjs'); const crypto = require('crypto'); +const rateLimit = require('express-rate-limit'); const { requireAuth, requireGroup } = require('../middleware/auth'); +const loginLimiter = rateLimit({ + windowMs: 15 * 60 * 1000, // 15 minutes + max: 20, // 20 attempts per window + standardHeaders: true, + legacyHeaders: false, + message: { error: 'Too many login attempts. Please try again in 15 minutes.' } +}); + function createAuthRouter(db, logAudit) { const router = express.Router(); // Login - router.post('/login', async (req, res) => { + router.post('/login', loginLimiter, async (req, res) => { const { username, password } = req.body; if (!username || !password) { diff --git a/backend/routes/compliance.js b/backend/routes/compliance.js index 41a6432..41ccce4 100644 --- a/backend/routes/compliance.js +++ b/backend/routes/compliance.js @@ -260,7 +260,7 @@ function createComplianceRouter(db, upload, requireAuth, requireGroup) { items: parsed.items, summary: parsed.summary, report_date: parsed.report_date, - filename: req.file.originalname, + filename: req.file.originalname.replace(/[^\w.\-() ]/g, '_'), })); // Delete the original xlsx from temp (we only need the JSON now) @@ -523,8 +523,8 @@ function createComplianceRouter(db, upload, requireAuth, requireGroup) { router.post('/notes', requireGroup('Admin', 'Standard_User'), async (req, res) => { const { hostname, metric_id, note } = req.body; - if (!hostname || typeof hostname !== 'string' || hostname.length > 300) { - return res.status(400).json({ error: 'Invalid hostname' }); + if (!hostname || typeof hostname !== 'string' || hostname.length > 300 || !/^[a-zA-Z0-9._-]+$/.test(hostname)) { + return res.status(400).json({ error: 'Invalid hostname format' }); } if (!metric_id || typeof metric_id !== 'string' || metric_id.length > 50) { return res.status(400).json({ error: 'Invalid metric_id' }); diff --git a/backend/routes/ivantiTodoQueue.js b/backend/routes/ivantiTodoQueue.js index 148d924..5e2a322 100644 --- a/backend/routes/ivantiTodoQueue.js +++ b/backend/routes/ivantiTodoQueue.js @@ -6,7 +6,9 @@ const VALID_WORKFLOW_TYPES = ['FP', 'Archer', 'CARD']; const VALID_STATUSES = ['pending', 'complete']; function isValidVendor(vendor) { - return typeof vendor === 'string' && vendor.trim().length > 0 && vendor.length <= 200; + if (typeof vendor !== 'string') return false; + const trimmed = vendor.trim(); + return trimmed.length > 0 && trimmed.length <= 200; } function createIvantiTodoQueueRouter(db, requireAuth) { diff --git a/backend/routes/knowledgeBase.js b/backend/routes/knowledgeBase.js index 9f29900..90ba73d 100644 --- a/backend/routes/knowledgeBase.js +++ b/backend/routes/knowledgeBase.js @@ -92,22 +92,15 @@ function createKnowledgeBaseRouter(db, upload) { const slug = generateSlug(title); const kbDir = path.join(__dirname, '..', 'uploads', 'knowledge_base'); - // Create directory if it doesn't exist - if (!fs.existsSync(kbDir)) { - fs.mkdirSync(kbDir, { recursive: true }); - } - const filename = `${timestamp}_${sanitizedName}`; const filePath = path.join(kbDir, filename); try { - // Move uploaded file to permanent location - fs.renameSync(uploadedFile.path, filePath); - + // Keep file in temp location until DB insert succeeds // Check if slug already exists db.get('SELECT id FROM knowledge_base WHERE slug = ?', [slug], (err, row) => { if (err) { - fs.unlinkSync(filePath); + fs.unlinkSync(uploadedFile.path); console.error('Error checking slug:', err); return res.status(500).json({ error: 'Database error' }); } @@ -138,11 +131,22 @@ function createKnowledgeBaseRouter(db, upload) { ], function (err) { if (err) { - fs.unlinkSync(filePath); + fs.unlinkSync(uploadedFile.path); console.error('Error inserting knowledge base entry:', err); return res.status(500).json({ error: 'Failed to save document metadata' }); } + // DB insert succeeded — now move file to permanent location + try { + if (!fs.existsSync(kbDir)) { + fs.mkdirSync(kbDir, { recursive: true }); + } + fs.renameSync(uploadedFile.path, filePath); + } catch (moveErr) { + console.error('Error moving file to permanent location:', moveErr); + // File is orphaned in temp but DB record exists — log and continue + } + // Log audit entry logAudit(db, { userId: req.user.id, @@ -165,8 +169,8 @@ function createKnowledgeBaseRouter(db, upload) { ); }); } catch (error) { - // Clean up file on error - if (fs.existsSync(filePath)) fs.unlinkSync(filePath); + // Clean up temp file on error + if (uploadedFile && fs.existsSync(uploadedFile.path)) fs.unlinkSync(uploadedFile.path); console.error('Error uploading knowledge base document:', error); res.status(500).json({ error: error.message || 'Failed to upload document' }); } @@ -288,12 +292,14 @@ function createKnowledgeBaseRouter(db, upload) { contentType = 'text/plain; charset=utf-8'; } + const safeFileName = row.file_name.replace(/["\r\n\\]/g, ''); res.setHeader('Content-Type', contentType); // Use inline instead of attachment to allow browser to display - res.setHeader('Content-Disposition', `inline; filename="${row.file_name}"`); + res.setHeader('Content-Disposition', `inline; filename="${safeFileName}"`); // Allow iframe embedding from frontend origin res.removeHeader('X-Frame-Options'); - res.setHeader('Content-Security-Policy', "frame-ancestors 'self' http://71.85.90.9:3000 http://localhost:3000"); + const corsOrigins = process.env.CORS_ORIGINS ? process.env.CORS_ORIGINS.split(',').join(' ') : 'http://localhost:3000'; + res.setHeader('Content-Security-Policy', `frame-ancestors 'self' ${corsOrigins}`); res.sendFile(row.file_path); }); }); @@ -338,8 +344,9 @@ function createKnowledgeBaseRouter(db, upload) { ipAddress: req.ip }); + const safeDownloadName = row.file_name.replace(/["\r\n\\]/g, ''); res.setHeader('Content-Type', row.file_type || 'application/octet-stream'); - res.setHeader('Content-Disposition', `attachment; filename="${row.file_name}"`); + res.setHeader('Content-Disposition', `attachment; filename="${safeDownloadName}"`); res.sendFile(row.file_path); }); }); diff --git a/backend/routes/users.js b/backend/routes/users.js index 4d6e749..6b663da 100644 --- a/backend/routes/users.js +++ b/backend/routes/users.js @@ -124,12 +124,12 @@ function createUsersRouter(db, requireAuth, requireGroup, logAudit) { } // Prevent admin self-demotion - if (userId == req.user.id && group && group !== 'Admin') { + if (String(userId) === String(req.user.id) && group && group !== 'Admin') { return res.status(400).json({ error: 'Cannot remove your own admin group' }); } // Prevent self-deactivation - if (userId == req.user.id && is_active === false) { + if (String(userId) === String(req.user.id) && is_active === false) { return res.status(400).json({ error: 'Cannot deactivate your own account' }); } @@ -247,7 +247,7 @@ function createUsersRouter(db, requireAuth, requireGroup, logAudit) { const userId = req.params.id; // Prevent self-deletion - if (userId == req.user.id) { + if (String(userId) === String(req.user.id)) { return res.status(400).json({ error: 'Cannot delete your own account' }); } diff --git a/backend/server.js b/backend/server.js index a265ac2..3402b3b 100644 --- a/backend/server.js +++ b/backend/server.js @@ -29,7 +29,11 @@ const createComplianceRouter = require('./routes/compliance'); const app = express(); const PORT = process.env.PORT || 3001; const API_HOST = process.env.API_HOST || 'localhost'; -const SESSION_SECRET = process.env.SESSION_SECRET || 'default-secret-change-me'; +const SESSION_SECRET = process.env.SESSION_SECRET; +if (!SESSION_SECRET) { + console.error('FATAL: SESSION_SECRET environment variable must be set'); + process.exit(1); +} const CORS_ORIGINS = process.env.CORS_ORIGINS ? process.env.CORS_ORIGINS.split(',') : ['http://localhost:3000']; diff --git a/backend/setup.js b/backend/setup.js index c4b73ed..8998753 100755 --- a/backend/setup.js +++ b/backend/setup.js @@ -3,6 +3,7 @@ const sqlite3 = require('sqlite3').verbose(); const bcrypt = require('bcryptjs'); +const crypto = require('crypto'); const fs = require('fs'); const path = require('path'); @@ -172,8 +173,9 @@ async function createDefaultAdmin(db) { return; } - // Create admin user with password 'admin123' - const passwordHash = await bcrypt.hash('admin123', 10); + // Generate a random admin password on first run + const generatedPassword = crypto.randomBytes(12).toString('base64url'); + const passwordHash = await bcrypt.hash(generatedPassword, 10); db.run( `INSERT INTO users (username, email, password_hash, role, is_active) @@ -183,7 +185,12 @@ async function createDefaultAdmin(db) { if (err) { reject(err); } else { - console.log('✓ Created default admin user (admin/admin123)'); + console.log('✓ Created default admin user'); + console.log(`\n ╔══════════════════════════════════════════╗`); + console.log(` ║ Admin credentials (save these now!) ║`); + console.log(` ║ Username: admin ║`); + console.log(` ║ Password: ${generatedPassword.padEnd(29)}║`); + console.log(` ╚══════════════════════════════════════════╝\n`); resolve(); } } @@ -269,7 +276,7 @@ function displaySummary() { console.log(' ✓ Indexes for fast queries'); console.log(' ✓ Document compliance view'); console.log(' ✓ Uploads directory for file storage'); - console.log(' ✓ Default admin user (admin/admin123)'); + console.log(' ✓ Default admin user (see credentials above)'); console.log('\n📁 File structure will be:'); console.log(' uploads/'); console.log(' └── CVE-XXXX-XXXX/'); diff --git a/frontend/package.json b/frontend/package.json index d7619ec..299b6ae 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -14,6 +14,7 @@ "react-markdown": "^10.1.0", "react-scripts": "5.0.1", "recharts": "^3.8.1", + "rehype-sanitize": "^6.0.0", "web-vitals": "^2.1.4", "xlsx": "^0.18.5" }, diff --git a/frontend/src/components/KnowledgeBaseViewer.js b/frontend/src/components/KnowledgeBaseViewer.js index a4a7144..0a6eb85 100644 --- a/frontend/src/components/KnowledgeBaseViewer.js +++ b/frontend/src/components/KnowledgeBaseViewer.js @@ -1,5 +1,6 @@ import React, { useState, useEffect, useRef } from 'react'; import ReactMarkdown from 'react-markdown'; +import rehypeSanitize from 'rehype-sanitize'; import mermaid from 'mermaid'; import { X, Download, Loader, AlertCircle, FileText, File } from 'lucide-react'; @@ -233,6 +234,7 @@ export default function KnowledgeBaseViewer({ article, onClose }) { {isMarkdown && (