### 🟠 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.
### 🟡 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.
### 🔵 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.
Currently no unit tests exist for components or hooks. The only testing appears to be functional tests in `test-functional.js`. Component-level testing is missing.
Code quality and maintainability. Unit tests catch regressions and document component behavior. Bill Tracker has complex business logic (bill calculations, monthly state, analytics) that should be tested.