From 39d874a7e22eb8061dc07077894692e8a0a20bdd Mon Sep 17 00:00:00 2001 From: Paperclip Agent Date: Thu, 29 Oct 2026 17:02:39 +0000 Subject: [PATCH 1/2] analytics: make each weekly funnel stage separately measurable (FRE-366) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gap 1 — click and served share one label: trackDownload now takes a stage ("click" | "served") carried on a new VisitorAnalytics.stage column; the Nook door and the tRPC proxy record clicks, the DMG file route records served. summariseDownloads() and the /analytics downloads block are untouched — the stage is a separate column, so the path grouping cannot change. Gap 2 — checkout start records nothing: the checkout route records a created Stripe session through the shared trackRequest writer (same referrer, campaign, country dimensions as every door) under its own path, and STAGES.checkout reads it instead of the /checkout page proxy. Gap 3 — loopback addresses, no host dimension: every writer now resolves the client address through resolveClientIp, which trusts x-forwarded-for only when the direct peer is the loopback production proxy (trust boundary stated in request-client.ts), and records the host on each row through the same site-context extractor routing uses. stage and host reach the table through VISITOR_ANALYTICS_ADDED_COLUMNS; the weekly job applies the same bootstrap before reading so it renders seven stages against a database no deploy has migrated yet. The weekly read splits downloadClick/dmgServed, scopes visits to nook.freno.me, and computes its caveats from the window: pre-instrumentation rows are stated and excluded, and the loopback warning now fires only when rows recorded since the instrumentation still carry it. --- marketing/metrics/weekly.md | 28 +-- scripts/weekly-metrics.ts | 10 +- src/db/types.ts | 2 + src/routes/api/downloads/[filename].ts | 2 +- src/routes/api/the-nook/checkout.ts | 18 ++ src/routes/api/the-nook/download.test.ts | 4 + src/routes/api/the-nook/download.ts | 8 +- src/server/analytics-schema.test.ts | 12 +- src/server/analytics-schema.ts | 18 +- src/server/analytics.ts | 10 +- src/server/api/routers/analytics.ts | 10 +- src/server/api/routers/downloads.ts | 2 +- src/server/api/utils.ts | 11 +- src/server/request-client.test.ts | 78 +++++++ src/server/request-client.ts | 67 ++++++ src/server/site-context-server.ts | 5 +- src/server/track-download.ts | 62 ++++- src/server/weekly-metrics.test.ts | 232 +++++++++++++----- src/server/weekly-metrics.ts | 285 +++++++++++++---------- 19 files changed, 643 insertions(+), 221 deletions(-) create mode 100644 src/server/request-client.test.ts create mode 100644 src/server/request-client.ts diff --git a/marketing/metrics/weekly.md b/marketing/metrics/weekly.md index 2ea9eaf..12b1f47 100644 --- a/marketing/metrics/weekly.md +++ b/marketing/metrics/weekly.md @@ -1,6 +1,6 @@ # Weekly funnel snapshot -Window: **2026-10-22 16:00:00 → 2026-10-29 16:00:00 UTC** (7 days). Previous: 2026-10-15 16:00:00 → 2026-10-22 16:00:00 UTC. +Window: **2026-10-22 17:01:33 → 2026-10-29 17:01:33 UTC** (7 days). Previous: 2026-10-15 17:01:33 → 2026-10-22 17:01:33 UTC. Generated by `bun --env-file=.env scripts/weekly-metrics.ts`; regenerated weekly, never hand-edited. The analysis is the CMO's. @@ -8,10 +8,11 @@ Generated by `bun --env-file=.env scripts/weekly-metrics.ts`; regenerated weekly | Stage | This week | Last week | Change | |---|---:|---:|---:| -| visit (page views) | 61 | 56 | +5 | -| download click → DMG served (one label) † | 156 | 0 | +156 | +| visit (nook.freno.me page views) | 0 | 0 | 0 | +| download click | 0 | 0 | 0 | +| DMG served | 0 | 0 | 0 | | trial started | 2 | 0 | +2 | -| checkout page visit † | 0 | 0 | 0 | +| checkout started (Stripe session) | 0 | 0 | 0 | | purchase (licence issued) | 0 | 0 | 0 | | activation | 0 | 0 | 0 | @@ -21,31 +22,22 @@ Top 10 by volume, untagged excluded — the same grouping `getAnalyticsSummary.t | Source | Medium | Campaign | Rows | |---|---|---|---:| -| hn | post | launch | 2 | -| direct | — | deploycheck-direct | 1 | -| hn | — | flushproof | 1 | -| hn | post | deploycheck-ref | 1 | -| r_macapps | — | week2 | 1 | | x | — | — | 1 | -7 of 356 rows in the window carry campaign tags; the rest are untagged and therefore unattributed. +1 of 291 rows in the window carry campaign tags; the rest are untagged and therefore unattributed. ## The one thing that moved -**download click → DMG served (one label): 0 → 156 (+156).** +**trial started: 0 → 2 (+2).** ## Before acting on these numbers -- **356 of 356 rows in the window came from a loopback address** (127.0.0.1 / ::1); every one of the 12035 addressed rows the table has ever held is loopback (13 carry no address). Either this host is the only audience, or the app sits behind a local proxy and `getRequestIP` records the proxy, not the visitor. Either way the address identifies nobody — "unique visitors" collapses to one host and per-channel reach cannot be read from it. -- **The download stage is one label, not two.** The landing-door route and the file route that serves the DMG both write `/api/downloads/thenook`, so "download click → DMG served" cannot be split: one served click writes two rows, a click that fails to resolve writes one, and a Sparkle update writes one. Of this window's 156 download rows, 156 are loopback and 117 are not browser user agents (curl, Sparkle, link-preview crawlers). -- **Checkout start is not recorded.** `POST /api/the-nook/checkout` — the call that actually creates the Stripe session — writes nothing to analytics; the only trace is the /checkout page view reported above, which is a page, not a started checkout. -- **There is no host dimension.** `VisitorAnalytics` records a path, not the site, so the Nook's landing page (`/` on nook.freno.me) is the same row as freno.me's root. "visit" above is site-wide page views, not Nook visits. +- **291 of 291 rows in the window predate the FRE-366 instrumentation.** They carry no host, and their recorded address is the production proxy's loopback (127.0.0.1 / ::1) — the writers then recorded the direct peer, not the visitor. They are excluded from every host-scoped stage above (visits, and the download click/served split), and "unique visitors" over them still collapses to one host. Rows recorded since carry the real client address and the site they addressed. +- **Download rows are automation-heavy.** Of this window's 137 download rows, 0 are clicks and 0 are served files; the remaining 137 predate the click/served stage label and cannot be split, so they count in neither stage. 112 of all 137 are not browser user agents (curl, Sparkle, link-preview crawlers). - **No conversion rate before the cohort matures.** Trial→paid belongs to the mature Day 28–35 read (FRE-197); a ratio from a partial cohort is not a metric. -† Proxy or merged label — see *Before acting on these numbers* above. - ## Sources - `VisitorAnalytics` (freno.me Turso DB) — visits, downloads, campaigns. - `trials`, `licenses`, `activations` (the-nook Turso DB) — the paid funnel. -- Generated 2026-10-29T16:00:00Z. +- Generated 2026-10-29T17:01:33Z. diff --git a/scripts/weekly-metrics.ts b/scripts/weekly-metrics.ts index f24c693..c670ac7 100644 --- a/scripts/weekly-metrics.ts +++ b/scripts/weekly-metrics.ts @@ -2,6 +2,7 @@ import { createClient } from "@libsql/client/web"; import { mkdir, writeFile } from "node:fs/promises"; import { dirname, resolve } from "node:path"; import { + ensureAnalyticsSchema, getWeeklyReport, renderWeeklyMarkdown, type Connections @@ -123,6 +124,11 @@ async function main(): Promise { nook: connect(requiredEnv("NOOK_DB_URL"), requiredEnv("NOOK_DB_TOKEN")) }; + // The app migrates the table on boot, but this job must not depend on a + // deploy having happened: add any missing post-hoc columns (stage, host) + // through the same idempotent bootstrap path first. + await ensureAnalyticsSchema(connections.analytics); + const report = await getWeeklyReport(connections, { days: options.days, now: options.now @@ -141,8 +147,8 @@ async function main(): Promise { console.log( `wrote ${options.out}\n` + `window ${report.window.start} → ${report.window.end}\n` + - `visit ${f.visit}, download ${f.download}, trial ${f.trial}, ` + - `checkout ${f.checkout}, purchase ${f.purchase}, activation ${f.activation}\n` + + `visit ${f.visit}, download click ${f.downloadClick}, DMG served ${f.dmgServed}, ` + + `trial ${f.trial}, checkout ${f.checkout}, purchase ${f.purchase}, activation ${f.activation}\n` + `mover: ${report.mover ? `${report.mover.label} ${report.mover.before} → ${report.mover.now}` : "none"}` ); } diff --git a/src/db/types.ts b/src/db/types.ts index acd1a4f..7276f39 100644 --- a/src/db/types.ts +++ b/src/db/types.ts @@ -160,6 +160,8 @@ export interface VisitorAnalytics { ttfb?: number | null; dom_load?: number | null; load_complete?: number | null; + stage?: string | null; + host?: string | null; created_at: string; } diff --git a/src/routes/api/downloads/[filename].ts b/src/routes/api/downloads/[filename].ts index 89496b6..8e210e7 100644 --- a/src/routes/api/downloads/[filename].ts +++ b/src/routes/api/downloads/[filename].ts @@ -95,7 +95,7 @@ export async function GET(event: APIEvent) { console.log(`✓ Serving ${filename} (${body.length} bytes)`); - await trackDownload(event, assetKeys[asset]!); + await trackDownload(event, assetKeys[asset]!, "served"); // SAFETY: `transformToByteArray` yields `Uint8Array`, which the // DOM lib's `BufferSource` (ArrayBuffer-backed) does not accept, though undici diff --git a/src/routes/api/the-nook/checkout.ts b/src/routes/api/the-nook/checkout.ts index 561dbb4..478755c 100644 --- a/src/routes/api/the-nook/checkout.ts +++ b/src/routes/api/the-nook/checkout.ts @@ -3,6 +3,7 @@ import { env } from "~/env/server"; import { TURNSTILE_CONFIG } from "~/config"; import { verifyTurnstileToken } from "~/server/fetch-utils"; import { json, error } from "./_lib"; +import { trackRequest } from "~/server/track-download"; /** * POST /api/the-nook/checkout @@ -12,6 +13,14 @@ import { json, error } from "./_lib"; * session for the one-time $10 The Nook license. Returns the hosted * checkout URL for the client to redirect to. * + * Analytics: a created session is the funnel's "checkout started" stage + * (FRE-366). Until then the only trace was the client-side /checkout page + * view — a page, not a started checkout. Recorded through the shared + * `trackRequest` writer (referrer, campaign tags, country, real client + * address, host) under this route's own path, best-effort: a failed + * analytics write never blocks a checkout, and an attempt Turnstile or + * Stripe rejects is correctly not a started checkout. + * * Uses raw `fetch` (Node 24 / Vercel Node functions have global fetch) — no * `stripe` npm dependency added. */ @@ -67,5 +76,14 @@ export async function POST(event: APIEvent) { if (!data.url) { return error("Stripe returned no checkout URL", 502); } + + // The session exists: that is "checkout started". Awaited so the row is + // flushed before the response — a buffered write is not guaranteed to + // survive the response on serverless. + await trackRequest(event, { + path: "/api/the-nook/checkout", + method: "POST" + }); + return json({ checkoutUrl: data.url }); } diff --git a/src/routes/api/the-nook/download.test.ts b/src/routes/api/the-nook/download.test.ts index a230002..026a77f 100644 --- a/src/routes/api/the-nook/download.test.ts +++ b/src/routes/api/the-nook/download.test.ts @@ -113,6 +113,10 @@ describe("GET /api/the-nook/download", () => { expect(logged[0]).toMatchObject({ path: "/api/downloads/thenook", method: "GET", + // The door is the click half of the split; the file route writes + // "served". And the row names the site it came from (FRE-366). + stage: "click", + host: "nook.freno.me", referrer: "https://nook.freno.me/" }); // Flushed within the request, not left to the batch timer. diff --git a/src/routes/api/the-nook/download.ts b/src/routes/api/the-nook/download.ts index 6d0a1ee..13479b0 100644 --- a/src/routes/api/the-nook/download.ts +++ b/src/routes/api/the-nook/download.ts @@ -13,8 +13,10 @@ import { resolveLatestDownload } from "./_download"; * Why a redirect and not a direct S3 link: the appcast route and the DMG * route never logged anything, so step 2 of the funnel (download click → * DMG served) was invisible. Every click now lands one row in - * VisitorAnalytics under `/api/downloads/thenook`, alongside the clicks and - * served files of every other product — see `~/server/track-download`. + * VisitorAnalytics under `/api/downloads/thenook` with `stage = "click"`, + * distinct from the `stage = "served"` row the file route writes when the + * DMG is actually streamed — the split FRE-366 added, alongside the clicks + * and served files of every other product — see `~/server/track-download`. * * Referrer, campaign tags, edge country (with `?c=` override) and user agent * are read by `trackDownload`, which is shared with the other download doors. @@ -28,7 +30,7 @@ import { resolveLatestDownload } from "./_download"; export async function GET(event: APIEvent): Promise { // Recorded before resolving: a click that dies at resolution is exactly the // failure the funnel needs to show. - await trackDownload(event, "thenook"); + await trackDownload(event, "thenook", "click"); let target: string | null = null; try { diff --git a/src/server/analytics-schema.test.ts b/src/server/analytics-schema.test.ts index 70c99dd..787dfc6 100644 --- a/src/server/analytics-schema.test.ts +++ b/src/server/analytics-schema.test.ts @@ -40,7 +40,9 @@ const sampleArgs = [ 2, // inp 3, // ttfb 4, // dom_load - 5 // load_complete + 5, // load_complete + "click", // stage + "nook.freno.me" // host ]; beforeEach(() => { @@ -69,7 +71,7 @@ describe("VisitorAnalytics schema", () => { } // The reverse direction matters too: a column the code writes but the // DDL omits is the failure this whole file exists to catch. - expect(VISITOR_ANALYTICS_COLUMNS.length).toBe(23); + expect(VISITOR_ANALYTICS_COLUMNS.length).toBe(25); }); it("has one placeholder per inserted column", () => { @@ -83,7 +85,7 @@ describe("VisitorAnalytics schema", () => { const row = db .prepare( - `SELECT path, country, utm_source, utm_medium, utm_campaign + `SELECT path, country, utm_source, utm_medium, utm_campaign, stage, host FROM VisitorAnalytics` ) .get() as Record; @@ -93,7 +95,9 @@ describe("VisitorAnalytics schema", () => { country: "GB", utm_source: "hn", utm_medium: "post", - utm_campaign: "launch" + utm_campaign: "launch", + stage: "click", + host: "nook.freno.me" }); }); diff --git a/src/server/analytics-schema.ts b/src/server/analytics-schema.ts index 93d43ba..19119cf 100644 --- a/src/server/analytics-schema.ts +++ b/src/server/analytics-schema.ts @@ -51,6 +51,8 @@ export const VISITOR_ANALYTICS_DDL = ` utm_source TEXT, utm_medium TEXT, utm_campaign TEXT, + stage TEXT, + host TEXT, FOREIGN KEY (user_id) REFERENCES User(id) ON DELETE SET NULL ) `; @@ -79,6 +81,18 @@ export const VISITOR_ANALYTICS_ADDED_COLUMNS = [ { name: "utm_campaign", ddl: `ALTER TABLE VisitorAnalytics ADD COLUMN utm_campaign TEXT` + }, + { + // Which funnel stage a download row is: "click" (a door) or "served" + // (the file route). Null on every non-download row (FRE-366). + name: "stage", + ddl: `ALTER TABLE VisitorAnalytics ADD COLUMN stage TEXT` + }, + { + // The Host the client addressed, e.g. "nook.freno.me" — the site + // dimension the table never had (FRE-366). + name: "host", + ddl: `ALTER TABLE VisitorAnalytics ADD COLUMN host TEXT` } ]; @@ -110,7 +124,9 @@ export const VISITOR_ANALYTICS_COLUMNS = [ "inp", "ttfb", "dom_load", - "load_complete" + "load_complete", + "stage", + "host" ] as const; /** diff --git a/src/server/analytics.ts b/src/server/analytics.ts index 198925e..b37cef0 100644 --- a/src/server/analytics.ts +++ b/src/server/analytics.ts @@ -64,6 +64,10 @@ export interface AnalyticsEntry { ttfb?: number | null; domLoad?: number | null; loadComplete?: number | null; + /** Funnel stage for download rows ("click" | "served"); null elsewhere. */ + stage?: string | null; + /** Host the client addressed, e.g. "nook.freno.me" — the site dimension. */ + host?: string | null; } /** @@ -124,7 +128,9 @@ async function flushAnalyticsBuffer(): Promise { entry.inp || null, entry.ttfb || null, entry.domLoad || null, - entry.loadComplete || null + entry.loadComplete || null, + entry.stage || null, + entry.host || null ] }); } @@ -258,6 +264,8 @@ export async function queryAnalytics( utm_medium: row.utm_medium as string | null, utm_campaign: row.utm_campaign as string | null, duration_ms: row.duration_ms as number | null, + stage: row.stage as string | null, + host: row.host as string | null, created_at: row.created_at as string })); } diff --git a/src/server/api/routers/analytics.ts b/src/server/api/routers/analytics.ts index 3cd6a5e..e5049f3 100644 --- a/src/server/api/routers/analytics.ts +++ b/src/server/api/routers/analytics.ts @@ -10,7 +10,7 @@ import { } from "~/server/analytics"; import { ConnectionFactory } from "~/server/database"; import { v4 as uuid } from "uuid"; -import { getRequestIP } from "vinxi/http"; +import { resolveClientIp, requestHost } from "~/server/request-client"; import { parseUtm } from "~/server/utm"; import { VISITOR_ANALYTICS_INSERT } from "~/server/analytics-schema"; @@ -98,7 +98,8 @@ export const analyticsRouter = createTRPCRouter({ getHeader(req.headers, "referer") || getHeader(req.headers, "referrer") || getHeader(ctx.event.request?.headers, "referer"); - const ipAddress = getRequestIP(ctx.event.nativeEvent) || undefined; + const ipAddress = resolveClientIp(ctx.event) || undefined; + const host = requestHost(ctx.event) || undefined; const enriched = enrichAnalyticsEntry({ userId: ctx.userId, path: input.path, @@ -106,6 +107,7 @@ export const analyticsRouter = createTRPCRouter({ userAgent, referrer, ipAddress, + host, // The client beacon sends `pathname + search`, so the page URL's // own query string is the campaign source; the referrer fills any // gap (e.g. clicks that arrive on a clean path). @@ -145,7 +147,9 @@ export const analyticsRouter = createTRPCRouter({ enriched.inp || null, enriched.ttfb || null, enriched.domLoad || null, - enriched.loadComplete || null + enriched.loadComplete || null, + enriched.stage || null, + enriched.host || null ] }); diff --git a/src/server/api/routers/downloads.ts b/src/server/api/routers/downloads.ts index fd79ed1..e053c0f 100644 --- a/src/server/api/routers/downloads.ts +++ b/src/server/api/routers/downloads.ts @@ -79,7 +79,7 @@ export const downloadsRouter = createTRPCRouter({ // exactly the event the funnel needs to show. Unknown asset names are // skipped rather than recorded as a download that never happened. if (assets[input.asset_name] || latestAssets[input.asset_name]) { - await trackDownload(ctx.event, input.asset_name); + await trackDownload(ctx.event, input.asset_name, "click"); } const bucket = env.VITE_DOWNLOAD_BUCKET_STRING; diff --git a/src/server/api/utils.ts b/src/server/api/utils.ts index 60ed1e9..7d2fbb8 100644 --- a/src/server/api/utils.ts +++ b/src/server/api/utils.ts @@ -1,7 +1,7 @@ import { initTRPC, TRPCError } from "@trpc/server"; import type { APIEvent } from "@solidjs/start/server"; import { logVisit, enrichAnalyticsEntry } from "~/server/analytics"; -import { getRequestIP } from "vinxi/http"; +import { resolveClientIp, requestHost } from "~/server/request-client"; import { getAuthPayloadFromEvent } from "~/server/auth"; export type Context = { @@ -51,7 +51,11 @@ async function createContextInner(event: APIEvent): Promise { getHeader(req.headers, "referer") || getHeader(req.headers, "referrer") || getHeader(event.request?.headers, "referer"); - const ipAddress = getRequestIP(event.nativeEvent) || undefined; + // Behind the production proxy the direct peer is loopback, so the real + // client address comes from the forwarded header the proxy sets — see + // `~/server/request-client` for the trust boundary (FRE-366). + const ipAddress = resolveClientIp(event) || undefined; + const host = requestHost(event) || undefined; // Don't log the performance logging endpoint itself to avoid circular tracking if (!path.includes("analytics.logPerformance")) { @@ -62,7 +66,8 @@ async function createContextInner(event: APIEvent): Promise { method, userAgent, referrer, - ipAddress + ipAddress, + host }) ); } diff --git a/src/server/request-client.test.ts b/src/server/request-client.test.ts new file mode 100644 index 0000000..fc40bb8 --- /dev/null +++ b/src/server/request-client.test.ts @@ -0,0 +1,78 @@ +import { beforeEach, describe, expect, it, mock } from "bun:test"; + +// The trust boundary of FRE-366: x-forwarded-for is client-controlled text, +// believed only when the direct peer is the loopback production proxy. These +// tests pin that boundary — a regression here either re-records the proxy as +// the visitor (unique visitors collapses to one host) or lets any client +// spoof its address. + +let directIp: string | null = null; +mock.module("vinxi/http", () => ({ + getRequestIP: () => directIp +})); + +const { resolveClientIp, isLoopbackAddress, requestHost } = await import( + "./request-client" +); + +function event( + headers: Record = {}, + url = "https://nook.freno.me/downloads" +) { + return { + request: new Request(url, { headers }), + nativeEvent: {} + } as never; +} + +beforeEach(() => { + directIp = null; +}); + +describe("resolveClientIp", () => { + it("takes the first forwarded hop when the direct peer is the loopback proxy", () => { + directIp = "127.0.0.1"; + expect( + resolveClientIp(event({ "x-forwarded-for": "198.51.100.9, 10.0.0.1" })) + ).toBe("198.51.100.9"); + + directIp = "::1"; + expect( + resolveClientIp(event({ "x-forwarded-for": "198.51.100.10" })) + ).toBe("198.51.100.10"); + }); + + it("ignores a forwarded header from a real peer — it is spoofable", () => { + directIp = "203.0.113.7"; + expect( + resolveClientIp(event({ "x-forwarded-for": "198.51.100.9" })) + ).toBe("203.0.113.7"); + }); + + it("keeps the loopback peer when the proxy set no forwarded header", () => { + directIp = "::ffff:127.0.0.1"; + expect(resolveClientIp(event())).toBe("::ffff:127.0.0.1"); + }); + + it("records nothing rather than inventing an address", () => { + expect(resolveClientIp(event())).toBeNull(); + }); +}); + +describe("isLoopbackAddress", () => { + it("covers the v4 range, the shorthand and the v6 forms", () => { + expect(isLoopbackAddress("127.0.0.1")).toBe(true); + expect(isLoopbackAddress("127.0.0.53")).toBe(true); + expect(isLoopbackAddress("::1")).toBe(true); + expect(isLoopbackAddress("::ffff:127.0.0.1")).toBe(true); + expect(isLoopbackAddress("203.0.113.7")).toBe(false); + expect(isLoopbackAddress("10.0.0.1")).toBe(false); + }); +}); + +describe("requestHost", () => { + it("reads the host the client addressed from the request URL", () => { + expect(requestHost(event())).toBe("nook.freno.me"); + expect(requestHost(event({}, "https://freno.me/"))).toBe("freno.me"); + }); +}); diff --git a/src/server/request-client.ts b/src/server/request-client.ts new file mode 100644 index 0000000..2d79080 --- /dev/null +++ b/src/server/request-client.ts @@ -0,0 +1,67 @@ +import type { APIEvent } from "@solidjs/start/server"; +import { getRequestIP } from "vinxi/http"; +import { hostFromEventLike } from "./site-context-server"; + +/** + * The real client address behind the production proxy (FRE-366). + * + * Until this existed every analytics writer called `getRequestIP` and only + * looked at `x-forwarded-for` when it returned null — so behind the + * production proxy the recorded address was the proxy itself, and all + * 12,035 addressed rows in VisitorAnalytics carried 127.0.0.1 / ::1. + * + * Trust boundary, stated plainly: `x-forwarded-for` is client-controlled + * text. It is believed only when the direct peer is a loopback address — + * the production shape, where a same-host proxy (nginx/caddy) terminates + * TLS and sets the header itself. Anywhere else the direct peer IS the + * client (dev, direct hit), and a forwarded header from it is ignored as + * spoofable. The first hop of the header is taken; later hops are the + * client's own claims. + */ +export function resolveClientIp(event: APIEvent): string | null { + const direct = getRequestIP(event.nativeEvent); + + if (direct && !isLoopbackAddress(direct)) { + return direct; + } + + const forwarded = event.request?.headers?.get?.("x-forwarded-for"); + const first = forwarded?.split(",")[0]?.trim(); + if (first) { + return first; + } + + return direct ?? null; +} + +export function isLoopbackAddress(ip: string): boolean { + return ( + ip === "::1" || + ip === "::ffff:127.0.0.1" || + ip === "127.0.0.1" || + ip.startsWith("127.") + ); +} + +/** + * The site the request addressed — the `Host` the client asked for, e.g. + * `nook.freno.me`. Recorded on every analytics row so a weekly read can + * scope to one site; without it `/` on nook.freno.me is the same row as + * `/` on freno.me. Thin alias over the site-context extractor so the + * analytics writers share one host-parsing implementation with routing. + */ +export function requestHost(event: APIEvent): string | null { + const header = hostFromEventLike(event); + if (header) return header; + + // `host` is a forbidden header and some server shapes drop it from + // `event.request.headers`; the request URL is absolute on the server and + // carries the same host the client addressed. + try { + const url = event.request?.url; + if (url) return new URL(url).host || null; + } catch { + /* noop */ + } + return null; +} diff --git a/src/server/site-context-server.ts b/src/server/site-context-server.ts index 21a272e..fc694b6 100644 --- a/src/server/site-context-server.ts +++ b/src/server/site-context-server.ts @@ -38,9 +38,10 @@ export interface ServerSiteEvent { * the raw Node request via `event.nativeEvent.node.req.headers`. We try * both so this works for API route handlers (`APIEvent`), the SSR document * handler (`PageEvent`), and tRPC procedures operating on the underlying - * `H3Event`. + * `H3Event`. Also used by the analytics writers to record the site dimension + * (`host` column) on every row (FRE-366). */ -function hostFromEventLike(event: ServerSiteEvent | unknown): string | null { +export function hostFromEventLike(event: ServerSiteEvent | unknown): string | null { // 1) Web FetchEvent / APIEvent / PageEvent → standard `Request` headers. try { const req = (event as { request?: Request } | null)?.request; diff --git a/src/server/track-download.ts b/src/server/track-download.ts index 533186d..7435bd8 100644 --- a/src/server/track-download.ts +++ b/src/server/track-download.ts @@ -1,5 +1,4 @@ import type { APIEvent } from "@solidjs/start/server"; -import { getRequestIP } from "vinxi/http"; import { logVisit, enrichAnalyticsEntry, @@ -7,6 +6,7 @@ import { } from "~/server/analytics"; import { parseUtm } from "~/server/utm"; import { downloadPath } from "~/lib/download-asset"; +import { resolveClientIp, requestHost } from "~/server/request-client"; /** * Record one download event under the canonical path `/api/downloads/`. @@ -15,35 +15,75 @@ import { downloadPath } from "~/lib/download-asset"; * label per asset to group on: the tRPC click proxy, the S3 file route that * serves Sparkle updates, and the Nook door. * + * The `stage` is what separates the doors within one asset label: "click" + * for a door the visitor chose (landing button, tRPC proxy), "served" for + * the file route that actually streams the DMG. Until FRE-366 both wrote the + * same unlabelled row, so "download click → DMG served" could not be split — + * one served click wrote two identical rows. `summariseDownloads()` and the + * /analytics downloads block still group on the path alone; the stage is a + * separate column precisely so that grouping cannot change. + * * Best-effort by design — a failed analytics write must never block a * download. Flushed rather than merely buffered: a download is low-volume and * the 30s batch timer is not guaranteed to survive the response on serverless. + */ +export async function trackDownload( + event: APIEvent, + asset: string, + stage: DownloadStage +): Promise { + await trackRequest(event, { + path: downloadPath(asset), + method: "GET", + stage + }); +} + +export type DownloadStage = "click" | "served"; + +export interface TrackedRequest { + path: string; + method: string; + /** Funnel stage label; only download rows carry one today. */ + stage?: string | null; +} + +/** + * Record one server-side request event with the shared visitor dimensions: + * referrer, campaign tags, edge country, real client address and host. + * + * This is the one writer every measured door goes through — download doors + * via `trackDownload`, the checkout door directly — so the dimensions can + * never drift between doors. Same best-effort, flushed-immediately contract + * as `trackDownload`. * * `event.request` is expected to be a Fetch `Request` (true for Vinxi route * handlers and browser tRPC calls). A server-side `createCaller` hands over a * Node request, whose headers have no `get` — that throw is swallowed below, - * so such a click is not recorded. No server-side caller downloads today. + * so such an event is not recorded. No server-side caller downloads today. */ -export async function trackDownload( +export async function trackRequest( event: APIEvent, - asset: string + tracked: TrackedRequest ): Promise { const { request } = event; try { const referrer = request.headers.get("referer") ?? request.headers.get("referrer"); - const forwarded = request.headers.get("x-forwarded-for"); await logVisit( enrichAnalyticsEntry({ - path: downloadPath(asset), - method: "GET", + path: tracked.path, + method: tracked.method, + stage: tracked.stage ?? null, referrer, userAgent: request.headers.get("user-agent"), - ipAddress: - getRequestIP(event.nativeEvent) ?? - (forwarded ? forwarded.split(",")[0]!.trim() : null), + // Behind the production proxy the direct peer is loopback; the real + // client address comes from the forwarded header the proxy sets — + // see `~/server/request-client` for the trust boundary. + ipAddress: resolveClientIp(event), + host: requestHost(event), // The click carries its own tags when a campaign links straight at the // door; otherwise the landing page's tags ride along in the referer. ...parseUtm(request.url, referrer), @@ -56,6 +96,6 @@ export async function trackDownload( ); await flushAnalytics(); } catch (error) { - console.error(`Failed to log download of ${asset}:`, error); + console.error(`Failed to log ${tracked.method} ${tracked.path}:`, error); } } diff --git a/src/server/weekly-metrics.test.ts b/src/server/weekly-metrics.test.ts index d7321ec..fa42009 100644 --- a/src/server/weekly-metrics.test.ts +++ b/src/server/weekly-metrics.test.ts @@ -1,12 +1,16 @@ import { beforeEach, describe, expect, it } from "bun:test"; import { Database } from "bun:sqlite"; import { + VISITOR_ANALYTICS_ADDED_COLUMNS, VISITOR_ANALYTICS_COLUMNS, VISITOR_ANALYTICS_DDL, VISITOR_ANALYTICS_INSERT } from "./analytics-schema"; import { + NOOK_CHECKOUT_PATH, NOOK_DOWNLOAD_PATH, + NOOK_HOST, + ensureAnalyticsSchema, getWeeklyReport, pickMover, readCampaigns, @@ -70,6 +74,8 @@ let conns: Connections; const NOW = new Date("2026-10-29T16:00:00Z"); +// A row as the post-FRE-366 writers record it: real client address, the host +// it addressed, and a stage label where the row is a download. const row = ( overrides: Record ): Record => ({ @@ -96,6 +102,8 @@ const row = ( ttfb: null, dom_load: null, load_complete: null, + stage: null, + host: NOOK_HOST, ...overrides }); @@ -142,20 +150,32 @@ describe("windowsFor", () => { }); describe("readFunnel", () => { - it("counts each stage inside its window and nothing outside it", async () => { - // Page views count; API calls do not. + it("counts each stage by its own label inside its window", async () => { + // Page views on the Nook host count; other sites, API calls, legacy + // rows with no host, and the other window do not. insertAnalytics({ path: "/", created_at: THIS_WEEK }); insertAnalytics({ path: "/downloads", created_at: THIS_WEEK }); + insertAnalytics({ path: "/", host: "freno.me", created_at: THIS_WEEK }); + insertAnalytics({ path: "/", host: null, created_at: THIS_WEEK }); insertAnalytics({ path: "/api/trpc/anything", created_at: THIS_WEEK }); insertAnalytics({ path: "/", created_at: LAST_WEEK }); - - // The merged download label, click and served alike. - insertAnalytics({ path: NOOK_DOWNLOAD_PATH, created_at: THIS_WEEK }); - insertAnalytics({ path: NOOK_DOWNLOAD_PATH, created_at: THIS_WEEK }); - - // Checkout page doors, including the legacy /nook/checkout path. + // The old checkout proxy: a /checkout page view is a visit, not a + // started checkout. insertAnalytics({ path: "/checkout", created_at: THIS_WEEK }); - insertAnalytics({ path: "/nook/checkout", created_at: THIS_WEEK }); + + // The download label splits by stage; a row with no stage predates the + // split and counts in neither. + insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "click", created_at: THIS_WEEK }); + insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "served", created_at: THIS_WEEK }); + insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "served", created_at: THIS_WEEK }); + insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: null, created_at: THIS_WEEK }); + + // A created Stripe session, recorded by the checkout route itself. + insertAnalytics({ + path: NOOK_CHECKOUT_PATH, + method: "POST", + created_at: THIS_WEEK + }); // The licence DB stores ISO-8601 with milliseconds. nookDb @@ -180,13 +200,12 @@ describe("readFunnel", () => { const window = windowsFor(NOW, 7).current; const funnel = await readFunnel(conns, window); - // "/checkout" and "/nook/checkout" are page views too, so they land in - // both the visit count and the checkout count. expect(funnel).toEqual({ - visit: 4, - download: 2, + visit: 3, // "/", "/downloads" and the "/checkout" page view on the Nook host + downloadClick: 1, + dmgServed: 2, trial: 1, - checkout: 2, + checkout: 1, purchase: 1, activation: 1 }); @@ -196,7 +215,8 @@ describe("readFunnel", () => { const window = windowsFor(NOW, 7).current; expect(await readFunnel(conns, window)).toEqual({ visit: 0, - download: 0, + downloadClick: 0, + dmgServed: 0, trial: 0, checkout: 0, purchase: 0, @@ -206,37 +226,51 @@ describe("readFunnel", () => { }); describe("readTraffic", () => { - it("separates loopback and automation from browser traffic", async () => { + it("separates legacy rows, live loopback and automation from clean traffic", async () => { + // Post-instrumentation rows: host set. One of them still loopback — the + // shape the report must flag as "the fix is not live". insertAnalytics({ path: NOOK_DOWNLOAD_PATH, + stage: "click", + created_at: THIS_WEEK + }); + insertAnalytics({ + path: NOOK_DOWNLOAD_PATH, + stage: "served", + ip_address: "127.0.0.1", + user_agent: "The Nook/0.13.4 Sparkle/2.9.6", + created_at: THIS_WEEK + }); + insertAnalytics({ path: "/", created_at: THIS_WEEK }); + + // Pre-instrumentation rows: no host, proxy's address — legacy, never + // counted as "the fix is not live". + insertAnalytics({ + path: NOOK_DOWNLOAD_PATH, + stage: null, + host: null, ip_address: "127.0.0.1", user_agent: "curl/8.7.1", created_at: THIS_WEEK }); insertAnalytics({ - path: NOOK_DOWNLOAD_PATH, + path: "/", + host: null, ip_address: "::1", - user_agent: "The Nook/0.13.4 Sparkle/2.9.6", created_at: THIS_WEEK }); - insertAnalytics({ - path: NOOK_DOWNLOAD_PATH, - ip_address: "203.0.113.9", - user_agent: "Mozilla/5.0 (Macintosh) Safari/605.1.15", - created_at: THIS_WEEK - }); - insertAnalytics({ path: "/", ip_address: "203.0.113.9", created_at: THIS_WEEK }); const traffic = await readTraffic(conns, windowsFor(NOW, 7).current); expect(traffic).toEqual({ - analyticsRows: 4, - loopbackRows: 2, - allTimeRows: 4, - allTimeAddressedRows: 4, - allTimeLoopbackRows: 2, + analyticsRows: 5, + hostRows: 3, + legacyRows: 2, + hostLoopbackRows: 1, downloadRows: 3, - downloadLoopbackRows: 2, + downloadClickRows: 1, + downloadServedRows: 1, + downloadUnstagedRows: 1, downloadNonBrowserRows: 2 }); }); @@ -288,10 +322,53 @@ describe("readCampaigns", () => { }); }); +describe("ensureAnalyticsSchema", () => { + it("adds the post-hoc columns to a table that predates them, idempotently", async () => { + // The table as production held it before any post-hoc column existed. + // (`created_at` is not in the insert column list — it defaults DB-side — + // so the simulated table has to name it explicitly.) + const oldDb = new Database(":memory:"); + const withoutAdded = [ + ...VISITOR_ANALYTICS_COLUMNS.filter( + (c) => !VISITOR_ANALYTICS_ADDED_COLUMNS.some((a) => a.name === c) + ), + "created_at" + ]; + oldDb.run( + `CREATE TABLE VisitorAnalytics (${withoutAdded.map((c) => `${c} TEXT`).join(", ")})` + ); + const oldConn = adapter(oldDb); + + await ensureAnalyticsSchema(oldConn); + await ensureAnalyticsSchema(oldConn); // a second run is a no-op + + // The writers' full INSERT — stage and host included — now works. + const values = row({ stage: "click", path: NOOK_DOWNLOAD_PATH }); + expect(() => + oldDb + .prepare(VISITOR_ANALYTICS_INSERT) + .run( + ...(VISITOR_ANALYTICS_COLUMNS.map((c) => values[c]) as never[]) + ) + ).not.toThrow(); + oldDb + .prepare(`UPDATE VisitorAnalytics SET created_at = ? WHERE id = ?`) + .run(THIS_WEEK, values.id as string); + + // And the funnel reads its stage split from the migrated table. + const funnel = await readFunnel( + { analytics: oldConn, nook: conns.nook }, + windowsFor(NOW, 7).current + ); + expect(funnel.downloadClick).toBe(1); + }); +}); + describe("pickMover", () => { const base: FunnelCounts = { visit: 0, - download: 0, + downloadClick: 0, + dmgServed: 0, trial: 0, checkout: 0, purchase: 0, @@ -300,12 +377,12 @@ describe("pickMover", () => { it("picks the stage with the largest absolute change", () => { const mover = pickMover( - { ...base, visit: 61, download: 156, trial: 2 }, + { ...base, visit: 61, dmgServed: 156, trial: 2 }, { ...base, visit: 56 } ); expect(mover).toEqual({ - key: "download", - label: "download click → DMG served (one label)", + key: "dmgServed", + label: "DMG served", now: 156, before: 0, delta: 156 @@ -314,8 +391,8 @@ describe("pickMover", () => { it("breaks ties toward the earlier stage", () => { const mover = pickMover( - { ...base, visit: 5, download: 5 }, - { ...base, visit: 0, download: 0 } + { ...base, visit: 5, downloadClick: 5 }, + { ...base, visit: 0, downloadClick: 0 } ); expect(mover?.key).toBe("visit"); }); @@ -326,20 +403,20 @@ describe("pickMover", () => { }); describe("getWeeklyReport / renderWeeklyMarkdown", () => { - it("reports a week in the agreed stage order with the computed caveats", async () => { + it("renders seven distinct stages with no merged-label or proxy caveats", async () => { insertAnalytics({ path: "/", created_at: THIS_WEEK }); + insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "click", created_at: THIS_WEEK }); insertAnalytics({ path: NOOK_DOWNLOAD_PATH, - ip_address: "127.0.0.1", - user_agent: "curl/8.7.1", + stage: "served", created_at: THIS_WEEK }); insertAnalytics({ path: NOOK_DOWNLOAD_PATH, - ip_address: "127.0.0.1", - user_agent: "curl/8.7.1", + stage: "served", created_at: THIS_WEEK }); + insertAnalytics({ path: NOOK_CHECKOUT_PATH, method: "POST", created_at: THIS_WEEK }); nookDb .prepare(`INSERT INTO trials (fingerprint, started_at) VALUES (?, ?)`) .run("fp-1", "2026-10-28T20:28:46.806Z"); @@ -347,21 +424,72 @@ describe("getWeeklyReport / renderWeeklyMarkdown", () => { const report = await getWeeklyReport(conns, { now: NOW, days: 7 }); const markdown = renderWeeklyMarkdown(report); - expect(report.mover?.key).toBe("download"); expect(markdown).toContain( "Window: **2026-10-22 16:00:00 → 2026-10-29 16:00:00 UTC** (7 days)" ); - expect(markdown).toContain("| visit (page views) | 1 | 0 | +1 |"); - expect(markdown.indexOf("visit (page views)")).toBeLessThan( - markdown.indexOf("download click → DMG served") + expect(markdown).toContain(`| visit (${NOOK_HOST} page views) | 1 | 0 | +1 |`); + expect(markdown).toContain("| download click | 1 | 0 | +1 |"); + expect(markdown).toContain("| DMG served | 2 | 0 | +2 |"); + expect(markdown).toContain("| checkout started (Stripe session) | 1 | 0 | +1 |"); + + const order = [ + `visit (${NOOK_HOST} page views)`, + "download click", + "DMG served", + "trial started", + "checkout started (Stripe session)", + "purchase (licence issued)", + "activation" + ]; + for (let i = 1; i < order.length; i += 1) { + expect(markdown.indexOf(order[i - 1]!)).toBeLessThan( + markdown.indexOf(order[i]!) + ); + } + + // The caveats this issue retired stay retired. + expect(markdown).not.toContain("one label"); + expect(markdown).not.toContain("not recorded"); + expect(markdown).not.toContain("no host dimension"); + expect(markdown).not.toContain("†"); + + expect(report.mover?.key).toBe("dmgServed"); + expect(markdown).toContain("**DMG served: 0 → 2 (+2).**"); + }); + + it("states what pre-instrumentation rows cannot say, computed from the window", async () => { + insertAnalytics({ path: "/", created_at: THIS_WEEK }); + // Legacy rows: no host, proxy's address, no stage label. + insertAnalytics({ path: "/", host: null, ip_address: "127.0.0.1", created_at: THIS_WEEK }); + insertAnalytics({ + path: NOOK_DOWNLOAD_PATH, + stage: null, + host: null, + ip_address: "127.0.0.1", + user_agent: "curl/8.7.1", + created_at: THIS_WEEK + }); + + const report = await getWeeklyReport(conns, { now: NOW, days: 7 }); + const markdown = renderWeeklyMarkdown(report); + + expect(markdown).toContain( + "2 of 3 rows in the window predate the FRE-366 instrumentation" ); - expect(markdown.indexOf("download click → DMG served")).toBeLessThan( - markdown.indexOf("trial started") + expect(markdown).toContain("predate the click/served stage label"); + // Legacy loopback is not "the fix is not live". + expect(markdown).not.toContain("still carry a loopback address"); + }); + + it("flags loopback only when rows recorded since the instrumentation carry it", async () => { + insertAnalytics({ path: "/", ip_address: "127.0.0.1", created_at: THIS_WEEK }); + + const report = await getWeeklyReport(conns, { now: NOW, days: 7 }); + const markdown = renderWeeklyMarkdown(report); + + expect(markdown).toContain( + "1 rows recorded since the instrumentation still carry a loopback address" ); - expect(markdown).toContain("**download click → DMG served (one label): 0 → 2 (+2).**"); - expect(markdown).toContain("2 of 3 rows in the window came from a loopback address"); - expect(markdown).toContain("Checkout start is not recorded"); - expect(markdown).toContain("There is no host dimension"); }); it("says nothing moved rather than inventing a winner", async () => { diff --git a/src/server/weekly-metrics.ts b/src/server/weekly-metrics.ts index 7ab1b72..25f1130 100644 --- a/src/server/weekly-metrics.ts +++ b/src/server/weekly-metrics.ts @@ -18,20 +18,33 @@ * whose `.env` carries nothing else. The SQL here is the same shape as the * analytics summaries, so the weekly read and the dashboard cannot drift * into different label sets. - * - Every stage carries what it actually measured. Two of the seven agreed - * stages share one label in production today and a third is only a page-view - * proxy; the report states that next to the number instead of inventing a - * figure (see `renderWeeklyMarkdown`). + * - Every stage is measured by its own label. FRE-366 gave the funnel that: + * the download door and the DMG file route record distinct `stage` values + * (`click` / `served`), a created Stripe checkout session records its own + * row, and every row carries the real client address and the host it + * addressed, so the visit stage scopes to the Nook. Rows that predate + * that instrumentation are stated next to the numbers, never silently + * counted (see `renderWeeklyMarkdown`). * * No `~/env`, no `~/server/database`: the caller owns the connections, the * tests own an in-memory SQLite, and the script owns the libsql clients. */ import { DOWNLOAD_PATH_PREFIX } from "~/lib/download-asset"; +import { + VISITOR_ANALYTICS_ADDED_COLUMNS, + VISITOR_ANALYTICS_DDL +} from "~/server/analytics-schema"; /** The one asset key the Nook lands under — `downloadPath("thenook")`. */ export const NOOK_DOWNLOAD_PATH = `${DOWNLOAD_PATH_PREFIX}thenook`; +/** The host the visit stage scopes to. Recorded per row since FRE-366. */ +export const NOOK_HOST = "nook.freno.me"; + +/** The path a created Stripe checkout session records (FRE-366). */ +export const NOOK_CHECKOUT_PATH = "/api/the-nook/checkout"; + /** An `execute`-shaped connection: the libsql client and the test adapter both fit. */ export interface MetricsConn { execute(query: { @@ -49,7 +62,8 @@ export interface Connections { export type StageKey = | "visit" - | "download" + | "downloadClick" + | "dmgServed" | "trial" | "checkout" | "purchase" @@ -58,35 +72,28 @@ export type StageKey = export interface StageDef { key: StageKey; label: string; - /** True when the number is a proxy or a merged label, not a clean stage. */ - approximate: boolean; } /** - * The seven agreed stages. `download` deliberately carries two of them: the - * door and the file route both write `/api/downloads/thenook`, so a click and - * the DMG it fetched are one label in the data (proved in the report's caveats). + * The seven agreed stages, each with its own label in the data: + * `downloadClick` and `dmgServed` split what used to be one merged download + * label (the `stage` column, FRE-366), and `checkout` reads created Stripe + * sessions instead of the /checkout page-view proxy. */ export const STAGES: StageDef[] = [ - { key: "visit", label: "visit (page views)", approximate: false }, - { - key: "download", - label: "download click → DMG served (one label)", - approximate: true - }, - { key: "trial", label: "trial started", approximate: false }, - { - key: "checkout", - label: "checkout page visit", - approximate: true - }, - { key: "purchase", label: "purchase (licence issued)", approximate: false }, - { key: "activation", label: "activation", approximate: false } + { key: "visit", label: `visit (${NOOK_HOST} page views)` }, + { key: "downloadClick", label: "download click" }, + { key: "dmgServed", label: "DMG served" }, + { key: "trial", label: "trial started" }, + { key: "checkout", label: "checkout started (Stripe session)" }, + { key: "purchase", label: "purchase (licence issued)" }, + { key: "activation", label: "activation" } ]; export interface FunnelCounts { visit: number; - download: number; + downloadClick: number; + dmgServed: number; trial: number; checkout: number; purchase: number; @@ -103,13 +110,17 @@ export interface CampaignRow { /** Traffic-shape facts the report has to state next to the funnel. */ export interface WindowTraffic { analyticsRows: number; - loopbackRows: number; - /** All-time row counts, so "every row ever" is a checked claim, not a guess. */ - allTimeRows: number; - allTimeAddressedRows: number; - allTimeLoopbackRows: number; + /** Rows carrying a host — written since the FRE-366 instrumentation. */ + hostRows: number; + /** Rows with no host — pre-FRE-366; their address is the proxy's loopback. */ + legacyRows: number; + /** Post-instrumentation rows still recording loopback: the fix is not live. */ + hostLoopbackRows: number; downloadRows: number; - downloadLoopbackRows: number; + downloadClickRows: number; + downloadServedRows: number; + /** Download rows with no stage label — they predate FRE-366 and cannot be split. */ + downloadUnstagedRows: number; downloadNonBrowserRows: number; } @@ -183,6 +194,28 @@ async function scalar( return toNumber(Object.values(row)[0]); } +/** + * Bring the analytics table up to the schema the readers expect, through the + * same idempotent bootstrap path the app server uses (`analytics.ts`): the + * DDL is a no-op against the live table and each post-hoc column is ALTERed + * in only when absent. The weekly job runs against databases the app may not + * have migrated yet — a fresh checkout, a restored backup, or production + * before the next deploy — and must render its seven stages regardless. + */ +export async function ensureAnalyticsSchema(conn: MetricsConn): Promise { + await conn.execute({ sql: VISITOR_ANALYTICS_DDL }); + + const info = await conn.execute({ + sql: `PRAGMA table_info(VisitorAnalytics)` + }); + const present = new Set(info.rows.map((row) => String((row as any).name))); + for (const column of VISITOR_ANALYTICS_ADDED_COLUMNS) { + if (!present.has(column.name)) { + await conn.execute({ sql: column.ddl }); + } + } +} + async function countInWindow( conn: MetricsConn, sql: string, @@ -200,30 +233,43 @@ export async function readFunnel( const { analytics, nook } = conns; const { start, end } = window; - const visit = await countInWindow( + // Scoped to the Nook host (FRE-366): before the host column existed, a + // page view of `/` on nook.freno.me was the same row as freno.me's root. + // Pre-instrumentation rows carry no host and are excluded — the traffic + // section states how many. + const visit = await scalar( analytics, `SELECT COUNT(*) AS n FROM VisitorAnalytics WHERE created_at >= ? AND created_at < ? - AND path NOT LIKE '/api/%'`, - start, - end + AND path NOT LIKE '/api/%' + AND host = ?`, + [start, end, NOOK_HOST] ); - const download = await scalar( + const downloadClick = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? + AND path = ? AND stage = 'click'`, + [start, end, NOOK_DOWNLOAD_PATH] + ); + + const dmgServed = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? + AND path = ? AND stage = 'served'`, + [start, end, NOOK_DOWNLOAD_PATH] + ); + + // Created Stripe checkout sessions, recorded by the checkout route itself + // (FRE-366) — not the /checkout page view that used to proxy for them. + const checkout = await scalar( analytics, `SELECT COUNT(*) AS n FROM VisitorAnalytics WHERE created_at >= ? AND created_at < ? AND path = ?`, - [start, end, NOOK_DOWNLOAD_PATH] - ); - - const checkout = await countInWindow( - analytics, - `SELECT COUNT(*) AS n FROM VisitorAnalytics - WHERE created_at >= ? AND created_at < ? - AND (path LIKE '/checkout%' OR path LIKE '/nook/checkout%')`, - start, - end + [start, end, NOOK_CHECKOUT_PATH] ); // The licence DB stores ISO-8601 (`2026-10-28T20:28:46.806Z`); the analytics @@ -252,7 +298,8 @@ export async function readFunnel( return { visit, - download, + downloadClick, + dmgServed, trial, checkout, purchase, @@ -261,9 +308,11 @@ export async function readFunnel( } /** - * Traffic shape for one window: how much of it came from this host, and how - * much of the download label was automation. Without these two facts the - * numbers read as demand, which they are not. + * Traffic shape for one window: how much of it predates the FRE-366 + * instrumentation (no host, proxy's address), whether rows recorded since + * still see loopback (which would mean the forwarded-header fix is not + * live), and how the download label splits. Without these facts the numbers + * read as demand, which they are not. */ export async function readTraffic( conns: Connections, @@ -273,25 +322,6 @@ export async function readTraffic( const { start, end } = window; const placeholders = LOOPBACK.map(() => "?").join(", "); - const allTimeRows = await scalar( - analytics, - `SELECT COUNT(*) AS n FROM VisitorAnalytics`, - [] - ); - - const allTimeAddressedRows = await scalar( - analytics, - `SELECT COUNT(*) AS n FROM VisitorAnalytics WHERE ip_address IS NOT NULL`, - [] - ); - - const allTimeLoopbackRows = await scalar( - analytics, - `SELECT COUNT(*) AS n FROM VisitorAnalytics - WHERE ip_address IN (${placeholders})`, - [...LOOPBACK] - ); - const analyticsRows = await countInWindow( analytics, `SELECT COUNT(*) AS n FROM VisitorAnalytics @@ -300,10 +330,27 @@ export async function readTraffic( end ); - const loopbackRows = await scalar( + const hostRows = await scalar( analytics, `SELECT COUNT(*) AS n FROM VisitorAnalytics WHERE created_at >= ? AND created_at < ? + AND host IS NOT NULL`, + [start, end] + ); + + const legacyRows = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? + AND host IS NULL`, + [start, end] + ); + + const hostLoopbackRows = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? + AND host IS NOT NULL AND ip_address IN (${placeholders})`, [start, end, ...LOOPBACK] ); @@ -315,12 +362,25 @@ export async function readTraffic( [start, end, NOOK_DOWNLOAD_PATH] ); - const downloadLoopbackRows = await scalar( + const downloadClickRows = await scalar( analytics, `SELECT COUNT(*) AS n FROM VisitorAnalytics - WHERE created_at >= ? AND created_at < ? AND path = ? - AND ip_address IN (${placeholders})`, - [start, end, NOOK_DOWNLOAD_PATH, ...LOOPBACK] + WHERE created_at >= ? AND created_at < ? AND path = ? AND stage = 'click'`, + [start, end, NOOK_DOWNLOAD_PATH] + ); + + const downloadServedRows = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? AND path = ? AND stage = 'served'`, + [start, end, NOOK_DOWNLOAD_PATH] + ); + + const downloadUnstagedRows = await scalar( + analytics, + `SELECT COUNT(*) AS n FROM VisitorAnalytics + WHERE created_at >= ? AND created_at < ? AND path = ? AND stage IS NULL`, + [start, end, NOOK_DOWNLOAD_PATH] ); // Anything whose UA is not a browser engine: curl, Sparkle's updater, @@ -335,12 +395,13 @@ export async function readTraffic( return { analyticsRows, - loopbackRows, - allTimeRows, - allTimeAddressedRows, - allTimeLoopbackRows, + hostRows, + legacyRows, + hostLoopbackRows, downloadRows, - downloadLoopbackRows, + downloadClickRows, + downloadServedRows, + downloadUnstagedRows, downloadNonBrowserRows }; } @@ -483,8 +544,7 @@ export function renderWeeklyMarkdown(report: WeeklyReport): string { for (const stage of STAGES) { const now = counts[stage.key]; const before = previousCounts[stage.key]; - const label = stage.approximate ? `${stage.label} †` : stage.label; - lines.push(`| ${label} | ${now} | ${before} | ${signed(now - before)} |`); + lines.push(`| ${stage.label} | ${now} | ${before} | ${signed(now - before)} |`); } lines.push(""); @@ -528,58 +588,45 @@ export function renderWeeklyMarkdown(report: WeeklyReport): string { lines.push("## Before acting on these numbers"); lines.push(""); - if (traffic.analyticsRows > 0 && traffic.loopbackRows > 0) { + if (traffic.legacyRows > 0) { lines.push( - `- **${traffic.loopbackRows} of ${traffic.analyticsRows} rows in the window came from a loopback address** ` + - `(127.0.0.1 / ::1)` + - (traffic.allTimeAddressedRows > 0 && - traffic.allTimeAddressedRows === traffic.allTimeLoopbackRows - ? `; every one of the ${traffic.allTimeAddressedRows} addressed rows the table ` + - `has ever held is loopback (${traffic.allTimeRows - traffic.allTimeAddressedRows} carry no address)` - : `; across the table's history, ${traffic.allTimeLoopbackRows} of ` + - `${traffic.allTimeAddressedRows} addressed rows are loopback`) + - `. Either this host is the only audience, or the app sits behind a local proxy ` + - `and \`getRequestIP\` records the proxy, not the visitor. Either way the address ` + - `identifies nobody — "unique visitors" collapses to one host and per-channel ` + - `reach cannot be read from it.` + `- **${traffic.legacyRows} of ${traffic.analyticsRows} rows in the window predate the FRE-366 instrumentation.** ` + + `They carry no host, and their recorded address is the production proxy's loopback ` + + `(127.0.0.1 / ::1) — the writers then recorded the direct peer, not the visitor. ` + + `They are excluded from every host-scoped stage above (visits, and the download ` + + `click/served split), and "unique visitors" over them still collapses to one host. ` + + `Rows recorded since carry the real client address and the site they addressed.` + ); + } + + if (traffic.hostLoopbackRows > 0) { + lines.push( + `- **${traffic.hostLoopbackRows} rows recorded since the instrumentation still carry a ` + + `loopback address** — the forwarded-header fix is not taking effect in production ` + + `(or the proxy is not setting \`x-forwarded-for\`). Do not trust per-channel reach ` + + `until this is zero.` ); } if (traffic.downloadRows > 0) { lines.push( - `- **The download stage is one label, not two.** The landing-door route and the file ` + - `route that serves the DMG both write \`${NOOK_DOWNLOAD_PATH}\`, so ` + - `"download click → DMG served" cannot be split: one served click writes two rows, ` + - `a click that fails to resolve writes one, and a Sparkle update writes one. ` + - `Of this window's ${traffic.downloadRows} download rows, ` + - `${traffic.downloadLoopbackRows} are loopback and ` + - `${traffic.downloadNonBrowserRows} are not browser user agents (curl, Sparkle, ` + - `link-preview crawlers).` + `- **Download rows are automation-heavy.** Of this window's ${traffic.downloadRows} ` + + `download rows, ${traffic.downloadClickRows} are clicks and ` + + `${traffic.downloadServedRows} are served files` + + (traffic.downloadUnstagedRows > 0 + ? `; the remaining ${traffic.downloadUnstagedRows} predate the click/served stage ` + + `label and cannot be split, so they count in neither stage` + : "") + + `. ${traffic.downloadNonBrowserRows} of all ${traffic.downloadRows} are not browser ` + + `user agents (curl, Sparkle, link-preview crawlers).` ); } - lines.push( - `- **Checkout start is not recorded.** \`POST /api/the-nook/checkout\` — the call that ` + - `actually creates the Stripe session — writes nothing to analytics; the only trace is ` + - `the /checkout page view reported above, which is a page, not a started checkout.` - ); - - lines.push( - `- **There is no host dimension.** \`VisitorAnalytics\` records a path, not the site, so ` + - `the Nook's landing page (\`/\` on nook.freno.me) is the same row as freno.me's root. ` + - `"visit" above is site-wide page views, not Nook visits.` - ); - lines.push( `- **No conversion rate before the cohort matures.** Trial→paid belongs to the mature ` + `Day 28–35 read (FRE-197); a ratio from a partial cohort is not a metric.` ); lines.push(""); - - lines.push( - "† Proxy or merged label — see *Before acting on these numbers* above." - ); - lines.push(""); lines.push("## Sources"); lines.push(""); lines.push("- `VisitorAnalytics` (freno.me Turso DB) — visits, downloads, campaigns."); From 1d9287a199f9ef25b10b8c5d0160bfd9be2a98e0 Mon Sep 17 00:00:00 2001 From: Paperclip Agent Date: Thu, 29 Oct 2026 18:05:44 +0000 Subject: [PATCH 2/2] analytics: fix the two FRE-369 findings in the funnel instrumentation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `, , ` 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. --- marketing/metrics/weekly.md | 6 +- src/env/server.ts | 6 + src/routes/api/analytics-retention.ts | 35 +++ src/routes/api/the-nook/download.test.ts | 20 +- src/server/analytics-retention.test.ts | 200 ++++++++++++++++ src/server/analytics-retention.ts | 129 ++++++++++ src/server/analytics.ts | 33 ++- src/server/request-client.test.ts | 157 ++++++++++-- src/server/request-client.ts | 289 ++++++++++++++++++++--- src/server/weekly-metrics.test.ts | 27 ++- src/server/weekly-metrics.ts | 25 +- vercel.json | 1 + 12 files changed, 842 insertions(+), 86 deletions(-) create mode 100644 src/routes/api/analytics-retention.ts create mode 100644 src/server/analytics-retention.test.ts create mode 100644 src/server/analytics-retention.ts diff --git a/marketing/metrics/weekly.md b/marketing/metrics/weekly.md index 12b1f47..03c41f0 100644 --- a/marketing/metrics/weekly.md +++ b/marketing/metrics/weekly.md @@ -1,6 +1,6 @@ # Weekly funnel snapshot -Window: **2026-10-22 17:01:33 → 2026-10-29 17:01:33 UTC** (7 days). Previous: 2026-10-15 17:01:33 → 2026-10-22 17:01:33 UTC. +Window: **2026-10-22 18:05:00 → 2026-10-29 18:05:00 UTC** (7 days). Previous: 2026-10-15 18:05:00 → 2026-10-22 18:05:00 UTC. Generated by `bun --env-file=.env scripts/weekly-metrics.ts`; regenerated weekly, never hand-edited. The analysis is the CMO's. @@ -32,7 +32,7 @@ Top 10 by volume, untagged excluded — the same grouping `getAnalyticsSummary.t ## Before acting on these numbers -- **291 of 291 rows in the window predate the FRE-366 instrumentation.** They carry no host, and their recorded address is the production proxy's loopback (127.0.0.1 / ::1) — the writers then recorded the direct peer, not the visitor. They are excluded from every host-scoped stage above (visits, and the download click/served split), and "unique visitors" over them still collapses to one host. Rows recorded since carry the real client address and the site they addressed. +- **291 of 291 rows in the window predate the FRE-366 instrumentation.** They carry no host, and their recorded address is the production proxy's loopback (127.0.0.1 / ::1) — the writers then recorded the direct peer, not the visitor. They are excluded from every host-scoped stage above (visits, and the download click/served split), and "unique visitors" over them still collapses to one host. Rows recorded since carry the site they addressed and the visitor's address truncated to a prefix (/24, or /56 for IPv6 — the FRE-369 privacy floor), so "unique visitors" counts networks, not machines. - **Download rows are automation-heavy.** Of this window's 137 download rows, 0 are clicks and 0 are served files; the remaining 137 predate the click/served stage label and cannot be split, so they count in neither stage. 112 of all 137 are not browser user agents (curl, Sparkle, link-preview crawlers). - **No conversion rate before the cohort matures.** Trial→paid belongs to the mature Day 28–35 read (FRE-197); a ratio from a partial cohort is not a metric. @@ -40,4 +40,4 @@ Top 10 by volume, untagged excluded — the same grouping `getAnalyticsSummary.t - `VisitorAnalytics` (freno.me Turso DB) — visits, downloads, campaigns. - `trials`, `licenses`, `activations` (the-nook Turso DB) — the paid funnel. -- Generated 2026-10-29T17:01:33Z. +- Generated 2026-10-29T18:05:00Z. diff --git a/src/env/server.ts b/src/env/server.ts index f0c9a85..e23561c 100644 --- a/src/env/server.ts +++ b/src/env/server.ts @@ -69,6 +69,12 @@ const serverEnvSchema = z.object({ // Bearer secret for the daily lifecycle-email cron. Falls back to // Vercel's CRON_SECRET; unset + no CRON_SECRET = the endpoint denies all. NOOK_LIFECYCLE_SECRET: z.string().min(1).optional(), + // Daily VisitorAnalytics retention cron (/api/analytics-retention). Same + // shape: falls back to CRON_SECRET, and unset denies every request. + ANALYTICS_RETENTION_SECRET: z.string().min(1).optional(), + // How long a VisitorAnalytics row lives. Unset (or unusable) = 365 days; + // see src/server/analytics-retention.ts before shortening it. + ANALYTICS_RETENTION_DAYS: z.string().optional(), CRON_SECRET: z.string().min(1).optional() }); diff --git a/src/routes/api/analytics-retention.ts b/src/routes/api/analytics-retention.ts new file mode 100644 index 0000000..087547f --- /dev/null +++ b/src/routes/api/analytics-retention.ts @@ -0,0 +1,35 @@ +import type { APIEvent } from "@solidjs/start/server"; +import { env } from "~/env/server"; +import { ConnectionFactory } from "~/server/db-connections"; +import { runRetentionCron } from "~/server/analytics-retention"; + +/** + * GET /api/analytics-retention — daily Vercel cron. + * + * Deletes `VisitorAnalytics` rows older than the retention window, so visitor + * data has a bound instead of accumulating forever (FRE-369, finding Q4 — the + * table's only cleaner used to be a manual admin call, and since FRE-366 the + * rows carry a truncated but still real client address). + * + * Window: `ANALYTICS_RETENTION_DAYS`, defaulting to 365 when unset or unusable + * (see `~/server/analytics-retention` for why the default is conservative + * rather than the reviewer's 90). + * + * Auth: `Authorization: Bearer ` where the secret is + * `ANALYTICS_RETENTION_SECRET` or Vercel's `CRON_SECRET`. An unconfigured secret + * denies every request — an unauthenticated trigger of a bulk delete is the one + * outcome this endpoint must never have. + */ +export async function GET(event: APIEvent) { + const result = await runRetentionCron({ + authorization: event.request.headers.get("authorization"), + secret: env.ANALYTICS_RETENTION_SECRET ?? env.CRON_SECRET, + configuredDays: env.ANALYTICS_RETENTION_DAYS, + conn: ConnectionFactory() + }); + + return new Response(JSON.stringify(result.body), { + status: result.status, + headers: { "Content-Type": "application/json" } + }); +} diff --git a/src/routes/api/the-nook/download.test.ts b/src/routes/api/the-nook/download.test.ts index 026a77f..a396442 100644 --- a/src/routes/api/the-nook/download.test.ts +++ b/src/routes/api/the-nook/download.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, mock, beforeEach } from "bun:test"; +import { truncateIpAddress } from "~/server/request-client"; // The route's contracts: // 1. Every click is recorded through the shared analytics buffer with the @@ -55,9 +56,13 @@ mock.module("~/server/analytics", () => ({ flushAnalytics: async () => { flushCalls += 1; }, - // Mirrors the real enrichment contract: deviceType is derived from the UA. + // Mirrors the real enrichment contract: deviceType is derived from the UA, + // and the address is cut to its prefix by the FRE-369 privacy floor. The + // real `enrichAnalyticsEntry` does both, so the mirror has to apply the same + // truncation — otherwise this file would prove the door stores a full address. enrichAnalyticsEntry: (entry: Entry) => ({ ...entry, + ipAddress: truncateIpAddress(entry.ipAddress as string | null), deviceType: String(entry.userAgent ?? "").includes("Macintosh") ? "desktop" : null @@ -141,9 +146,16 @@ describe("GET /api/the-nook/download", () => { expect(logged[2]!.country).toBe("JP"); }); - it("falls back to the forwarded address when no request IP is available", async () => { - await GET(request({ "x-forwarded-for": "198.51.100.9, 10.0.0.1" })); - expect(logged[0]!.ipAddress).toBe("198.51.100.9"); + it("records the forwarded visitor below the trusted edge, truncated", async () => { + // Client → Cloudflare → origin proxy: the sender's spoof is first, the real + // client second, the edge last. And the stored value is the /24, never the + // address itself (FRE-369). + await GET( + request({ + "x-forwarded-for": "198.51.100.9, 203.0.113.7, 198.51.100.1" + }) + ); + expect(logged[0]!.ipAddress).toBe("203.0.113.0"); }); it("still logs the click when the appcast cannot be read", async () => { diff --git a/src/server/analytics-retention.test.ts b/src/server/analytics-retention.test.ts new file mode 100644 index 0000000..7fbdac1 --- /dev/null +++ b/src/server/analytics-retention.test.ts @@ -0,0 +1,200 @@ +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(); + }); +}); diff --git a/src/server/analytics-retention.ts b/src/server/analytics-retention.ts new file mode 100644 index 0000000..cef1f58 --- /dev/null +++ b/src/server/analytics-retention.ts @@ -0,0 +1,129 @@ +/** + * 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 { + 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; +} + +/** + * 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 { + 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 } }; + } +} diff --git a/src/server/analytics.ts b/src/server/analytics.ts index b37cef0..9eda92d 100644 --- a/src/server/analytics.ts +++ b/src/server/analytics.ts @@ -9,6 +9,8 @@ import { VISITOR_ANALYTICS_INDEXES, VISITOR_ANALYTICS_INSERT } from "./analytics-schema"; +import { truncateIpAddress } from "./request-client"; +import { pruneVisitorAnalytics } from "./analytics-retention"; /** * Idempotent schema bootstrap, mirroring `nook.ts`. The table had no DDL in @@ -317,6 +319,10 @@ export async function getAnalyticsSummary(days: number = 30): Promise<{ }); const totalApiCalls = (totalApiCallsResult.rows[0]?.count as number) || 0; + // Since the FRE-369 privacy floor, `ip_address` holds a truncated prefix + // (/24 for v4, /56 for v6), so this counts distinct prefixes — households and + // mobile exits, not machines. It was never accurate before that either: every + // row held the proxy's loopback, so the count was 1. const uniqueVisitorsResult = await conn.execute({ sql: `SELECT COUNT(DISTINCT ip_address) as count FROM VisitorAnalytics WHERE ip_address IS NOT NULL @@ -628,19 +634,15 @@ export async function getPerformanceStats(days: number = 30): Promise<{ }; } +/** + * Delete visitor analytics older than `olderThanDays`. Reachable from tRPC + * (admin) and from the daily retention cron (`/api/analytics/retention`); + * the SQL and the input rules live in `./analytics-retention`. + */ export async function cleanupOldAnalytics( olderThanDays: number ): Promise { - const conn = ConnectionFactory(); - - const result = await conn.execute({ - sql: `DELETE FROM VisitorAnalytics - WHERE created_at < datetime('now', '-${olderThanDays} days') - RETURNING id`, - args: [] - }); - - return result.rows.length; + return pruneVisitorAnalytics(ConnectionFactory(), olderThanDays); } function parseUserAgent(userAgent?: string): { @@ -675,6 +677,16 @@ function parseUserAgent(userAgent?: string): { return { deviceType, browser, os }; } +/** + * Fill in what the writer did not supply, and apply the privacy floor. + * + * Every analytics entry — page views, tRPC context, downloads, checkout — + * passes through here, which is why the truncation lives here and not at the + * call sites: a future writer cannot forget it, and no caller can persist a + * full client address by accident (FRE-369, finding Q4). The audit log + * (`./audit.ts`) is a separate, pre-existing surface with its own stakes and is + * deliberately untouched by this change. + */ export function enrichAnalyticsEntry(entry: AnalyticsEntry): AnalyticsEntry { const { deviceType, browser, os } = parseUserAgent( entry.userAgent || undefined @@ -682,6 +694,7 @@ export function enrichAnalyticsEntry(entry: AnalyticsEntry): AnalyticsEntry { return { ...entry, + ipAddress: truncateIpAddress(entry.ipAddress), deviceType: entry.deviceType || deviceType, browser: entry.browser || browser, os: entry.os || os diff --git a/src/server/request-client.test.ts b/src/server/request-client.test.ts index fc40bb8..012e4c2 100644 --- a/src/server/request-client.test.ts +++ b/src/server/request-client.test.ts @@ -1,19 +1,30 @@ import { beforeEach, describe, expect, it, mock } from "bun:test"; -// The trust boundary of FRE-366: x-forwarded-for is client-controlled text, -// believed only when the direct peer is the loopback production proxy. These -// tests pin that boundary — a regression here either re-records the proxy as -// the visitor (unique visitors collapses to one host) or lets any client -// spoof its address. +// The trust boundary of FRE-366, as corrected by the FRE-369 security review. +// Two things must stay true here: +// 1. The address we record is the visitor, not whoever typed a header first. +// Cloudflare and the origin proxy both *append* to x-forwarded-for, so the +// leftmost hop is the client's own invention — `cf-connecting-ip` (which +// Cloudflare overwrites) comes first, then the hop below the trusted edge. +// 2. What reaches the database is a prefix, never a full address +// (`truncateIpAddress`), because a visitor row is not worth a precise +// location. +// A regression on (1) re-records whatever an attacker claims; on (2) it stores +// personal data we promised not to. let directIp: string | null = null; mock.module("vinxi/http", () => ({ getRequestIP: () => directIp })); -const { resolveClientIp, isLoopbackAddress, requestHost } = await import( - "./request-client" -); +const { + resolveClientIp, + isLoopbackAddress, + isIpAddress, + truncateIpAddress, + normaliseHost, + requestHost +} = await import("./request-client"); function event( headers: Record = {}, @@ -30,26 +41,68 @@ beforeEach(() => { }); describe("resolveClientIp", () => { - it("takes the first forwarded hop when the direct peer is the loopback proxy", () => { + it("prefers Cloudflare's connecting address over any forwarded header", () => { directIp = "127.0.0.1"; expect( - resolveClientIp(event({ "x-forwarded-for": "198.51.100.9, 10.0.0.1" })) - ).toBe("198.51.100.9"); + resolveClientIp( + event({ + "cf-connecting-ip": "203.0.113.7", + "x-forwarded-for": "198.51.100.9, 203.0.113.7, 198.51.100.1" + }) + ) + ).toBe("203.0.113.7"); + }); + it("takes the hop below the trusted edge — never the leftmost one", () => { + // client → Cloudflare → same-host origin proxy: the sender's spoof is + // first, the real client second, the edge last. directIp = "::1"; expect( - resolveClientIp(event({ "x-forwarded-for": "198.51.100.10" })) - ).toBe("198.51.100.10"); + resolveClientIp( + event({ + "x-forwarded-for": "198.51.100.9, 203.0.113.7, 198.51.100.1" + }) + ) + ).toBe("203.0.113.7"); + }); + + it("refuses a single-hop forwarded header: nothing above it to discount", () => { + directIp = "127.0.0.1"; + expect( + resolveClientIp(event({ "x-forwarded-for": "198.51.100.9" })) + ).toBe("127.0.0.1"); }); it("ignores a forwarded header from a real peer — it is spoofable", () => { directIp = "203.0.113.7"; expect( - resolveClientIp(event({ "x-forwarded-for": "198.51.100.9" })) + resolveClientIp( + event({ + "cf-connecting-ip": "198.51.100.9", + "x-forwarded-for": "198.51.100.9, 10.0.0.1" + }) + ) ).toBe("203.0.113.7"); }); - it("keeps the loopback peer when the proxy set no forwarded header", () => { + it("ignores a header value that is not an address", () => { + directIp = "127.0.0.1"; + expect( + resolveClientIp( + event({ + "cf-connecting-ip": "not-an-address", + "x-forwarded-for": "198.51.100.9, 203.0.113.7, 198.51.100.1" + }) + ) + ).toBe("203.0.113.7"); + + directIp = "127.0.0.1"; + expect( + resolveClientIp(event({ "cf-connecting-ip": "evil.example.com" })) + ).toBe("127.0.0.1"); + }); + + it("keeps the loopback peer when the proxy set nothing usable", () => { directIp = "::ffff:127.0.0.1"; expect(resolveClientIp(event())).toBe("::ffff:127.0.0.1"); }); @@ -60,13 +113,79 @@ describe("resolveClientIp", () => { }); describe("isLoopbackAddress", () => { - it("covers the v4 range, the shorthand and the v6 forms", () => { + it("covers the v4 range and every spelling of v6 loopback", () => { expect(isLoopbackAddress("127.0.0.1")).toBe(true); expect(isLoopbackAddress("127.0.0.53")).toBe(true); expect(isLoopbackAddress("::1")).toBe(true); + expect(isLoopbackAddress("0:0:0:0:0:0:0:1")).toBe(true); expect(isLoopbackAddress("::ffff:127.0.0.1")).toBe(true); + expect(isLoopbackAddress("::ffff:127.9.9.9")).toBe(true); + }); + + it("is not loopback for a real peer, or for text it cannot parse", () => { expect(isLoopbackAddress("203.0.113.7")).toBe(false); expect(isLoopbackAddress("10.0.0.1")).toBe(false); + expect(isLoopbackAddress("localhost")).toBe(false); + expect(isLoopbackAddress("127.0.53")).toBe(false); // not a complete address + expect(isLoopbackAddress("")).toBe(false); + }); +}); + +describe("isIpAddress", () => { + it("accepts a dotted quad and an IPv6 literal", () => { + expect(isIpAddress("198.51.100.9")).toBe(true); + expect(isIpAddress("2001:db8::1")).toBe(true); + expect(isIpAddress("::ffff:198.51.100.9")).toBe(true); + }); + + it("rejects anything a proxy could not have meant", () => { + expect(isIpAddress("999.1.1.1")).toBe(false); + expect(isIpAddress("1.2.3")).toBe(false); + expect(isIpAddress("2001:db8:::1")).toBe(false); + expect(isIpAddress("evil.example.com")).toBe(false); + expect(isIpAddress("")).toBe(false); + expect(isIpAddress(null)).toBe(false); + }); +}); + +describe("truncateIpAddress", () => { + it("cuts IPv4 to its /24", () => { + expect(truncateIpAddress("198.51.100.9")).toBe("198.51.100.0"); + expect(truncateIpAddress("127.0.0.1")).toBe("127.0.0.0"); + }); + + it("cuts IPv6 to its /56, uncompressed and stable", () => { + expect( + truncateIpAddress("2001:db8:85a3:1234:5678:9abc:defa:1234") + ).toBe("2001:db8:85a3:1200:0:0:0:0"); + // The compressed spelling of the same address truncates to the same string, + // so `COUNT(DISTINCT ip_address)` cannot be split by how a proxy rendered it. + expect(truncateIpAddress("2001:db8:85a3:1234::")).toBe( + "2001:db8:85a3:1200:0:0:0:0" + ); + expect(truncateIpAddress("::1")).toBe("0:0:0:0:0:0:0:0"); + }); + + it("treats an IPv4-mapped v6 address as the v4 it is", () => { + expect(truncateIpAddress("::ffff:198.51.100.9")).toBe("198.51.100.0"); + }); + + it("drops what it cannot parse rather than storing it", () => { + expect(truncateIpAddress("203.0.113.999")).toBeNull(); + expect(truncateIpAddress("")).toBeNull(); + expect(truncateIpAddress("")).toBeNull(); + expect(truncateIpAddress(null)).toBeNull(); + expect(truncateIpAddress(undefined)).toBeNull(); + }); +}); + +describe("normaliseHost", () => { + it("lowercases and strips the port so the weekly host scope can match", () => { + expect(normaliseHost("Nook.Freno.ME:443")).toBe("nook.freno.me"); + expect(normaliseHost("nook.freno.me.")).toBe("nook.freno.me"); + expect(normaliseHost("[::1]:3000")).toBe("::1"); + expect(normaliseHost(" ")).toBeNull(); + expect(normaliseHost(null)).toBeNull(); }); }); @@ -75,4 +194,10 @@ describe("requestHost", () => { expect(requestHost(event())).toBe("nook.freno.me"); expect(requestHost(event({}, "https://freno.me/"))).toBe("freno.me"); }); + + it("normalises a host that carries a port", () => { + expect(requestHost(event({}, "https://nook.freno.me:443/downloads"))).toBe( + "nook.freno.me" + ); + }); }); diff --git a/src/server/request-client.ts b/src/server/request-client.ts index 2d79080..41ea96c 100644 --- a/src/server/request-client.ts +++ b/src/server/request-client.ts @@ -3,65 +3,280 @@ import { getRequestIP } from "vinxi/http"; import { hostFromEventLike } from "./site-context-server"; /** - * The real client address behind the production proxy (FRE-366). + * The client address and the site a request addressed, for the analytics + * writers (FRE-366). * - * Until this existed every analytics writer called `getRequestIP` and only - * looked at `x-forwarded-for` when it returned null — so behind the - * production proxy the recorded address was the proxy itself, and all - * 12,035 addressed rows in VisitorAnalytics carried 127.0.0.1 / ::1. + * Until this existed every writer called `getRequestIP` and only looked at + * `x-forwarded-for` when it returned null — so behind the production proxy the + * recorded address was the proxy itself, and all 12,035 addressed rows in + * VisitorAnalytics carried 127.0.0.1 / ::1. * - * Trust boundary, stated plainly: `x-forwarded-for` is client-controlled - * text. It is believed only when the direct peer is a loopback address — - * the production shape, where a same-host proxy (nginx/caddy) terminates - * TLS and sets the header itself. Anywhere else the direct peer IS the - * client (dev, direct hit), and a forwarded header from it is ignored as - * spoofable. The first hop of the header is taken; later hops are the - * client's own claims. + * STANDING RULE (FRE-369, the security review of that change): nothing here is + * a security signal. The address is a *best guess at a visitor*, not an + * authenticated identity — do not import it into auth, rate limiting, abuse + * control, fraud checks, or anything that grants or denies access. A caller + * that needs a security decision must re-derive trust for its own stakes; the + * blast radius of a wrong answer here is a poisoned analytics row, and that is + * the only reason the shortcuts below are acceptable. + * + * Trust boundary, stated plainly: `x-forwarded-for` is client-controlled text. + * It is believed only when the direct peer is a loopback address — the + * production shape, where a same-host proxy (nginx/caddy) terminates TLS and + * sets the header itself. Anywhere else the direct peer IS the client (dev, + * direct hit) and a forwarded header from it is ignored as spoofable. + * + * Which hop is the visitor (FRE-369 finding Q1): the request arrives as + * `client → Cloudflare → same-host origin proxy → app`, and both proxies + * *append*. So `x-forwarded-for` reaches us as + * `, , ` and the leftmost + * hop is exactly the one the sender chose — the original FRE-366 read took it + * and was wrong. Order of preference: + * + * 1. `cf-connecting-ip` — Cloudflare *overwrites* this header, so a + * client-supplied value never survives the edge. Authoritative. + * 2. `x-forwarded-for`, counted from the right: drop the rightmost hop (the + * Cloudflare edge address the origin proxy appended) and take the one + * before it. Never the leftmost. + * 3. the direct peer, as before. + * + * h3's own `getRequestIP(event, { xForwardedFor: true })` also takes the + * leftmost hop, so it is not a fix for this and must not be swapped in. + * + * Residual caveat, deliberately not papered over: step 2 assumes the pinned + * topology (one trusted proxy above us, i.e. Cloudflare). Behind a *single* + * forwarding proxy that does not set `cf-connecting-ip`, a client that sent its + * own `x-forwarded-for` sits at the hop we read. That is why the value is only + * ever stored truncated (see `truncateIpAddress`) and why it stays out of every + * security path. Deploy-side precondition this repo cannot verify: the origin + * must be reachable only from Cloudflare (origin firewall / authenticated + * origin pull). If it is directly reachable, an attacker can set every header + * in this file and no read here can recover the truth. + */ + +/** Cloudflare's "who actually connected" header: overwritten at the edge. */ +const CF_CONNECTING_IP = "cf-connecting-ip"; +const X_FORWARDED_FOR = "x-forwarded-for"; + +/** + * Hops discounted from the right of `x-forwarded-for` before the value is + * taken: the Cloudflare edge address the origin proxy appended. Pinned to 1 + * because there is exactly one trusted proxy between Cloudflare and the app; + * raise it only if the topology gains a trusted hop. + */ +const FORWARDED_HOPS_TO_DISCOUNT = 1; + +function header(event: APIEvent, name: string): string | null { + try { + return event.request?.headers?.get?.(name) ?? null; + } catch { + return null; + } +} + +/** + * The visitor's address, resolved through the trust boundary above. + * + * Never store the result of this directly: `enrichAnalyticsEntry` runs every + * analytics entry through {@link truncateIpAddress} so a full address cannot + * reach the database even from a writer that forgets. Callers that need the + * un-truncated value must justify it in review. */ export function resolveClientIp(event: APIEvent): string | null { const direct = getRequestIP(event.nativeEvent); - if (direct && !isLoopbackAddress(direct)) { - return direct; - } - - const forwarded = event.request?.headers?.get?.("x-forwarded-for"); - const first = forwarded?.split(",")[0]?.trim(); - if (first) { - return first; + // A real peer is its own client: believed, and no forwarded header from it + // is (it would be the peer lying about who it is). + if (direct && !isLoopbackAddress(direct)) return direct; + + // 1. Cloudflare's own record of the connecting address. + const connecting = cleanHop(header(event, CF_CONNECTING_IP)); + if (isIpAddress(connecting)) return connecting; + + // 2. Rightmost-minus-the-trusted-hop, never leftmost. + const hops = (header(event, X_FORWARDED_FOR) ?? "") + .split(",") + .map(cleanHop) + .filter(Boolean); + const index = hops.length - 1 - FORWARDED_HOPS_TO_DISCOUNT; + if (index >= 0) { + const candidate = hops[index]!; + if (isIpAddress(candidate)) return candidate; } + // 3. Nothing trustworthy: record what we actually saw, even though it is the + // proxy. `readTraffic` reports these rows rather than hiding them. return direct ?? null; } -export function isLoopbackAddress(ip: string): boolean { - return ( - ip === "::1" || - ip === "::ffff:127.0.0.1" || - ip === "127.0.0.1" || - ip.startsWith("127.") - ); +function cleanHop(value: string | null | undefined): string { + return (value ?? "").trim(); } /** - * The site the request addressed — the `Host` the client asked for, e.g. - * `nook.freno.me`. Recorded on every analytics row so a weekly read can - * scope to one site; without it `/` on nook.freno.me is the same row as - * `/` on freno.me. Thin alias over the site-context extractor so the - * analytics writers share one host-parsing implementation with routing. + * Whether an address is the loopback the production proxy presents. + * + * Parses rather than string-matches, so the forms a real peer can present in — + * `::ffff:127.0.0.53`, `0:0:0:0:0:0:0:1`, an IPv4-mapped literal — are all + * recognised. An address this does not parse is *not* loopback, which fails + * closed: forwarded headers from an unrecognised peer are ignored rather than + * believed. + */ +export function isLoopbackAddress(ip: string): boolean { + const bytes = addressBytes(ip); + if (!bytes) return false; + if (bytes.length === 4) return bytes[0] === 127; + + // IPv6 loopback ::1 — and ::ffff:127/8, which is loopback too. + const mapped = ipv4FromBytes(bytes); + if (mapped) return isLoopbackAddress(mapped); + const tail = bytes[15]!; + return bytes.slice(0, 15).every((byte) => byte === 0) && tail === 1; +} + +// ── Addresses: parse, validate, truncate ──────────────────────────────────── + +/** + * The privacy floor the security review required before this ships (FRE-369, + * finding Q4): VisitorAnalytics must never hold a full client address. IPv4 is + * cut to /24, IPv6 to /56 — the prefix that identifies a household and that a + * rotating "privacy" IPv6 address does not actually change. Funnel stages count + * rows, and `uniqueVisitors` survives as a count of distinct prefixes, so + * nothing measured here needs the last octet. + * + * An address that does not parse is dropped rather than stored: an unparseable + * string is client text we cannot reason about, and keeping it would smuggle + * the very granularity this function exists to remove. + */ +export function truncateIpAddress(ip: string | null | undefined): string | null { + const value = cleanHop(ip); + if (!value) return null; + + const bytes = addressBytes(value); + if (!bytes) return null; + + const mapped = bytes.length === 16 ? ipv4FromBytes(bytes) : null; + if (bytes.length === 4 || mapped) { + const octets = (mapped ?? value).split(".").map(Number) as number[]; + return `${octets[0]}.${octets[1]}.${octets[2]}.0`; + } + + // /56 = keep the first 7 bytes, zero the rest. Rendered uncompressed and + // lowercase so one client always truncates to one stored string. + const kept = bytes.map((_, i) => (i >= 7 ? 0 : _)); + const groups: string[] = []; + for (let i = 0; i < 16; i += 2) { + groups.push((((kept[i]! << 8) | kept[i + 1]!) >>> 0).toString(16)); + } + return groups.join(":"); +} + +/** Any address this file understands: a dotted quad or an IPv6 literal. */ +export function isIpAddress(value: string | null | undefined): boolean { + return value !== null && value !== undefined && addressBytes(value) !== null; +} + +function isIpv4(value: string): boolean { + const parts = value.split("."); + if (parts.length !== 4) return false; + return parts.every( + (part) => /^\d{1,3}$/.test(part) && Number(part) <= 255 && !/^0\d/.test(part) + ); +} + +/** 4 bytes for IPv4, 16 for IPv6 (including `::ffff:a.b.c.d`), else null. */ +function addressBytes(value: string): number[] | null { + const raw = value.trim().replace(/^\[|\]$/g, "").split("%")[0]!; + if (isIpv4(raw)) return raw.split(".").map(Number); + if (!raw.includes(":")) return null; + + const halves = raw.split("::"); + if (halves.length > 2) return null; + const compressed = halves.length === 2; + + const head = groupsToBytes(halves[0] ?? ""); + const tail = groupsToBytes(halves[1] ?? ""); + if (!head || !tail) return null; + + const total = head.length + tail.length; + if (compressed) { + if (total > 8) return null; + return [...head, ...new Array(8 - total).fill(0), ...tail].flatMap(split16); + } + if (total !== 8) return null; + return [...head, ...tail].flatMap(split16); +} + +/** One side of `::` as 16-bit groups; a trailing `1.2.3.4` counts as two. */ +function groupsToBytes(side: string): number[] | null { + if (side === "") return []; + const out: number[] = []; + for (const part of side.split(":")) { + if (/^[0-9a-fA-F]{1,4}$/.test(part)) { + out.push(parseInt(part, 16)); + } else if (isIpv4(part)) { + const o = part.split(".").map(Number); + out.push((o[0]! << 8) | o[1]!, (o[2]! << 8) | o[3]!); + } else { + return null; + } + } + return out; +} + +function split16(group: number): number[] { + return [(group >> 8) & 0xff, group & 0xff]; +} + +/** The embedded IPv4 of an IPv4-mapped IPv6 address (`::ffff:1.2.3.4`), else null. */ +function ipv4FromBytes(bytes: number[]): string | null { + const prefix = bytes.slice(0, 10).every((byte) => byte === 0); + const marker = bytes[10] === 0xff && bytes[11] === 0xff; + if (!prefix || !marker) return null; + return `${bytes[12]}.${bytes[13]}.${bytes[14]}.${bytes[15]}`; +} + +/** + * The site the request addressed — the `Host` the client asked for, normalised + * to `nook.freno.me`. Recorded on every analytics row so a weekly read can + * scope to one site; without it `/` on nook.freno.me is the same row as `/` on + * freno.me. Thin alias over the site-context extractor so the analytics writers + * share one host-parsing implementation with routing. + * + * Normalised (lowercased, port stripped) because the weekly read scopes with + * `host = ?`: `nook.freno.me:443` or `Nook.Freno.ME` would otherwise store a + * value that silently never matches the scope (FRE-369, finding Q3 — no + * injection risk, but a correctness one worth fixing at write time). */ export function requestHost(event: APIEvent): string | null { - const header = hostFromEventLike(event); - if (header) return header; + return normaliseHost(hostFromEventLike(event)) ?? normaliseHost(urlHost(event)); +} +function urlHost(event: APIEvent): string | null { // `host` is a forbidden header and some server shapes drop it from // `event.request.headers`; the request URL is absolute on the server and // carries the same host the client addressed. try { const url = event.request?.url; - if (url) return new URL(url).host || null; + return url ? new URL(url).host || null : null; } catch { - /* noop */ + return null; } - return null; +} + +/** Lowercase, drop the port and any trailing dot; null when nothing is left. */ +export function normaliseHost(host: string | null | undefined): string | null { + const raw = cleanHop(host).toLowerCase(); + if (!raw) return null; + + // A bracketed IPv6 literal keeps its brackets' contents (`[::1]:3000` → + // `::1`); anything else drops a trailing `:port`. + const closing = raw.indexOf("]"); + const withoutPort = raw.startsWith("[") + ? closing > 0 + ? raw.slice(1, closing) + : raw.slice(1) + : raw.replace(/:\d+$/, ""); + + const trimmed = withoutPort.replace(/\.$/, ""); + return trimmed || null; } diff --git a/src/server/weekly-metrics.test.ts b/src/server/weekly-metrics.test.ts index fa42009..c7e1fd9 100644 --- a/src/server/weekly-metrics.test.ts +++ b/src/server/weekly-metrics.test.ts @@ -85,7 +85,9 @@ const row = ( method: "GET", referrer: null, user_agent: "Mozilla/5.0 (Macintosh)", - ip_address: "203.0.113.9", + // What the writers store since the FRE-369 privacy floor: a /24 (or /56 for + // IPv6), never the address itself. + ip_address: "203.0.113.0", country: "GB", device_type: "desktop", browser: "safari", @@ -227,8 +229,9 @@ describe("readFunnel", () => { describe("readTraffic", () => { it("separates legacy rows, live loopback and automation from clean traffic", async () => { - // Post-instrumentation rows: host set. One of them still loopback — the - // shape the report must flag as "the fix is not live". + // Post-instrumentation rows: host set. Two of them still loopback — in the + // truncated shapes the privacy floor writes — which is the shape the report + // must flag as "the fix is not live". insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "click", @@ -237,10 +240,15 @@ describe("readTraffic", () => { insertAnalytics({ path: NOOK_DOWNLOAD_PATH, stage: "served", - ip_address: "127.0.0.1", + ip_address: "127.0.0.0", user_agent: "The Nook/0.13.4 Sparkle/2.9.6", created_at: THIS_WEEK }); + insertAnalytics({ + path: "/", + ip_address: "0:0:0:0:0:0:0:0", + created_at: THIS_WEEK + }); insertAnalytics({ path: "/", created_at: THIS_WEEK }); // Pre-instrumentation rows: no host, proxy's address — legacy, never @@ -263,10 +271,10 @@ describe("readTraffic", () => { const traffic = await readTraffic(conns, windowsFor(NOW, 7).current); expect(traffic).toEqual({ - analyticsRows: 5, - hostRows: 3, + analyticsRows: 6, + hostRows: 4, legacyRows: 2, - hostLoopbackRows: 1, + hostLoopbackRows: 2, downloadRows: 3, downloadClickRows: 1, downloadServedRows: 1, @@ -482,13 +490,16 @@ describe("getWeeklyReport / renderWeeklyMarkdown", () => { }); it("flags loopback only when rows recorded since the instrumentation carry it", async () => { + // Both spellings a live writer can produce: the raw proxy address, and the + // /24 the privacy floor stores instead of it. insertAnalytics({ path: "/", ip_address: "127.0.0.1", created_at: THIS_WEEK }); + insertAnalytics({ path: "/", ip_address: "127.0.53.0", created_at: THIS_WEEK }); const report = await getWeeklyReport(conns, { now: NOW, days: 7 }); const markdown = renderWeeklyMarkdown(report); expect(markdown).toContain( - "1 rows recorded since the instrumentation still carry a loopback address" + "2 rows recorded since the instrumentation still carry a loopback address" ); }); diff --git a/src/server/weekly-metrics.ts b/src/server/weekly-metrics.ts index 25f1130..d74c592 100644 --- a/src/server/weekly-metrics.ts +++ b/src/server/weekly-metrics.ts @@ -144,8 +144,16 @@ export interface WeeklyReport { generatedAt: string; } -/** Loopback addresses — every row the live table has ever held. */ -const LOOPBACK = ["127.0.0.1", "::1", "::ffff:127.0.0.1"]; +/** + * A recorded address that is not a visitor, in every spelling the column can + * hold: the proxy the request came through (`127.0.0.1`, `::1`, an IPv4-mapped + * form) *and* the prefixes the FRE-369 privacy floor writes instead of them + * (`127.0.0.0`, all-zero IPv6). Whole `127/8` because truncation keeps the first + * three octets, so `127.0.53.1` and `127.0.53.0` are the same non-visitor. + */ +const LOOPBACK_SQL = `(ip_address LIKE '127.%' + OR ip_address LIKE '::ffff:127.%' + OR ip_address IN ('::1', '0:0:0:0:0:0:0:1', '0:0:0:0:0:0:0:0'))`; /** SQLite's `datetime()` string, UTC: `YYYY-MM-DD HH:MM:SS`. */ export function sqlTime(date: Date): string { @@ -320,7 +328,6 @@ export async function readTraffic( ): Promise { const { analytics } = conns; const { start, end } = window; - const placeholders = LOOPBACK.map(() => "?").join(", "); const analyticsRows = await countInWindow( analytics, @@ -351,8 +358,8 @@ export async function readTraffic( `SELECT COUNT(*) AS n FROM VisitorAnalytics WHERE created_at >= ? AND created_at < ? AND host IS NOT NULL - AND ip_address IN (${placeholders})`, - [start, end, ...LOOPBACK] + AND ${LOOPBACK_SQL}`, + [start, end] ); const downloadRows = await scalar( @@ -595,7 +602,9 @@ export function renderWeeklyMarkdown(report: WeeklyReport): string { `(127.0.0.1 / ::1) — the writers then recorded the direct peer, not the visitor. ` + `They are excluded from every host-scoped stage above (visits, and the download ` + `click/served split), and "unique visitors" over them still collapses to one host. ` + - `Rows recorded since carry the real client address and the site they addressed.` + `Rows recorded since carry the site they addressed and the visitor's address ` + + `truncated to a prefix (/24, or /56 for IPv6 — the FRE-369 privacy floor), so ` + + `"unique visitors" counts networks, not machines.` ); } @@ -603,8 +612,8 @@ export function renderWeeklyMarkdown(report: WeeklyReport): string { lines.push( `- **${traffic.hostLoopbackRows} rows recorded since the instrumentation still carry a ` + `loopback address** — the forwarded-header fix is not taking effect in production ` + - `(or the proxy is not setting \`x-forwarded-for\`). Do not trust per-channel reach ` + - `until this is zero.` + `(no \`cf-connecting-ip\`, and no usable \`x-forwarded-for\` hop). Do not trust ` + + `per-channel reach until this is zero.` ); } diff --git a/vercel.json b/vercel.json index 57b9eff..03b5a7f 100644 --- a/vercel.json +++ b/vercel.json @@ -57,6 +57,7 @@ } ], "crons": [ + { "path": "/api/analytics-retention", "schedule": "0 12 * * *" }, { "path": "/api/the-nook/lifecycle", "schedule": "0 13 * * *" } ] }