harden(portal): Pi-hole-Portal footgun-Review-Befunde (v5-Isolation + Härtung)

public/js/portal.js

@@ -360,7 +360,90 @@
     var card = document.querySelector('.pihole-widget');
     if (!card) return;                       // widget toggled off → not in DOM → no fetch
     setLoading(card, true);
-    var url = PI_ENDPOINTS[scope] || PI_ENDPOINTS.device;
+    // Whitelist scope to guard against proto-poisoning (__proto__, constructor, etc.)
+    var url = (scope === 'device' || scope === 'owner' || scope === 'household') ? PI_ENDPOINTS[scope] : PI_ENDPOINTS.device;
+
+    // ── Render helpers (inner functions — closed over card, scope) ────────
+
+    function renderPiholeReason(msg, bodyEl, reason) {
+      if (bodyEl) bodyEl.style.display = 'none';
+      if (msg) {
+        msg.replaceChildren();
+        if (reason === 'no_owner' || reason === 'login_required') {
+          // DOM-safe login affordance — no innerHTML with i18n text or /login href
+          var hintKey = reason === 'no_owner' ? 'piholeNoOwner' : 'piholeLoginRequired';
+          var hintSpan = document.createElement('span');
+          hintSpan.textContent = PT[hintKey] || '';
+          var a = document.createElement('a');
+          a.href = '/login';
+          a.textContent = PT.piholeLoginLink || 'Log in';
+          msg.appendChild(hintSpan);
+          msg.appendChild(document.createTextNode(' '));
+          msg.appendChild(a);
+        } else {
+          var key = { collapsed:'piholeCollapsed', no_data:'piholeNoData', unidentified:'piholeUnidentified' }[reason] || 'piholeUnavailable';
+          msg.textContent = PT[key] || ''; // PT = i18n map (portal.js ~line 14)
+        }
+        msg.style.display = 'block';
+      }
+    }
+
+    function renderPiholeStats(d, msg) {
+      // ── Scope-visibility: deterministically show/hide cross-scope fields ─
+      var allowedWrap = document.getElementById('piAllowedWrap');
+      if (allowedWrap) allowedWrap.style.display = (scope === 'household') ? 'none' : '';
+      var ownerExtra = document.getElementById('piOwnerExtra');
+      if (ownerExtra) ownerExtra.style.display = (scope === 'owner') ? '' : 'none';
+      var hhExtra = document.getElementById('piHouseholdExtra');
+      if (hhExtra) hhExtra.style.display = (scope === 'household') ? '' : 'none';
+
+      // ── Stats common to all scopes ──────────────────────────────────────
+      var pctEl = document.getElementById('piPct');
+      if (pctEl) pctEl.textContent = String(d.blockedPct);
+      var bar = document.getElementById('piBar');
+      if (bar) bar.style.width = d.blockedPct + '%';
+      var totalEl = document.getElementById('piTotal');
+      if (totalEl) totalEl.textContent = String(d.total);
+      var blockedEl = document.getElementById('piBlocked');
+      if (blockedEl) blockedEl.textContent = String(d.blocked);
+
+      // ── Scope-specific fields ───────────────────────────────────────────
+      if (scope !== 'household') {
+        // device + owner: show allowed count
+        var allowedEl = document.getElementById('piAllowed');
+        if (allowedEl) allowedEl.textContent = String(d.allowed);
+      }
+
+      if (scope === 'owner') {
+        // owner: device count across the owner's peers
+        var devCountEl = document.getElementById('piOwnerDevices');
+        if (devCountEl) {
+          devCountEl.textContent = (PT['piholeOwnerDevices'] || '{n}').replace('{n}', String(d.deviceCount));
+        }
+        var devHintEl = document.getElementById('piOwnerDevicesHint');
+        if (devHintEl) devHintEl.textContent = PT['piholeOwnerDevicesHint'] || '';
+      }
+
+      if (scope === 'household') {
+        // household: active client count, no allowed stat
+        var clientsEl = document.getElementById('piActiveClients');
+        if (clientsEl) {
+          clientsEl.textContent = (PT['piholeActiveClients'] || '{n}').replace('{n}', String(d.activeClients || 0));
+        }
+      }
+
+      // ── Zero-queries notice (spec §5) ───────────────────────────────────
+      if (msg) {
+        msg.replaceChildren();
+        if (d.total === 0) {
+          msg.textContent = PT['piholeZeroQueries'] || '';
+          msg.style.display = 'block';
+        } else {
+          msg.style.display = 'none';
+        }
+      }
+    }
+
     fetch(url)
       .then(function (r) { if (!r.ok) throw new Error('HTTP ' + r.status); return r.json(); })
       .then(function (body) {
@@ -370,84 +453,11 @@
         if (!body.ok || body.data === null) {
           var reason = body.reason;
           if (reason === 'unavailable' && scope === 'device') { card.style.display = 'none'; return; }
-          if (bodyEl) bodyEl.style.display = 'none';
-          if (msg) {
-            msg.replaceChildren();
-            if (reason === 'no_owner' || reason === 'login_required') {
-              // DOM-safe login affordance — no innerHTML with i18n text or /login href
-              var hintKey = reason === 'no_owner' ? 'piholeNoOwner' : 'piholeLoginRequired';
-              var hintSpan = document.createElement('span');
-              hintSpan.textContent = PT[hintKey] || '';
-              var a = document.createElement('a');
-              a.href = '/login';
-              a.textContent = PT.piholeLoginLink || 'Log in';
-              msg.appendChild(hintSpan);
-              msg.appendChild(document.createTextNode(' '));
-              msg.appendChild(a);
-            } else {
-              var key = { collapsed:'piholeCollapsed', no_data:'piholeNoData', unidentified:'piholeUnidentified' }[reason] || 'piholeUnavailable';
-              msg.textContent = PT[key] || ''; // PT = i18n map (portal.js ~line 14)
-            }
-            msg.style.display = 'block';
-          }
+          renderPiholeReason(msg, bodyEl, reason);
           return;
         }
         if (bodyEl) bodyEl.style.display = '';
-        var d = body.data;
-
-        // ── Scope-visibility: deterministically show/hide cross-scope fields ─
-        var allowedWrap = document.getElementById('piAllowedWrap');
-        if (allowedWrap) allowedWrap.style.display = (scope === 'household') ? 'none' : '';
-        var ownerExtra = document.getElementById('piOwnerExtra');
-        if (ownerExtra) ownerExtra.style.display = (scope === 'owner') ? '' : 'none';
-        var hhExtra = document.getElementById('piHouseholdExtra');
-        if (hhExtra) hhExtra.style.display = (scope === 'household') ? '' : 'none';
-
-        // ── Stats common to all scopes ──────────────────────────────────────
-        var pctEl = document.getElementById('piPct');
-        if (pctEl) pctEl.textContent = String(d.blockedPct);
-        var bar = document.getElementById('piBar');
-        if (bar) bar.style.width = d.blockedPct + '%';
-        var totalEl = document.getElementById('piTotal');
-        if (totalEl) totalEl.textContent = String(d.total);
-        var blockedEl = document.getElementById('piBlocked');
-        if (blockedEl) blockedEl.textContent = String(d.blocked);
-
-        // ── Scope-specific fields ───────────────────────────────────────────
-        if (scope !== 'household') {
-          // device + owner: show allowed count
-          var allowedEl = document.getElementById('piAllowed');
-          if (allowedEl) allowedEl.textContent = String(d.allowed);
-        }
-
-        if (scope === 'owner') {
-          // owner: device count across the owner's peers
-          var devCountEl = document.getElementById('piOwnerDevices');
-          if (devCountEl) {
-            devCountEl.textContent = (PT['piholeOwnerDevices'] || '{n}').replace('{n}', String(d.deviceCount));
-          }
-          var devHintEl = document.getElementById('piOwnerDevicesHint');
-          if (devHintEl) devHintEl.textContent = PT['piholeOwnerDevicesHint'] || '';
-        }
-
-        if (scope === 'household') {
-          // household: active client count, no allowed stat
-          var clientsEl = document.getElementById('piActiveClients');
-          if (clientsEl) {
-            clientsEl.textContent = (PT['piholeActiveClients'] || '{n}').replace('{n}', String(d.activeClients || 0));
-          }
-        }
-
-        // ── Zero-queries notice (spec §5) ───────────────────────────────────
-        if (msg) {
-          msg.replaceChildren();
-          if (d.total === 0) {
-            msg.textContent = PT['piholeZeroQueries'] || '';
-            msg.style.display = 'block';
-          } else {
-            msg.style.display = 'none';
-          }
-        }
+        renderPiholeStats(body.data, msg);
       })
       .catch(function () { setLoading(card, false); showError(card, function () { hydratePiholeScope(scope); }); });
   }
@@ -470,7 +480,7 @@
         hydratePiholeScope(piScopeActive);
       });
     }
-    hydratePiholeScope('device');
+    hydratePiholeScope(piScopeActive);
   }
 
   // ─── Boot ───────────────────────────────────────────────────────────────────

src/middleware/portalOwner.js

@@ -25,6 +25,10 @@ function portalOwner(req, _res, next) {
   if (req.portalLoggedIn) {
     req.portalOwnerId = req.session.userId;
     req.portalOwnerSource = 'session';
+  // Kiosk trade-off (Design §4.6): when device-trust is admin-enabled, co-users of a
+  // shared peer IP see the owner's aggregation without logging in. This is intentional
+  // kiosk behaviour — secured by default-off + admin opt-in + the mandatory help text
+  // shown in the admin UI. Session login always takes precedence (checked above).
   } else if (trustEnabled() && req.portalPeerId != null) {
     const uid = ownerOfPeer(req.portalPeerId);
     req.portalOwnerId = uid;

src/routes/api/portal.js

@@ -22,6 +22,10 @@ function unidentified(res) {
   return res.json({ ok: true, data: null, reason: 'unidentified' });
 }
 
+function piholeUnavailable(cache) {
+  return !license.hasFeature('pihole_integration') || !cache.instances || cache.instances.length === 0;
+}
+
 // Convert JS Date to 'YYYY-MM-DD HH:MM:SS' (UTC, no ms) for comparison
 // with SQLite's datetime('now') output format.
 function toSQLite(date) {
@@ -138,7 +142,7 @@ router.get('/pihole', (req, res) => {
     // Reuse the existing Pro feature gate; INLINE (not requireFeature middleware) so the
     // frontend gets a clean "hide" signal (data:null) instead of a 403.
     const cache = pihole.getCache();
-    if (!license.hasFeature('pihole_integration') || !cache.instances || cache.instances.length === 0) {
+    if (piholeUnavailable(cache)) {
       return res.json({ ok: true, data: null, reason: 'unavailable' });
     }
     if (req.portalPeerId == null) return unidentified(res); // reuse the existing helper (siblings do too)
@@ -167,15 +171,21 @@ router.get('/pihole/owner', (req, res) => {
   try {
     if (!portalConfig().widgets.pihole) return res.status(404).json({ ok: false });
     const cache = pihole.getCache();
-    if (!license.hasFeature('pihole_integration') || !cache.instances || cache.instances.length === 0) {
+    if (piholeUnavailable(cache)) {
       return res.json({ ok: true, data: null, reason: 'unavailable' });
     }
     if (req.portalOwnerId == null) return res.json({ ok: true, data: null, reason: 'no_owner' });
     if (cache.attribution === 'collapsed') return res.json({ ok: true, data: null, reason: 'collapsed' });
     const ownerPeerIds = new Set(peers.peersOfOwner(req.portalOwnerId)); // owner id NEVER from req body/query
-    let allowed = 0, blocked = 0; const seen = new Set();
-    for (const c of (cache.topClients || []))        if (ownerPeerIds.has(c.peerId)) { allowed += c.count; seen.add(c.peerId); }
-    for (const c of (cache.topClientsBlocked || [])) if (ownerPeerIds.has(c.peerId)) { blocked += c.count; seen.add(c.peerId); }
+    let allowed = 0;
+    let blocked = 0;
+    const seen = new Set();
+    for (const c of (cache.topClients || [])) {
+      if (ownerPeerIds.has(c.peerId)) { allowed += c.count; seen.add(c.peerId); }
+    }
+    for (const c of (cache.topClientsBlocked || [])) {
+      if (ownerPeerIds.has(c.peerId)) { blocked += c.count; seen.add(c.peerId); }
+    }
     if (seen.size === 0) return res.json({ ok: true, data: null, reason: 'no_data' });
     const total = allowed + blocked;
     const blockedPct = total ? Math.round((blocked / total) * 100) : 0;
@@ -190,7 +200,7 @@ router.get('/pihole/household', (req, res) => {
   try {
     if (!portalConfig().widgets.pihole) return res.status(404).json({ ok: false });
     const cache = pihole.getCache();
-    if (!license.hasFeature('pihole_integration') || !cache.instances || cache.instances.length === 0) {
+    if (piholeUnavailable(cache)) {
       return res.json({ ok: true, data: null, reason: 'unavailable' });
     }
     if (!req.portalLoggedIn) return res.json({ ok: true, data: null, reason: 'login_required' }); // trust switch never relaxes household

src/services/piholeSync.js

@@ -78,9 +78,10 @@ function createSync(deps) {
         client.getHistory(),
         client.getTopDomains(true),
         client.getTopClients(),
-        // NOTE: getTopClients(true) requires Pi-hole v6 FTL. A v5 instance throws here,
-        // which rejects pull() entirely → Promise.allSettled marks it connected:false.
-        client.getTopClients(true),
+        // NOTE: getTopClients(true) requires Pi-hole v6 FTL. On a v5 instance this call
+        // throws; the .catch(() => []) degrades silently to an empty blocked list instead
+        // of rejecting pull() entirely (which would mark the instance connected:false).
+        client.getTopClients(true).catch(() => []),
         client.getQueryTypes(),
         client.getBlocking(),
       ]);

tests/pihole_sync_v5_degrade.test.js

@@ -0,0 +1,65 @@
+'use strict';
+const { test } = require('node:test');
+const assert = require('node:assert/strict');
+const { createSync } = require('../src/services/piholeSync');
+
+// Simulates a Pi-hole v5 client: getTopClients(blocked=true) throws (v6-only call),
+// while all other methods return minimal valid data.
+function fakeV5Client(id) {
+  return {
+    id,
+    getSummary:    async () => ({ queries:{total:5,blocked:1}, gravity:{domains_being_blocked:3}, clients:{active:1} }),
+    getHistory:    async () => [],
+    getTopDomains: async () => [],
+    getTopClients: async (blockedArg = false) => {
+      if (blockedArg) throw new Error('v5 API: unknown endpoint');
+      return [{ ip: '10.8.0.1', count: 5 }];
+    },
+    getQueryTypes: async () => ({}),
+    getBlocking:   async () => ({ blocking: true }),
+  };
+}
+
+test('v5 instance: getTopClients(true) rejection degrades topClientsBlocked to [] without marking instance disconnected', async () => {
+  const client = fakeV5Client('p1');
+  const sync = createSync({
+    loadConfig: () => ({ enabled: true, sync_interval_sec: 30, manage_dns_chain: false, instances: [{ id: 'p1' }] }),
+    clientFactory: () => client,
+    peersProvider: () => [],
+    eventBus: { publish() {} },
+    dnsChain: { apply() {}, revert() {} },
+    loadDesired: () => null,
+  });
+  const cache = await sync.syncOnce();
+  assert.equal(cache.instances[0].connected, true, 'v5 instance must stay connected:true');
+  assert.deepEqual(cache.topClientsBlocked, [], 'topClientsBlocked must degrade to [] on v5');
+  // Remaining data must still be populated — the instance was not dropped
+  assert.equal(cache.summary.queries.total, 5, 'summary must be populated from v5 data');
+  assert.ok(Array.isArray(cache.topClients), 'topClients (allowed) must still be populated');
+  assert.ok(cache.topClients.length > 0, 'topClients must contain entries from v5');
+});
+
+test('v5 instance alongside a failing instance: v5 stays connected, other stays disconnected', async () => {
+  const v5 = fakeV5Client('v5');
+  const bad = {
+    id: 'bad',
+    getSummary:    async () => { throw new Error('down'); },
+    getHistory:    async () => { throw new Error('down'); },
+    getTopDomains: async () => { throw new Error('down'); },
+    getTopClients: async () => { throw new Error('down'); },
+    getQueryTypes: async () => { throw new Error('down'); },
+    getBlocking:   async () => { throw new Error('down'); },
+  };
+  const sync = createSync({
+    loadConfig: () => ({ enabled: true, sync_interval_sec: 30, manage_dns_chain: false, instances: [{ id: 'v5' }, { id: 'bad' }] }),
+    clientFactory: (inst) => (inst.id === 'v5' ? v5 : bad),
+    peersProvider: () => [],
+    eventBus: { publish() {} },
+    dnsChain: { apply() {}, revert() {} },
+    loadDesired: () => null,
+  });
+  const cache = await sync.syncOnce();
+  assert.equal(cache.instances.find(i => i.id === 'v5').connected, true);
+  assert.equal(cache.instances.find(i => i.id === 'bad').connected, false);
+  assert.deepEqual(cache.topClientsBlocked, [], 'topClientsBlocked empty since v5 has none');
+});