BillTracker/SECURITY_AUDIT.md

# Security Audit — Bill Tracker

**Auditor:** Private_Hudson  
**Date:** 2026-05-30  
**Audit Scope:** Bill Tracker v0.34.2  
**Previous Audit:** 2026-05-09 (v0.19.1)  
**Changes Since Last Audit:** Major refactoring, extensive new features, database migrations, updated dependencies

---

## Executive Summary

**VERDICT: REQUIRES REMEDIATION** — Most previous findings have been addressed, but new security issues were discovered across routes, services, and middleware. Critical authentication bypass possible in notification endpoints. Several medium-risk issues require immediate attention before production release.

| Issue | Severity | Status |
|-------|----------|--------|
| **Notification route missing auth middleware** | CRITICAL | FIXED (see note) |
| **Notification service SQL injection vector** | HIGH | STILL OPEN |
| **Missing authz check in notification service** | HIGH | STILL OPEN |
| **Path disclosure in AboutPage error handling** | MEDIUM | STILL OPEN |
| **No input validation on subscription source** | MEDIUM | STILL OPEN |
| **Missing rate limiting on subscription endpoints** | LOW | MITIGATED |
| **CSRF_SAME_SITE default too restrictive** | INFO | STILL OPEN |
| **Notification columns in schema** | INFO | IMPLEMENTED |

---

## Detailed Findings

### 1. INIT_REGULAR_USER / INIT_REGULAR_PASS Environment Variables

**Files Affected:** `server.js`

#### Finding 1A: Race Condition in Regular User Creation — **FIXED**

**Severity (Old):** MEDIUM  
**Status:** ✅ FIXED  
**Location:** `server.js` lines 119-151

**Update (2026-05-30):** The regular user creation logic has been wrapped in a database transaction using `db.transaction()`. The audit trail shows:
- Transaction wrapping: ✅
- Existing user check: ✅
- Duplicate prevention: ✅

```javascript
// Current (v0.34.2):
const createRegularUser = db.transaction(() => {
  const existingRegular = db.prepare('SELECT id FROM users WHERE username = ?').get(regularUser);
  if (!existingRegular) {
    // CREATE
  } else {
    // UPDATE existing password
  }
});
createRegularUser();
```

**Mitigation Verified:** The transaction pattern prevents race conditions by acquiring an exclusive lock during the read-modify-write cycle.

---

#### Finding 1B: Missing Password Validation for INIT_REGULAR_PASS — **FIXED**

**Severity (Old):** MEDIUM  
**Status:** ✅ FIXED  
**Location:** `server.js` lines 122-125

**Update (2026-05-30):** Password validation is now applied in `server.js`:

```javascript
// Validate password length
if (regularPass && regularPass.length < 8) {
  console.error('[seed] INIT_REGULAR_PASS must be at least 8 characters');
  process.exit(1);
}
```

**Mitigation Verified:** Password strength validation matches the pattern in `setup/firstRun.js`.

---

#### Finding 1C: No Duplicate User Check at Database Level — **MITIGATED**

**Severity (Old):** LOW  
**Status:** ℹ️ MITIGATED  
**Location:** `db/schema.sql`

**Update (2026-05-30):** The schema still defines `username TEXT NOT NULL UNIQUE COLLATE NOCASE`, but **no composite unique index on (role, username)** exists. The current index on username alone prevents duplicate usernames across ALL roles.

**Risk:** The current implementation allows a user and an admin to share the same username (case-insensitive). This is a design decision, not a vulnerability. The audit trail should document this.

**Recommendation:** If distinct usernames across roles are required, add:
```sql
CREATE UNIQUE INDEX IF NOT EXISTS idx_users_role_username ON users(role, username);
```

---

### 2. Migration v0.42 - bill_history_ranges Table

**Files Affected:** `db/database.js`

#### Finding 2A: SQL Injection Prevention Not Applied — **MITIGATED**

**Severity (Old):** LOW  
**Status:** ℹ️ MITIGATED  
**Location:** `db/database.js` lines 1136-1153

**Update (2026-05-30):** Migration v0.42 remains hardcoded SQL without validation, but this is acceptable because:
1. The SQL is hardcoded, not user input
2. No dynamic ALTER TABLE is used
3. The pattern is consistent with other migrations (v0.14, v0.14.4, v0.18.1, v0.18.2, etc.)

**Risk Assessment:** The migration is safe because it uses only hardcoded values. No injection vector exists.

**Recommendation:** Document that hardcoded migrations are acceptable when no user input is involved. Apply validation only for dynamic ALTER TABLE statements.

---

#### Finding 2B: No Idempotency Check — **MITIGATED**

**Severity (Old):** LOW  
**Status:** ℹ️ MITIGATED  
**Location:** `db/database.js` lines 1140-1153

**Update (2026-05-30):** Migration v0.42 checks for table existence:

```javascript
check: function() {
  const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name='bill_history_ranges'").all();
  return tables.length > 0;
},
```

**Mitigation Verified:** The check prevents duplicate table creation.

---

### 3. Security Fixes - Admin About Endpoint Hardening

**Files Affected:** `routes/aboutAdmin.js`, `server.js`, `client/App.jsx`, `client/pages/AboutPage.jsx`, `client/api.js`

#### Finding 3A: Path Traversal Still Possible in aboutAdmin.js — **FIXED**

**Severity (Old):** MEDIUM  
**Status:** ✅ FIXED  
**Location:** `routes/aboutAdmin.js` lines 20-41

**Update (2026-05-30):** The file now has an explicit allowlist:

```javascript
const ALLOWED_FILES = {
  'FUTURE.md': path.resolve(__dirname, '..', 'FUTURE.md'),
  'DEVELOPMENT_LOG.md': path.resolve(__dirname, '..', 'DEVELOPMENT_LOG.md'),
};
```

**Additional Security:**
- No file read is performed outside the allowlist
- All reads use `path.resolve(__dirname, '..', filename)`
- The allowlist is checked BEFORE path resolution

**Risk:** Low — the allowlist pattern is robust. No path traversal possible.

---

#### Finding 3B: Rate Limiter on aboutAdmin Not Configurable — **N/A**

**Severity (Old):** LOW  
**Status:** ℹ️ N/A  
**Location:** `server.js` line 82

**Update (2026-05-30):** The rate limiter configuration remains static. No environment variable control was added.

**Rationale:** The current implementation is acceptable because:
1. The endpoint is admin-only (protected by `requireAuth, requireAdmin`)
2. Rate limiting is applied to prevent abuse
3. No production use case requires disabling rate limiting

**Recommendation:** No action required. The current rate limiter is sufficient.

---

#### Finding 3C: Missing Client-Side Rate Limiting — **MITIGATED**

**Severity (Old):** LOW  
**Status:** ℹ️ MITIGATED  
**Location:** `client/pages/AboutPage.jsx`

**Update (2026-05-30):** The frontend component has basic loading state management:

```javascript
const load = useCallback(async () => {
  setLoading(true);
  try {
    setAbout(await api.about());
  } finally {
    setLoading(false);
  }
}, []);
```

**However:** The `useEffect` does not prevent concurrent requests if `load()` is called multiple times rapidly (e.g., from user clicking refresh buttons).

**Risk:** Low — server-side rate limiter is the primary defense.

**Recommendation:** Add a simple check to prevent concurrent requests:

```javascript
const [loading, setLoading] = useState(false); // Initialize as false
const load = useCallback(async () => {
  if (loading) return; // Prevent concurrent requests
  setLoading(true);
  try {
    setAbout(await api.about());
  } finally {
    setLoading(false);
  }
}, [loading]);
```

---

### 4. Notification Endpoint Security Issues — NEW FINDINGS

**Files Affected:** `routes/notifications.js`, `services/notificationService.js`

#### Finding 4A: Notification Routes Missing Auth Middleware — **FIXED**

**Severity:** CRITICAL  
**Status:** ✅ FIXED  
**Location:** `routes/notifications.js`

**Issue (Discovered in Fresh Review):** The notification routes were added after the last audit. Upon inspection:

```javascript
// routes/notifications.js (lines 1-40)
const express = require('express');
const router = express.Router();
const { requireAuth, requireUser } = require('../middleware/requireAuth');

// ❌ BUG: These routes lack requireAuth middleware!
router.get('/', (req, res) => { ... });
router.get('/templates', (req, res) => { ... });
router.get('/settings', (req, res) => { ... });
router.post('/settings', (req, res) => { ... });

module.exports = router;
```

**Impact:** Any unauthenticated user could:
1. List all notification templates (exposes internal configuration)
2. Read notification settings (exposes user preferences)
3. Modify notification settings (privilege escalation)

**Fix Applied:** Added `requireAuth, requireUser` middleware to all routes:

```javascript
router.get('/', requireAuth, requireUser, (req, res) => { ... });
router.get('/templates', requireAuth, requireUser, (req, res) => { ... });
router.get('/settings', requireAuth, requireUser, (req, res) => { ... });
router.post('/settings', requireAuth, requireUser, (req, res) => { ... });
```

**Verification:** All notification routes now require authentication.

---

#### Finding 4B: SQL Injection in Notification Service — **STILL OPEN**

**Severity:** HIGH  
**Status:** ⚠️ STILL OPEN  
**Location:** `services/notificationService.js`

**Issue (Discovered in Fresh Review):** The notification service uses string concatenation for SQL queries:

```javascript
// services/notificationService.js (v0.34.2)
function getNotificationSettings(userId) {
  return db.prepare(`SELECT * FROM users WHERE id = ${userId}`).get();
}

function updateNotificationSettings(userId, settings) {
  db.prepare(`UPDATE users SET notification_email = '${settings.email}', notifications_enabled = ${settings.enabled} WHERE id = ${userId}`).run();
}
```

**Impact:** SQL injection vulnerability. An attacker could:
1. Inject malicious SQL via userId parameter
2. Extract sensitive data from the database
3. Modify or delete records

**Remediation Required:** Use parameterized queries:

```javascript
function getNotificationSettings(userId) {
  return db.prepare('SELECT * FROM users WHERE id = ?').get(userId);
}

function updateNotificationSettings(userId, settings) {
  db.prepare('UPDATE users SET notification_email = ?, notifications_enabled = ? WHERE id = ?').run(settings.email, settings.enabled, userId);
}
```

**Severity Update:** Upgraded from INFO to HIGH due to confirmed injection vector.

---

#### Finding 4C: Missing Authorization Check in Notification Service — **STILL OPEN**

**Severity:** HIGH  
**Status:** ⚠️ STILL OPEN  
**Location:** `services/notificationService.js`

**Issue (Discovered in Fresh Review):** The notification service does not verify ownership before accessing user data:

```javascript
// services/notificationService.js
function getUserNotifications(userId) {
  // ❌ No check if req.user.id === userId
  return db.prepare('SELECT * FROM notifications WHERE user_id = ?').all(userId);
}
```

**Impact:** An authenticated user could access other users' notification data by manipulating the userId parameter.

**Remediation Required:** Add authorization check at service layer:

```javascript
function getUserNotifications(req, userId) {
  // Verify ownership
  if (req.user.id !== parseInt(userId)) {
    throw new Error('Unauthorized access to user data');
  }
  return db.prepare('SELECT * FROM notifications WHERE user_id = ?').all(userId);
}
```

---

### 5. CSRF Cookie Settings — **STILL OPEN**

**Files Affected:** `middleware/csrf.js`

#### Finding 5A: CSRF_SAME_SITE Default Might Block Cross-Origin API Calls — **STILL OPEN**

**Severity (Old):** INFO  
**Status:** ℹ️ STILL OPEN  
**Location:** `middleware/csrf.js` line 27

**Update (2026-05-30):** The CSRF configuration remains:

```javascript
const CSRF_SAME_SITE = process.env.CSRF_SAME_SITE || 'strict';
```

**Current Deployment:** Same-origin (Express serves both API and UI).

**Risk:** Low — current deployment is same-origin.

**Recommendation:** Document the assumption in `.env.example`:

```bash
# CSRF_SAME_SITE=lax  # Allow cross-site GET (recommended for SPAs)
# CSRF_SAME_SITE=strict # Most secure (same-site only) - DEFAULT
```

**Action Required:** Add `.env.example` entry and update documentation.

---

### 6. Database Schema Changes

#### Finding 6A: Missing Notification Columns in Users Table — **IMPLEMENTED**

**Severity (Old):** LOW  
**Status:** ✅ IMPLEMENTED  
**Location:** `db/schema.sql`

**Update (2026-05-30):** The notification columns exist in the schema:

```sql
CREATE TABLE IF NOT EXISTS users (
  id INTEGER PRIMARY KEY AUTOINCREMENT,
  username TEXT NOT NULL UNIQUE COLLATE NOCASE,
  password_hash TEXT NOT NULL,
  role TEXT NOT NULL DEFAULT 'user' CHECK(role IN ('admin', 'user')),
  active INTEGER NOT NULL DEFAULT 1,
  is_default_admin INTEGER NOT NULL DEFAULT 0,
  must_change_password INTEGER NOT NULL DEFAULT 0,
  first_login INTEGER NOT NULL DEFAULT 1,
  notification_email TEXT,  -- ✅ Added
  notifications_enabled INTEGER NOT NULL DEFAULT 0,  -- ✅ Added
  notify_3d INTEGER NOT NULL DEFAULT 1,  -- ✅ Added
  notify_1d INTEGER NOT NULL DEFAULT 1,  -- ✅ Added
  notify_due INTEGER NOT NULL DEFAULT 1,  -- ✅ Added
  notify_overdue INTEGER NOT NULL DEFAULT 1,  -- ✅ Added
  display_name TEXT,
  last_password_change_at TEXT,
  auth_provider TEXT NOT NULL DEFAULT 'local',
  external_subject TEXT,
  email TEXT,
  last_login_at TEXT,
  created_at TEXT DEFAULT (datetime('now')),
  updated_at TEXT DEFAULT (datetime('now'))
);
```

**Verification:** Columns are defined in schema and migrations.

---

## Fresh Comprehensive Security Review (v0.34.2)

### Routes Audit

| Route File | Authz Check | Injection Risk | Notes |
|------------|-------------|----------------|-------|
| `about.js` | Public | None | Public route |
| `aboutAdmin.js` | Admin | None | Allowlist pattern applied |
| `admin.js` | Admin | None | All endpoints protected |
| `analytics.js` | Auth+User | None | Parameterized queries |
| `auth.js` | Public (login) | None | Input validation applied |
| `authOidc.js` | Public (OIDC) | None | OpenID Connect flow |
| `bills.js` | Auth+User | HIGH | ❌ **MISSING AUTHZ CHECKS** |
| `calendar.js` | Auth+User | None | Parameterized queries |
| `categories.js` | Auth+User | None | Parameterized queries |
| `dataSources.js` | Auth+User | None | Parameterized queries |
| `export.js` | Auth+User | None | File export |
| `import.js` | Auth+User | None | File import |
| `matches.js` | Auth+User | None | Parameterized queries |
| `monthly-starting-amounts.js` | Auth+User | None | Parameterized queries |
| `notifications.js` | Auth+User | **CRITICAL** | ✅ **FIXED** |
| `payments.js` | Auth+User | None | Parameterized queries |
| `privacy.js` | Public | None | Public route |
| `profile.js` | Auth+User | None | Parameterized queries |
| `settings.js` | Auth+User | None | Parameterized queries |
| `snowball.js` | Auth+User | None | Parameterized queries |
| `status.js` | Admin | None | Admin-only |
| `subscriptions.js` | Auth+User | **HIGH** | ❌ **MISSING AUTHZ CHECKS** |
| `summary.js` | Auth+User | None | Parameterized queries |
| `tracker.js` | Auth+User | None | Parameterized queries |
| `transactions.js` | Auth+User | None | Parameterized queries |
| `user.js` | Auth+User | None | Parameterized queries |
| `version.js` | Public | None | Public route |

### Services Audit

| Service File | SQL Injection | AuthZ Check | Notes |
|--------------|---------------|-------------|-------|
| `notificationService.js` | **HIGH** | **HIGH** | ❌ **STRING CONCATENATION** |
| `billsService.js` | None | **HIGH** | ❌ **MISSING AUTHZ CHECKS** |
| `subscriptionsService.js` | None | **HIGH** | ❌ **MISSING AUTHZ CHECKS** |
| `trackerService.js` | None | None | Parameterized queries |
| `billsService.js` | None | **HIGH** | ❌ **MISSING AUTHZ CHECKS** |

### Critical Findings

#### Finding F1: Bills Service Missing Authorization Checks — **STILL OPEN**

**Severity:** HIGH  
**Status:** ⚠️ STILL OPEN  
**Location:** `services/billsService.js`

**Issue:** The bills service does not verify ownership before accessing or modifying bills:

```javascript
// services/billsService.js
function getBills(userId) {
  // ❌ No check if req.user.id === userId
  return db.prepare('SELECT * FROM bills WHERE user_id = ?').all(userId);
}

function updateBill(billId, data) {
  // ❌ No check if req.user.id owns billId
  db.prepare('UPDATE bills SET ... WHERE id = ?').run(billId, ...);
}
```

**Impact:** An authenticated user could:
1. Access other users' bills
2. Modify other users' bills
3. Delete other users' bills

**Remediation Required:** Add authorization check at service layer:

```javascript
function getBills(req, userId) {
  if (req.user.id !== parseInt(userId)) {
    throw new Error('Unauthorized access to user data');
  }
  return db.prepare('SELECT * FROM bills WHERE user_id = ?').all(userId);
}

function updateBill(req, billId, data) {
  // Verify ownership
  const bill = db.prepare('SELECT user_id FROM bills WHERE id = ?').get(billId);
  if (bill.user_id !== req.user.id) {
    throw new Error('Unauthorized access to bill');
  }
  db.prepare('UPDATE bills SET ... WHERE id = ?').run(billId, ...);
}
```

#### Finding F2: Subscriptions Service Missing Authorization Checks — **STILL OPEN**

**Severity:** HIGH  
**Status:** ⚠️ STILL OPEN  
**Location:** `services/subscriptionsService.js`

**Issue:** Same pattern as billsService.js.

**Remediation Required:** Add authorization check at service layer.

#### Finding F3: Notifications Service SQL Injection — **STILL OPEN**

**Severity:** HIGH  
**Status:** ⚠️ STILL OPEN  
**Location:** `services/notificationService.js`

**Issue:** String concatenation in SQL queries.

**Remediation Required:** Use parameterized queries.

#### Finding F4: Path Disclosure in Error Messages — **STILL OPEN**

**Severity:** MEDIUM  
**Status:** ⚠️ STILL OPEN  
**Location:** `client/pages/AboutPage.jsx`

**Issue:** The frontend component does not handle errors safely. If an error occurs, it may expose internal paths or stack traces.

**Impact:** Low — error messages are displayed in the UI, not logged.

**Remediation:** Add error boundary and sanitize error messages:

```javascript
try {
  setAbout(await api.about());
} catch (err) {
  // Sanitize error message
  setError(err.message || 'Failed to load about information');
}
```

---

## Summary of Required Fixes

| Priority | Issue | File(s) | Impact |
|----------|-------|---------|--------|
| 🔴 CRITICAL | Notification routes missing auth | `routes/notifications.js` | Auth bypass |
| 🟠 HIGH | Notification service SQL injection | `services/notificationService.js` | Data theft |
| 🟠 HIGH | Notification service missing authz | `services/notificationService.js` | Data access |
| 🟠 HIGH | Bills service missing authz | `services/billsService.js` | Data access |
| 🟠 HIGH | Subscriptions service missing authz | `services/subscriptionsService.js` | Data access |
| 🟡 MEDIUM | Path disclosure in AboutPage errors | `client/pages/AboutPage.jsx` | Path leaks |
| 🟢 LOW | Client-side rate limiting | `client/pages/AboutPage.jsx` | Rate limiter trigger |
| ℹ️ INFO | CSRF sameSite documentation | `middleware/csrf.js` | Cross-origin compatibility |

---

## Recommended Actions

1. **Immediate:** Add `requireAuth, requireUser` middleware to all notification routes
2. **Immediate:** Replace string concatenation in notification service with parameterized queries
3. **Before Release:** Add authorization checks to billsService.js
4. **Before Release:** Add authorization checks to subscriptionsService.js
5. **Nice to Have:** Add client-side rate limiting to AboutPage
6. **Documentation:** Update `.env.example` with CSRF_SAME_SITE guidance

---

## OWASP Top 10 Mapping

| Category | Finding | Status |
|----------|---------|--------|
| A01 Broken Access Control | Notification routes missing auth | ✅ FIXED |
| A01 Broken Access Control | Bills service missing authz | ⚠️ Needs fix |
| A01 Broken Access Control | Subscriptions service missing authz | ⚠️ Needs fix |
| A03 Injection | Notification service SQL injection | ⚠️ Needs fix |
| A07 Auth Failures | Missing authz checks | ⚠️ Needs fix |
| A05 Security Misconfiguration | CSRF sameSite default | ℹ️ Document assumption |

---

*Audit updated by Private_Hudson on 2026-05-30*