fix(api): close authz gap and tighten input validation

- Require account access on POST /api/pdf/preview: any authenticated user could previously render any account's MICR line (routing and account number) regardless of role - Re-validate QBO import records server-side on confirm (date format, positive amounts, integer check numbers) instead of trusting client JSON - Make PUT /api/users/:id atomic: validate all fields up front, then apply every change in a single transaction (a validation failure could previously leave a half-applied update) - Block demoting the last remaining admin - Validate routing numbers as exactly 9 digits on account setup and update - Cap upload sizes (50 MB .mdb, 10 MB CSV) and add a JSON error handler so oversized uploads and bad JSON return clean 4xx errors instead of HTML stack traces - Disable the X-Powered-By header
2026-06-11 21:56:53 -06:00
parent 3fd3285c13
commit 674506bd2d
4 changed files with 120 additions and 51 deletions
@@ -4,7 +4,6 @@ const express   = require('express');
 const path      = require('path');
 const fs        = require('fs');
 const os        = require('os');
-const crypto    = require('crypto');
 const { execFileSync } = require('child_process');
 const multer    = require('multer');
 const session   = require('express-session');
@@ -14,7 +13,14 @@ const { seedLayoutFields } = require('./db/database');
 const { requireAuth, requireAdmin, canAccessAccount, isEditorForAccount } = require('./middleware/auth');

 const app    = express();
-const upload = multer({ dest: os.tmpdir() });
+app.disable('x-powered-by');
+const upload = multer({ dest: os.tmpdir(), limits: { fileSize: 50 * 1024 * 1024 } });
+
+// US ABA routing numbers are exactly 9 digits (spaces/dashes tolerated on input)
+function normalizeRoutingNumber(value) {
+  const digits = String(value || '').replace(/[\s-]/g, '');
+  return /^\d{9}$/.test(digits) ? digits : null;
+}

 // ── Session store (SQLite-backed, no extra packages) ──────────────────────────
 const SessionStore = require('./lib/SessionStore');
@@ -120,6 +126,10 @@ app.put('/api/account/:id', requireAdmin, (req, res) => {
  if (!company1 || !routing_number || !account_number) {
    return res.status(400).json({ error: 'Organization name, routing number, and account number are required.' });
  }
+  const normalizedRouting = normalizeRoutingNumber(routing_number);
+  if (!normalizedRouting) {
+    return res.status(400).json({ error: 'Routing number must be exactly 9 digits.' });
+  }
  const MAX_IMAGE_BYTES = 512 * 1024; // 512 KB base64 limit
  if (logo_data && Buffer.byteLength(logo_data, 'utf8') > MAX_IMAGE_BYTES) {
    return res.status(400).json({ error: 'Logo image must be smaller than 512 KB.' });
@@ -141,7 +151,7 @@ app.put('/api/account/:id', requireAdmin, (req, res) => {
  `).run(
    company1 || null, company2 || null, company3 || null, company4 || null,
    bank_name || '', bank_info1 || null, bank_info2 || null, bank_info3 || null, transit_code || null,
-    routing_number, account_number,
+    normalizedRouting, account_number,
    parseFloat(offset_left) || 0, parseFloat(offset_right) || 0,
    parseFloat(offset_up) || 0, parseFloat(offset_down) || 0,
    second_signature ? 1 : 0, resolvedPosition,
@@ -214,6 +224,10 @@ app.post('/api/account/setup', requireAdmin, (req, res) => {
  if (!company1 || !routing_number || !account_number || !start_check_no) {
    return res.status(400).json({ error: 'Organization name, routing number, account number, and starting check number are required.' });
  }
+  const normalizedRouting = normalizeRoutingNumber(routing_number);
+  if (!normalizedRouting) {
+    return res.status(400).json({ error: 'Routing number must be exactly 9 digits.' });
+  }
  const checkNo = parseInt(start_check_no, 10);
  if (isNaN(checkNo) || checkNo < 1) {
    return res.status(400).json({ error: 'Starting check number must be a positive integer.' });
@@ -234,7 +248,7 @@ app.post('/api/account/setup', requireAdmin, (req, res) => {
    bank_info1:       bank_info1 || null,
    bank_info2:       bank_info2 || null,
    transit_code:     transit_code || null,
-    routing_number,
+    routing_number:   normalizedRouting,
    account_number,
    start_check_no:   checkNo,
    current_check_no: checkNo,
@@ -312,6 +326,22 @@ app.get('*', (req, res) => {
  res.sendFile(path.join(__dirname, '../public/index.html'));
 });

+// JSON error handler — keeps stack traces out of responses
+// eslint-disable-next-line no-unused-vars
+app.use((err, req, res, next) => {
+  if (err.code === 'LIMIT_FILE_SIZE') {
+    return res.status(413).json({ error: 'Uploaded file is too large.' });
+  }
+  if (err.type === 'entity.too.large') {
+    return res.status(413).json({ error: 'Request body is too large.' });
+  }
+  if (err.type === 'entity.parse.failed') {
+    return res.status(400).json({ error: 'Invalid JSON in request body.' });
+  }
+  console.error('[error]', err);
+  res.status(500).json({ error: 'Internal server error.' });
+});
+
 const PORT = process.env.PORT || 3000;
 app.listen(PORT, () => {
  console.log(`ezcheck running on http://localhost:${PORT}`);
@@ -4,7 +4,7 @@ const express = require('express');
 const router = express.Router();
 const db = require('../db/database');
 const { generateCheckPdf } = require('../services/pdfService');
-const { isEditorForAccount } = require('../middleware/auth');
+const { canAccessAccount, isEditorForAccount } = require('../middleware/auth');

 /**
 * POST /api/pdf
@@ -80,6 +80,11 @@ router.post('/', async (req, res) => {
 router.post('/preview', async (req, res) => {
  const resolvedAccountId = parseInt(req.body.account_id, 10);
  if (!resolvedAccountId) return res.status(400).json({ error: 'account_id required' });
+  // The preview renders the MICR line (routing + account number) — same access
+  // rules as reading the account itself
+  if (!canAccessAccount(req.session, resolvedAccountId)) {
+    return res.status(403).json({ error: 'Access denied.' });
+  }

  const account = db.prepare('SELECT * FROM account WHERE id = ?').get(resolvedAccountId);
  if (!account) return res.status(404).json({ error: 'Account not found.' });
@@ -6,7 +6,7 @@ const multer  = require('multer');
 const os      = require('os');
 const fs      = require('fs');

-const upload = multer({ dest: os.tmpdir() });
+const upload = multer({ dest: os.tmpdir(), limits: { fileSize: 10 * 1024 * 1024 } });
 const { isEditorForAccount } = require('../middleware/auth');

 // ── CSV helpers ───────────────────────────────────────────────────────────────
@@ -165,6 +165,29 @@ function extractRows(text, type) {

 // ── Confirm helpers ───────────────────────────────────────────────────────────

+// Records come back from the client as JSON, not from the parsed file —
+// re-validate them server-side. Normalizes amount/check_no in place.
+// Returns an error string, or null if all records are valid.
+function validateRecords(records, type) {
+  for (const rec of records) {
+    if (!rec || typeof rec !== 'object') return 'Invalid record.';
+    if (!/^\d{4}-\d{2}-\d{2}$/.test(rec.date || '')) {
+      return 'Each record must have a date in YYYY-MM-DD format.';
+    }
+    const amount = Number(rec.amount);
+    if (!isFinite(amount) || amount <= 0) {
+      return 'Each record amount must be a positive number.';
+    }
+    rec.amount = Math.round(amount * 100) / 100;
+    if (type === 'checks' && rec.check_no !== null && rec.check_no !== undefined) {
+      const n = parseInt(rec.check_no, 10);
+      if (!Number.isInteger(n) || n < 1) return 'Check numbers must be positive integers.';
+      rec.check_no = n;
+    }
+  }
+  return null;
+}
+
 function confirmChecks(db, records, account_id) {
  const existing = new Set(
    db.prepare('SELECT check_no FROM checks WHERE account_id = ?').all(account_id).map(r => r.check_no)
@@ -312,6 +335,8 @@ router.post('/confirm', express.json(), (req, res) => {
  if (records.length > 1000) {
    return res.status(400).json({ error: 'Cannot import more than 1000 records at a time.' });
  }
+  const validationError = validateRecords(records, type);
+  if (validationError) return res.status(400).json({ error: validationError });

  const db = require('../db/database');
  try {
@@ -57,77 +57,86 @@ router.post('/', async (req, res) => {

 // PUT /api/users/:id
 router.put('/:id', async (req, res) => {
-  const user = db.prepare('SELECT id, role FROM users WHERE id = ?').get(req.params.id);
+  const userId = parseInt(req.params.id, 10);
+  const user = db.prepare('SELECT id, role FROM users WHERE id = ?').get(userId);
  if (!user) return res.status(404).json({ error: 'User not found.' });

  const { username, password, role, accounts, email, oidc_sub, oidc_issuer } = req.body;

+  // ── Validate everything before writing anything ──
  if (role && !['admin', 'editor', 'viewer'].includes(role)) {
    return res.status(400).json({ error: 'Invalid role.' });
  }
-
-  if (username && username.trim() !== '') {
-    try {
-      db.prepare("UPDATE users SET username = ?, updated_at = datetime('now') WHERE id = ?")
-        .run(username.trim(), req.params.id);
-    } catch (err) {
-      if (err.message.includes('UNIQUE')) return res.status(409).json({ error: 'Username already taken.' });
-      throw err;
+  if (role && role !== 'admin' && user.role === 'admin') {
+    const { n } = db.prepare("SELECT COUNT(*) AS n FROM users WHERE role = 'admin' AND id != ?").get(userId);
+    if (n === 0) return res.status(400).json({ error: 'Cannot demote the last admin.' });
  }
-  }
-
-  if (role) {
-    db.prepare("UPDATE users SET role = ?, updated_at = datetime('now') WHERE id = ?")
-      .run(role, req.params.id);
-  }
-
-  if (email !== undefined) {
-    db.prepare("UPDATE users SET email = ?, updated_at = datetime('now') WHERE id = ?")
-      .run(email ? email.trim() : null, req.params.id);
-  }
-
  if (password) {
    const pwErr = validatePassword(password);
    if (pwErr) return res.status(400).json({ error: pwErr });
-    const hash = await bcrypt.hash(password, 12);
-    db.prepare("UPDATE users SET password_hash = ?, updated_at = datetime('now') WHERE id = ?")
-      .run(hash, req.params.id);
  }

-  // OIDC linking — admin can set or clear oidc_sub/oidc_issuer
+  let newSub, newIssuer;
  if (oidc_sub !== undefined) {
-    const newSub    = oidc_sub    ? oidc_sub.trim()    : null;
-    const newIssuer = oidc_issuer ? oidc_issuer.trim() : null;
+    newSub    = oidc_sub    ? oidc_sub.trim()    : null;
+    newIssuer = oidc_issuer ? oidc_issuer.trim() : null;
    if (newSub && !newIssuer) {
      return res.status(400).json({ error: 'OIDC issuer is required when setting OIDC subject.' });
    }
    if (newSub) {
      const existing = db.prepare(
        'SELECT id FROM users WHERE oidc_issuer = ? AND oidc_sub = ? AND id != ?'
-      ).get(newIssuer, newSub, req.params.id);
+      ).get(newIssuer, newSub, userId);
      if (existing) return res.status(409).json({ error: 'This OIDC identity is already linked to another user.' });
    }
-    db.prepare("UPDATE users SET oidc_sub = ?, oidc_issuer = ?, updated_at = datetime('now') WHERE id = ?")
-      .run(newSub, newSub ? newIssuer : null, req.params.id);
  }

+  const passwordHash = password ? await bcrypt.hash(password, 12) : null;
+
+  // ── Apply all changes atomically ──
+  try {
+    db.transaction(() => {
+      if (username && username.trim() !== '') {
+        db.prepare("UPDATE users SET username = ?, updated_at = datetime('now') WHERE id = ?")
+          .run(username.trim(), userId);
+      }
+      if (role) {
+        db.prepare("UPDATE users SET role = ?, updated_at = datetime('now') WHERE id = ?")
+          .run(role, userId);
+      }
+      if (email !== undefined) {
+        db.prepare("UPDATE users SET email = ?, updated_at = datetime('now') WHERE id = ?")
+          .run(email ? email.trim() : null, userId);
+      }
+      if (passwordHash) {
+        db.prepare("UPDATE users SET password_hash = ?, updated_at = datetime('now') WHERE id = ?")
+          .run(passwordHash, userId);
+      }
+      if (oidc_sub !== undefined) {
+        db.prepare("UPDATE users SET oidc_sub = ?, oidc_issuer = ?, updated_at = datetime('now') WHERE id = ?")
+          .run(newSub, newSub ? newIssuer : null, userId);
+      }
      if (Array.isArray(accounts)) {
-    db.prepare('DELETE FROM user_accounts WHERE user_id = ?').run(req.params.id);
+        db.prepare('DELETE FROM user_accounts WHERE user_id = ?').run(userId);
        const effectiveRole = role || user.role;
        if (effectiveRole !== 'admin' && accounts.length > 0) {
          const stmt = db.prepare('INSERT OR IGNORE INTO user_accounts (user_id, account_id, role) VALUES (?, ?, ?)');
-      accounts.forEach(a => stmt.run(req.params.id, a.id, a.role === 'editor' ? 'editor' : 'viewer'));
+          accounts.forEach(a => stmt.run(userId, a.id, a.role === 'editor' ? 'editor' : 'viewer'));
        }
      }
-
-  // If role or account assignments changed, invalidate all active sessions for this user
-  // so the new permissions take effect immediately rather than at session expiry.
+      // If role or account assignments changed, invalidate all active sessions for
+      // this user so the new permissions take effect immediately.
      if (role || Array.isArray(accounts)) {
        db.prepare("DELETE FROM sessions WHERE CAST(json_extract(sess, '$.userId') AS INTEGER) = ?")
-      .run(parseInt(req.params.id, 10));
+          .run(userId);
+      }
+    })();
+  } catch (err) {
+    if (err.message.includes('UNIQUE')) return res.status(409).json({ error: 'Username already taken.' });
+    throw err;
  }

-  res.json(userWithAccounts(req.params.id));
+  res.json(userWithAccounts(userId));
 });

 // DELETE /api/users/:id