fix(qa): retention GC orphaned matched transactions on bill purge (QA-B5-04)
Found probing a copy of the live SimpleFIN DB: 3 transactions were match_status='matched' with matched_bill_id=NULL. Bills are soft-deleted (retained for recovery), then the retention GC 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. pruneSoftDeletedFinancialRecords now releases those matches back to 'unmatched' in the same transaction and self-heals pre-existing orphans; retention behaviour is unchanged. Verified on a live-DB copy (3→0 orphans, 0 transactions lost). Regression: 3 tests in backupAndCleanup.test.js. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
07b6a04a97
commit
b3168fca70
|
|
@ -3,6 +3,7 @@
|
||||||
|
|
||||||
### 🐛 QA Fixes
|
### 🐛 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)
|
- **[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)
|
- **[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 (`<strong>${bill.name}</strong> is due…`), so a bill named `<img src=x onerror=…>` 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)
|
- **[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 (`<strong>${bill.name}</strong> is due…`), so a bill named `<img src=x onerror=…>` 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)
|
||||||
|
|
|
||||||
|
|
@ -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 |
|
| 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 |
|
| 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 |
|
| 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 |
|
| 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 |
|
| 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 |
|
| 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 | 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·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)
|
**Result key:** 🔄 in progress · 🔁 findings fixed, re-run required · ✅ clean (zero findings — QA complete)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -135,14 +135,29 @@ function pruneImportHistory(maxAgeDays) {
|
||||||
/**
|
/**
|
||||||
* Permanently purge soft-deleted bills and categories after a 30-day recovery
|
* Permanently purge soft-deleted bills and categories after a 30-day recovery
|
||||||
* window. Bill deletion cascades to bill-owned records via foreign keys.
|
* 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) {
|
function pruneSoftDeletedFinancialRecords(maxAgeDays = 30) {
|
||||||
const db = getDb();
|
const db = getDb();
|
||||||
const cutoff = `-${maxAgeDays} days`;
|
const cutoff = `-${maxAgeDays} days`;
|
||||||
const purge = db.transaction(() => {
|
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 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;
|
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();
|
return purge();
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -15,7 +15,7 @@ const backupPath = path.join(os.tmpdir(), `${testId}-backups`);
|
||||||
process.env.DB_PATH = dbPath;
|
process.env.DB_PATH = dbPath;
|
||||||
process.env.BACKUP_PATH = backupPath;
|
process.env.BACKUP_PATH = backupPath;
|
||||||
|
|
||||||
const { closeDb, setSetting } = require('../db/database');
|
const { closeDb, setSetting, getDb } = require('../db/database');
|
||||||
const {
|
const {
|
||||||
BACKUP_DIR,
|
BACKUP_DIR,
|
||||||
createBackup,
|
createBackup,
|
||||||
|
|
@ -34,6 +34,7 @@ const {
|
||||||
const {
|
const {
|
||||||
pruneOrphanedBackupPartials,
|
pruneOrphanedBackupPartials,
|
||||||
pruneStaleExportFiles,
|
pruneStaleExportFiles,
|
||||||
|
pruneSoftDeletedFinancialRecords,
|
||||||
} = require('../services/cleanupService');
|
} = require('../services/cleanupService');
|
||||||
|
|
||||||
// ── Teardown ─────────────────────────────────────────────────────────────────
|
// ── Teardown ─────────────────────────────────────────────────────────────────
|
||||||
|
|
@ -312,3 +313,75 @@ test('pruneStaleExportFiles ignores non-bill-tracker files in tmpdir', () => {
|
||||||
|
|
||||||
try { fs.unlinkSync(other); } catch {}
|
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);
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue