fix(domains,routes): address footgun review findings (async/correctness/perf/security)

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",

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.

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

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) {

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);
+});

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