From 3fd3285c137dc0468ce43385307efa22de4bea1c Mon Sep 17 00:00:00 2001 From: Steve Dogiakos Date: Thu, 11 Jun 2026 21:54:35 -0600 Subject: [PATCH 1/4] fix(auth): harden session lifecycle, reset links, and OIDC logging - Fix session store expiry: cookie.maxAge is already in milliseconds, so stored sessions outlived the cookie by 1000x - Regenerate the session ID on login, first-run setup, and OIDC login to prevent session fixation - Mark session cookies Secure on TLS connections (secure: 'auto') and add TRUST_PROXY support for reverse-proxy deployments - Build password reset links from APP_BASE_URL instead of the Host header to prevent reset-link poisoning - Rate-limit forgot-password requests (5 per IP per 15 minutes) - Strip OIDC debug logging that leaked authorization codes, subject IDs, and emails to logs --- .env.example | 8 ++ README.md | 5 ++ docker-compose.yml | 4 + src/app.js | 10 ++- src/lib/SessionStore.js | 3 +- src/routes/auth.js | 163 ++++++++++++++++++++-------------------- 6 files changed, 109 insertions(+), 84 deletions(-) diff --git a/.env.example b/.env.example index 4afb6f3..efd94b9 100644 --- a/.env.example +++ b/.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 diff --git a/README.md b/README.md index 6f1203b..8ba8e18 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,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 +187,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:-} diff --git a/docker-compose.yml b/docker-compose.yml index 9e4ad2f..e0f456b 100644 --- a/docker-compose.yml +++ b/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:-} diff --git a/src/app.js b/src/app.js index d407bef..88021d7 100644 --- a/src/app.js +++ b/src/app.js @@ -27,12 +27,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 diff --git a/src/lib/SessionStore.js b/src/lib/SessionStore.js index b169f1e..bf9d8ef 100644 --- a/src/lib/SessionStore.js +++ b/src/lib/SessionStore.js @@ -28,8 +28,9 @@ 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( diff --git a/src/routes/auth.js b/src/routes/auth.js index 4211abf..6423a1f 100644 --- a/src/routes/auth.js +++ b/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 makeRateLimiter(max, windowMs) { + const attempts = new Map(); // key -> { count, resetAt } -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; + // Purge stale entries every 30 minutes to prevent unbounded memory growth + setInterval(() => { + const now = Date.now(); + 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); + }, + }; } -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++; - } -} +// 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); -function clearLoginFailures(ip) { - loginAttempts.delete(ip); +// 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); + }); } -// 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); - } -}, 30 * 60 * 1000).unref(); - // 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,22 +107,21 @@ 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; - - res.json({ id: user.id, username: user.username, 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 @@ -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; - - // 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; - } - - res.redirect('/'); + 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') { + 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.')); } }); From 674506bd2db96a5b0ef439de55c16fc4c4ee0404 Mon Sep 17 00:00:00 2001 From: Steve Dogiakos Date: Thu, 11 Jun 2026 21:56:53 -0600 Subject: [PATCH 2/4] 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 --- src/app.js | 38 +++++++++++++-- src/routes/pdf.js | 7 ++- src/routes/qbo-import.js | 27 ++++++++++- src/routes/users.js | 99 ++++++++++++++++++++++------------------ 4 files changed, 120 insertions(+), 51 deletions(-) diff --git a/src/app.js b/src/app.js index 88021d7..ead6f5b 100644 --- a/src/app.js +++ b/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'); @@ -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}`); diff --git a/src/routes/pdf.js b/src/routes/pdf.js index 55fa46b..3d090ba 100644 --- a/src/routes/pdf.js +++ b/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 @@ -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.' }); diff --git a/src/routes/qbo-import.js b/src/routes/qbo-import.js index 5413db4..e092682 100644 --- a/src/routes/qbo-import.js +++ b/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 { diff --git a/src/routes/users.js b/src/routes/users.js index 3effcb6..18ac074 100644 --- a/src/routes/users.js +++ b/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); } - if (Array.isArray(accounts)) { - db.prepare('DELETE FROM user_accounts WHERE user_id = ?').run(req.params.id); - 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')); - } + 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(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(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. + if (role || Array.isArray(accounts)) { + db.prepare("DELETE FROM sessions WHERE CAST(json_extract(sess, '$.userId') AS INTEGER) = ?") + .run(userId); + } + })(); + } catch (err) { + if (err.message.includes('UNIQUE')) return res.status(409).json({ error: 'Username already taken.' }); + throw err; } - // 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 || Array.isArray(accounts)) { - db.prepare("DELETE FROM sessions WHERE CAST(json_extract(sess, '$.userId') AS INTEGER) = ?") - .run(parseInt(req.params.id, 10)); - } - - res.json(userWithAccounts(req.params.id)); + res.json(userWithAccounts(userId)); }); // DELETE /api/users/:id From b4824655dd2735a05454d9a2490ad868f1398bda Mon Sep 17 00:00:00 2001 From: Steve Dogiakos Date: Thu, 11 Jun 2026 21:57:39 -0600 Subject: [PATCH 3/4] fix(docker): run container as non-root and exclude local files from image - Add .dockerignore: a local .env, the live SQLite database in data/, .git, and node_modules were previously copied into the published image by COPY - Run the app as the unprivileged node user; pre-create /app/data with matching ownership so named volumes inherit it - Set NODE_ENV=production in the image - Document the one-time volume chown needed when upgrading existing deployments --- .dockerignore | 15 +++++++++++++++ README.md | 11 +++++++++++ docker/Dockerfile | 8 +++++++- 3 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..06cf085 --- /dev/null +++ b/.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 diff --git a/README.md b/README.md index 8ba8e18..5ac4a50 100644 --- a/README.md +++ b/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 diff --git a/docker/Dockerfile b/docker/Dockerfile index dafd214..2a32c25 100644 --- a/docker/Dockerfile +++ b/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"] From 2504766be75ad14268514457a9fcc525ce4fedb9 Mon Sep 17 00:00:00 2001 From: Steve Dogiakos Date: Thu, 11 Jun 2026 22:03:56 -0600 Subject: [PATCH 4/4] perf(db): reuse prepared statements on hot paths - Prepare the user_accounts role lookup once in the auth middleware; it runs on nearly every authenticated request - Prepare session store get/set/destroy/purge statements once in the constructor instead of per request - Prepare the per-check SELECT once per PDF job instead of once per check --- src/lib/SessionStore.js | 16 +++++++++------- src/middleware/auth.js | 14 +++++++------- src/routes/pdf.js | 3 ++- 3 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/lib/SessionStore.js b/src/lib/SessionStore.js index bf9d8ef..87d89d9 100644 --- a/src/lib/SessionStore.js +++ b/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, () => {}); @@ -33,16 +37,14 @@ class SessionStore extends Store { ? 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); } } diff --git a/src/middleware/auth.js b/src/middleware/auth.js index 545a16d..60633e1 100644 --- a/src/middleware/auth.js +++ b/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'); } diff --git a/src/routes/pdf.js b/src/routes/pdf.js index 3d090ba..e21cb59 100644 --- a/src/routes/pdf.js +++ b/src/routes/pdf.js @@ -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;