fix(qa): escape bill name in reminder email HTML — XSS via bill name (B14-04)
- notificationService buildEmailHtml: the message line interpolated bill.name
raw (`<strong>${bill.name}</strong> is due…`) while the detail table escaped
it; a `<img src=x onerror=…>` name landed unescaped in the email HTML. Now
escaped everywhere. (self-XSS — reminders go to the bill's owner — but a clear
inconsistent-escaping defect)
- expose buildEmailHtml via _email; add an escaping test across all 4 email types
- docs: archive QA-B14-04
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
2050e13407
commit
c31d8cbe9e
|
|
@ -3,6 +3,7 @@
|
||||||
|
|
||||||
### 🐛 QA Fixes
|
### 🐛 QA Fixes
|
||||||
|
|
||||||
|
- **[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)
|
||||||
- **[Notifications] "Send test push" was completely broken** — `services/notificationService.js` attached its `_push` export (the ntfy/Gotify/Discord/Telegram helpers) *before* the final `module.exports = {…}`, which clobbered it, so `require('…/notificationService')._push` was `undefined`. `routes/notifications.js` (`const { sendTestPush } = require(…)._push || {}`) therefore always hit `throw 'Push service not initialised'` → **`POST /api/notifications/test-push` always returned 500** for every user testing their push channel. Scheduled reminders were unaffected (they call `sendPushToUser` in-scope). Moved the `_push` assignment after the reassignment. Covered by `tests/notificationDelivery.test.js` (per-channel payloads, dispatch, error handling, and a check that the auth token never leaks into the message body). (was QA-B10-01)
|
- **[Notifications] "Send test push" was completely broken** — `services/notificationService.js` attached its `_push` export (the ntfy/Gotify/Discord/Telegram helpers) *before* the final `module.exports = {…}`, which clobbered it, so `require('…/notificationService')._push` was `undefined`. `routes/notifications.js` (`const { sendTestPush } = require(…)._push || {}`) therefore always hit `throw 'Push service not initialised'` → **`POST /api/notifications/test-push` always returned 500** for every user testing their push channel. Scheduled reminders were unaffected (they call `sendPushToUser` in-scope). Moved the `_push` assignment after the reassignment. Covered by `tests/notificationDelivery.test.js` (per-channel payloads, dispatch, error handling, and a check that the auth token never leaks into the message body). (was QA-B10-01)
|
||||||
- **[Summary/Analytics] Non-monthly bills were counted in every month** — the Summary expense list/total and the Analytics "expected vs actual" line both counted annual (and off-month quarterly) bills for months they weren't due, over-stating the obligation and disagreeing with the Tracker (e.g. a yearly insurance bill inflated every month). Both `routes/summary.js` and `services/analyticsService.js` now gate bills by `resolveDueDate` — the same occurrence check the Tracker uses. Guarded by Tracker↔Summary and Tracker↔Analytics reconciliation checks in `e2e/api.probe.spec.js`. The SimpleFIN bank-tracking `unpaid_this_month` metric had the same gap and is fixed the same way (fetch + JS `resolveDueDate` filter, since SQL can't call it), covered by `tests/summaryBankTracking.test.js`. (was QA-B5-01, QA-B5-02, QA-B5-03)
|
- **[Summary/Analytics] Non-monthly bills were counted in every month** — the Summary expense list/total and the Analytics "expected vs actual" line both counted annual (and off-month quarterly) bills for months they weren't due, over-stating the obligation and disagreeing with the Tracker (e.g. a yearly insurance bill inflated every month). Both `routes/summary.js` and `services/analyticsService.js` now gate bills by `resolveDueDate` — the same occurrence check the Tracker uses. Guarded by Tracker↔Summary and Tracker↔Analytics reconciliation checks in `e2e/api.probe.spec.js`. The SimpleFIN bank-tracking `unpaid_this_month` metric had the same gap and is fixed the same way (fetch + JS `resolveDueDate` filter, since SQL can't call it), covered by `tests/summaryBankTracking.test.js`. (was QA-B5-01, QA-B5-02, QA-B5-03)
|
||||||
- **[Data] Seed Demo Data amounts were 100× too small** — `scripts/seedDemoData.js` inserted demo dollar amounts straight into `bills.expected_amount` / `current_balance` / `minimum_payment`, which became integer-cents columns in the v1.03 migration, so a seeded "$85" bill showed as $0.85 (a whole demo month totalled ~$32 instead of ~$3,200). Now wraps demo values in `toCents()` before insert. Regression guard added in `e2e/api.probe.spec.js`. (was QA-B9-01)
|
- **[Data] Seed Demo Data amounts were 100× too small** — `scripts/seedDemoData.js` inserted demo dollar amounts straight into `bills.expected_amount` / `current_balance` / `minimum_payment`, which became integer-cents columns in the v1.03 migration, so a seeded "$85" bill showed as $0.85 (a whole demo month totalled ~$32 instead of ~$3,200). Now wraps demo values in `toCents()` before insert. Regression guard added in `e2e/api.probe.spec.js`. (was QA-B9-01)
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,6 @@
|
||||||
# BillTracker — Master QA Plan (living document)
|
# BillTracker — Master QA Plan (living document)
|
||||||
|
|
||||||
**Version target:** v0.41.x · **Executor:** Claude (active) · **Last updated:** 2026-07-02 (Cycle 1: 13 findings fixed & archived, 0 open — incl. a broken "Send test push")
|
**Version target:** v0.41.x · **Executor:** Claude (active) · **Last updated:** 2026-07-02 (Cycle 1: 14 findings fixed & archived, 0 open — incl. broken "Send test push" + email XSS)
|
||||||
|
|
||||||
This is a **living, operational** QA document, not a static spec. Claude runs it,
|
This is a **living, operational** QA document, not a static spec. Claude runs it,
|
||||||
in **batches**, actively hunting for bugs/errors/rough edges, **fixing** them, and
|
in **batches**, actively hunting for bugs/errors/rough edges, **fixing** them, and
|
||||||
|
|
@ -101,7 +101,7 @@ before cross-cutting; regression last). Update **Status** and **Findings** every
|
||||||
| B11 | Admin panel | users, login mode, auth methods, backups, cleanup, status, onboarding | admin | 🔄 | 0 / 0 |
|
| B11 | Admin panel | users, login mode, auth methods, backups, cleanup, status, onboarding | admin | 🔄 | 0 / 0 |
|
||||||
| B12 | Settings, Profile & global UI | `/settings`, `/profile`, static pages, command palette, sidebar/nav | any | 🔄 | 0 / 0 |
|
| B12 | Settings, Profile & global UI | `/settings`, `/profile`, static pages, command palette, sidebar/nav | any | 🔄 | 0 / 0 |
|
||||||
| B13 | API / backend direct | all `/api/*`: auth, CSRF, validation, rate limits, error shape, IDOR, cents | via HTTP client | 🔄 | 0 / 1 |
|
| B13 | API / backend direct | all `/api/*`: auth, CSRF, validation, rate limits, error shape, IDOR, cents | via HTTP client | 🔄 | 0 / 1 |
|
||||||
| B14 | Non-functional | a11y, performance, PWA/offline, XSS/secrets, timezone/DST | large + adversarial | 🔄 | 0 / 3 |
|
| B14 | Non-functional | a11y, performance, PWA/offline, XSS/secrets, timezone/DST | large + adversarial | 🔄 | 0 / 4 |
|
||||||
| B15 | Regression & sign-off | full smoke on **production build**, exit criteria | seeded | 🔄 | 0 / 0 |
|
| B15 | Regression & sign-off | full smoke on **production build**, exit criteria | seeded | 🔄 | 0 / 0 |
|
||||||
|
|
||||||
> After B15, if any batch is 🔁 or has open S1/S2, loop back. Then start a new
|
> After B15, if any batch is 🔁 or has open S1/S2, loop back. Then start a new
|
||||||
|
|
|
||||||
|
|
@ -165,11 +165,14 @@ function buildEmailHtml(bill, type, dueDate) {
|
||||||
return `${parseInt(m)}/${parseInt(day)}/${y}`;
|
return `${parseInt(m)}/${parseInt(day)}/${y}`;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// QA-B14-04: escape the bill name everywhere it lands in the HTML, including
|
||||||
|
// these message strings (previously raw — an XSS vector via the bill name).
|
||||||
|
const name = esc(bill.name);
|
||||||
const messages = {
|
const messages = {
|
||||||
due_3d: `<strong>${bill.name}</strong> is due in 3 days.`,
|
due_3d: `<strong>${name}</strong> is due in 3 days.`,
|
||||||
due_1d: `<strong>${bill.name}</strong> is due <strong>tomorrow</strong>.`,
|
due_1d: `<strong>${name}</strong> is due <strong>tomorrow</strong>.`,
|
||||||
due_today: `<strong>${bill.name}</strong> is due <strong>today</strong>.`,
|
due_today: `<strong>${name}</strong> is due <strong>today</strong>.`,
|
||||||
overdue: `<strong>${bill.name}</strong> was due on ${fmt(dueDate)} and has not been marked as paid.`,
|
overdue: `<strong>${name}</strong> was due on ${fmt(dueDate)} and has not been marked as paid.`,
|
||||||
};
|
};
|
||||||
|
|
||||||
return `<!DOCTYPE html>
|
return `<!DOCTYPE html>
|
||||||
|
|
@ -551,3 +554,4 @@ module.exports = { runNotifications, runDriftNotifications, sendTestEmail, creat
|
||||||
// above so it isn't clobbered by the reassignment (QA-B10-01: previously set
|
// above so it isn't clobbered by the reassignment (QA-B10-01: previously set
|
||||||
// before it, leaving `_push` undefined → "Send test push" always 500'd).
|
// before it, leaving `_push` undefined → "Send test push" always 500'd).
|
||||||
module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret };
|
module.exports._push = { sendNtfy, sendGotify, sendDiscord, sendTelegram, sendTestPush, sendPushToUser, encryptSecret };
|
||||||
|
module.exports._email = { buildEmailHtml, esc };
|
||||||
|
|
|
||||||
|
|
@ -106,3 +106,13 @@ test('dispatch: an unknown channel throws instead of silently doing nothing', as
|
||||||
/Unknown push channel: carrier-pigeon/,
|
/Unknown push channel: carrier-pigeon/,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('email HTML escapes the bill name everywhere — no XSS via the name (QA-B14-04)', () => {
|
||||||
|
const { buildEmailHtml } = require('../services/notificationService')._email;
|
||||||
|
const evil = '<img src=x onerror=alert(1)>';
|
||||||
|
for (const type of ['due_3d', 'due_1d', 'due_today', 'overdue']) {
|
||||||
|
const html = buildEmailHtml({ name: evil, expected_amount: 8500 }, type, '2026-07-15');
|
||||||
|
assert.ok(!html.includes(evil), `raw payload must not appear in the ${type} email HTML`);
|
||||||
|
assert.ok(html.includes('<img src=x onerror='), `bill name must be HTML-escaped in the ${type} email (message + detail row)`);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue