Merge pull request #17 from snachodog/refactor/security-hardening

Security hardening and hot-path performance fixes

.dockerignore

@@ -0,0 +1,15 @@
+.git
+.github
+node_modules
+data
+*.db
+*.db-shm
+*.db-wal
+.env
+.env.*
+!.env.example
+*.log
+.claude
+CLAUDE.md
+TODO.md
+docker-compose.yml

.env.example

@@ -6,6 +6,14 @@ SESSION_MAX_AGE_HOURS=168   # default: 168 (7 days)
 PORT=3000
 DB_PATH=/app/data/check-printing.db
 
+# Public base URL of the app — used to build password reset links.
+# Strongly recommended in production (prevents host-header link poisoning).
+APP_BASE_URL=https://checks.example.com
+
+# Set to 1 when running behind a reverse proxy (TLS termination) so client IPs
+# and HTTPS detection work correctly. Leave unset for direct LAN access.
+TRUST_PROXY=
+
 # OIDC / SSO (optional — omit or leave blank to disable)
 OIDC_ENABLED=false
 OIDC_DISCOVERY_URL=https://auth.example.com/.well-known/openid-configuration

README.md

@@ -46,6 +46,17 @@ docker compose up -d
 
 4. Use the setup wizard to configure your first checking account (organization info, bank info, routing/account numbers), or import an existing ezCheckPrinting `.mdb` file.
 
+#### Upgrading from images before v0.5
+
+The container now runs as the unprivileged `node` user (UID 1000). Existing data
+volumes were written as root, so fix ownership once before upgrading:
+
+```bash
+docker compose down
+docker run --rm -v check-printing-data:/data alpine chown -R 1000:1000 /data
+docker compose up -d
+```
+
 ### Development (local)
 
 ```bash
@@ -162,6 +173,8 @@ docker exec -it check-printing node migrations/import-mdb.js \
 | `SESSION_MAX_AGE_HOURS` | `168` | Session lifetime in hours (default 7 days) |
 | `PORT` | `3000` | HTTP listen port |
 | `DB_PATH` | `/app/data/check-printing.db` | SQLite database file path |
+| `APP_BASE_URL` | *(empty)* | Public base URL used in password reset links, e.g. `https://checks.example.com`. Recommended in production |
+| `TRUST_PROXY` | *(empty)* | Set to `1` when running behind a reverse proxy so client IPs and HTTPS detection work correctly |
 | `OIDC_ENABLED` | *(empty)* | Set to `true` or `1` to enable OIDC login |
 | `OIDC_DISCOVERY_URL` | *(empty)* | Provider's `.well-known/openid-configuration` URL |
 | `OIDC_CLIENT_ID` | *(empty)* | OIDC client ID |
@@ -185,6 +198,9 @@ services:
       - check-printing-data:/app/data
     environment:
       - SESSION_SECRET=${SESSION_SECRET}
+      # Optional: public base URL for reset links, reverse-proxy support
+      - APP_BASE_URL=${APP_BASE_URL:-}
+      - TRUST_PROXY=${TRUST_PROXY:-}
       # Optional: OIDC / SSO
       - OIDC_ENABLED=${OIDC_ENABLED:-}
       - OIDC_DISCOVERY_URL=${OIDC_DISCOVERY_URL:-}

docker-compose.yml

@@ -14,6 +14,10 @@ services:
       - DB_PATH=/app/data/check-printing.db
       # Required in production — generate with: openssl rand -hex 32
       - SESSION_SECRET=${SESSION_SECRET}
+      # Public base URL for password reset links (recommended)
+      - APP_BASE_URL=${APP_BASE_URL:-}
+      # Set to 1 when behind a reverse proxy / TLS termination
+      - TRUST_PROXY=${TRUST_PROXY:-}
       # OIDC / SSO (optional — omit or leave blank to disable)
       - OIDC_ENABLED=${OIDC_ENABLED:-}
       - OIDC_DISCOVERY_URL=${OIDC_DISCOVERY_URL:-}

docker/Dockerfile

@@ -1,5 +1,7 @@
 FROM node:20-slim
 
+ENV NODE_ENV=production
+
 # mdbtools for migration script (only needed on first run, stays in image for convenience)
 RUN apt-get update && apt-get install -y --no-install-recommends mdbtools && rm -rf /var/lib/apt/lists/*
 
@@ -10,9 +12,13 @@ RUN npm ci --omit=dev
 
 COPY . .
 
-# Data volume: SQLite database and any runtime uploads
+# Data volume: SQLite database and any runtime uploads.
+# Pre-create it owned by the unprivileged user so named volumes inherit ownership.
+RUN mkdir -p /app/data && chown -R node:node /app
 VOLUME ["/app/data"]
 
+USER node
+
 EXPOSE 3000
 
 CMD ["node", "src/app.js"]

src/app.js

@@ -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');
@@ -27,12 +33,20 @@ const SESSION_SECRET = process.env.SESSION_SECRET;
 
 const SESSION_MAX_AGE_MS = (parseInt(process.env.SESSION_MAX_AGE_HOURS, 10) || 168) * 60 * 60 * 1000;
 
+// Behind a reverse proxy (TLS termination), set TRUST_PROXY=1 so req.ip and
+// req.protocol reflect the original client instead of the proxy.
+if (process.env.TRUST_PROXY === '1' || process.env.TRUST_PROXY === 'true') {
+  app.set('trust proxy', 1);
+}
+
 app.use(session({
   store: new SessionStore(db),
   secret: SESSION_SECRET,
   resave: false,
   saveUninitialized: false,
-  cookie: { httpOnly: true, sameSite: 'strict', maxAge: SESSION_MAX_AGE_MS },
+  // secure: 'auto' marks the cookie Secure only on TLS connections, so plain-HTTP
+  // LAN deployments keep working while proxied HTTPS deployments get Secure cookies
+  cookie: { httpOnly: true, sameSite: 'strict', secure: 'auto', maxAge: SESSION_MAX_AGE_MS },
 }));
 
 // Security headers
@@ -112,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.' });
@@ -133,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,
@@ -206,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.' });
@@ -226,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,
@@ -304,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}`);

src/lib/SessionStore.js

@@ -7,16 +7,20 @@ const { Store } = require('express-session');
 class SessionStore extends Store {
   constructor(db) {
     super();
-    this.db = db;
+    // Prepared once — get/set run on every request
+    this.getStmt   = db.prepare('SELECT sess, expired FROM sessions WHERE sid = ?');
+    this.setStmt   = db.prepare('INSERT OR REPLACE INTO sessions (sid, sess, expired) VALUES (?, ?, ?)');
+    this.delStmt   = db.prepare('DELETE FROM sessions WHERE sid = ?');
+    this.purgeStmt = db.prepare('DELETE FROM sessions WHERE expired < ?');
     // Purge expired sessions every 10 minutes
     setInterval(() => {
-      try { db.prepare('DELETE FROM sessions WHERE expired < ?').run(Date.now()); } catch (_) {}
+      try { this.purgeStmt.run(Date.now()); } catch (_) {}
     }, 10 * 60 * 1000).unref();
   }
 
   get(sid, cb) {
     try {
-      const row = this.db.prepare('SELECT sess, expired FROM sessions WHERE sid = ?').get(sid);
+      const row = this.getStmt.get(sid);
       if (!row) return cb(null, null);
       if (Date.now() > row.expired) {
         this.destroy(sid, () => {});
@@ -28,20 +32,19 @@ class SessionStore extends Store {
 
   set(sid, sess, cb) {
     try {
+      // cookie.maxAge is already in milliseconds
       const maxAge = (sess.cookie && sess.cookie.maxAge)
-        ? sess.cookie.maxAge * 1000
+        ? sess.cookie.maxAge
         : 7 * 24 * 60 * 60 * 1000;
       const expired = Date.now() + maxAge;
-      this.db.prepare(
-        'INSERT OR REPLACE INTO sessions (sid, sess, expired) VALUES (?, ?, ?)'
-      ).run(sid, JSON.stringify(sess), expired);
+      this.setStmt.run(sid, JSON.stringify(sess), expired);
       cb(null);
     } catch (e) { cb(e); }
   }
 
   destroy(sid, cb) {
     try {
-      this.db.prepare('DELETE FROM sessions WHERE sid = ?').run(sid);
+      this.delStmt.run(sid);
       cb(null);
     } catch (e) { cb(e); }
   }

src/middleware/auth.js

@@ -2,6 +2,11 @@
 
 const db = require('../db/database');
 
+// Prepared once — this lookup runs on nearly every authenticated request
+const accountRoleStmt = db.prepare(
+  'SELECT role FROM user_accounts WHERE user_id = ? AND account_id = ?'
+);
+
 function requireAuth(req, res, next) {
   if (!req.session || !req.session.userId) {
     return res.status(401).json({ error: 'Not authenticated.' });
@@ -28,10 +33,7 @@ function requireEditor(req, res, next) {
 function canAccessAccount(session, accountId) {
   if (!session || !session.userId) return false;
   if (session.role === 'admin') return true;
-  const row = db.prepare(
-    'SELECT 1 FROM user_accounts WHERE user_id = ? AND account_id = ?'
-  ).get(session.userId, accountId);
-  return !!row;
+  return !!accountRoleStmt.get(session.userId, accountId);
 }
 
 // Returns true if the user has editor (write) access to the given account.
@@ -39,9 +41,7 @@ function canAccessAccount(session, accountId) {
 function isEditorForAccount(session, accountId) {
   if (!session || !session.userId) return false;
   if (session.role === 'admin') return true;
-  const row = db.prepare(
-    "SELECT role FROM user_accounts WHERE user_id = ? AND account_id = ?"
-  ).get(session.userId, accountId);
+  const row = accountRoleStmt.get(session.userId, accountId);
   return !!(row && row.role === 'editor');
 }
 

src/routes/auth.js

@@ -15,45 +15,57 @@ function validatePassword(password) {
   return null;
 }
 
-// ── Login rate limiter ────────────────────────────────────────────────────────
-// Tracks failed login attempts per IP. After 10 failures within 15 minutes,
+// ── Rate limiting ─────────────────────────────────────────────────────────────
+// Sliding-window counter per key (IP). After `max` hits within `windowMs`,
 // further attempts are blocked until the window resets.
-const loginAttempts = new Map(); // ip -> { count, resetAt }
-const RATE_WINDOW_MS  = 15 * 60 * 1000; // 15 minutes
-const RATE_MAX_FAILS  = 10;
-
-function checkLoginRate(ip) {
-  const now  = Date.now();
-  const entry = loginAttempts.get(ip);
-  if (!entry || now > entry.resetAt) {
-    loginAttempts.set(ip, { count: 0, resetAt: now + RATE_WINDOW_MS });
-    return true; // allow
-  }
-  return entry.count < RATE_MAX_FAILS;
-}
-
-function recordLoginFailure(ip) {
-  const now   = Date.now();
-  const entry = loginAttempts.get(ip);
-  if (!entry || now > entry.resetAt) {
-    loginAttempts.set(ip, { count: 1, resetAt: now + RATE_WINDOW_MS });
-  } else {
-    entry.count++;
-  }
-}
-
-function clearLoginFailures(ip) {
-  loginAttempts.delete(ip);
-}
+function makeRateLimiter(max, windowMs) {
+  const attempts = new Map(); // key -> { count, resetAt }
 
   // Purge stale entries every 30 minutes to prevent unbounded memory growth
   setInterval(() => {
     const now = Date.now();
-  for (const [ip, entry] of loginAttempts) {
-    if (now > entry.resetAt) loginAttempts.delete(ip);
+    for (const [key, entry] of attempts) {
+      if (now > entry.resetAt) attempts.delete(key);
     }
   }, 30 * 60 * 1000).unref();
 
+  return {
+    allowed(key) {
+      const entry = attempts.get(key);
+      if (!entry || Date.now() > entry.resetAt) return true;
+      return entry.count < max;
+    },
+    record(key) {
+      const now   = Date.now();
+      const entry = attempts.get(key);
+      if (!entry || now > entry.resetAt) {
+        attempts.set(key, { count: 1, resetAt: now + windowMs });
+      } else {
+        entry.count++;
+      }
+    },
+    clear(key) {
+      attempts.delete(key);
+    },
+  };
+}
+
+// 10 failed logins per IP per 15 minutes
+const loginLimiter = makeRateLimiter(10, 15 * 60 * 1000);
+// 5 reset emails per IP per 15 minutes (counts every request, success or not)
+const resetLimiter = makeRateLimiter(5, 15 * 60 * 1000);
+
+// Regenerates the session ID before establishing a login (prevents session fixation)
+function establishSession(req, user, cb) {
+  req.session.regenerate(err => {
+    if (err) return cb(err);
+    req.session.userId   = user.id;
+    req.session.username = user.username;
+    req.session.role     = user.role;
+    cb(null);
+  });
+}
+
 // GET /api/auth/setup-needed — true when no users exist (first-run)
 router.get('/setup-needed', (req, res) => {
   const { n } = db.prepare('SELECT COUNT(*) AS n FROM users').get();
@@ -75,18 +87,18 @@ router.post('/setup', async (req, res) => {
     "INSERT INTO users (username, password_hash, role) VALUES (?, ?, 'admin')"
   ).run(username.trim(), hash);
 
-  req.session.userId   = result.lastInsertRowid;
-  req.session.username = username.trim();
-  req.session.role     = 'admin';
-
-  res.status(201).json({ id: result.lastInsertRowid, username: username.trim(), role: 'admin' });
+  const user = { id: result.lastInsertRowid, username: username.trim(), role: 'admin' };
+  establishSession(req, user, err => {
+    if (err) return res.status(500).json({ error: 'Failed to create session.' });
+    res.status(201).json(user);
+  });
 });
 
 // POST /api/auth/login
 router.post('/login', async (req, res) => {
   const ip = req.ip || req.socket.remoteAddress || 'unknown';
 
-  if (!checkLoginRate(ip)) {
+  if (!loginLimiter.allowed(ip)) {
     return res.status(429).json({ error: 'Too many failed login attempts. Please try again later.' });
   }
 
@@ -95,23 +107,22 @@ router.post('/login', async (req, res) => {
 
   const user = db.prepare('SELECT * FROM users WHERE username = ? COLLATE NOCASE').get(username.trim());
   if (!user) {
-    recordLoginFailure(ip);
+    loginLimiter.record(ip);
     return res.status(401).json({ error: 'Invalid username or password.' });
   }
 
   const match = await bcrypt.compare(password, user.password_hash);
   if (!match) {
-    recordLoginFailure(ip);
+    loginLimiter.record(ip);
     return res.status(401).json({ error: 'Invalid username or password.' });
   }
 
-  clearLoginFailures(ip);
-  req.session.userId   = user.id;
-  req.session.username = user.username;
-  req.session.role     = user.role;
-
+  loginLimiter.clear(ip);
+  establishSession(req, user, err => {
+    if (err) return res.status(500).json({ error: 'Failed to create session.' });
     res.json({ id: user.id, username: user.username, role: user.role });
   });
+});
 
 // POST /api/auth/logout
 router.post('/logout', (req, res) => {
@@ -154,6 +165,12 @@ router.post('/change-password', async (req, res) => {
 
 // POST /api/auth/forgot-password — always 200 to avoid user enumeration
 router.post('/forgot-password', async (req, res) => {
+  const ip = req.ip || req.socket.remoteAddress || 'unknown';
+  if (!resetLimiter.allowed(ip)) {
+    return res.status(429).json({ error: 'Too many reset requests. Please try again later.' });
+  }
+  resetLimiter.record(ip);
+
   const { email } = req.body;
   if (!email) return res.status(400).json({ error: 'Email is required.' });
 
@@ -168,7 +185,9 @@ router.post('/forgot-password', async (req, res) => {
       db.prepare('INSERT INTO password_reset_tokens (user_id, token_hash, expires_at) VALUES (?, ?, ?)').run(user.id, tokenHash, expiresAt);
     })();
 
-    const baseUrl   = `${req.protocol}://${req.get('host')}`;
+    // Prefer the configured base URL; deriving it from the Host header lets an
+    // attacker poison reset links (host header injection)
+    const baseUrl   = (process.env.APP_BASE_URL || `${req.protocol}://${req.get('host')}`).replace(/\/+$/, '');
     const resetLink = `${baseUrl}/#reset?token=${token}`;
 
     try {
@@ -221,17 +240,13 @@ function getOidcSettings() {
 
 async function getOidcClient(settings) {
   const { Issuer } = require('openid-client');
-  console.log('[oidc] discovering issuer from:', settings.discovery_url);
   const issuer = await Issuer.discover(settings.discovery_url);
-  console.log('[oidc] discovered issuer:', issuer.issuer);
-  const client = new issuer.Client({
+  return new issuer.Client({
     client_id:     settings.client_id,
     client_secret: settings.client_secret,
     redirect_uris: [settings.redirect_uri],
     response_types: ['code'],
   });
-  console.log('[oidc] client created, redirect_uri:', settings.redirect_uri);
-  return client;
 }
 
 // GET /api/auth/oidc/config — public, returns whether OIDC is enabled + button label
@@ -244,8 +259,6 @@ router.get('/oidc/config', (req, res) => {
 router.get('/oidc/authorize', async (req, res) => {
   try {
     const settings = getOidcSettings();
-    console.log('[oidc] authorize: enabled=%s, discovery_url=%s, client_id=%s, redirect_uri=%s',
-      settings.enabled, settings.discovery_url, settings.client_id, settings.redirect_uri);
     if (!settings.enabled) return res.status(400).json({ error: 'OIDC is not enabled.' });
 
     const { generators } = require('openid-client');
@@ -266,11 +279,10 @@ router.get('/oidc/authorize', async (req, res) => {
       code_challenge_method: 'S256',
     });
 
-    console.log('[oidc] authorize: redirecting to:', authUrl.substring(0, 200) + '...');
     // Ensure session is persisted before redirecting (saveUninitialized is false)
     req.session.save(() => res.redirect(authUrl));
   } catch (err) {
-    console.error('[oidc] authorize error:', err.message, err.stack);
+    console.error('[oidc] authorize error:', err.message);
     res.redirect('/#oidc-error=' + encodeURIComponent('Failed to initiate SSO login.'));
   }
 });
@@ -278,7 +290,6 @@ router.get('/oidc/authorize', async (req, res) => {
 // GET /api/auth/oidc/callback — handles the provider redirect
 router.get('/oidc/callback', async (req, res) => {
   try {
-    console.log('[oidc] callback: query params:', JSON.stringify(req.query));
     const settings = getOidcSettings();
     if (!settings.enabled) return res.redirect('/#oidc-error=' + encodeURIComponent('OIDC is not enabled.'));
 
@@ -287,12 +298,9 @@ router.get('/oidc/callback', async (req, res) => {
       console.error('[oidc] callback: no oidc session data found — session may have expired or cookie lost');
       return res.redirect('/#oidc-error=' + encodeURIComponent('Session expired. Please try again.'));
     }
-    console.log('[oidc] callback: session has oidc data, linking=%s, linkUserId=%s',
-      !!oidcSession.linking, oidcSession.linkUserId || 'n/a');
 
     const client = await getOidcClient(settings);
     const params = client.callbackParams(req);
-    console.log('[oidc] callback: exchanging code for tokens...');
 
     const tokenSet = await client.callback(settings.redirect_uri, params, {
       code_verifier: oidcSession.code_verifier,
@@ -303,25 +311,21 @@ router.get('/oidc/callback', async (req, res) => {
     const claims = tokenSet.claims();
     const sub    = claims.sub;
     const issuer = claims.iss;
-    console.log('[oidc] callback: token exchange OK, sub=%s, iss=%s, email=%s, name=%s',
-      sub, issuer, claims.email || 'n/a', claims.name || 'n/a');
 
     delete req.session.oidc;
 
     // Self-service linking flow
     if (oidcSession.linking && oidcSession.linkUserId) {
-      console.log('[oidc] callback: linking flow for userId=%s', oidcSession.linkUserId);
       const existing = db.prepare(
         'SELECT id FROM users WHERE oidc_issuer = ? AND oidc_sub = ? AND id != ?'
       ).get(issuer, sub, oidcSession.linkUserId);
       if (existing) {
-        console.warn('[oidc] callback: identity already linked to userId=%s', existing.id);
         return res.redirect('/#oidc-error=' + encodeURIComponent('This identity is already linked to another account.'));
       }
 
       db.prepare("UPDATE users SET oidc_sub = ?, oidc_issuer = ?, updated_at = datetime('now') WHERE id = ?")
         .run(sub, issuer, oidcSession.linkUserId);
-      console.log('[oidc] callback: linked sub=%s to userId=%s', sub, oidcSession.linkUserId);
+      console.log('[oidc] linked identity to userId=%s', oidcSession.linkUserId);
       return res.redirect('/#oidc-linked');
     }
 
@@ -331,26 +335,23 @@ router.get('/oidc/callback', async (req, res) => {
     ).get(issuer, sub);
 
     if (!user) {
-      console.warn('[oidc] callback: no user found for iss=%s sub=%s — not linked', issuer, sub);
+      console.warn('[oidc] callback: identity not linked to any user');
       return res.redirect('/#oidc-error=' + encodeURIComponent(
         'No account is linked to this identity. Ask an admin to link your account, or sign in with your password and link it yourself.'
       ));
     }
 
-    console.log('[oidc] callback: login success, userId=%s, username=%s, role=%s', user.id, user.username, user.role);
-    req.session.userId   = user.id;
-    req.session.username = user.username;
-    req.session.role     = user.role;
-
+    console.log('[oidc] login success for userId=%s', user.id);
+    establishSession(req, user, err => {
+      if (err) return res.redirect('/#oidc-error=' + encodeURIComponent('SSO login failed. Please try again.'));
       // Load account access into session (mirrors login behavior)
       if (user.role !== 'admin') {
-      const accts = db.prepare('SELECT account_id, role FROM user_accounts WHERE user_id = ?').all(user.id);
-      req.session.accounts = accts;
+        req.session.accounts = db.prepare('SELECT account_id, role FROM user_accounts WHERE user_id = ?').all(user.id);
       }
-
       res.redirect('/');
+    });
   } catch (err) {
-    console.error('[oidc] callback error:', err.message, err.stack);
+    console.error('[oidc] callback error:', err.message);
     res.redirect('/#oidc-error=' + encodeURIComponent('SSO login failed. Please try again.'));
   }
 });
@@ -362,7 +363,6 @@ router.get('/oidc/link', async (req, res) => {
   }
 
   try {
-    console.log('[oidc] link: userId=%s initiating linking flow', req.session.userId);
     const settings = getOidcSettings();
     if (!settings.enabled) return res.redirect('/#oidc-error=' + encodeURIComponent('OIDC is not enabled.'));
 
@@ -384,10 +384,9 @@ router.get('/oidc/link', async (req, res) => {
       code_challenge_method: 'S256',
     });
 
-    console.log('[oidc] link: redirecting to provider');
     req.session.save(() => res.redirect(authUrl));
   } catch (err) {
-    console.error('[oidc] link error:', err.message, err.stack);
+    console.error('[oidc] link error:', err.message);
     res.redirect('/#oidc-error=' + encodeURIComponent('Failed to initiate SSO linking.'));
   }
 });

src/routes/pdf.js

@@ -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
@@ -30,10 +30,11 @@ router.post('/', async (req, res) => {
   }
 
   // Fetch checks in the order provided; verify each belongs to the declared account
+  const checkStmt = db.prepare('SELECT * FROM checks WHERE id = ?');
   let checks;
   try {
     checks = checkIds.map(id => {
-      const check = db.prepare('SELECT * FROM checks WHERE id = ?').get(id);
+      const check = checkStmt.get(id);
       if (!check) throw new Error(`Check ID ${id} not found`);
       if (check.account_id !== resolvedAccountId) throw new Error(`Check ID ${id} does not belong to this account`);
       return check;
@@ -80,6 +81,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.' });

src/routes/qbo-import.js

@@ -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 {

src/routes/users.js

@@ -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