Files
freno-dev/src/server/analytics-retention.test.ts
Paperclip Agent 1d9287a199 analytics: fix the two FRE-369 findings in the funnel instrumentation
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.
2026-10-29 18:05:44 +00:00

201 lines
6.7 KiB
TypeScript

import { beforeEach, describe, expect, it } from "bun:test";
import { Database } from "bun:sqlite";
import { VISITOR_ANALYTICS_DDL } from "./analytics-schema";
import {
DEFAULT_ANALYTICS_RETENTION_DAYS,
MAX_ANALYTICS_RETENTION_DAYS,
isAuthorizedCron,
pruneVisitorAnalytics,
resolveRetentionDays,
runRetentionCron
} from "./analytics-retention";
// Why a real in-memory SQLite database: retention is a `DELETE` whose whole
// job is a date comparison. A stub returning a canned count would pass with a
// window that is off by a unit, inverted, or — the failure this file exists for
// — never applied at all. Same reasoning as the backfill and schema suites, and
// nothing is module-mocked here on purpose: bun's `mock.module` is process-wide.
let db: Database;
const conn = {
async execute(query: { sql: string; args?: unknown[] }) {
const statement = db.query(query.sql);
const args = (query.args ?? []) as never[];
// `DELETE … RETURNING id` answers with rows, which is how the prune counts
// what it removed; bun:sqlite exposes those through `.all()`.
if (
/^\s*(select|pragma|with)\b/i.test(query.sql) ||
/\breturning\b/i.test(query.sql)
) {
return { rows: statement.all(...args) as unknown[], rowsAffected: 0 };
}
return {
rows: [] as unknown[],
rowsAffected: statement.run(...args).changes
};
}
};
function insert(id: string, createdAt: string) {
db.run(
`INSERT INTO VisitorAnalytics (id, path, method, created_at) VALUES (?, '/x', 'GET', ?)`,
[id, createdAt]
);
}
function rowCount(): number {
return (db.query(`SELECT COUNT(*) AS n FROM VisitorAnalytics`).get() as {
n: number;
}).n;
}
beforeEach(() => {
db = new Database(":memory:");
db.run(VISITOR_ANALYTICS_DDL);
});
describe("resolveRetentionDays", () => {
it("takes a usable configured window", () => {
expect(resolveRetentionDays("90")).toBe(90);
expect(resolveRetentionDays(30)).toBe(30);
});
it("falls back to the default when the value is missing or nonsense", () => {
expect(resolveRetentionDays(undefined)).toBe(
DEFAULT_ANALYTICS_RETENTION_DAYS
);
expect(resolveRetentionDays("")).toBe(DEFAULT_ANALYTICS_RETENTION_DAYS);
expect(resolveRetentionDays("0")).toBe(DEFAULT_ANALYTICS_RETENTION_DAYS);
expect(resolveRetentionDays("-5")).toBe(DEFAULT_ANALYTICS_RETENTION_DAYS);
expect(resolveRetentionDays("1.5")).toBe(DEFAULT_ANALYTICS_RETENTION_DAYS);
expect(resolveRetentionDays("all")).toBe(DEFAULT_ANALYTICS_RETENTION_DAYS);
});
it("refuses a window so long it means 'never prune'", () => {
expect(resolveRetentionDays(String(MAX_ANALYTICS_RETENTION_DAYS + 1))).toBe(
DEFAULT_ANALYTICS_RETENTION_DAYS
);
});
});
describe("pruneVisitorAnalytics", () => {
it("deletes only rows older than the window", async () => {
insert("young", "2020-01-01 00:00:00");
insert("old", "2019-01-01 00:00:00");
// A row exactly at `now - 365 days` boundary in the other direction: the
// comparison is `<`, so the recent one survives.
db.run(
`UPDATE VisitorAnalytics SET created_at = datetime('now', '-10 days') WHERE id = 'young'`
);
db.run(
`UPDATE VisitorAnalytics SET created_at = datetime('now', '-400 days') WHERE id = 'old'`
);
const deleted = await pruneVisitorAnalytics(conn, 365);
expect(deleted).toBe(1);
expect(rowCount()).toBe(1);
});
it("deletes nothing when every row is inside the window", async () => {
insert("a", "x");
db.run(
`UPDATE VisitorAnalytics SET created_at = datetime('now', '-1 day') WHERE id = 'a'`
);
expect(await pruneVisitorAnalytics(conn, 365)).toBe(0);
expect(rowCount()).toBe(1);
});
it("refuses a window it was not given a number for", async () => {
insert("a", "2019-01-01 00:00:00");
await expect(pruneVisitorAnalytics(conn, 0)).rejects.toThrow(/window/);
await expect(pruneVisitorAnalytics(conn, -30)).rejects.toThrow(/window/);
await expect(pruneVisitorAnalytics(conn, NaN)).rejects.toThrow(/window/);
// "0" is not a window either — and must not delete the table.
await expect(
pruneVisitorAnalytics(conn, "0" as unknown as number)
).rejects.toThrow(/window/);
expect(rowCount()).toBe(1);
});
it("passes the window as a parameter, not as interpolated SQL", async () => {
insert("a", "2019-01-01 00:00:00");
const injected = "365); DROP TABLE VisitorAnalytics; --" as unknown as number;
// Truncating a non-numeric string yields NaN, which the guard refuses — so
// the statement never runs and the table is still there.
await expect(pruneVisitorAnalytics(conn, injected)).rejects.toThrow(/window/);
expect(rowCount()).toBe(1);
});
});
describe("isAuthorizedCron", () => {
it("matches the bearer secret and nothing else", () => {
expect(isAuthorizedCron("Bearer sekrit", "sekrit")).toBe(true);
expect(isAuthorizedCron("bearer SEKRIT".toLowerCase(), "sekrit")).toBe(true);
expect(isAuthorizedCron("Bearer nope", "sekrit")).toBe(false);
expect(isAuthorizedCron(null, "sekrit")).toBe(false);
expect(isAuthorizedCron("Bearer sekrit", null)).toBe(false);
});
it("denies everything when no secret is configured (fail closed)", () => {
expect(isAuthorizedCron("Bearer ", undefined)).toBe(false);
expect(isAuthorizedCron(undefined, "")).toBe(false);
});
});
describe("runRetentionCron", () => {
it("denies an unauthenticated trigger before touching the table", async () => {
insert("a", "2019-01-01 00:00:00");
const res = await runRetentionCron({
authorization: null,
secret: "sekrit",
configuredDays: "365",
conn
});
expect(res.status).toBe(401);
expect(rowCount()).toBe(1);
});
it("prunes on its window and reports what it removed", async () => {
db.run(
`INSERT INTO VisitorAnalytics (id, path, method, created_at) VALUES ('old','/x','GET',datetime('now','-400 days'))`
);
db.run(
`INSERT INTO VisitorAnalytics (id, path, method, created_at) VALUES ('new','/x','GET',datetime('now','-1 day'))`
);
const res = await runRetentionCron({
authorization: "Bearer sekrit",
secret: "sekrit",
configuredDays: "365",
conn
});
expect(res.status).toBe(200);
expect(res.body).toEqual({ success: true, days: 365, deleted: 1 });
expect(rowCount()).toBe(1);
});
it("says it failed rather than reporting a quiet zero", async () => {
db.close(); // the table is gone: the prune's statement must now throw
const res = await runRetentionCron({
authorization: "Bearer sekrit",
secret: "sekrit",
configuredDays: "365",
conn
});
expect(res.status).toBe(500);
expect(res.body.days).toBe(365);
expect(res.body.deleted).toBeUndefined();
});
});