diff --git a/changelog.d/fixes/9033-fix.plan.md b/changelog.d/fixes/9033-fix.plan.md new file mode 100644 index 0000000000..b5d8a37ca3 --- /dev/null +++ b/changelog.d/fixes/9033-fix.plan.md @@ -0,0 +1 @@ +- fix(auth): IP blacklist now blocks on direct connections via trusted peer stamp and re-reads config without restart (#9033) diff --git a/open-sse/services/ipFilter.ts b/open-sse/services/ipFilter.ts index c023397891..e54930c5b1 100644 --- a/open-sse/services/ipFilter.ts +++ b/open-sse/services/ipFilter.ts @@ -22,15 +22,16 @@ let _config = { // lazily loaded on first access. better-sqlite3 is synchronous, so both the load // and the save stay in the sync hot path without extra startup wiring. tempBans // are intentionally NOT persisted — they are ephemeral, TTL-swept runtime state. +// +// D2 (#9033): the _loaded one-shot gate was removed so a config persisted by the +// dashboard settings route (a separate module instance, since @omniroute/open-sse +// is bundled per-entry via transpilePackages) propagates to the proxy runtime +// without a restart. A DB failure still degrades to the in-memory defaults, and +// tempBans remain in-memory-only as before. const IP_FILTER_NAMESPACE = "ipFilter"; const IP_FILTER_KEY = "config"; -let _loaded = false; function ensureLoaded() { - if (_loaded) return; - // Mark loaded up-front so a DB failure (build phase / cloud / migration not yet - // run) degrades to in-memory only instead of retrying on every request. - _loaded = true; try { const row = getDbInstance() .prepare("SELECT value FROM key_value WHERE namespace = ? AND key = ?") @@ -235,9 +236,17 @@ export function createIPFilterMiddleware() { /** * For Next.js App Router — check IP from request object + * + * D1 (#9033): accepts an optional trustedPeerIp (resolved from the authenticated + * peer stamp, available on direct connections where the proxy runtime has no + * socket). When provided, it is checked FIRST before falling through to the + * forwarding headers, so a blacklisted IP on a direct connection (no XFF, no + * socket) is blocked. When behind a reverse proxy (via-proxy marker set), the + * caller passes null so the XFF path continues to work. */ -export function checkRequestIP(request) { +export function checkRequestIP(request, trustedPeerIp) { const ip = + pickFirstValidIp(trustedPeerIp || null) || pickFirstValidIp(request.headers?.get?.("cf-connecting-ip")) || pickFirstValidIp(request.headers?.get?.("x-forwarded-for")) || pickFirstValidIp(request.headers?.get?.("x-real-ip")) || @@ -329,7 +338,6 @@ function extractClientIP(req) { * Reset config (for testing) */ export function resetIPFilter() { - _loaded = false; _config = { enabled: false, mode: "blacklist", diff --git a/src/server/authz/pipeline.ts b/src/server/authz/pipeline.ts index b440bad49f..f2619a189b 100644 --- a/src/server/authz/pipeline.ts +++ b/src/server/authz/pipeline.ts @@ -8,7 +8,11 @@ import { applyCorsHeaders } from "../cors/origins"; import { validateBrowserMutationOrigin } from "../origin/publicOrigin"; import { classifyRoute } from "./classify"; import { validateDashboardCsrfToken } from "./csrf"; -import { classifyStampedPeerLocality } from "./peerStamp"; +import { + classifyStampedPeerLocality, + resolveStampedPeer, + resolveStampedViaProxy, +} from "./peerStamp"; import { checkRequestIP } from "@omniroute/open-sse/services/ipFilter.ts"; import { clientApiPolicy } from "./policies/clientApi"; import { managementPolicy } from "./policies/management"; @@ -347,8 +351,23 @@ export async function runAuthzPipeline( // external surface. Loopback is exempt so the local operator can never lock // themselves out of the dashboard (they can always fix the list from // localhost). checkIP is a no-op when the filter is disabled. + // + // D1 (#9033): on a direct connection the proxy runtime has no socket, so + // checkRequestIP reads only forwarding headers + undefined request.ip and + // falls to "unknown", never blocking the blacklisted client. Resolve the + // trusted peer IP from the authenticated stamp and pass it to checkRequestIP, + // but only when NOT behind a reverse proxy (the via-proxy marker means the + // peer IP is the proxy hop, e.g. 127.0.0.1, and the real client is in XFF). if (peerLocality !== "loopback") { - const ipVerdict = checkRequestIP(request); + const trustedPeerIp = resolveStampedPeer( + request.headers.get(PEER_IP_HEADER), + process.env.OMNIROUTE_PEER_STAMP_TOKEN + ); + const viaProxy = resolveStampedViaProxy( + request.headers.get(VIA_PROXY_HEADER), + process.env.OMNIROUTE_PEER_STAMP_TOKEN + ); + const ipVerdict = checkRequestIP(request, viaProxy ? null : trustedPeerIp); if (!ipVerdict.allowed) { const blocked = NextResponse.json( { error: ipVerdict.reason || "Access denied" }, diff --git a/src/shared/validation/schemas/misc.ts b/src/shared/validation/schemas/misc.ts index a7cecbd012..3ed8f5cf87 100644 --- a/src/shared/validation/schemas/misc.ts +++ b/src/shared/validation/schemas/misc.ts @@ -108,7 +108,7 @@ export const resetStatsActionSchema = z.object({ action: z.literal("reset-stats"), }); -export const ipFilterModeSchema = z.enum(["blacklist", "whitelist"]); +export const ipFilterModeSchema = z.enum(["blacklist", "whitelist", "whitelist-priority"]); export const tempBanSchema = z.object({ ip: z.string().trim().min(1), diff --git a/tests/unit/authz/probe-9033-repro.test.ts b/tests/unit/authz/probe-9033-repro.test.ts index 563739d34f..6044de755e 100644 --- a/tests/unit/authz/probe-9033-repro.test.ts +++ b/tests/unit/authz/probe-9033-repro.test.ts @@ -55,11 +55,7 @@ test("D1: blacklisted IP is blocked on a DIRECT connection (trusted peer stamp, { enforce: true } ); - assert.equal( - res.status, - 403, - `direct blacklisted IP must be blocked, got status=${res.status}` - ); + assert.equal(res.status, 403, `direct blacklisted IP must be blocked, got status=${res.status}`); }); test("D2: persisted config written after first load is honored WITHOUT restart", async () => { @@ -74,9 +70,7 @@ test("D2: persisted config written after first load is honored WITHOUT restart", // Now simulate a "settings route" write: write directly to the DB key_value table // with a DIFFERENT config (e.g. empty blacklist, effectively "allow all"). const db = core.getDbInstance(); - db.prepare( - "INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)" - ).run( + db.prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)").run( "ipFilter", "config", JSON.stringify({ enabled: true, mode: "blacklist", blacklist: [], whitelist: [] }) @@ -116,13 +110,11 @@ test("D3: behind reverse proxy (peer stamp=loopback + via-proxy marker + XFF=bla }); test("Bonus: ipFilterModeSchema accepts whitelist-priority", async () => { - const { ipFilterModeSchema } = await import( - "../../../src/shared/validation/schemas/misc.ts" - ); + const { ipFilterModeSchema } = await import("../../../src/shared/validation/schemas/misc.ts"); const result = ipFilterModeSchema.safeParse("whitelist-priority"); assert.equal( result.success, true, `ipFilterModeSchema must accept "whitelist-priority", got: ${JSON.stringify(result)}` ); -}); \ No newline at end of file +});