538 lines
17 KiB
Markdown
538 lines
17 KiB
Markdown
# 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*
|