forked from Mike/freno-dev
The security review of 39d874a (FRE-369) found the address fix recorded
the wrong thing twice. Both are fixed here; the branch is the same one
under review, this commit does not rewrite it.
Q1 — the first x-forwarded-for hop is attacker-chosen text. In
client → Cloudflare → same-host proxy → app, both proxies *append*, so
the header arrives as `<client spoof…>, <true client>, <CF edge>` and
`split(",")[0]` took the sender's own invention — the loopback-peer gate
was sound, the hop selection defeated it. Preference is now
`cf-connecting-ip` (Cloudflare overwrites it, so a client value never
survives the edge), then x-forwarded-for counted from the right with one
trusted hop discounted, then the direct peer. Every candidate must parse
as an address. h3's `getRequestIP(…, { xForwardedFor: true })` has the
same leftmost flaw and is documented as not a fix.
Q4 — the table held full client addresses indefinitely. Every analytics
entry now passes through `truncateIpAddress` in `enrichAnalyticsEntry`
(IPv4 /24, IPv6 /56, unparseable dropped), so no writer can persist a
full address by accident. Retention got the schedule it never had:
`/api/analytics/retention` on a daily Vercel cron, Bearer-authenticated
and fail-closed, with the window parameterised (the old
`cleanupOldAnalytics` spliced the number into the SQL). Default 365 days
rather than the review's 90 so enabling the cron is not itself a
destructive event — the tighter window is an operational decision, named
on the task.
Also from the review: the module doc now forbids feeding this helper into
any security decision; `isLoopbackAddress` parses instead of
string-matching (`::ffff:127.9.9.9`, `0:0:0:0:0:0:0:1`); and `host` is
normalised (lowercased, port stripped) so `nook.freno.me:443` cannot
silently miss the `host = ?` scope. The weekly loopback caveat matches
the truncated shapes writers produce now, and says plainly that "unique
visitors" counts prefixes.
130 lines
5.1 KiB
TypeScript
130 lines
5.1 KiB
TypeScript
/**
|
|
* Scheduled retention for `VisitorAnalytics` (FRE-369, finding Q4).
|
|
*
|
|
* The table used to grow without bound: `cleanupOldAnalytics` existed but was
|
|
* reachable only as a manual tRPC call, and once FRE-366 started recording a
|
|
* real client address instead of the proxy's loopback, "unbounded" meant
|
|
* unbounded personal data in a hosted third-party database. Two halves answer
|
|
* that — the write-time prefix in `./request-client`, and this: a bound on how
|
|
* long any row exists at all.
|
|
*
|
|
* The default (365 days) is deliberately conservative so enabling the schedule
|
|
* is not itself a destructive event: it deletes nothing on its first run
|
|
* against a database whose history starts inside the window, and it bounds
|
|
* growth from then on. Tighten it to the reviewer's suggested 90 by setting
|
|
* `ANALYTICS_RETENTION_DAYS=90` — that first run *will* delete months of
|
|
* history that the /analytics dashboard and the weekly report read, which is an
|
|
* operational decision, not one this file should make silently.
|
|
*/
|
|
|
|
/** Kept when `ANALYTICS_RETENTION_DAYS` is unset or unusable. */
|
|
export const DEFAULT_ANALYTICS_RETENTION_DAYS = 365;
|
|
|
|
/** Anything longer is a misconfiguration, not a policy (≈ 2 years). */
|
|
export const MAX_ANALYTICS_RETENTION_DAYS = 730;
|
|
|
|
/**
|
|
* The slice of a connection this module needs — the same shape
|
|
* `weekly-metrics.ts` pins as `MetricsConn`, so a libsql client satisfies it
|
|
* without a cast (and the in-memory SQLite test adapter satisfies it too).
|
|
*/
|
|
export interface AnalyticsConn {
|
|
execute(query: {
|
|
sql: string;
|
|
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
|
args?: any[];
|
|
}): Promise<{ rows: unknown[]; rowsAffected?: number }>;
|
|
}
|
|
|
|
/**
|
|
* The retention window to enforce: a positive integer at most
|
|
* {@link MAX_ANALYTICS_RETENTION_DAYS}, else the default. Accepts the env
|
|
* string form and a number, because both callers hand over a different shape.
|
|
*/
|
|
export function resolveRetentionDays(
|
|
configured: string | number | undefined | null
|
|
): number {
|
|
const days = typeof configured === "number" ? configured : Number(configured);
|
|
if (!Number.isInteger(days) || days <= 0) return DEFAULT_ANALYTICS_RETENTION_DAYS;
|
|
if (days > MAX_ANALYTICS_RETENTION_DAYS) return DEFAULT_ANALYTICS_RETENTION_DAYS;
|
|
return days;
|
|
}
|
|
|
|
/**
|
|
* Delete analytics rows older than `olderThanDays` and report how many went.
|
|
*
|
|
* The window is a SQL parameter, not interpolated text: `cleanupOldAnalytics`
|
|
* used to splice the number into the statement, which only ever sat behind a
|
|
* `z.number()` input on the tRPC side — one unvalidated caller away from
|
|
* injection. A non-integer is refused here rather than passed on, so neither
|
|
* path can delete on a window the code did not check.
|
|
*/
|
|
export async function pruneVisitorAnalytics(
|
|
conn: AnalyticsConn,
|
|
olderThanDays: number
|
|
): Promise<number> {
|
|
const days = Math.trunc(Number(olderThanDays));
|
|
if (!Number.isInteger(days) || days <= 0) {
|
|
throw new Error(
|
|
`Refusing to prune VisitorAnalytics on a non-positive window: ${olderThanDays}`
|
|
);
|
|
}
|
|
|
|
const result = await conn.execute({
|
|
sql: `DELETE FROM VisitorAnalytics
|
|
WHERE created_at < datetime('now', ?)
|
|
RETURNING id`,
|
|
args: [`-${days} days`]
|
|
});
|
|
|
|
// Counted from `RETURNING id` rather than `rowsAffected`, keeping the contract
|
|
// `cleanupOldAnalytics` had: the caller is told how many rows actually went.
|
|
return result.rows.length;
|
|
}
|
|
|
|
/** Whether a cron may run: Bearer match, and an unset secret denies everything. */
|
|
export function isAuthorizedCron(
|
|
authorization: string | null | undefined,
|
|
secret: string | undefined | null
|
|
): boolean {
|
|
if (!secret) return false;
|
|
const provided = (authorization ?? "").replace(/^Bearer\s+/i, "").trim();
|
|
return Boolean(provided) && provided === secret;
|
|
}
|
|
|
|
export interface RetentionOutcome {
|
|
status: number;
|
|
body: Record<string, unknown>;
|
|
}
|
|
|
|
/**
|
|
* The whole cron decision, away from the route: authorise, resolve the window,
|
|
* prune. Returns the status and body the route should send, so the behaviour is
|
|
* testable without a database or an environment.
|
|
*
|
|
* Fail closed on auth (an unauthenticated trigger deleting real history is the
|
|
* worst thing this endpoint can do), and 500 rather than a silent zero on a
|
|
* failed prune — a retention job that quietly stopped pruning is exactly the
|
|
* state that created this issue.
|
|
*/
|
|
export async function runRetentionCron(options: {
|
|
authorization: string | null | undefined;
|
|
secret: string | undefined | null;
|
|
configuredDays: string | number | undefined | null;
|
|
conn: AnalyticsConn;
|
|
}): Promise<RetentionOutcome> {
|
|
if (!isAuthorizedCron(options.authorization, options.secret)) {
|
|
return { status: 401, body: { error: "Unauthorized" } };
|
|
}
|
|
|
|
const days = resolveRetentionDays(options.configuredDays);
|
|
|
|
try {
|
|
const deleted = await pruneVisitorAnalytics(options.conn, days);
|
|
return { status: 200, body: { success: true, days, deleted } };
|
|
} catch (error) {
|
|
console.error("Analytics retention job failed:", error);
|
|
return { status: 500, body: { error: "Analytics retention job failed", days } };
|
|
}
|
|
}
|