diff --git a/HISTORY.md b/HISTORY.md index be05cb9..a4aa372 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -3,6 +3,7 @@ ### πŸ› QA Fixes +- **[SimpleFIN] Purging a soft-deleted bill orphaned its matched transactions** β€” found on the live SimpleFIN DB: 3 transactions were `match_status='matched'` with `matched_bill_id=NULL`. Root cause: bills are soft-deleted (retained for recovery), then the retention GC (`pruneSoftDeletedFinancialRecords`, `services/cleanupService.js`) hard-deletes them past the 30-day window. `transactions.matched_bill_id` is `ON DELETE SET NULL`, so the purge nulled the pointer but left `match_status='matched'` β€” a limbo row **excluded from spending/analytics (`match_status != 'matched'`) yet attributed to no bill**, silently dropping that spend. The purge now releases those matches back to `'unmatched'` in the same transaction and self-heals any pre-existing orphans; retention behaviour is unchanged. Verified on a copy of the live DB (3β†’0 orphans, 0 transactions lost). Regression: 3 tests in `tests/backupAndCleanup.test.js`. (QA-B5-04) - **[Security] SQLite DB was created world-readable (644)** β€” `docker-entrypoint.sh` locked the data *directory* (`chmod 700`) but never the DB *file*, and SQLite created `bills.db`/`-wal`/`-shm` under the default umask (644). On a real deploy the DB (financial data + encrypted SimpleFIN token, sessions, SMTP/OIDC secrets) was world-readable, shielded only by the parent dir's 700. Added `umask 077` to the entrypoint (DB, WAL/SHM, backups, exports now created 600/700) plus an explicit `chmod 600` for pre-existing files on upgrade. (QA-B16-02) - **[Privacy] The version check is now opt-out-able** β€” the privacy policy described the external version check as "optional", but there was no way to disable it (it hit a hardcoded upstream host whenever the About/Status/version page loaded). Added an **admin toggle**: an `update_check_enabled` setting gates the request in `services/updateCheckService.js` (default on β€” when off, **no external request is made**), exposed via `GET`/`PUT /api/about-admin/update-check-setting` and a switch on the admin **System Status** page. Privacy policy updated to state an admin can disable it. Test: `tests/updateCheckOptOut.test.js`. (was QA-B16-01) - **[Security] Bill name could inject HTML into reminder emails** β€” `buildEmailHtml` (`services/notificationService.js`) escaped the bill name in the detail table but interpolated it **raw** into the reminder message line (`${bill.name} is due…`), so a bill named `` landed unescaped in the email HTML. Self-XSS (reminder emails go to the bill's owner), but a clear inconsistent-escaping bug β€” now escaped everywhere. Covered by `tests/notificationDelivery.test.js`. (was QA-B14-04) diff --git a/docs/QA_PLAN.md b/docs/QA_PLAN.md index d19cf73..17ac4b4 100644 --- a/docs/QA_PLAN.md +++ b/docs/QA_PLAN.md @@ -93,7 +93,7 @@ before cross-cutting; regression last). Update **Status** and **Findings** every | B2 | Tracker (core) | `/` buckets, pay/skip/notes/overrides, balance cards, overdue, ledger, drift | seeded + adversarial | βœ… | 0 / 0 | | B3 | Bills & schedules | `/bills` CRUD, custom schedules, reorder, merchant rules, historical import | adversarial | βœ… | 0 / 0 | | B4 | Subscriptions & Categories | `/subscriptions`, catalog, `/categories`, groups, reorder | seeded | βœ… | 0 / 0 | -| B5 | Reporting reconciliation | `/summary`, `/calendar`, `/analytics`, `/health` cross-check totals | seeded + large | βœ… | 0 / 3 | +| B5 | Reporting reconciliation | `/summary`, `/calendar`, `/analytics`, `/health` cross-check totals | seeded + large + **live SimpleFIN DB** | βœ… | 0 / 4 | | B6 | Spending | `/spending` YNAB view, averages, cover-overspending, safe-to-spend | seeded + edge months | βœ… | 0 / 1 | | B7 | Debt planning (math) | `/snowball`, `/payoff` APR/amortization vs hand-calc | edge (APR=0, $0 debt) | βœ… | 0 / 2 | | B8 | Banking & bank sync | `/bank-transactions`, SimpleFIN sync, matching, merchant/store, advisory filter | seeded txns | βœ… | 0 / 0 | @@ -131,6 +131,7 @@ until you get a clean cycle. |-------|---------|----------------|-----------------|------------------|--------| | 1 | 2026-07-02 | `bdbf231`β†’`5ffe2db` (dev) | 14 | **14 β†’ all fixed, verified & archived** (3Γ— S2 incl. broken "Send test push", email XSS, reconciliation family, seed 100Γ— cents) | πŸ” Phase 2 complete β€” 0 open. Every batch B0β†’B15 (+B-UI) run; 16 QA commits; guard suite green. | | 1Β·re-run | 2026-07-02 | `5ffe2db` (dev) | **0 new** | β€” | βœ… **Automated re-run clean.** CI (server 109 + client 34, build), UI E2E 27, probe 16 (authz 403, Tracker↔Summary↔Analytics reconcile exactly, seed guard, a11y 8/8), prod-smoke PASS. **All 17 batches βœ… for automatable scope; external-infra residuals listed below are non-blocking and carried to Cycle 2.** | +| 1Β·simplefin-live | 2026-07-03 | `5ffe2db` (dev) vs prod DB | **1** (QA-B5-04) | **1 β†’ fixed, verified & archived** | πŸ” Probed a **copy of the live SimpleFIN DB** (19 MB, v1.06: 3 users, 44 bills, 1,159 txns, 19 accounts, active SimpleFIN source). Integrity checks: dedup (1159/1159 distinct), money=integer cents, no double-match, pending have provider ids, no orphan-account txns β€” all pass **except** 3 matched txns with NULL bill β†’ QA-B5-04 (retention GC + `ON DELETE SET NULL`). Fixed in `cleanupService`; healing verified on a DB copy (3β†’0, 0 txns lost). | **Result key:** πŸ”„ in progress Β· πŸ” findings fixed, re-run required Β· βœ… clean (zero findings β€” QA complete) diff --git a/services/cleanupService.js b/services/cleanupService.js index 09ad3bc..d1f2b6b 100644 --- a/services/cleanupService.js +++ b/services/cleanupService.js @@ -135,14 +135,29 @@ function pruneImportHistory(maxAgeDays) { /** * Permanently purge soft-deleted bills and categories after a 30-day recovery * window. Bill deletion cascades to bill-owned records via foreign keys. + * + * transactions.matched_bill_id is ON DELETE SET NULL, so purging a bill nulls the + * pointer on any matched transaction but would leave match_status='matched' β€” a + * limbo row excluded from spending (match_status != 'matched') yet attributed to no + * bill. Release those matches back to 'unmatched' in the same transaction (and + * self-heal any pre-existing orphans) so purged bills don't silently drop spend. */ function pruneSoftDeletedFinancialRecords(maxAgeDays = 30) { const db = getDb(); const cutoff = `-${maxAgeDays} days`; const purge = db.transaction(() => { + const releasedMatches = db.prepare(` + UPDATE transactions + SET match_status = 'unmatched', matched_bill_id = NULL, updated_at = datetime('now') + WHERE match_status = 'matched' + AND (matched_bill_id IS NULL + OR matched_bill_id IN ( + SELECT id FROM bills WHERE deleted_at IS NOT NULL AND deleted_at < datetime('now', ?) + )) + `).run(cutoff).changes; const bills = db.prepare("DELETE FROM bills WHERE deleted_at IS NOT NULL AND deleted_at < datetime('now', ?)").run(cutoff).changes; const categories = db.prepare("DELETE FROM categories WHERE deleted_at IS NOT NULL AND deleted_at < datetime('now', ?)").run(cutoff).changes; - return { bills, categories }; + return { bills, categories, releasedMatches }; }); return purge(); } diff --git a/tests/backupAndCleanup.test.js b/tests/backupAndCleanup.test.js index b8917ca..379397d 100644 --- a/tests/backupAndCleanup.test.js +++ b/tests/backupAndCleanup.test.js @@ -15,7 +15,7 @@ const backupPath = path.join(os.tmpdir(), `${testId}-backups`); process.env.DB_PATH = dbPath; process.env.BACKUP_PATH = backupPath; -const { closeDb, setSetting } = require('../db/database'); +const { closeDb, setSetting, getDb } = require('../db/database'); const { BACKUP_DIR, createBackup, @@ -34,6 +34,7 @@ const { const { pruneOrphanedBackupPartials, pruneStaleExportFiles, + pruneSoftDeletedFinancialRecords, } = require('../services/cleanupService'); // ── Teardown ───────────────────────────────────────────────────────────────── @@ -312,3 +313,75 @@ test('pruneStaleExportFiles ignores non-bill-tracker files in tmpdir', () => { try { fs.unlinkSync(other); } catch {} }); + +// ───────────────────────────────────────────────────────────────────────────── +// cleanupService β€” pruneSoftDeletedFinancialRecords (QA-B5-04) +// Purging a soft-deleted bill fires the matched_bill_id ON DELETE SET NULL FK. +// Without releasing the match, the transaction would be left match_status='matched' +// with matched_bill_id=NULL β€” a limbo row excluded from spending yet tied to no bill. +// ───────────────────────────────────────────────────────────────────────────── + +function seedMatchedTxn(db, { deletedAt }) { + const userId = db.prepare( + "INSERT INTO users (username, password_hash, role, active) VALUES (?, 'x', 'user', 1)", + ).run(`prune-${Math.random().toString(36).slice(2)}`).lastInsertRowid; + const billId = db.prepare( + "INSERT INTO bills (user_id, name, due_day, expected_amount, active, deleted_at) VALUES (?, 'Purge Me', 1, 1000, 1, ?)", + ).run(userId, deletedAt).lastInsertRowid; + const txId = db.prepare( + "INSERT INTO transactions (user_id, source_type, provider_transaction_id, amount, match_status, matched_bill_id, pending, ignored) VALUES (?, 'manual', ?, -1000, 'matched', ?, 0, 0)", + ).run(userId, `prune-tx-${billId}`, billId).lastInsertRowid; + return { userId, billId, txId }; +} + +test('pruneSoftDeletedFinancialRecords releases matches on purged bills (QA-B5-04)', () => { + const db = getDb(); + db.pragma('foreign_keys = ON'); + const { billId, txId } = seedMatchedTxn(db, { deletedAt: '2020-01-01 00:00:00' }); + + const before = db.prepare('SELECT match_status, matched_bill_id FROM transactions WHERE id = ?').get(txId); + assert.equal(before.match_status, 'matched'); + assert.equal(before.matched_bill_id, billId); + + const result = pruneSoftDeletedFinancialRecords(30); + assert.ok(result.bills >= 1, 'the old soft-deleted bill was purged'); + assert.ok(result.releasedMatches >= 1, 'the match was released'); + + assert.equal(db.prepare('SELECT id FROM bills WHERE id = ?').get(billId), undefined, 'bill is gone'); + const after = db.prepare('SELECT match_status, matched_bill_id FROM transactions WHERE id = ?').get(txId); + assert.equal(after.match_status, 'unmatched', 'transaction is no longer stuck as matched'); + assert.equal(after.matched_bill_id, null, 'matched_bill_id cleared'); +}); + +test('pruneSoftDeletedFinancialRecords self-heals pre-existing orphans (matched + NULL bill)', () => { + const db = getDb(); + db.pragma('foreign_keys = ON'); + const userId = db.prepare( + "INSERT INTO users (username, password_hash, role, active) VALUES (?, 'x', 'user', 1)", + ).run(`orphan-${Math.random().toString(36).slice(2)}`).lastInsertRowid; + const txId = db.prepare( + "INSERT INTO transactions (user_id, source_type, provider_transaction_id, amount, match_status, matched_bill_id, pending, ignored) VALUES (?, 'manual', 'orphan-tx', -500, 'matched', NULL, 0, 0)", + ).run(userId).lastInsertRowid; + + pruneSoftDeletedFinancialRecords(30); + + const after = db.prepare('SELECT match_status, matched_bill_id FROM transactions WHERE id = ?').get(txId); + assert.equal(after.match_status, 'unmatched', 'orphan healed back to unmatched'); + assert.equal(after.matched_bill_id, null); +}); + +test('pruneSoftDeletedFinancialRecords leaves active matches untouched', () => { + const db = getDb(); + db.pragma('foreign_keys = ON'); + // A recently soft-deleted bill (inside the recovery window) must NOT be purged, + // and its match must be preserved. + const recent = new Date(Date.now() - 5 * 24 * 60 * 60 * 1000).toISOString().replace('T', ' ').slice(0, 19); + const { billId, txId } = seedMatchedTxn(db, { deletedAt: recent }); + + pruneSoftDeletedFinancialRecords(30); + + assert.ok(db.prepare('SELECT id FROM bills WHERE id = ?').get(billId), 'recent soft-deleted bill retained'); + const after = db.prepare('SELECT match_status, matched_bill_id FROM transactions WHERE id = ?').get(txId); + assert.equal(after.match_status, 'matched', 'match within recovery window preserved'); + assert.equal(after.matched_bill_id, billId); +});