fix(qa): version check is opt-out-able (QA-B16-01)
- updateCheckService: gate the external request on `update_check_enabled`
(default on); when off, no network call, returns { disabled: true }
- aboutAdmin: GET/PUT /update-check-setting (admin-only) to toggle it
- StatusPage: a Switch on the admin System Status card to enable/disable
- privacy.js: state that an admin can disable it (was called "optional" with
no actual opt-out)
- tests/updateCheckOptOut.test.js: proves no external fetch when disabled
- docs: archive QA-B16-01, B16 ✅
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
e8190170dc
commit
2963d11d1b
|
|
@ -3,6 +3,7 @@
|
||||||
|
|
||||||
### 🐛 QA Fixes
|
### 🐛 QA Fixes
|
||||||
|
|
||||||
|
- **[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)
|
||||||
- **[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)
|
||||||
|
|
|
||||||
|
|
@ -344,6 +344,8 @@ export const api = {
|
||||||
roadmap: (refresh = false) => get(`/about-admin/roadmap${refresh ? '?refresh=1' : ''}`),
|
roadmap: (refresh = false) => get(`/about-admin/roadmap${refresh ? '?refresh=1' : ''}`),
|
||||||
updateStatus: () => get('/version/update-status'),
|
updateStatus: () => get('/version/update-status'),
|
||||||
checkForUpdates: () => post('/about-admin/check-updates'),
|
checkForUpdates: () => post('/about-admin/check-updates'),
|
||||||
|
getUpdateCheckSetting: () => get('/about-admin/update-check-setting'),
|
||||||
|
setUpdateCheckSetting: (enabled) => put('/about-admin/update-check-setting', { enabled }),
|
||||||
devLog: () => get('/about-admin/dev-log'),
|
devLog: () => get('/about-admin/dev-log'),
|
||||||
version: () => get('/version'),
|
version: () => get('/version'),
|
||||||
releaseHistory: () => get('/version/history'),
|
releaseHistory: () => get('/version/history'),
|
||||||
|
|
|
||||||
|
|
@ -9,6 +9,7 @@ import { toast } from 'sonner';
|
||||||
import { api } from '@/api';
|
import { api } from '@/api';
|
||||||
import { cn, fmtUptime, fmtBytes } from '@/lib/utils';
|
import { cn, fmtUptime, fmtBytes } from '@/lib/utils';
|
||||||
import { Button } from '@/components/ui/button';
|
import { Button } from '@/components/ui/button';
|
||||||
|
import { Switch } from '@/components/ui/switch';
|
||||||
import { MarkdownText } from '@/components/MarkdownText';
|
import { MarkdownText } from '@/components/MarkdownText';
|
||||||
|
|
||||||
// ─── Helpers ──────────────────────────────────────────────────────────────────
|
// ─── Helpers ──────────────────────────────────────────────────────────────────
|
||||||
|
|
@ -128,7 +129,7 @@ function SkeletonCard() {
|
||||||
|
|
||||||
const CATEGORY_ORDER = ['Added', 'Changed', 'Fixed', 'Removed', 'Deprecated', 'Security'];
|
const CATEGORY_ORDER = ['Added', 'Changed', 'Fixed', 'Removed', 'Deprecated', 'Security'];
|
||||||
|
|
||||||
function UpdateCard({ update, onCheckNow, checking }) {
|
function UpdateCard({ update, onCheckNow, checking, enabled = true, onToggle }) {
|
||||||
const hasUpdate = !!update.has_update;
|
const hasUpdate = !!update.has_update;
|
||||||
const isKnown = update.up_to_date !== null && update.up_to_date !== undefined;
|
const isKnown = update.up_to_date !== null && update.up_to_date !== undefined;
|
||||||
const hasError = !!update.error;
|
const hasError = !!update.error;
|
||||||
|
|
@ -173,8 +174,15 @@ function UpdateCard({ update, onCheckNow, checking }) {
|
||||||
{update.error && (
|
{update.error && (
|
||||||
<p className="text-[11px] text-red-400 leading-relaxed pt-1.5">{update.error}</p>
|
<p className="text-[11px] text-red-400 leading-relaxed pt-1.5">{update.error}</p>
|
||||||
)}
|
)}
|
||||||
|
<div className="flex items-center justify-between gap-3 py-1.5 border-t border-border/40 mt-1">
|
||||||
|
<span className="text-xs text-muted-foreground">
|
||||||
|
Automatic version check
|
||||||
|
<span className="block text-[10px] text-muted-foreground/60">External request; off = no phone-home</span>
|
||||||
|
</span>
|
||||||
|
<Switch checked={enabled} onCheckedChange={onToggle} aria-label="Enable automatic version check" />
|
||||||
|
</div>
|
||||||
<div className="pt-3">
|
<div className="pt-3">
|
||||||
<Button variant="outline" size="sm" onClick={onCheckNow} disabled={checking}
|
<Button variant="outline" size="sm" onClick={onCheckNow} disabled={checking || !enabled}
|
||||||
className="h-7 text-xs gap-1.5">
|
className="h-7 text-xs gap-1.5">
|
||||||
<RefreshCw className={cn('h-3 w-3', checking && 'animate-spin')} />
|
<RefreshCw className={cn('h-3 w-3', checking && 'animate-spin')} />
|
||||||
{checking ? 'Checking…' : 'Check Now'}
|
{checking ? 'Checking…' : 'Check Now'}
|
||||||
|
|
@ -229,6 +237,7 @@ export default function StatusPage() {
|
||||||
const [loading, setLoading] = useState(true);
|
const [loading, setLoading] = useState(true);
|
||||||
const [updateData, setUpdateData] = useState(null);
|
const [updateData, setUpdateData] = useState(null);
|
||||||
const [updateChecking, setUpdateChecking] = useState(false);
|
const [updateChecking, setUpdateChecking] = useState(false);
|
||||||
|
const [updateEnabled, setUpdateEnabled] = useState(true);
|
||||||
|
|
||||||
const load = useCallback(async () => {
|
const load = useCallback(async () => {
|
||||||
setLoading(true);
|
setLoading(true);
|
||||||
|
|
@ -243,6 +252,10 @@ export default function StatusPage() {
|
||||||
const historyData = await api.releaseHistory();
|
const historyData = await api.releaseHistory();
|
||||||
setHistoryMeta({ version: historyData.version, updated_at: historyData.updated_at });
|
setHistoryMeta({ version: historyData.version, updated_at: historyData.updated_at });
|
||||||
} catch { setHistoryMeta(null); }
|
} catch { setHistoryMeta(null); }
|
||||||
|
try {
|
||||||
|
const s = await api.getUpdateCheckSetting();
|
||||||
|
setUpdateEnabled(s.enabled !== false);
|
||||||
|
} catch { /* keep default */ }
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
toast.error(err.message || 'Failed to load status.');
|
toast.error(err.message || 'Failed to load status.');
|
||||||
} finally {
|
} finally {
|
||||||
|
|
@ -263,6 +276,17 @@ export default function StatusPage() {
|
||||||
}
|
}
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
const handleToggleUpdateCheck = useCallback(async (enabled) => {
|
||||||
|
setUpdateEnabled(enabled); // optimistic
|
||||||
|
try {
|
||||||
|
await api.setUpdateCheckSetting(enabled);
|
||||||
|
if (enabled) handleCheckNow();
|
||||||
|
} catch (err) {
|
||||||
|
setUpdateEnabled(!enabled); // revert
|
||||||
|
toast.error(err.message || 'Failed to update setting');
|
||||||
|
}
|
||||||
|
}, [handleCheckNow]);
|
||||||
|
|
||||||
// Normalize the nested response shape
|
// Normalize the nested response shape
|
||||||
const app = data?.application ?? data?.app ?? {};
|
const app = data?.application ?? data?.app ?? {};
|
||||||
const rt = data?.runtime ?? {};
|
const rt = data?.runtime ?? {};
|
||||||
|
|
@ -502,6 +526,8 @@ export default function StatusPage() {
|
||||||
update={updateData}
|
update={updateData}
|
||||||
onCheckNow={handleCheckNow}
|
onCheckNow={handleCheckNow}
|
||||||
checking={updateChecking}
|
checking={updateChecking}
|
||||||
|
enabled={updateEnabled}
|
||||||
|
onToggle={handleToggleUpdateCheck}
|
||||||
/>
|
/>
|
||||||
)}
|
)}
|
||||||
<ReleaseNotesCard version={version} historyMeta={historyMeta} />
|
<ReleaseNotesCard version={version} historyMeta={historyMeta} />
|
||||||
|
|
|
||||||
|
|
@ -104,7 +104,7 @@ before cross-cutting; regression last). Update **Status** and **Findings** every
|
||||||
| 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 / 4 |
|
| 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 |
|
||||||
| B16 | Migrations, secrets & deploy | migration idempotency/rollback/fresh==migrated, encryption-key lifecycle, `docker-entrypoint` (perms/first-run/migrate), update-check phone-home | scratch + docker | 🔄 | 1 / 0 |
|
| B16 | Migrations, secrets & deploy | migration idempotency/rollback/fresh==migrated, encryption-key lifecycle, `docker-entrypoint` (perms/first-run/migrate), update-check phone-home | scratch + docker | ✅ | 0 / 1 |
|
||||||
|
|
||||||
> 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
|
||||||
> cycle from B0 against the next build/version.
|
> cycle from B0 against the next build/version.
|
||||||
|
|
@ -149,7 +149,7 @@ fixing. Keep only **Open / Fixing / Fixed** rows here. Once a finding is
|
||||||
|
|
||||||
| ID | Sev | Area (`file:line`) | Summary | Status | Notes / repro |
|
| ID | Sev | Area (`file:line`) | Summary | Status | Notes / repro |
|
||||||
|----|-----|--------------------|---------|--------|---------------|
|
|----|-----|--------------------|---------|--------|---------------|
|
||||||
| QA-B16-01 | S4 | `services/updateCheckService.js` + `routes/privacy.js` | Privacy policy calls the version check "**optional**", but there is **no opt-out** — it phones a hardcoded host (`dream.scheller.ltd`) whenever About/Status/version is loaded | 🔴 Open | decision needed: add a toggle vs reword |
|
| _(none — all Cycle 1 findings fixed, verified & archived to `HISTORY.md` v0.41.0)_ | | | | | |
|
||||||
|
|
||||||
**Finding template** (paste a new row above; keep the full write-up here until archived):
|
**Finding template** (paste a new row above; keep the full write-up here until archived):
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -487,4 +487,22 @@ router.post('/check-updates', requireAuth, requireAdmin, async (req, res) => {
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// QA-B16-01: opt-out control for the external version check.
|
||||||
|
const { getSetting, setSetting } = require('../db/database');
|
||||||
|
|
||||||
|
// GET /api/about-admin/update-check-setting — is the external version check enabled?
|
||||||
|
router.get('/update-check-setting', requireAuth, requireAdmin, (req, res) => {
|
||||||
|
res.json({ enabled: getSetting('update_check_enabled') !== 'false' });
|
||||||
|
});
|
||||||
|
|
||||||
|
// PUT /api/about-admin/update-check-setting — enable/disable the external version check
|
||||||
|
router.put('/update-check-setting', requireAuth, requireAdmin, (req, res) => {
|
||||||
|
const { enabled } = req.body || {};
|
||||||
|
if (typeof enabled !== 'boolean') {
|
||||||
|
return res.status(400).json({ error: 'enabled must be a boolean' });
|
||||||
|
}
|
||||||
|
setSetting('update_check_enabled', enabled ? 'true' : 'false');
|
||||||
|
res.json({ enabled });
|
||||||
|
});
|
||||||
|
|
||||||
module.exports = router;
|
module.exports = router;
|
||||||
|
|
|
||||||
|
|
@ -31,6 +31,7 @@ router.get('/', (req, res) => {
|
||||||
items: [
|
items: [
|
||||||
'The only external communication performed by the application is an optional version check to determine whether the latest software release is installed.',
|
'The only external communication performed by the application is an optional version check to determine whether the latest software release is installed.',
|
||||||
'This communication does not include your bill data or personal information.',
|
'This communication does not include your bill data or personal information.',
|
||||||
|
'An administrator can disable the version check entirely in the admin panel; when disabled, the application makes no external requests.',
|
||||||
'Version check information is not tracked or stored server-side and is used solely to determine software update availability.',
|
'Version check information is not tracked or stored server-side and is used solely to determine software update availability.',
|
||||||
],
|
],
|
||||||
},
|
},
|
||||||
|
|
|
||||||
|
|
@ -4,9 +4,18 @@
|
||||||
* 5 minutes for errors) so the status page stays fast under load.
|
* 5 minutes for errors) so the status page stays fast under load.
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
const { getSetting } = require('../db/database');
|
||||||
|
|
||||||
const REPO_API_BASE = process.env.REPO_API_URL
|
const REPO_API_BASE = process.env.REPO_API_URL
|
||||||
|| 'https://dream.scheller.ltd/api/v1/repos/null/BillTracker';
|
|| 'https://dream.scheller.ltd/api/v1/repos/null/BillTracker';
|
||||||
|
|
||||||
|
// QA-B16-01: the version check is opt-out-able. Admins can disable the external
|
||||||
|
// request via the `update_check_enabled` setting (default on). When off, no
|
||||||
|
// network call is made and a `disabled` status is returned.
|
||||||
|
function updateCheckEnabled() {
|
||||||
|
return getSetting('update_check_enabled') !== 'false';
|
||||||
|
}
|
||||||
|
|
||||||
const TTL_OK_MS = 60 * 60 * 1000; // 1 hour on success
|
const TTL_OK_MS = 60 * 60 * 1000; // 1 hour on success
|
||||||
const TTL_ERROR_MS = 5 * 60 * 1000; // 5 min on error (avoid hammering)
|
const TTL_ERROR_MS = 5 * 60 * 1000; // 5 min on error (avoid hammering)
|
||||||
const FETCH_TIMEOUT_MS = 8_000;
|
const FETCH_TIMEOUT_MS = 8_000;
|
||||||
|
|
@ -39,6 +48,22 @@ function compareVersions(a, b) {
|
||||||
async function checkForUpdates(force = false) {
|
async function checkForUpdates(force = false) {
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
|
|
||||||
|
// Opt-out: no external request when disabled by the admin.
|
||||||
|
if (!updateCheckEnabled()) {
|
||||||
|
return {
|
||||||
|
current_version: getCurrentVersion(),
|
||||||
|
latest_version: null,
|
||||||
|
up_to_date: null,
|
||||||
|
has_update: false,
|
||||||
|
latest_release_url: null,
|
||||||
|
published_at: null,
|
||||||
|
last_checked_at: null,
|
||||||
|
error: null,
|
||||||
|
disabled: true,
|
||||||
|
cached: false,
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
if (!force && _cache.result && now < _cache.expiresAt) {
|
if (!force && _cache.result && now < _cache.expiresAt) {
|
||||||
return { ..._cache.result, cached: true };
|
return { ..._cache.result, cached: true };
|
||||||
}
|
}
|
||||||
|
|
@ -113,4 +138,4 @@ async function checkForUpdates(force = false) {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = { checkForUpdates };
|
module.exports = { checkForUpdates, updateCheckEnabled };
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,60 @@
|
||||||
|
'use strict';
|
||||||
|
|
||||||
|
// B16 / QA-B16-01: the external version check must be opt-out-able. When the
|
||||||
|
// `update_check_enabled` setting is 'false', checkForUpdates must make NO network
|
||||||
|
// request and report `disabled`. Default (unset/'true') performs the check.
|
||||||
|
const test = require('node:test');
|
||||||
|
const assert = require('node:assert/strict');
|
||||||
|
const os = require('node:os');
|
||||||
|
const path = require('node:path');
|
||||||
|
const fs = require('node:fs');
|
||||||
|
|
||||||
|
const dbPath = path.join(os.tmpdir(), `bill-tracker-updatecheck-test-${process.pid}.sqlite`);
|
||||||
|
process.env.DB_PATH = dbPath;
|
||||||
|
|
||||||
|
const { getDb, setSetting, closeDb } = require('../db/database');
|
||||||
|
const svc = require('../services/updateCheckService');
|
||||||
|
|
||||||
|
test('disabled: makes no external request and returns disabled', async () => {
|
||||||
|
getDb();
|
||||||
|
setSetting('update_check_enabled', 'false');
|
||||||
|
assert.equal(svc.updateCheckEnabled(), false);
|
||||||
|
|
||||||
|
const orig = global.fetch;
|
||||||
|
let called = false;
|
||||||
|
global.fetch = async () => { called = true; throw new Error('fetch must not be called when disabled'); };
|
||||||
|
try {
|
||||||
|
const r = await svc.checkForUpdates(true);
|
||||||
|
assert.equal(r.disabled, true, 'reports disabled');
|
||||||
|
assert.equal(called, false, 'no external fetch when disabled');
|
||||||
|
} finally {
|
||||||
|
global.fetch = orig;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('enabled (default): performs the version check', async () => {
|
||||||
|
setSetting('update_check_enabled', 'true');
|
||||||
|
assert.equal(svc.updateCheckEnabled(), true);
|
||||||
|
|
||||||
|
const orig = global.fetch;
|
||||||
|
global.fetch = async () => ({
|
||||||
|
ok: true,
|
||||||
|
status: 200,
|
||||||
|
json: async () => ({ tag_name: 'v99.0.0', html_url: 'https://example/rel', published_at: '2026-01-01' }),
|
||||||
|
});
|
||||||
|
try {
|
||||||
|
const r = await svc.checkForUpdates(true);
|
||||||
|
assert.notEqual(r.disabled, true);
|
||||||
|
assert.equal(r.latest_version, '99.0.0');
|
||||||
|
assert.equal(r.has_update, true);
|
||||||
|
} finally {
|
||||||
|
global.fetch = orig;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test.after(() => {
|
||||||
|
closeDb();
|
||||||
|
for (const suffix of ['', '-wal', '-shm']) {
|
||||||
|
try { fs.rmSync(dbPath + suffix); } catch { /* ignore */ }
|
||||||
|
}
|
||||||
|
});
|
||||||
Loading…
Reference in New Issue