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.
283 lines
12 KiB
TypeScript
283 lines
12 KiB
TypeScript
import type { APIEvent } from "@solidjs/start/server";
|
|
import { getRequestIP } from "vinxi/http";
|
|
import { hostFromEventLike } from "./site-context-server";
|
|
|
|
/**
|
|
* The client address and the site a request addressed, for the analytics
|
|
* writers (FRE-366).
|
|
*
|
|
* 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.
|
|
*
|
|
* 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
|
|
* `<client-supplied spoof…>, <true client>, <Cloudflare edge>` 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);
|
|
|
|
// 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;
|
|
}
|
|
|
|
function cleanHop(value: string | null | undefined): string {
|
|
return (value ?? "").trim();
|
|
}
|
|
|
|
/**
|
|
* 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 {
|
|
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;
|
|
return url ? new URL(url).host || null : null;
|
|
} catch {
|
|
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;
|
|
}
|