diff --git a/FUTURE.md b/FUTURE.md index 64ce21f..0f021fe 100644 --- a/FUTURE.md +++ b/FUTURE.md @@ -34,11 +34,206 @@ Items are grouped under their priority section heading (`## 🔴 CRITICAL`, `## ### 🔴 CRITICAL +### 🔴 Notification Runner Leaks Bill Details Across Users — CRITICAL +**Priority:** CRITICAL +**Added:** 2026-05-10 by Prime (code review) +**Type:** SECURITY +**Description:** +`services/notificationService.js` loads every active bill globally (`SELECT * FROM bills WHERE active = 1`) and then loops every notification recipient. In per-user notification mode, this can email one user's bill names, due dates, and amounts to every other opted-in user because the recipient is never matched to `bill.user_id`. + +**Affected Files:** +- `services/notificationService.js:180-233` + +**Potential Fix:** +Fetch bills per recipient with `WHERE user_id = ?`, or group active bills by `user_id` and only send each bill to its owner. Add a regression test with two users to prove cross-user notifications cannot occur. + +**Severity:** CRITICAL + + +### 🟠 HIGH + +### 🟠 Admin Can Toggle Payments on Any User Bill — HIGH +**Priority:** HIGH +**Added:** 2026-05-10 by Prime (code review) +**Type:** SECURITY + +**Description:** +`POST /api/bills/:id/toggle-paid` has a special admin branch that selects bills without `user_id = req.user.id`, then soft-deletes or creates payments on that bill. The surrounding router is mounted with `requireUser`, and `requireUser` allows `role='admin'`, so a non-default admin can modify another user's financial records through this endpoint. This contradicts the documented admin/privacy boundary used elsewhere. + +**Affected Files:** +- `routes/bills.js:393-443` +- `middleware/requireAuth.js:38-45` +- `server.js:86-87` + +**Potential Fix:** +Remove the admin-wide branch and require ownership for all bill payment mutations. If admin support access is intentional, put it behind a separate audited admin endpoint with explicit UI/permission labeling. + +**Severity:** HIGH + +### 🟠 Analytics Validation Errors Crash Instead of Returning 400 — HIGH +**Priority:** HIGH +**Added:** 2026-05-10 by Prime (code review) +**Type:** BUG / API_CONTRACT + +**Description:** +`routes/analytics.js` calls `standardizeError()` on invalid query parameters, but never imports it. Valid analytics requests work, but invalid `year`, `month`, `months`, `category_id`, or `bill_id` input will throw `ReferenceError: standardizeError is not defined` and return a 500 instead of a standardized 400 response. + +**Affected Files:** +- `routes/analytics.js:1-3` +- `routes/analytics.js:97-99` + +**Potential Fix:** +Import `standardizeError` from `../middleware/errorFormatter` and add route tests for invalid analytics query parameters. + +**Severity:** HIGH + +### 🟠 User Export Drops Recurrence and History-Range Data — HIGH +**Priority:** HIGH +**Added:** 2026-05-10 by Prime (code review) +**Type:** DATA_INTEGRITY + +**Description:** +`routes/export.js` does not include `bills.cycle_type`, `bills.cycle_day`, or `bill_history_ranges` in user Excel/SQLite exports. Users who rely on exports as portable backups can lose non-monthly recurrence settings and history visibility ranges after exporting and re-importing their data. + +**Affected Files:** +- `routes/export.js` +- `db/schema.sql` + +**Potential Fix:** +Add `cycle_type` and `cycle_day` to exported bill rows and the SQLite export schema. Export `bill_history_ranges` as its own Excel sheet/SQLite table and update the matching import path to restore those rows under the current user. + +**Severity:** HIGH + +### 🟠 Single-User Mode Can Lock Itself Out When Expired Sessions Exist — HIGH +**Priority:** HIGH +**Added:** 2026-05-10 by Prime (code review) +**Type:** BUG + +**Description:** +`getSingleModeUser()` uses a `LEFT JOIN sessions` and requires `(s.expires_at > datetime('now') OR s.id IS NULL)`. If the configured default single-mode user has only expired session rows, the join returns expired rows, `s.id` is not NULL, and no user is returned. That defeats the intended session bypass for single-user mode until expired sessions are pruned. + +**Affected Files:** +- `middleware/requireAuth.js` +- `services/authService.js` + +**Potential Fix:** +Do not join sessions for single-user mode. Validate only that the configured default user exists, is active, and has role `user`, or explicitly prune expired sessions before checking. + +**Severity:** HIGH ### 🟡 MEDIUM + +### 🟡 Password Change Rate Limiter Applies to Every Profile Endpoint — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** API_CONTRACT / CONFIG + +**Description:** +`server.js` mounts `passwordLimiter` across all `/api/profile` routes, not only `/api/profile/change-password`. Five harmless profile reads/settings updates in 15 minutes can block profile access with a password-change rate-limit message, while also making profile pages fragile under normal UI polling or retries. + +**Affected Files:** +- `server.js:102-103` +- `middleware/rateLimiter.js:18-23` +- `routes/profile.js` + +**Potential Fix:** +Apply `passwordLimiter` only to `POST /api/profile/change-password` (or inside the route), leaving profile reads/settings on a normal API limiter if needed. + +**Severity:** MEDIUM + +### 🟡 Profile Password Change Does Not Invalidate Other Sessions — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** SECURITY + +**Description:** +`POST /api/profile/change-password` only invalidates other sessions and rotates the current session when `req.sessionId` exists, but `requireAuth()` never sets `req.sessionId`. Password changes through the profile endpoint can therefore leave all existing sessions active and skip current-session rotation. The separate `/api/auth/change-password` route uses the cookie value directly and does not have this specific gap. + +**Affected Files:** +- `routes/profile.js:211-223` +- `middleware/requireAuth.js:22-31` +- `services/authService.js:175-194` + +**Potential Fix:** +Set `req.sessionId` in `requireAuth()` from the session cookie after successful authentication, or make the profile route use `req.cookies?.[COOKIE_NAME]` consistently with `/api/auth/change-password`. Add a regression test that verifies other sessions are removed after profile password change. + +**Severity:** MEDIUM + +### 🟡 CSRF Defaults Conflict with SPA Token Loading — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** CONFIG / SECURITY + +**Description:** +The CSRF middleware defaults the CSRF cookie to `httpOnly: true`, but the SPA reads `bt_csrf_token` from `document.cookie` and sends it in `x-csrf-token`. With default settings, state-changing requests from the client will not include the token. `docker-compose.yml` compensates with `CSRF_HTTP_ONLY=false`, which means the secure default and the actual SPA contract disagree. + +**Affected Files:** +- `middleware/csrf.js:12-16` +- `middleware/csrf.js:42-52` +- `client/api.js:1-16` +- `docker-compose.yml` + +**Potential Fix:** +Choose one CSRF pattern and align defaults/docs: either intentionally use a readable double-submit token for the SPA, or provide a `/api/csrf-token` endpoint/header bootstrap so the cookie can stay HTTP-only. + +**Severity:** MEDIUM + +### 🟡 Change-Password Routes Are Globally Exempted from CSRF — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** SECURITY + +**Description:** +`server.js` sets `req.csrfSkip = true` for `/api/auth/change-password` and `/api/profile/change-password`. These routes still require the current password, so this is not an immediate account-takeover issue, but password-changing is a sensitive state mutation and should not be excluded from the same CSRF protection as other authenticated writes. + +**Affected Files:** +- `server.js:70-71` +- `routes/auth.js:116-156` +- `routes/profile.js:177-230` + +**Potential Fix:** +Remove the global CSRF skip for password-change routes once the SPA token flow is aligned, and keep only login/OIDC bootstrap routes exempt where no authenticated session exists yet. + +**Severity:** MEDIUM + +### 🟡 Notification Due-Day Math Can Miss Same-Day Reminders — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** BUG + +**Description:** +`runNotifications()` computes `diffDays` by subtracting the current timestamp from midnight on the due date and flooring the result. After midnight has passed on the due date, the difference becomes a small negative value and floors to `-1`, so due-today bills can be classified as overdue instead of `due_today`. Similar timezone/partial-day drift can affect 1-day and 3-day reminders. + +**Affected Files:** +- `services/notificationService.js:172-175` +- `services/notificationService.js:213-222` + +**Potential Fix:** +Normalize both dates to local date-only midnight or compare ISO date strings/calendar days before calculating reminder type. Add tests around morning/afternoon due-today execution. + +**Severity:** MEDIUM + +### 🟡 Upcoming Bills Allows Negative Day Windows — MEDIUM +**Priority:** MEDIUM +**Added:** 2026-05-10 by Prime (code review) +**Type:** BUG / API_CONTRACT + +**Description:** +`GET /api/tracker/upcoming` caps `days` at 365 but does not enforce a lower bound or reject non-numeric input. `days=-30` creates a cutoff before today and returns no upcoming bills; `days=abc` leaves `cutoff` invalid. The endpoint contract says upcoming bills in the next N days, so invalid windows should be rejected or normalized. + +**Affected Files:** +- `routes/tracker.js:245-253` +- `routes/tracker.js:284-291` + +**Potential Fix:** +Parse `days` with integer validation, require `1 <= days <= 365`, and return standardized 400 errors for invalid input. + +**Severity:** MEDIUM + + ### Architecture: Business Logic Mixed with Route Handlers **Priority:** MEDIUM **Added:** 2026-05-08 by Neo @@ -91,6 +286,44 @@ Many routes contain business logic that should be extracted to service layers. ### 🔵 LOW + +### 🔵 Export Formats Include Sensitive Bill Credential Fields by Default — LOW +**Priority:** LOW +**Added:** 2026-05-10 by Prime (code review) +**Type:** SECURITY / PRIVACY + +**Description:** +Full user exports include `website`, `username`, `account_info`, notes, and monthly notes by default. This may be intended for portability, but it turns every Excel/SQLite export into a high-sensitivity artifact and there is no lightweight/redacted export option. + +**Affected Files:** +- `routes/export.js:88-153` +- `routes/export.js:156-199` +- `routes/profile.js:236-254` + +**Potential Fix:** +Add explicit UI copy warning that exports may contain account metadata, and consider a redacted export mode that excludes credential/account fields and free-form notes. + +**Severity:** LOW + +### 🔵 Duplicate Local Login Route Increases Auth Drift Risk — LOW +**Priority:** LOW +**Added:** 2026-05-10 by Prime (code review) +**Type:** TECH_DEBT / SECURITY + +**Description:** +Local username/password login logic exists in both `routes/auth.js` and `routes/authLogin.js`. Only `routes/auth.js` appears mounted in `server.js`, while `authLogin.js` is a near-duplicate public login handler. Duplicate auth code can silently drift on rate limiting, audit logging, CSRF exemptions, and error handling. + +**Affected Files:** +- `routes/auth.js:17-44` +- `routes/authLogin.js:1-39` +- `server.js:73-75` + +**Potential Fix:** +Delete the unused route module or refactor both route registrations to call one shared login handler. Add a startup/router inventory test so orphan auth routes do not linger. + +**Severity:** LOW + + ### Add comprehensive unit and integration tests **Priority:** LOW **Added:** 2026-05-08 by Scarlett diff --git a/routes/auth.js b/routes/auth.js index 8433495..0b4dc3f 100644 --- a/routes/auth.js +++ b/routes/auth.js @@ -31,16 +31,21 @@ router.post('/login', (req, res, next) => { return res.status(400).json(standardizeError('Username and password are required', 'VALIDATION_ERROR', !username ? 'username' : 'password')); } - const result = await login(username, password); - if (!result) { - logAudit({ user_id: null, action: 'login.failure', details: { username: req.body.username }, ip_address: req.ip, user_agent: req.get('user-agent') }); - return res.status(401).json(standardizeError('Invalid username or password', 'AUTH_ERROR')); + try { + const result = await login(username, password); + if (!result) { + logAudit({ user_id: null, action: 'login.failure', details: { username }, ip_address: req.ip, user_agent: req.get('user-agent') }); + return res.status(401).json(standardizeError('Invalid username or password', 'AUTH_ERROR')); + } + + logAudit({ user_id: result.user.id, action: 'login.success', ip_address: req.ip, user_agent: req.get('user-agent') }); + + res.cookie(COOKIE_NAME, result.sessionId, cookieOpts(req)); + res.json({ user: result.user }); + } catch (err) { + console.error('Login error:', err); + res.status(500).json(standardizeError('Login failed', 'SERVER_ERROR')); } - - logAudit({ user_id: result.user.id, action: 'login.success', ip_address: req.ip, user_agent: req.get('user-agent') }); - - res.cookie(COOKIE_NAME, result.sessionId, cookieOpts(req)); - res.json({ user: result.user }); }); // POST /api/auth/logout diff --git a/routes/authLogin.js b/routes/authLogin.js deleted file mode 100644 index 4462b9f..0000000 --- a/routes/authLogin.js +++ /dev/null @@ -1,39 +0,0 @@ -const express = require('express'); -const router = express.Router(); - -const { getSetting } = require('../db/database'); -const { login, cookieOpts, COOKIE_NAME } = require('../services/authService'); -const { logAudit } = require('../services/auditService'); -const { standardizeError } = require('../middleware/errorFormatter'); - -// POST /api/auth/login -// Public endpoint - no CSRF protection needed (no session to hijack) -router.post('/login', async (req, res) => { - // Respect admin-configured login method toggle - if (getSetting('local_login_enabled') === 'false') { - return res.status(403).json(standardizeError('Local username/password login is not enabled on this server.', 'FORBIDDEN')); - } - - const { username, password } = req.body; - if (!username || !password) { - return res.status(400).json(standardizeError('Username and password are required', 'VALIDATION_ERROR', !username ? 'username' : 'password')); - } - - try { - const result = await login(username, password); - if (!result) { - logAudit({ user_id: null, action: 'login.failure', details: { username }, ip_address: req.ip, user_agent: req.get('user-agent') }); - return res.status(401).json(standardizeError('Invalid username or password', 'AUTH_ERROR')); - } - - logAudit({ user_id: result.user.id, action: 'login.success', ip_address: req.ip, user_agent: req.get('user-agent') }); - - res.cookie(COOKIE_NAME, result.sessionId, cookieOpts(req)); - res.json({ user: result.user }); - } catch (err) { - console.error('Login error:', err); - res.status(500).json(standardizeError('Login failed', 'SERVER_ERROR')); - } -}); - -module.exports = router; diff --git a/server.js b/server.js index eddfd77..c477ed2 100644 --- a/server.js +++ b/server.js @@ -12,7 +12,6 @@ const { importLimiter, exportLimiter, adminActionLimiter, oidcLimiter, loginLimi require('./middleware/rateLimiter'); const { csrfMiddleware, csrfTokenProvider } = require('./middleware/csrf'); -const authLoginRouter = require('./routes/authLogin'); const app = express(); const PORT = process.env.PORT || 3000; const DIST = path.join(__dirname, 'dist'); @@ -64,7 +63,7 @@ function skipRateLimitIfNoUsers(limiter) { // Mount login router with conditional rate limiting // If no users exist, rate limit is bypassed; otherwise it applies -app.use('/api/auth/login', skipRateLimitIfNoUsers(loginLimiter), authLoginRouter); +app.use('/api/auth/login', skipRateLimitIfNoUsers(loginLimiter)); // Password change routes are exempt from CSRF - session-based auth is primary protection // Set csrfSkip before the auth routes middleware runs app.use('/api/auth/change-password', (req, res, next) => { req.csrfSkip = true; next(); });