forked from Mike/freno-dev
analytics: fix the two FRE-369 findings in the funnel instrumentation
The security review of 39d874a (FRE-369) found the address fix recorded
the wrong thing twice. Both are fixed here; the branch is the same one
under review, this commit does not rewrite it.
Q1 — the first x-forwarded-for hop is attacker-chosen text. In
client → Cloudflare → same-host proxy → app, both proxies *append*, so
the header arrives as `<client spoof…>, <true client>, <CF edge>` and
`split(",")[0]` took the sender's own invention — the loopback-peer gate
was sound, the hop selection defeated it. Preference is now
`cf-connecting-ip` (Cloudflare overwrites it, so a client value never
survives the edge), then x-forwarded-for counted from the right with one
trusted hop discounted, then the direct peer. Every candidate must parse
as an address. h3's `getRequestIP(…, { xForwardedFor: true })` has the
same leftmost flaw and is documented as not a fix.
Q4 — the table held full client addresses indefinitely. Every analytics
entry now passes through `truncateIpAddress` in `enrichAnalyticsEntry`
(IPv4 /24, IPv6 /56, unparseable dropped), so no writer can persist a
full address by accident. Retention got the schedule it never had:
`/api/analytics/retention` on a daily Vercel cron, Bearer-authenticated
and fail-closed, with the window parameterised (the old
`cleanupOldAnalytics` spliced the number into the SQL). Default 365 days
rather than the review's 90 so enabling the cron is not itself a
destructive event — the tighter window is an operational decision, named
on the task.
Also from the review: the module doc now forbids feeding this helper into
any security decision; `isLoopbackAddress` parses instead of
string-matching (`::ffff:127.9.9.9`, `0:0:0:0:0:0:0:1`); and `host` is
normalised (lowercased, port stripped) so `nook.freno.me:443` cannot
silently miss the `host = ?` scope. The weekly loopback caveat matches
the truncated shapes writers produce now, and says plainly that "unique
visitors" counts prefixes.
This commit is contained in:
35
src/routes/api/analytics-retention.ts
Normal file
35
src/routes/api/analytics-retention.ts
Normal file
@@ -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 <secret>` 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" }
|
||||
});
|
||||
}
|
||||
@@ -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 () => {
|
||||
|
||||
Reference in New Issue
Block a user