20 KiB

Raw Blame History

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: ✅

// 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:

// 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:

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:

The SQL is hardcoded, not user input
No dynamic ALTER TABLE is used
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:

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:

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:

The endpoint is admin-only (protected by requireAuth, requireAdmin)
Rate limiting is applied to prevent abuse
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:

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:

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:

// 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:

List all notification templates (exposes internal configuration)
Read notification settings (exposes user preferences)
Modify notification settings (privilege escalation)

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

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:

// 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:

Inject malicious SQL via userId parameter
Extract sensitive data from the database
Modify or delete records

Remediation Required: Use parameterized queries:

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:

// 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:

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);
}

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:

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:

# 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:

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:

// 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:

Access other users' bills
Modify other users' bills
Delete other users' bills

Remediation Required: Add authorization check at service layer:

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:

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

Immediate: Add requireAuth, requireUser middleware to all notification routes
Immediate: Replace string concatenation in notification service with parameterized queries
Before Release: Add authorization checks to billsService.js
Before Release: Add authorization checks to subscriptionsService.js
Nice to Have: Add client-side rate limiting to AboutPage
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

20 KiB Raw Blame History Unescape Escape

Security Audit — Bill Tracker

Executive Summary

Detailed Findings

1. INIT_REGULAR_USER / INIT_REGULAR_PASS Environment Variables

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

Finding 1B: Missing Password Validation for INIT_REGULAR_PASS — FIXED

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

2. Migration v0.42 - bill_history_ranges Table

Finding 2A: SQL Injection Prevention Not Applied — MITIGATED

Finding 2B: No Idempotency Check — MITIGATED

3. Security Fixes - Admin About Endpoint Hardening

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

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

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

4. Notification Endpoint Security Issues — NEW FINDINGS

Finding 4A: Notification Routes Missing Auth Middleware — FIXED

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

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

5. CSRF Cookie Settings — STILL OPEN

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

6. Database Schema Changes

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

Fresh Comprehensive Security Review (v0.34.2)

Routes Audit

Services Audit

Critical Findings

Finding F1: Bills Service Missing Authorization Checks — STILL OPEN

Finding F2: Subscriptions Service Missing Authorization Checks — STILL OPEN

Finding F3: Notifications Service SQL Injection — STILL OPEN

Finding F4: Path Disclosure in Error Messages — STILL OPEN

Summary of Required Fixes

Recommended Actions

OWASP Top 10 Mapping

20 KiB

Raw Blame History