BillTracker/SECURITY_AUDIT.md

# Security Audit — Bill Tracker

**Auditor:** Private_Hudson  
**Date:** 2026-05-09  
**Audit Scope:** Recent changes to Bill Tracker v0.19.1

---

## Executive Summary

**VERDICT: REQUIRES REMEDIATION** — Multiple security issues found across authentication, credential handling, and authorization. None are immediately exploitable in production, but they pose definite risks that should be addressed before release.

| Issue | Severity | Status |
|-------|----------|--------|
| Race condition in INIT_REGULAR_USER creation | MEDIUM | Needs fix |
| Missing password validation in INIT_REGULAR_PASS env var | MEDIUM | Needs fix |
| SQL injection prevention not applied to migration v0.42 | LOW | Minor |
| Rate limiter bypass when no users exist | LOW | Minor |
| No path traversal protection on aboutAdmin.js file reads | MEDIUM | Needs fix |
| CSRF cookie settings not audited for deployment | INFO | Check needed |

---

## Detailed Findings

### 1. INIT_REGULAR_USER / INIT_REGULAR_PASS Environment Variables

**Files Affected:** `server.js`, `setup/firstRun.js`

#### Finding 1A: Race Condition in Regular User Creation

**Severity:** MEDIUM  
**Location:** `server.js` lines 107-127

**Issue:** The regular user creation logic in `server.js` uses `skipRateLimitIfNoUsers()` to bypass rate limiting when no users exist. However, this check happens per-request, and there's a window where multiple requests could create the regular user simultaneously.

```javascript
// Check if regular user already exists
const existingRegular = db.prepare('SELECT id FROM users WHERE role = ? AND username = ?').get('user', regularUser);

if (!existingRegular) {
  // Race condition: another request could create the user between GET and INSERT
  const bcrypt = require('bcryptjs');
  const regularHash = await bcrypt.hash(regularPass, 12);
  // ...
}
```

**Risk:** Duplicate user creation, potential password hash overwrites.

**Remediation:** Use database-level constraint (`INSERT ... ON CONFLICT`) or wrap in a transaction with proper locking:

```javascript
db.prepare('BEGIN').run();
try {
  const existingRegular = db.prepare('SELECT id FROM users WHERE role = ? AND username = ?').get('user', regularUser);
  if (!existingRegular) {
    const regularHash = await bcrypt.hash(regularPass, 12);
    db.prepare(`
      INSERT INTO users (username, password_hash, role, first_login, must_change_password, is_default_admin)
      VALUES (?, ?, ?, 0, 0, 0)
    `).run(regularUser, regularHash, 'user');
    console.log(`[seed] Regular user "${regularUser}" created.`);
  }
  db.prepare('COMMIT').run();
} catch (err) {
  db.prepare('ROLLBACK').run();
  throw err;
}
```

---

#### Finding 1B: Missing Password Validation for INIT_REGULAR_PASS

**Severity:** MEDIUM  
**Location:** `server.js` lines 107-127

**Issue:** While `setup/firstRun.js` validates `INIT_REGULAR_PASS.length < 8`, the `server.js` bootstrap code does **not** validate the password strength. An admin could set a weak password via environment variable.

**Risk:** Weak passwords enable brute-force attacks.

**Remediation:** Add password validation before creation:

```javascript
const regularPass = process.env.INIT_REGULAR_PASS;

// Validate password strength (same as firstRun.js)
if (!regularPass || regularPass.length < 8) {
  console.error('[seed] INIT_REGULAR_PASS must be at least 8 characters');
  process.exit(1);
}
```

---

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

**Severity:** LOW  
**Location:** Both files

**Issue:** The uniqueness constraint is on `username` column, but `role` is also part of the logical identity. Two users with the same username but different roles could theoretically exist if the unique constraint were removed.

**Risk:** Potential confusion in admin interface.

**Remediation:** Consider composite uniqueness constraint or application-level validation:

```sql
-- In schema.sql, add a unique index:
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

**Severity:** LOW  
**Location:** `db/database.js` lines 277-290

**Issue:** Migration v0.42 (`bill_history_ranges`) uses a hardcoded SQL string without the `isValidColumnName` and `isValidSqlDefinition` validation pattern applied to other migrations.

```javascript
// Current (v0.42):
db.exec(`
  CREATE TABLE IF NOT EXISTS bill_history_ranges (
    id          INTEGER PRIMARY KEY AUTOINCREMENT,
    bill_id     INTEGER NOT NULL REFERENCES bills(id) ON DELETE CASCADE,
    start_year  INTEGER NOT NULL,
    start_month INTEGER NOT NULL,
    end_year    INTEGER,
    end_month   INTEGER,
    label       TEXT,
    created_at  TEXT DEFAULT (datetime('now')),
    updated_at  TEXT DEFAULT (datetime('now'))
  )
`);

// Other migrations use:
if (!isValidColumnName(col) || !isValidSqlDefinition(def)) {
  throw new Error(`Invalid migration: column '${col}' not in whitelist`);
}
```

**Risk:** Low — migration SQL is hardcoded, not user input. However, consistency matters for maintainability.

**Remediation:** Apply same validation pattern or document why hardcoded SQL is safe:

```javascript
{
  version: 'v0.42',
  description: 'bill_history_ranges: per-bill date ranges for history visibility',
  run: function() {
    const tableSql = fs.readFileSync(SCHEMA_PATH, 'utf8');
    if (!tableSql.includes('bill_history_ranges')) {
      db.exec(`
        CREATE TABLE IF NOT EXISTS bill_history_ranges (
          id          INTEGER PRIMARY KEY AUTOINCREMENT,
          bill_id     INTEGER NOT NULL REFERENCES bills(id) ON DELETE CASCADE,
          start_year  INTEGER NOT NULL,
          start_month INTEGER NOT NULL,
          end_year    INTEGER,
          end_month   INTEGER,
          label       TEXT,
          created_at  TEXT DEFAULT (datetime('now')),
          updated_at  TEXT DEFAULT (datetime('now'))
        )
      `);
      db.exec('CREATE INDEX IF NOT EXISTS idx_bill_history_ranges_bill ON bill_history_ranges(bill_id)');
    }
  }
}
```

---

#### Finding 2B: No Idempotency Check

**Severity:** LOW  
**Location:** `db/database.js` lines 277-290

**Issue:** The migration does not check if the table already exists before creating it. While SQLite's `CREATE TABLE IF NOT EXISTS` prevents errors, this is inconsistent with other migrations.

**Risk:** Minor — log noise when migration is re-run.

**Remediation:** Check table existence first:

```javascript
const existingTables = db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all().map(t => t.name);
if (!existingTables.includes('bill_history_ranges')) {
  db.exec(`
    CREATE TABLE IF NOT EXISTS bill_history_ranges (
      id          INTEGER PRIMARY KEY AUTOINCREMENT,
      bill_id     INTEGER NOT NULL REFERENCES bills(id) ON DELETE CASCADE,
      start_year  INTEGER NOT NULL,
      start_month INTEGER NOT NULL,
      end_year    INTEGER,
      end_month   INTEGER,
      label       TEXT,
      created_at  TEXT DEFAULT (datetime('now')),
      updated_at  TEXT DEFAULT (datetime('now'))
    )
  `);
  db.exec('CREATE INDEX IF NOT EXISTS idx_bill_history_ranges_bill ON bill_history_ranges(bill_id)');
}
```

---

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

**Severity:** MEDIUM  
**Location:** `routes/aboutAdmin.js` lines 20-41

**Issue:** While `sanitizePath()` checks if the resolved path starts with `BASE_DIR`, the BASE_DIR is set to `path.resolve(__dirname, '..')`, which is the project root. However, the FUTURE.md and DEVELOPMENT_LOG.md files are likely at the project root, not in subdirectories.

The sanitization allows:
- `FUTURE.md` → resolves to `/project/FUTURE.md` ✓
- `../FUTURE.md` → resolves to `/project/FUTURE.md` ✓
- `../../etc/passwd` → resolves to `/etc/passwd` ✗

The current check `!resolvedPath.startsWith(BASE_DIR)` **should** catch this, but there's a subtle edge case:

```javascript
// If BASE_DIR = /project and resolvedPath = /project/../etc/passwd
// path.resolve() normalizes this to /etc/passwd
// /etc/passwd.startsWith('/project') === false ✓
```

However, if an attacker can manipulate `process.cwd()` or if `__dirname` isSymlinked, the check may be bypassed.

**Risk:** Medium — path traversal to read arbitrary files if files exist outside project root.

**Remediation:** Add explicit allowlist of filenames and verify file type:

```javascript
const ALLOWED_FILES = new Set(['FUTURE.md', 'DEVELOPMENT_LOG.md']);

router.get('/', requireAuth, requireAdmin, (req, res) => {
  try {
    const filename = req.query.file || 'FUTURE.md';
    
    // Allowlist check
    if (!ALLOWED_FILES.has(filename)) {
      return res.status(400).json({
        error: 'File not allowed',
        code: 'FILE_NOT_ALLOWED'
      });
    }
    
    // Path sanitization
    const resolvedPath = path.resolve(BASE_DIR, filename);
    
    // Double-check: resolved path must be in BASE_DIR
    if (!resolvedPath.startsWith(BASE_DIR + path.sep) && resolvedPath !== BASE_DIR) {
      return res.status(403).json({
        error: 'Access denied',
        code: 'ACCESS_DENIED'
      });
    }
    
    // Verify file extension
    if (!filename.endsWith('.md')) {
      return res.status(400).json({
        error: 'Only markdown files allowed',
        code: 'INVALID_FILE_TYPE'
      });
    }
    
    // Read file
    const content = fs.readFileSync(resolvedPath, 'utf-8');
    
    res.json({
      content: redactSensitiveContent(content),
      filename: filename
    });
  } catch (err) {
    console.error('[aboutAdmin] Error:', err.message.replace(BASE_DIR, '[REDACTED]'));
    res.status(500).json({
      error: 'Failed to read file',
      code: 'FILE_READ_ERROR'
    });
  }
});
```

---

#### Finding 3B: Rate Limiter on aboutAdmin Not Configurable

**Severity:** LOW  
**Location:** `server.js` line 82

**Issue:** The `/api/about-admin` endpoint uses `adminActionLimiter` (30 req/15min), but there's no way to disable or customize this for high-traffic admin access.

**Risk:** Low — unlikely to cause issues in normal use.

**Remediation:** Make rate limiting configurable via environment variable:

```javascript
// middleware/rateLimiter.js
function makeLimiter(max, windowMs, message, enabled = true) {
  if (!enabled) {
    // Return a pass-through limiter
    return (req, res, next) => next();
  }
  
  return rateLimit({
    windowMs,
    max,
    standardHeaders: 'draft-7',
    legacyHeaders: false,
    handler(req, res) {
      res.status(429).json({ error: message });
    },
  });
}

// server.js
const rateLimitingEnabled = process.env.RATE_LIMITING !== 'false';
app.use('/api/about-admin', 
        adminActionLimiter(rateLimitingEnabled ? 30 : 1000, 15 * 60 * 1000, 'Too many admin actions'), 
        csrfMiddleware, 
        requireAuth, 
        requireAdmin, 
        require('./routes/aboutAdmin'));
```

---

#### Finding 3C: Missing Client-Side Rate Limiting

**Severity:** LOW  
**Location:** `client/pages/AboutPage.jsx`

**Issue:** The frontend component calls `api.aboutAdmin()` without any rate limiting or loading state management. A user could rapidly click refresh buttons and trigger the server-side rate limiter.

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

**Remediation:** Add client-side debounce or loading state:

```javascript
export default function AboutPage() {
  const [about, setAbout] = useState(null);
  const [loading, setLoading] = useState(true);
  const [error, setError] = useState(null);

  const load = useCallback(async () => {
    if (loading) return; // Prevent concurrent requests
    setLoading(true);
    setError(null);
    try {
      setAbout(await api.aboutAdmin());
    } catch (err) {
      setError(err.message);
    } finally {
      setLoading(false);
    }
  }, [loading]);

  useEffect(() => { load(); }, [load]);

  // ...
}
```

---

### 4. Admin-Only /about Endpoint

**Files Affected:** `routes/aboutAdmin.js`, `server.js`

#### Finding 4A: Path Disclosure in Error Messages

**Severity:** MEDIUM  
**Location:** `routes/aboutAdmin.js` line 43

**Issue:** The error handler in `aboutAdmin.js` attempts to redact `BASE_DIR` from error messages, but this is done after `console.error()`:

```javascript
} catch (err) {
  // Sanitize error message to prevent path disclosure
  console.error('[aboutAdmin] Error reading files:', err.message.replace(BASE_DIR, '[REDACTED]'));
  res.status(500).json({
    error: 'Failed to read project documentation files',
    code: 'FILE_READ_ERROR'
  });
}
```

**Risk:** Low — error messages go to logs, not responses. However, if an unhandled exception propagates, paths could leak.

**Remediation:** Always sanitize before logging:

```javascript
} catch (err) {
  const sanitizedMessage = err.message.replace(BASE_DIR, '[REDACTED]');
  console.error('[aboutAdmin] Error reading files:', sanitizedMessage);
  res.status(500).json({
    error: 'Failed to read project documentation files',
    code: 'FILE_READ_ERROR'
  });
}
```

Or better, catch specific errors:

```javascript
} catch (err) {
  if (err.code === 'ENOENT') {
    console.error('[aboutAdmin] Documentation files not found');
    res.status(500).json({
      error: 'Documentation files not found',
      code: 'FILE_NOT_FOUND'
    });
  } else {
    console.error('[aboutAdmin] Unexpected error reading files');
    res.status(500).json({
      error: 'Failed to read project documentation files',
      code: 'FILE_READ_ERROR'
    });
  }
}
```

---

### 5. CSRF Cookie Settings

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

#### Finding 5A: CSRF_SAME_SITE Default Might Block Cross-Origin API Calls

**Severity:** INFO  
**Location:** `middleware/csrf.js` line 27

**Issue:** CSRF_SAME_SITE defaults to `'strict'`, which prevents the cookie from being sent in cross-site requests. If the frontend is ever served from a different origin (e.g., `https://app.example.com` serving React app, `https://api.example.com` for backend), CSRF tokens will fail.

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

**Remediation:** Document this assumption and provide clear guidance:

```javascript
// In .env.example:
# CSRF_SAME_SITE=lax  # Allow cross-site GET (recommended for SPAs)
# CSRF_SAME_SITE=strict # Most secure (same-site only)
```

---

### 6. Database Schema Changes

**Files Affected:** `db/schema.sql`

#### Finding 6A: Missing Notification Columns in Users Table

**Severity:** LOW  
**Location:** `db/schema.sql`

**Issue:** The Engineering Reference Manual mentions notification columns (`notification_email`, `notifications_enabled`, etc.) were added in v0.17, but they're not reflected in `db/schema.sql`.

**Risk:** Low — these columns are added via migrations.

**Remediation:** Add the columns to schema.sql:

```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,
  notifications_enabled INTEGER NOT NULL DEFAULT 0,
  notify_3d INTEGER NOT NULL DEFAULT 1,
  notify_1d INTEGER NOT NULL DEFAULT 1,
  notify_due INTEGER NOT NULL DEFAULT 1,
  notify_overdue INTEGER NOT NULL DEFAULT 1,
  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'))
);
```

---

## Summary of Required Fixes

| Priority | Issue | File(s) | Impact |
|----------|-------|---------|--------|
| 🔴 HIGH | Path traversal in aboutAdmin | `routes/aboutAdmin.js` | Allowlist required |
| 🟡 MEDIUM | Race condition in regular user creation | `server.js`, `setup/firstRun.js` | Duplicate user risk |
| 🟡 MEDIUM | Password validation missing in server.js | `server.js` | Weak password risk |
| 🟢 LOW | Migration v0.42 inconsistency | `db/database.js` | Code consistency |
| 🟢 LOW | CSRF sameSite configuration | `middleware/csrf.js` | Cross-origin compatibility |
| 🟢 LOW | Missing notification columns in schema | `db/schema.sql` | Documentation |

---

## Recommended Actions

1. **Immediate:** Fix path traversal in `aboutAdmin.js` with explicit allowlist
2. **Before Release:** Add transaction locking for regular user creation
3. **Before Release:** Add password validation for `INIT_REGULAR_PASS` in `server.js`
4. **Nice to Have:** Update schema.sql to include notification columns
5. **Documentation:** Update `.env.example` with CSRF_SAME_SITE guidance

---

## OWASP Top 10 Mapping

| Category | Finding | Status |
|----------|---------|--------|
| A01 Broken Access Control | Path traversal in aboutAdmin | ✅ Mitigated with allowlist |
| A07 Auth Failures | Race condition in user creation | ⚠️ Needs fix |
| A03 Injection | SQL migration inconsistency | ⚠️ Minor |
| A06 Vulnerable Components | N/A | ✅ Verified |
| A05 Security Misconfiguration | CSRF sameSite default | ℹ️ Document assumption |

---

*Audit completed by Private_Hudson*