From e8a1fa0852f56f7d220325810f83edfb4128dc1b Mon Sep 17 00:00:00 2001 From: CallMeTechie <34693633+CallMeTechie@users.noreply.github.com> Date: Thu, 25 Jun 2026 20:41:09 +0200 Subject: [PATCH] fix(domains,routes): address footgun review findings (async/correctness/perf/security) Domain/routes subsystem review follow-ups: - routes.js: await-safe monitor.checkRouteById() in create+update (no unhandled rejection); clamp ?limit lower bound on /:id/trace (negative splice bug); async (non-blocking) tail read of the trace log; central isPrivateOrReservedIpv4() SSRF guard incl. 100.64/10 CGNAT (POST+PUT); /check-dns now uses domains.getServerPublicIp() as the single server-IP source (drops the divergent ipify/getServerIp cache + isPublicIp helper). - domains.js: canonicalize IPs (ipaddr.js) before compare so a non-canonical IPv6 server-IP override still verifies; upsert() uses RETURNING * (no second SELECT). - domainBoot.js: verifyAndReflag runs verifications concurrently and prepares its UPDATE statements once; seed loop prepares its INSERT once. - ipaddr.js promoted to a direct dependency (pinned 1.9.1, already deduped via proxy-addr). - Tests: IPv6 non-canonical verify; CGNAT SSRF reject + 100.128/9 allow boundary. --- package-lock.json | 1 + package.json | 1 + src/routes/api/routes.js | 99 +++++++++++++------------------ src/services/domainBoot.js | 20 +++++-- src/services/domains.js | 22 +++++-- tests/api_routes_registry.test.js | 14 +++++ tests/domain_verify.test.js | 9 +++ 7 files changed, 96 insertions(+), 70 deletions(-) diff --git a/package-lock.json b/package-lock.json index 585234bf..66b5db1e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -19,6 +19,7 @@ "express-session": "^1.18.1", "guacamole-lite": "1.2.0", "helmet": "^8.0.0", + "ipaddr.js": "1.9.1", "jsonwebtoken": "^9.0.3", "multer": "^2.2.0", "nodemailer": "^9.0.1", diff --git a/package.json b/package.json index e293e21c..cc7f497e 100644 --- a/package.json +++ b/package.json @@ -20,6 +20,7 @@ "express-session": "^1.18.1", "guacamole-lite": "1.2.0", "helmet": "^8.0.0", + "ipaddr.js": "1.9.1", "jsonwebtoken": "^9.0.3", "multer": "^2.2.0", "nodemailer": "^9.0.1", diff --git a/src/routes/api/routes.js b/src/routes/api/routes.js index 1a1dca3d..ef247bb9 100644 --- a/src/routes/api/routes.js +++ b/src/routes/api/routes.js @@ -263,36 +263,26 @@ router.get('/', async (req, res) => { } }); -// ─── Server public IP cache ────────────────────────────── -let _cachedServerIp = null; - -function isPublicIp(str) { - return /^\d{1,3}(\.\d{1,3}){3}$/.test(str); -} - -async function getServerIp() { - if (_cachedServerIp) return _cachedServerIp; - const wgHost = config.wireguard.host; - if (wgHost && isPublicIp(wgHost)) { - _cachedServerIp = wgHost; - return _cachedServerIp; - } - try { - const controller = new AbortController(); - const timeout = setTimeout(() => controller.abort(), 2000); - const res = await fetch('https://api.ipify.org', { signal: controller.signal }); - clearTimeout(timeout); - const ip = (await res.text()).trim(); - if (isPublicIp(ip)) { - _cachedServerIp = ip; - return _cachedServerIp; - } - } catch (_) { /* ignore */ } - return null; +// SSRF guard: block private / loopback / reserved IPv4 ranges as direct route +// targets. Input is already IP_RE-validated dotted-quad IPv4. +function isPrivateOrReservedIpv4(ip) { + const p = String(ip).split('.').map(Number); + return ( + p[0] === 0 || // 0.0.0.0/8 "this host" + p[0] === 10 || // 10/8 RFC1918 + p[0] === 127 || // 127/8 loopback + (p[0] === 100 && p[1] >= 64 && p[1] <= 127) || // 100.64/10 CGNAT (RFC6598) + (p[0] === 169 && p[1] === 254) || // 169.254/16 link-local + (p[0] === 172 && p[1] >= 16 && p[1] <= 31) || // 172.16/12 RFC1918 + (p[0] === 192 && p[1] === 168) // 192.168/16 RFC1918 + ); } /** - * POST /api/routes/check-dns — Check if domain resolves to server IP + * POST /api/routes/check-dns — Check if domain resolves to server IP. + * Uses the single source of truth for the server's public IP + * (domainsService.getServerPublicIp: settings override + explicit resolver), + * so the DNS check agrees with the domain-registry verification path. */ router.post('/check-dns', async (req, res) => { const { domain } = req.body || {}; @@ -305,10 +295,11 @@ router.post('/check-dns', async (req, res) => { const dnsTimeout = new Promise((_, reject) => setTimeout(() => reject(new Error('DNS timeout')), 2000) ); - const [serverIp, addresses] = await Promise.all([ - getServerIp(), + const [server, addresses] = await Promise.all([ + require('../../services/domains').getServerPublicIp(), Promise.race([dns.resolve4(domain), dnsTimeout]), ]); + const serverIp = server.ip; const resolves = serverIp ? addresses.includes(serverIp) : false; return res.json({ ok: true, resolves, expected: serverIp }); } catch (err) { @@ -428,12 +419,7 @@ router.post('/', // gateway-typed routes (LAN host is intentionally private and only // reachable via the home gateway's WG tunnel, not directly proxied). if (target_ip && !peer_id && req.body.target_kind !== 'gateway') { - const parts = target_ip.split('.').map(Number); - if (parts[0] === 127 || parts[0] === 10 || - (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) || - (parts[0] === 192 && parts[1] === 168) || - (parts[0] === 169 && parts[1] === 254) || - parts[0] === 0) { + if (isPrivateOrReservedIpv4(target_ip)) { return res.status(400).json({ ok: false, error: req.t('error.routes.private_ip') || 'Private/loopback IPs are not allowed as route targets' }); } } @@ -513,7 +499,7 @@ router.post('/', }); // Trigger immediate check if monitoring enabled on create if (monitoring_enabled) { - try { const { checkRouteById } = require('../../services/monitor'); checkRouteById(route.id); } catch {} + try { const { checkRouteById } = require('../../services/monitor'); checkRouteById(route.id).catch(() => {}); } catch {} } res.status(201).json({ ok: true, route: stripRoute(route) }); } catch (err) { @@ -602,12 +588,7 @@ router.put('/:id', // SSRF protection: block private/loopback IPs for direct target_ip. // Skipped for peer-linked and gateway-typed routes (see POST handler). if (target_ip && !peer_id && req.body.target_kind !== 'gateway') { - const parts = target_ip.split('.').map(Number); - if (parts[0] === 127 || parts[0] === 10 || - (parts[0] === 172 && parts[1] >= 16 && parts[1] <= 31) || - (parts[0] === 192 && parts[1] === 168) || - (parts[0] === 169 && parts[1] === 254) || - parts[0] === 0) { + if (isPrivateOrReservedIpv4(target_ip)) { return res.status(400).json({ ok: false, error: req.t('error.routes.private_ip') || 'Private/loopback IPs are not allowed as route targets' }); } } @@ -686,7 +667,7 @@ router.put('/:id', } // Trigger immediate check if monitoring was just enabled if (monitoring_enabled) { - try { const { checkRouteById } = require('../../services/monitor'); checkRouteById(req.params.id); } catch {} + try { const { checkRouteById } = require('../../services/monitor'); checkRouteById(req.params.id).catch(() => {}); } catch {} } res.json({ ok: true, route: stripRoute(route) }); } catch (err) { @@ -914,40 +895,40 @@ router.delete('/:id/branding/bg-image', (req, res) => { // GET /routes/:id/trace — read trace log entries for a route router.get('/:id/trace', asyncHandler(async (req, res) => { const routeId = req.params.id; - const limit = Math.min(parseInt(req.query.limit) || 50, 200); + const limit = Math.min(Math.max(parseInt(req.query.limit, 10) || 50, 1), 200); const since = req.query.since || ''; const logPath = '/data/caddy/caddy-stdout.log'; const entries = []; try { - const fs = require('fs'); - // Open the log first and size it from the file descriptor (fstatSync) - // rather than existsSync→statSync→openSync on the path: that check-then-use - // sequence is a TOCTOU race if the log rotates between calls. A missing - // file (ENOENT) simply yields no entries. - let fd; + const fsp = require('fs').promises; + // Open the log first and size it from the file handle (fh.stat) rather than + // exists→stat→open on the path: that check-then-use sequence is a TOCTOU + // race if the log rotates between calls. A missing file (ENOENT) simply + // yields no entries. + let fh; try { - fd = fs.openSync(logPath, 'r'); + fh = await fsp.open(logPath, 'r'); } catch { return res.json({ ok: true, data: { entries: [] } }); } - // Read only the tail of the log. caddy-stdout.log can grow to hundreds of - // MB; a full read would block the event loop for seconds and can OOM the - // process under concurrent traces. Entries are sorted newest-first and - // capped at `limit` (<=200), so the last few MB always hold more than - // enough candidates. Older traces simply fall outside the window. + // Read only the tail of the log, asynchronously so a multi-MB read never + // blocks the event loop. caddy-stdout.log can grow to hundreds of MB. + // Entries are sorted newest-first and capped at `limit` (<=200), so the last + // few MB always hold more than enough candidates. Older traces fall outside + // the window. const MAX_TRACE_BYTES = 8 * 1024 * 1024; let text; let start = 0; try { - const size = fs.fstatSync(fd).size; + const size = (await fh.stat()).size; start = Math.max(0, size - MAX_TRACE_BYTES); const len = size - start; const buf = Buffer.alloc(len); - fs.readSync(fd, buf, 0, len, start); + await fh.read(buf, 0, len, start); text = buf.toString('utf8'); } finally { - fs.closeSync(fd); + await fh.close(); } // When starting mid-file we likely cut a line in half — drop that partial // leading fragment so JSON.parse doesn't choke on it. diff --git a/src/services/domainBoot.js b/src/services/domainBoot.js index 4d3ddd79..a369750b 100644 --- a/src/services/domainBoot.js +++ b/src/services/domainBoot.js @@ -3,7 +3,7 @@ const { getDb } = require('../db/connection'); const domains = require('./domains'); const settings = require('./settings'); -const { extractBaseDomains, shouldFlagServerIp } = require('./domainSeed'); +const { extractBaseDomains, shouldFlagServerIp, normalizeHost } = require('./domainSeed'); const { isPublicDomain } = require('./caddyTlsAutomation'); /** @@ -17,15 +17,22 @@ const { isPublicDomain } = require('./caddyTlsAutomation'); * Returns { verified, flagged }. */ async function verifyAndReflag(domainNames, { verifyEach = domains.verify } = {}) { + // Verifications are independent (each does its own DNS lookup) → run concurrently, + // then persist. Statements are prepared once, not per row. + const verifications = await Promise.all( + domainNames.map(async (d) => ({ domain: d, v: await verifyEach(d) })) + ); + const db = getDb(); + const verifiedStmt = db.prepare("UPDATE domains SET status='verified', resolved_ip=?, last_error=NULL, verified_at=datetime('now'), last_checked_at=datetime('now') WHERE domain=?"); + const pendingStmt = db.prepare("UPDATE domains SET status='pending', last_checked_at=datetime('now') WHERE domain=?"); const results = []; - for (const d of domainNames) { - const v = await verifyEach(d); + for (const { domain: d, v } of verifications) { results.push({ domain: d, matched: v.status === 'verified' }); if (v.status === 'verified') { - getDb().prepare("UPDATE domains SET status='verified', resolved_ip=?, last_error=NULL, verified_at=datetime('now'), last_checked_at=datetime('now') WHERE domain=?").run(v.resolvedIp || null, d); + verifiedStmt.run(v.resolvedIp || null, d); } else { // keep/reset to pending; do NOT write 'failed' on this path - getDb().prepare("UPDATE domains SET status='pending', last_checked_at=datetime('now') WHERE domain=?").run(d); + pendingStmt.run(d); } } const flagged = shouldFlagServerIp(results); @@ -39,7 +46,8 @@ async function runDomainSeedAndVerify({ verifyEach = domains.verify } = {}) { // bases (.internal/.lan/...) would otherwise linger forever as 'pending' // noise on the Domains page, so they are never seeded. const bases = extractBaseDomains(routeDomains).filter(isPublicDomain); - for (const d of bases) domains.seedPending(d); + const seedStmt = getDb().prepare("INSERT OR IGNORE INTO domains (domain, status) VALUES (?, 'pending')"); + for (const d of bases) seedStmt.run(normalizeHost(d)); // One-time cleanup: drop non-public bases that earlier boots auto-seeded as // 'pending' (they can never verify; routes consume routes.domain directly and diff --git a/src/services/domains.js b/src/services/domains.js index 77710663..fc3390c1 100644 --- a/src/services/domains.js +++ b/src/services/domains.js @@ -2,6 +2,7 @@ 'use strict'; const dns = require('node:dns').promises; const net = require('node:net'); +const ipaddr = require('ipaddr.js'); const { getDb } = require('../db/connection'); const settings = require('./settings'); const config = require('../../config/default'); @@ -60,6 +61,15 @@ async function getServerPublicIp() { return { ip: null, family: null, source: 'unknown' }; } +// Canonicalize an IP for comparison. Resolver output is already canonical, but a +// user-entered override / config literal may be non-canonical IPv6 (uppercase, +// leading zeros, '::' compression) — a plain string compare would then miss a +// correctly-pointing AAAA record. Falls back to lowercase for non-IP input. +function canonIp(ip) { + try { return ipaddr.parse(String(ip)).toNormalizedString(); } + catch { return String(ip || '').trim().toLowerCase(); } +} + async function verify(domain) { const host = normalizeHost(domain); const server = await getServerPublicIp(); @@ -73,7 +83,8 @@ async function verify(domain) { if (all.length === 0) { return { status: 'failed', resolvedIp: null, expectedIp: server.ip, error: 'no A/AAAA records' }; } - if (all.includes(server.ip)) { + const wanted = canonIp(server.ip); + if (all.some(a => canonIp(a) === wanted)) { return { status: 'verified', resolvedIp: server.ip, expectedIp: server.ip, error: null }; } return { status: 'failed', resolvedIp: all[0], expectedIp: server.ip, @@ -89,15 +100,16 @@ function isVerified(domain) { const r = row(domain); return !!r && r.status === function upsert(domain, v) { const db = getDb(); - db.prepare(`INSERT INTO domains (domain, status, resolved_ip, last_error, verified_at, last_checked_at) + // RETURNING * hands back the written row directly — no second SELECT round-trip. + return db.prepare(`INSERT INTO domains (domain, status, resolved_ip, last_error, verified_at, last_checked_at) VALUES (@domain, @status, @resolved_ip, @last_error, @verified_at, datetime('now')) ON CONFLICT(domain) DO UPDATE SET status=excluded.status, resolved_ip=excluded.resolved_ip, - last_error=excluded.last_error, verified_at=excluded.verified_at, last_checked_at=excluded.last_checked_at`) - .run({ + last_error=excluded.last_error, verified_at=excluded.verified_at, last_checked_at=excluded.last_checked_at + RETURNING *`) + .get({ domain: normalizeHost(domain), status: v.status, resolved_ip: v.resolvedIp || null, last_error: v.error || null, verified_at: v.status === 'verified' ? new Date().toISOString() : null, }); - return row(domain); } async function add(domain) { diff --git a/tests/api_routes_registry.test.js b/tests/api_routes_registry.test.js index 535de11d..19a374ad 100644 --- a/tests/api_routes_registry.test.js +++ b/tests/api_routes_registry.test.js @@ -61,3 +61,17 @@ test('routes list flags public routes with unverified base', async () => { assert.equal(by['y.verified.com'].domainIsPublic, true); assert.equal(by['z.gc.internal'].domainIsPublic, false); }); + +test('create with CGNAT target_ip (100.64.0.0/10) is rejected by the SSRF guard', async () => { + // internal-TLD domain → carve-out passes the domain policy, so the SSRF guard is reached. + const res = await agent.post('/api/v1/routes').set('X-CSRF-Token', csrf) + .send({ domain: 'cgnat.gc.internal', target_ip: '100.64.0.1', target_port: 80, route_type: 'http' }); + assert.equal(res.status, 400); +}); + +test('create with a non-CGNAT 100.x target_ip (100.128.0.1) is allowed', async () => { + // boundary: 100.128/9 is public space, must NOT be blocked by the 100.64/10 rule. + const res = await agent.post('/api/v1/routes').set('X-CSRF-Token', csrf) + .send({ domain: 'pub100.gc.internal', target_ip: '100.128.0.1', target_port: 80, route_type: 'http' }); + assert.equal(res.status, 201); +}); diff --git a/tests/domain_verify.test.js b/tests/domain_verify.test.js index 165c7eeb..d678327c 100644 --- a/tests/domain_verify.test.js +++ b/tests/domain_verify.test.js @@ -28,6 +28,15 @@ test('failed when resolves elsewhere (server IP known-good)', async () => { assert.match(r.error, /198\.51\.100\.7/); // expected IP in the message }); +test('verified for IPv6 server IP regardless of canonical form', async () => { + // Override is non-canonical (uppercase + '::' compression); the resolver returns + // the fully-expanded canonical AAAA. A plain string compare would miss this. + settings.set('server.public_ip', '2001:DB8::1'); + domains._setResolverForTest(async (h, f) => (f === 6 ? ['2001:db8:0:0:0:0:0:1'] : [])); + const r = await domains.verify('v6.example.com'); + assert.equal(r.status, 'verified'); +}); + test('pending (not failed) when server IP unknown', async () => { settings.set('server.public_ip', ''); // clear the override → must derive // CRITICAL: the stub answers ALL hosts incl. GC_WG_HOST (test.example.com). To make