backend polish: stats LEFT JOIN, CSV formula injection, trustProxy, validated env vars, dead code
- Stats: bars table is now the LEFT side of the join to transactions, so a bar with zero sales still gets a zero row instead of vanishing from the totals table until its first sale (indistinguishable from a deleted bar). by_day stays JS-computed on purpose — a SQL rewrite would trade DST-aware timezone handling for a fixed-hour-offset 'localtime' expression that's wrong on DST transition nights, to fix a cost the original review noted is 'fine today'. Not worth that trade for a money-adjacent report; left a comment explaining why. - CSV export: cells starting with =/+/-/@ are now prefixed with ' before quoting, closing a formula-injection path (an admin-entered drink/bar name like =HYPERLINK(...) would otherwise execute when the export is opened in Excel/LibreOffice). - server/index.ts: PORT is now parsed and range-checked instead of a bare Number(...) (an unparseable value silently became NaN, and Fastify listens on a random free port for that); ADMIN_PASSWORD missing now warns at boot instead of only surfacing as a 500 at the first login attempt; new WUTZ_TRUST_PROXY env flag (off by default) so req.ip can actually reflect the real client behind a reverse proxy, documented in the README alongside the other env vars. - time.ts: WUTZ_DAY_CUTOFF_HOUR gets the same parse+range-check treatment, for the same reason (a typo used to silently disable the business-day rollback with no error). - shared/src/index.ts: Drink.archived is now typed 0 | 1, matching what SQLite actually returns (was boolean, which only worked by accident since 0 is falsy); removed TransactionRecord/ TransactionItemRecord, declared but never returned by any route — leftovers from a planned endpoint that was never built. Verified: pnpm --filter server|client typecheck/build all clean; also ran the built server with a bad PORT and no ADMIN_PASSWORD to confirm both warnings fire and the port falls back correctly.
This commit is contained in:
parent
fb02cc0f94
commit
9b82cd54c4
5 changed files with 85 additions and 31 deletions
|
|
@ -11,11 +11,40 @@ import { registerAdminRoutes } from './routes/admin.js';
|
|||
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
|
||||
const PORT = Number(process.env.PORT ?? 3000);
|
||||
function parsePortEnv(name: string, fallback: number): number {
|
||||
const raw = process.env[name];
|
||||
if (raw === undefined) return fallback;
|
||||
const n = Number(raw);
|
||||
if (!Number.isInteger(n) || n < 1 || n > 65535) {
|
||||
// A bare Number(...) turned an unparseable PORT into NaN, and
|
||||
// Fastify listens on a random free port for a NaN — silently wrong,
|
||||
// not an error anyone would see.
|
||||
console.warn(`${name}=${JSON.stringify(raw)} is not a valid port — using default ${fallback}`);
|
||||
return fallback;
|
||||
}
|
||||
return n;
|
||||
}
|
||||
|
||||
const PORT = parsePortEnv('PORT', 3000);
|
||||
const HOST = process.env.HOST ?? '0.0.0.0';
|
||||
const DB_PATH = process.env.DB_PATH ?? join(process.cwd(), 'wutz.db');
|
||||
|
||||
const app = Fastify({ logger: true });
|
||||
if (!process.env.ADMIN_PASSWORD) {
|
||||
// Not fatal — public routes work fine without it — but previously this
|
||||
// only surfaced as a 500 at the first login attempt, which in practice
|
||||
// is the middle of the event. Warn at boot instead.
|
||||
console.warn('ADMIN_PASSWORD is not set — the admin backoffice login will fail until it is.');
|
||||
}
|
||||
|
||||
const app = Fastify({
|
||||
logger: true,
|
||||
// Off by default: only trust X-Forwarded-* when the deployer has
|
||||
// actually put a reverse proxy in front (the README recommends one for
|
||||
// TLS termination). Without this, req.ip behind such a proxy is always
|
||||
// the proxy's own address, making the stored client_ip column and its
|
||||
// CSV export uniformly useless.
|
||||
trustProxy: process.env.WUTZ_TRUST_PROXY === '1',
|
||||
});
|
||||
|
||||
await app.register(fastifyCookie);
|
||||
|
||||
|
|
|
|||
|
|
@ -202,15 +202,21 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) {
|
|||
app.get('/admin/api/stats', async (req, reply) => {
|
||||
if (!requireAuth(req, reply)) return;
|
||||
|
||||
// LEFT JOIN (not JOIN) so a bar with zero sales still gets a zero row —
|
||||
// an inner join made a brand-new bar indistinguishable from a deleted
|
||||
// one until its first sale, which reads as "my new bar isn't working"
|
||||
// during setup. COUNT(t.id), not COUNT(*): the outer join produces one
|
||||
// NULL-filled row per bar-with-no-transactions, and COUNT(*) would
|
||||
// count that as 1 instead of 0.
|
||||
const totals = db
|
||||
.prepare(
|
||||
`SELECT b.id AS bar_id, b.name AS bar_name,
|
||||
COUNT(*) AS tx_count,
|
||||
COALESCE(SUM(CASE WHEN crew = 0 THEN total_cents ELSE 0 END), 0) AS paid_cents,
|
||||
COALESCE(SUM(CASE WHEN crew = 1 THEN 1 ELSE 0 END), 0) AS crew_count,
|
||||
COALESCE(SUM(pfand_returns), 0) AS pfand_returns
|
||||
FROM transactions t
|
||||
JOIN bars b ON b.id = t.bar_id
|
||||
COUNT(t.id) AS tx_count,
|
||||
COALESCE(SUM(CASE WHEN t.crew = 0 THEN t.total_cents ELSE 0 END), 0) AS paid_cents,
|
||||
COALESCE(SUM(CASE WHEN t.crew = 1 THEN 1 ELSE 0 END), 0) AS crew_count,
|
||||
COALESCE(SUM(t.pfand_returns), 0) AS pfand_returns
|
||||
FROM bars b
|
||||
LEFT JOIN transactions t ON t.bar_id = b.id
|
||||
GROUP BY b.id, b.name
|
||||
ORDER BY b.id`
|
||||
)
|
||||
|
|
@ -228,6 +234,16 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) {
|
|||
.all();
|
||||
|
||||
// Per business day (sales night runs past midnight — see time.ts).
|
||||
//
|
||||
// Deliberately still computed in JS rather than SQL, despite selecting
|
||||
// every transaction row on every stats load: businessDay() uses
|
||||
// Intl.DateTimeFormat with a named IANA zone (WUTZ_TZ), which handles
|
||||
// DST transitions correctly. A SQL `date(created_at, '-Nh', 'localtime')`
|
||||
// rewrite would use the *server process's* OS timezone (not WUTZ_TZ) and
|
||||
// a fixed hour offset that's wrong on the two nights a year DST changes
|
||||
// — a real correctness regression for a money-adjacent report, to fix a
|
||||
// performance concern that (per the code review that flagged this) is
|
||||
// "fine today" at festival scale. Not worth the trade.
|
||||
const txRows = db
|
||||
.prepare('SELECT created_at, total_cents, crew, pfand_returns FROM transactions')
|
||||
.all() as Array<{ created_at: string; total_cents: number; crew: number; pfand_returns: number }>;
|
||||
|
|
@ -311,7 +327,12 @@ export function registerAdminRoutes(app: FastifyInstance, db: DB) {
|
|||
|
||||
function csvCell(v: unknown): string {
|
||||
if (v === null || v === undefined) return '';
|
||||
const s = String(v);
|
||||
let s = String(v);
|
||||
// Neutralise spreadsheet formula injection: a cell starting with one of
|
||||
// these characters is interpreted as a formula by Excel/LibreOffice when
|
||||
// the CSV is opened (e.g. a drink named =HYPERLINK("http://…")). Admin-
|
||||
// entered data only, so low risk, but the fix is one character.
|
||||
if (/^[=+\-@]/.test(s)) s = `'${s}`;
|
||||
if (/[",\n]/.test(s)) return `"${s.replace(/"/g, '""')}"`;
|
||||
return s;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -9,7 +9,24 @@ export const TZ = process.env.WUTZ_TZ ?? 'Europe/Berlin';
|
|||
|
||||
// A sale at e.g. 03:00 still belongs to the previous night's business day.
|
||||
// Anything before this local hour counts towards the day before.
|
||||
export const BUSINESS_DAY_CUTOFF_HOUR = Number(process.env.WUTZ_DAY_CUTOFF_HOUR ?? 5);
|
||||
//
|
||||
// Parsed and range-checked rather than a bare Number(...) — an unparseable
|
||||
// value (a typo like "5am" instead of "5") used to silently become NaN,
|
||||
// and `hour < NaN` is always false, so the business-day rollback would
|
||||
// just stop happening with no error anywhere: every after-midnight sale
|
||||
// would land on the wrong day in the stats table.
|
||||
export const BUSINESS_DAY_CUTOFF_HOUR = parseHourEnv('WUTZ_DAY_CUTOFF_HOUR', 5);
|
||||
|
||||
function parseHourEnv(name: string, fallback: number): number {
|
||||
const raw = process.env[name];
|
||||
if (raw === undefined) return fallback;
|
||||
const n = Number(raw);
|
||||
if (!Number.isInteger(n) || n < 0 || n > 23) {
|
||||
console.warn(`${name}=${JSON.stringify(raw)} is not a valid hour (0-23) — using default ${fallback}`);
|
||||
return fallback;
|
||||
}
|
||||
return n;
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse a value stored in `created_at`. New rows are UTC ISO strings (with `Z`),
|
||||
|
|
|
|||
Loading…
Reference in a new issue