From 444e24a191173957b6edb6eece8483e54eb08023 Mon Sep 17 00:00:00 2001 From: Steve Dogiakos Date: Fri, 20 Mar 2026 13:28:18 -0600 Subject: [PATCH] Fix remaining critical, medium, and low security issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IDOR (critical): GET /api/checks/:id and GET /api/deposits/:id now verify the requesting user has access to the record's account before returning data. Previously any authenticated user could fetch any record by ID across accounts. Printed check guard (critical): PUT and DELETE on checks now return 409 if the check has already been printed, enforcing the business rule that printed checks are immutable. Previously the printed flag was only enforced in the frontend. PDF DoS (medium): checkIds array capped at 300 (100 pages × 3 per page). QBO import DoS (medium): records array capped at 1000 per confirm call. PDF error detail (medium): internal err.message no longer returned to the client on PDF generation failure. SESSION_SECRET (low): removed NODE_ENV=production condition — the server now exits immediately on startup if SESSION_SECRET is unset regardless of environment. Dev script updated to load .env via node --env-file=.env so developers set it once in a local .env file. Password hints (low): updated all three UI labels from "min 8 chars" to "min 10 chars, include a digit or symbol" to match the actual server-side validation. --- package.json | 2 +- public/index.html | 6 +++--- src/app.js | 7 +++---- src/routes/checks.js | 5 +++++ src/routes/deposits.js | 3 +++ src/routes/pdf.js | 6 +++++- src/routes/qbo-import.js | 3 +++ 7 files changed, 23 insertions(+), 9 deletions(-) diff --git a/package.json b/package.json index 8da1b68..bbc8824 100644 --- a/package.json +++ b/package.json @@ -5,7 +5,7 @@ "main": "src/app.js", "scripts": { "start": "node src/app.js", - "dev": "nodemon src/app.js", + "dev": "nodemon --exec \"node --env-file=.env\" src/app.js", "migrate": "node migrations/import-mdb.js" }, "dependencies": { diff --git a/public/index.html b/public/index.html index 8c53200..519007c 100644 --- a/public/index.html +++ b/public/index.html @@ -20,7 +20,7 @@
- +
@@ -621,7 +621,7 @@
- +
@@ -652,7 +652,7 @@
- +
diff --git a/src/app.js b/src/app.js index 16139ee..e0a5fd7 100644 --- a/src/app.js +++ b/src/app.js @@ -18,12 +18,11 @@ const upload = multer({ dest: os.tmpdir() }); // ── Session store (SQLite-backed, no extra packages) ────────────────────────── const SessionStore = require('./lib/SessionStore'); -if (!process.env.SESSION_SECRET && process.env.NODE_ENV === 'production') { - console.error('[fatal] SESSION_SECRET environment variable must be set in production. Exiting.'); +if (!process.env.SESSION_SECRET) { + console.error('[fatal] SESSION_SECRET environment variable is not set. See .env.example. Exiting.'); process.exit(1); } -const SESSION_SECRET = process.env.SESSION_SECRET || - (() => { console.warn('[warn] SESSION_SECRET not set — using random secret (sessions will reset on restart)'); return crypto.randomBytes(32).toString('hex'); })(); +const SESSION_SECRET = process.env.SESSION_SECRET; const SESSION_MAX_AGE_MS = (parseInt(process.env.SESSION_MAX_AGE_HOURS, 10) || 168) * 60 * 60 * 1000; diff --git a/src/routes/checks.js b/src/routes/checks.js index 0bbcffa..4099930 100644 --- a/src/routes/checks.js +++ b/src/routes/checks.js @@ -41,6 +41,9 @@ router.get('/', (req, res) => { router.get('/:id', (req, res) => { const check = db.prepare('SELECT * FROM checks WHERE id = ?').get(req.params.id); if (!check) return res.status(404).json({ error: 'Check not found' }); + if (!canAccessAccount(req.session, check.account_id)) { + return res.status(403).json({ error: 'Access denied.' }); + } res.json(check); }); @@ -99,6 +102,7 @@ router.put('/:id', (req, res) => { if (!isEditorForAccount(req.session, check.account_id)) { return res.status(403).json({ error: 'Write access required.' }); } + if (check.printed) return res.status(409).json({ error: 'Cannot modify a printed check.' }); const { payee, amount, check_date, memo, note1, note2, payee_address1, payee_address2, payee_address3, payee_address4 } = req.body; @@ -140,6 +144,7 @@ router.delete('/:id', (req, res) => { if (!isEditorForAccount(req.session, check.account_id)) { return res.status(403).json({ error: 'Write access required.' }); } + if (check.printed) return res.status(409).json({ error: 'Cannot delete a printed check.' }); db.prepare('DELETE FROM checks WHERE id = ?').run(req.params.id); res.status(204).send(); }); diff --git a/src/routes/deposits.js b/src/routes/deposits.js index 01fcc60..5236607 100644 --- a/src/routes/deposits.js +++ b/src/routes/deposits.js @@ -38,6 +38,9 @@ router.get('/', (req, res) => { router.get('/:id', (req, res) => { const deposit = getDepositWithItems(req.params.id); if (!deposit) return res.status(404).json({ error: 'Deposit not found.' }); + if (!canAccessAccount(req.session, deposit.account_id)) { + return res.status(403).json({ error: 'Access denied.' }); + } res.json(deposit); }); diff --git a/src/routes/pdf.js b/src/routes/pdf.js index 8f30346..36940b8 100644 --- a/src/routes/pdf.js +++ b/src/routes/pdf.js @@ -17,9 +17,13 @@ const { isEditorForAccount } = require('../middleware/auth'); router.post('/', async (req, res) => { const { checkIds, account_id } = req.body; + const MAX_CHECKS_PER_JOB = 300; // 100 pages × 3 checks per page if (!Array.isArray(checkIds) || checkIds.length === 0) { return res.status(400).json({ error: 'checkIds must be a non-empty array' }); } + if (checkIds.length > MAX_CHECKS_PER_JOB) { + return res.status(400).json({ error: `Cannot generate more than ${MAX_CHECKS_PER_JOB} checks per PDF job.` }); + } const resolvedAccountId = parseInt(account_id, 10); if (!isEditorForAccount(req.session, resolvedAccountId)) { return res.status(403).json({ error: 'Write access required.' }); @@ -62,7 +66,7 @@ router.post('/', async (req, res) => { res.send(pdfBuffer); } catch (err) { console.error('PDF generation error:', err); - res.status(500).json({ error: 'PDF generation failed', detail: err.message }); + res.status(500).json({ error: 'PDF generation failed.' }); } }); diff --git a/src/routes/qbo-import.js b/src/routes/qbo-import.js index 454eda3..5413db4 100644 --- a/src/routes/qbo-import.js +++ b/src/routes/qbo-import.js @@ -309,6 +309,9 @@ router.post('/confirm', express.json(), (req, res) => { if (!Array.isArray(records) || records.length === 0) { return res.status(400).json({ error: 'No records provided.' }); } + if (records.length > 1000) { + return res.status(400).json({ error: 'Cannot import more than 1000 records at a time.' }); + } const db = require('../db/database'); try {