From f9b2fb4138c95ff10366b3a190ae737c09ba7955 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza <8016841+diegosouzapw@users.noreply.github.com> Date: Fri, 3 Jul 2026 18:50:29 -0300 Subject: [PATCH] fix(security): persist IP filter config + enforce it in the authz pipeline (#6131) (#6132) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrated into release/v3.8.44 — IP filter persistence + authz-pipeline enforcement (closes #6131). HARD-neutro: validate-release-green on the merge shows the same 3 pre-existing base-reds as the release baseline (test-masking cycle-wide, unit red-herring, integration batch-E2E env); #6131's own tests + ip-filter/pipeline suites all green. --- open-sse/services/ipFilter.ts | 65 +++++++++++++ src/server/authz/pipeline.ts | 30 ++++-- .../authz/ip-filter-enforcement-6131.test.ts | 92 ++++++++++++++++++ tests/unit/ip-filter-persistence-6131.test.ts | 96 +++++++++++++++++++ tests/unit/ip-filter.test.ts | 22 ++++- 5 files changed, 297 insertions(+), 8 deletions(-) create mode 100644 tests/unit/authz/ip-filter-enforcement-6131.test.ts create mode 100644 tests/unit/ip-filter-persistence-6131.test.ts diff --git a/open-sse/services/ipFilter.ts b/open-sse/services/ipFilter.ts index d809883988..c023397891 100644 --- a/open-sse/services/ipFilter.ts +++ b/open-sse/services/ipFilter.ts @@ -5,6 +5,7 @@ */ import { isIP } from "node:net"; +import { getDbInstance } from "../../src/lib/db/core.ts"; // In-memory IP lists let _config = { @@ -15,6 +16,57 @@ let _config = { tempBans: new Map(), }; +// Persistence (#6131): the config used to live in memory only, so every restart +// (i.e. every OmniRoute update) reset it to Disabled + empty lists. It is now +// persisted to the key_value table (namespace 'ipFilter', key 'config') and +// 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. +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 = ?") + .get(IP_FILTER_NAMESPACE, IP_FILTER_KEY) as { value?: string } | undefined; + if (!row?.value) return; + const parsed = JSON.parse(row.value) as { + enabled?: boolean; + mode?: string; + blacklist?: string[]; + whitelist?: string[]; + }; + _config.enabled = parsed.enabled === true; + if (typeof parsed.mode === "string") _config.mode = parsed.mode; + _config.blacklist = new Set(Array.isArray(parsed.blacklist) ? parsed.blacklist : []); + _config.whitelist = new Set(Array.isArray(parsed.whitelist) ? parsed.whitelist : []); + } catch { + // No DB / table yet — keep the in-memory defaults. + } +} + +function persist() { + try { + const payload = JSON.stringify({ + enabled: _config.enabled, + mode: _config.mode, + blacklist: Array.from(_config.blacklist), + whitelist: Array.from(_config.whitelist), + }); + getDbInstance() + .prepare("INSERT OR REPLACE INTO key_value (namespace, key, value) VALUES (?, ?, ?)") + .run(IP_FILTER_NAMESPACE, IP_FILTER_KEY, payload); + } catch { + // Best-effort persistence: never let a DB write failure break the request path. + } +} + const _tempBanSweep = setInterval(() => { const now = Date.now(); const bans = _config.tempBans as Map; @@ -30,16 +82,19 @@ if (typeof _tempBanSweep === "object" && "unref" in _tempBanSweep) { * Configure the IP filter */ export function configureIPFilter(config) { + ensureLoaded(); if (config.enabled !== undefined) _config.enabled = config.enabled; if (config.mode) _config.mode = config.mode; if (config.blacklist) _config.blacklist = new Set(config.blacklist); if (config.whitelist) _config.whitelist = new Set(config.whitelist); + persist(); } /** * Get current IP filter config (for API) */ export function getIPFilterConfig() { + ensureLoaded(); return { enabled: _config.enabled, mode: _config.mode, @@ -60,6 +115,7 @@ export function getIPFilterConfig() { * @returns {{ allowed: boolean, reason?: string }} */ export function checkIP(ip) { + ensureLoaded(); if (!_config.enabled) return { allowed: true }; if (!ip) return { allowed: true }; @@ -124,28 +180,36 @@ export function removeTempBan(ip) { * Add IP to blacklist */ export function addToBlacklist(ip) { + ensureLoaded(); _config.blacklist.add(normalizeIP(ip)); + persist(); } /** * Remove IP from blacklist */ export function removeFromBlacklist(ip) { + ensureLoaded(); _config.blacklist.delete(normalizeIP(ip)); + persist(); } /** * Add IP to whitelist */ export function addToWhitelist(ip) { + ensureLoaded(); _config.whitelist.add(normalizeIP(ip)); + persist(); } /** * Remove IP from whitelist */ export function removeFromWhitelist(ip) { + ensureLoaded(); _config.whitelist.delete(normalizeIP(ip)); + persist(); } /** @@ -265,6 +329,7 @@ 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 5312974006..84b6550aa3 100644 --- a/src/server/authz/pipeline.ts +++ b/src/server/authz/pipeline.ts @@ -9,6 +9,7 @@ import { validateBrowserMutationOrigin } from "../origin/publicOrigin"; import { classifyRoute } from "./classify"; import { validateDashboardCsrfToken } from "./csrf"; import { classifyStampedPeerLocality } from "./peerStamp"; +import { checkRequestIP } from "@omniroute/open-sse/services/ipFilter.ts"; import { clientApiPolicy } from "./policies/clientApi"; import { managementPolicy } from "./policies/management"; import { publicPolicy } from "./policies/public"; @@ -287,14 +288,12 @@ export async function runAuthzPipeline( // to "remote" so the LOCAL_ONLY gate is not bypassed by a request arriving // through an external reverse proxy (nginx / Caddy / Cloudflare Tunnel). // See peerStamp.ts and the upstream da667836 reference for the full rationale. - requestHeaders.set( - AUTHZ_HEADER_PEER_LOCALITY, - classifyStampedPeerLocality( - request.headers.get(PEER_IP_HEADER), - request.headers.get(VIA_PROXY_HEADER), - process.env.OMNIROUTE_PEER_STAMP_TOKEN - ) + const peerLocality = classifyStampedPeerLocality( + request.headers.get(PEER_IP_HEADER), + request.headers.get(VIA_PROXY_HEADER), + process.env.OMNIROUTE_PEER_STAMP_TOKEN ); + requestHeaders.set(AUTHZ_HEADER_PEER_LOCALITY, peerLocality); if (method === "OPTIONS") { const preflight = new NextResponse(null, { status: 204 }); @@ -312,6 +311,23 @@ export async function runAuthzPipeline( return response; } + // IP filter (#6131): enforce the operator's IP blacklist/whitelist on the + // 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. + if (peerLocality !== "loopback") { + const ipVerdict = checkRequestIP(request); + if (!ipVerdict.allowed) { + const blocked = NextResponse.json( + { error: ipVerdict.reason || "Access denied" }, + { status: 403 } + ); + stampRouteResponse(blocked, requestId, classification.routeClass); + applyCorsHeaders(blocked, request, corsRelaxOrigin); + return blocked; + } + } + const policy = POLICIES[classification.routeClass]; const outcome = await policy.evaluate({ request, classification, requestId }); diff --git a/tests/unit/authz/ip-filter-enforcement-6131.test.ts b/tests/unit/authz/ip-filter-enforcement-6131.test.ts new file mode 100644 index 0000000000..da55ff11ff --- /dev/null +++ b/tests/unit/authz/ip-filter-enforcement-6131.test.ts @@ -0,0 +1,92 @@ +// Regression for #6131 (Part B — enforcement): the IP blacklist was never wired +// into the request pipeline, so blacklisted IPs were not actually blocked. This +// locks that runAuthzPipeline blocks a blacklisted client IP with 403 before the +// route policy runs, allows a clean IP through to the normal auth outcome, and +// exempts loopback so the local operator can never lock themselves out. +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { NextRequest } from "next/server"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-ipenforce-6131-")); +process.env.DATA_DIR = TEST_DATA_DIR; +process.env.JWT_SECRET = "test-secret-6131"; + +const core = await import("../../../src/lib/db/core.ts"); +const ipFilter = await import("../../../open-sse/services/ipFilter.ts"); +const pipeline = await import("../../../src/server/authz/pipeline.ts"); + +const ORIGINAL_STAMP_TOKEN = process.env.OMNIROUTE_PEER_STAMP_TOKEN; + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + if (ORIGINAL_STAMP_TOKEN === undefined) delete process.env.OMNIROUTE_PEER_STAMP_TOKEN; + else process.env.OMNIROUTE_PEER_STAMP_TOKEN = ORIGINAL_STAMP_TOKEN; +}); + +test.beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + ipFilter.resetIPFilter(); + delete process.env.OMNIROUTE_PEER_STAMP_TOKEN; +}); + +const BLOCKED = "203.0.113.9"; +const CLEAN = "203.0.113.10"; + +function req(xff: string, extraHeaders: Record = {}) { + return new NextRequest("http://localhost/v1/models", { + headers: { "x-forwarded-for": xff, ...extraHeaders }, + }); +} + +async function isIpBlocked(res: Response): Promise { + if (res.status !== 403) return false; + const body = (await res.clone().json()) as { error?: unknown }; + // The IP-filter block returns a plain string error; policy 403s use a nested object. + return typeof body.error === "string" && /blacklist|not in whitelist|banned/i.test(body.error); +} + +test("#6131 blacklisted remote IP is blocked with 403 before the route policy", async () => { + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist(BLOCKED); + + const res = await pipeline.runAuthzPipeline(req(BLOCKED), { enforce: true }); + assert.equal(res.status, 403); + assert.equal(await isIpBlocked(res), true, "expected the IP-filter 403 block"); +}); + +test("#6131 a clean remote IP passes the IP filter (reaches the normal auth outcome)", async () => { + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist(BLOCKED); + + const res = await pipeline.runAuthzPipeline(req(CLEAN), { enforce: true }); + // The IP filter must let it through to the normal route/auth outcome, whatever + // that is (the point is: NOT the IP-filter 403 block). + assert.equal(await isIpBlocked(res), false, "clean IP must not be blocked by the IP filter"); +}); + +test("#6131 disabled filter never blocks (even a listed IP)", async () => { + ipFilter.configureIPFilter({ enabled: false, mode: "blacklist" }); + ipFilter.addToBlacklist(BLOCKED); + + const res = await pipeline.runAuthzPipeline(req(BLOCKED), { enforce: true }); + assert.equal(await isIpBlocked(res), false, "disabled filter must not block"); +}); + +test("#6131 loopback is exempt — operator can't lock themselves out locally", async () => { + process.env.OMNIROUTE_PEER_STAMP_TOKEN = "stamp-tok"; + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist(BLOCKED); + + // A trusted stamped loopback peer IP downgrades the request to "loopback". + const res = await pipeline.runAuthzPipeline( + req(BLOCKED, { "x-omniroute-peer-ip": "stamp-tok|127.0.0.1" }), + { enforce: true } + ); + assert.equal(await isIpBlocked(res), false, "loopback must be exempt from the IP filter"); +}); diff --git a/tests/unit/ip-filter-persistence-6131.test.ts b/tests/unit/ip-filter-persistence-6131.test.ts new file mode 100644 index 0000000000..279193627e --- /dev/null +++ b/tests/unit/ip-filter-persistence-6131.test.ts @@ -0,0 +1,96 @@ +// Regression for #6131: the IP filter config lived in memory only, so every +// restart (i.e. every OmniRoute update) reset it to Disabled + empty lists and +// blacklisted IPs were never actually blocked. This locks the fix: +// 1. configure/blacklist persists to the DB and survives a simulated restart; +// 2. after the restart the blacklisted IP is still blocked by checkIP. +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-ipfilter-6131-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const ipFilter = await import("../../open-sse/services/ipFilter.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test.beforeEach(() => { + // Fresh DB per test + fresh in-memory module state. + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + ipFilter.resetIPFilter(); +}); + +// Simulate an OmniRoute restart: the module's in-memory state is wiped (as it +// would be on a fresh import) but the DB file persists — exactly what happens +// across an update/restart. +function simulateRestart() { + ipFilter.resetIPFilter(); +} + +test("#6131 blacklist + enabled survive a restart (persisted to DB)", () => { + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist("203.0.113.7"); + ipFilter.addToBlacklist("198.51.100.42"); + + simulateRestart(); + + const cfg = ipFilter.getIPFilterConfig(); + assert.equal(cfg.enabled, true, "enabled must persist across restart"); + assert.equal(cfg.mode, "blacklist"); + assert.deepEqual(cfg.blacklist.sort(), ["198.51.100.42", "203.0.113.7"]); +}); + +test("#6131 blacklisted IP is still blocked after a restart", () => { + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist("203.0.113.7"); + + simulateRestart(); + + assert.equal(ipFilter.checkIP("203.0.113.7").allowed, false); + assert.equal(ipFilter.checkIP("203.0.113.8").allowed, true); +}); + +test("#6131 removing an IP and disabling also persist across restart", () => { + ipFilter.configureIPFilter({ enabled: true, mode: "blacklist" }); + ipFilter.addToBlacklist("203.0.113.7"); + ipFilter.addToBlacklist("203.0.113.8"); + ipFilter.removeFromBlacklist("203.0.113.7"); + ipFilter.configureIPFilter({ enabled: false }); + + simulateRestart(); + + const cfg = ipFilter.getIPFilterConfig(); + assert.equal(cfg.enabled, false); + assert.deepEqual(cfg.blacklist, ["203.0.113.8"]); + // Disabled → everything allowed regardless of the persisted blacklist. + assert.equal(ipFilter.checkIP("203.0.113.8").allowed, true); +}); + +test("#6131 whitelist mode persists across restart", () => { + ipFilter.configureIPFilter({ enabled: true, mode: "whitelist" }); + ipFilter.addToWhitelist("203.0.113.7"); + + simulateRestart(); + + const cfg = ipFilter.getIPFilterConfig(); + assert.equal(cfg.mode, "whitelist"); + assert.deepEqual(cfg.whitelist, ["203.0.113.7"]); + assert.equal(ipFilter.checkIP("203.0.113.7").allowed, true); + assert.equal(ipFilter.checkIP("10.0.0.1").allowed, false); +}); + +test("#6131 defaults are safe when nothing was ever persisted (disabled, allow-all)", () => { + simulateRestart(); + const cfg = ipFilter.getIPFilterConfig(); + assert.equal(cfg.enabled, false); + assert.deepEqual(cfg.blacklist, []); + assert.equal(ipFilter.checkIP("203.0.113.7").allowed, true); +}); diff --git a/tests/unit/ip-filter.test.ts b/tests/unit/ip-filter.test.ts index 463e73dcc6..4559bc6812 100644 --- a/tests/unit/ip-filter.test.ts +++ b/tests/unit/ip-filter.test.ts @@ -1,6 +1,16 @@ import test from "node:test"; import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +// Isolate DATA_DIR before importing ipFilter: since #6131 the filter lazily +// loads/persists its config to the DB, so an un-isolated run would touch the +// real ~/.omniroute DB (side-effect + WAL-lock flake). Pin a throwaway dir. +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-ipfilter-unit-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); const { checkIP, configureIPFilter, @@ -15,7 +25,17 @@ const { resetIPFilter, } = await import("../../open-sse/services/ipFilter.ts"); -test.beforeEach(() => resetIPFilter()); +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test.beforeEach(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DATA_DIR, { recursive: true }); + resetIPFilter(); +}); // ─── Disabled ───────────────────────────────────────────────────────────────