fix: address final review — cloud TLS, MAC guard, honest ISP metrics

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-13 22:21:43 +02:00
1 parent 367eb82db5
commit d0865c3b15
@@ -1,5 +1,5 @@
 // @vitest-environment node
-import { Response } from "undici";
+import { fetch as platformFetch, Response } from "undici";
 import { beforeEach, describe, expect, test, vi } from "vitest";

 vi.hoisted(() => {
@@ -17,7 +17,21 @@ vi.mock("@homarr/core/infrastructure/http", () => ({
  fetchWithTrustedCertificatesAsync: vi.fn(),
 }));

+// Les deux transports sont mockés séparément parce qu'ils *doivent* rester
+// séparés : le local passe par les certificats de confiance de Homarr, le cloud
+// par le `fetch` de la plateforme. Mocker les deux permet d'affirmer qu'aucune
+// requête vers api.ui.com n'emprunte le transport local — la garantie du §5.3.
+vi.mock("undici", async (importOriginalAsync) => {
+  const actual = await importOriginalAsync<typeof import("undici")>();
+  return { ...actual, fetch: vi.fn() };
+});
+
+/** Transport local : certificats de confiance, console auto-signée. */
 const mockFetch = vi.mocked(fetchWithTrustedCertificatesAsync);
+/** Transport cloud : `fetch` de la plateforme, certificat public ordinaire. */
+const mockCloudFetch = vi.mocked(platformFetch);
+
+const CLOUD_ORIGIN = "https://api.ui.com";

 const GATEWAY_MAC = "aa:bb:cc:11:22:33";

@@ -59,7 +73,6 @@ const localRoutes: Record<string, unknown> = {
    memoryUtilizationPct: 63.1,
    uplink: { txRateBps: 14140912, rxRateBps: 3346784 },
  },
-  "/sites/site-1/vpn/servers": page([{ id: "v1", name: "vpn", enabled: true }]),
 };

 const routeLocal = (rawUrl: string): Response | null => {
@@ -83,8 +96,14 @@ const hybridSecrets: IntegrationSecret[] = [
  { kind: "siteManagerApiKey", value: "cloud-key" },
 ];

+/** Toute URL demandée au transport local, quel que soit l'appelant. */
+const localTransportUrls = () => mockFetch.mock.calls.map(([url]) => String(url));
+/** Toute URL demandée au transport cloud. */
+const cloudTransportUrls = () => mockCloudFetch.mock.calls.map(([url]) => String(url));
+
 beforeEach(() => {
  mockFetch.mockReset();
+  mockCloudFetch.mockReset();
  UnifiSmApi.clearCache();
 });

@@ -102,19 +121,23 @@ describe("UnifiOsIntegration getUnifiOsSummaryAsync", () => {
    expect(summary.gateway).toMatchObject({ name: "Gateway", uptimeSec: 1009049, cpuPct: 19.1 });
  });

-  test("leaves the ISP block null when no Site Manager key is configured", async () => {
+  test("leaves the ISP block null and touches no transport at all without a Site Manager key", async () => {
    mockFetch.mockImplementation(async (url) => routeLocal(String(url)) ?? jsonResponse({}, 404));

    const summary = await createIntegration(localOnlySecrets).getUnifiOsSummaryAsync();

    expect(summary.wan.isp).toBeNull();
-    expect(mockFetch.mock.calls.every(([url]) => !String(url).startsWith("https://api.ui.com"))).toBe(true);
+    // Le transport cloud est le seul chemin vers api.ui.com : ne pas l'appeler
+    // du tout prouve qu'aucune requête cloud n'a été émise.
+    expect(mockCloudFetch).not.toHaveBeenCalled();
+    expect(localTransportUrls().every((url) => !url.startsWith(CLOUD_ORIGIN))).toBe(true);
  });

-  test("fills the ISP block when a Site Manager key is configured", async () => {
-    mockFetch.mockImplementation(async (url) => {
+  test("fills the ISP block from the cloud transport, never from the trusted-certificate one", async () => {
+    mockFetch.mockImplementation(async (url) => routeLocal(String(url)) ?? jsonResponse({}, 404));
+    mockCloudFetch.mockImplementation(async (url) => {
      const raw = String(url);
-      if (raw.startsWith("https://api.ui.com/v1/sites")) {
+      if (raw.startsWith(`${CLOUD_ORIGIN}/v1/sites`)) {
        return jsonResponse({
          data: [
            {
@@ -144,13 +167,34 @@ describe("UnifiOsIntegration getUnifiOsSummaryAsync", () => {
          },
        });
      }
-      return routeLocal(raw) ?? jsonResponse({}, 404);
+      return jsonResponse({}, 404);
    });

    const summary = await createIntegration(hybridSecrets).getUnifiOsSummaryAsync();

    expect(summary.wan.isp).toMatchObject({ name: "Example ISP", latencyMs: 11, externalIp: "203.0.113.10" });
    expect(summary.wan.isp?.measuredAt).toEqual(new Date("2026-09-08T09:10:00Z"));
+
+    // Le bloc ISP n'a pu être rempli que par des réponses servies par le mock
+    // cloud : les deux appels y sont donc bien passés.
+    expect(cloudTransportUrls()).toEqual([`${CLOUD_ORIGIN}/v1/sites`, `${CLOUD_ORIGIN}/v1/isp-metrics/5m/query`]);
+    // Le cœur du §5.3 : la clé Site Manager ne voyage jamais sur le transport
+    // qui embarque les autorités personnalisées de l'administrateur.
+    expect(localTransportUrls().every((url) => url.startsWith("https://console.example.test"))).toBe(true);
+  });
+
+  test("never sends the Site Manager key through the trusted-certificate transport", async () => {
+    mockFetch.mockImplementation(async (url) => routeLocal(String(url)) ?? jsonResponse({}, 404));
+    mockCloudFetch.mockImplementation(async () => jsonResponse({}, 500));
+
+    await createIntegration(hybridSecrets).getUnifiOsSummaryAsync();
+
+    const localKeys = mockFetch.mock.calls.map(
+      ([, init]) => (init?.headers as Record<string, string> | undefined)?.["X-API-KEY"],
+    );
+
+    expect(localKeys.every((key) => key === "local-key")).toBe(true);
+    expect(localKeys).not.toContain("cloud-key");
  });

  test("degrades only the clients block when the clients endpoint fails", async () => {
@@ -127,6 +127,24 @@ describe("matchCloudSite", () => {
  test("returns null when the local gateway is unknown", () => {
    expect(matchCloudSite(null, [siteA, siteB])).toBeNull();
  });
+
+  // `normalizeMacAddress` réduit "", "unknown" et "null" à la même chaîne vide.
+  // Sans garde, ces valeurs se correspondraient entre elles et le widget
+  // afficherait le réseau d'un autre compte comme celui de l'utilisateur.
+  test("returns null when the local MAC normalizes to nothing", () => {
+    const placeholder = { siteId: "placeholder", meta: { gatewayMac: "unknown" } };
+
+    for (const localMac of ["", "unknown", "null"]) {
+      expect(matchCloudSite(localMac, [placeholder, siteA, siteB])).toBeNull();
+    }
+  });
+
+  test("skips cloud sites whose gateway MAC normalizes to nothing", () => {
+    const placeholder = { siteId: "placeholder", meta: { gatewayMac: "unknown" } };
+
+    expect(matchCloudSite("aa:bb:cc:11:22:33", [placeholder, siteA])?.siteId).toBe("a");
+    expect(matchCloudSite("11:22:33:44:55:66", [placeholder])).toBeNull();
+  });
 });

 describe("unifiOsClientSchema", () => {
@@ -112,6 +112,34 @@ describe("UnifiSmApi getIspMetricsAsync", () => {
    expect(metrics?.measuredAt).toEqual(new Date("2026-09-08T09:10:00Z"));
  });

+  // Ubiquiti omet la latence pour les périodes où le WAN était coupé. Un `0`
+  // fabriqué s'afficherait comme « 0 ms » au pire moment possible.
+  test("passes missing cloud measurements through as null instead of zero", async () => {
+    const fetchAsync = vi.fn(async (url: string, _init?: RequestInit) => {
+      if (!url.includes("isp-metrics")) return jsonResponse(sitesBody);
+
+      return jsonResponse({
+        data: {
+          metrics: [
+            {
+              periods: [{ metricTime: "2026-09-08T09:10:00Z", data: { wan: { ispName: "Example ISP" } } }],
+            },
+          ],
+        },
+      });
+    });
+    const api = new UnifiSmApi("cloud-key", fetchAsync as never);
+
+    const metrics = await api.getIspMetricsAsync(GATEWAY_MAC);
+
+    expect(metrics).toMatchObject({
+      latencyMs: null,
+      maxLatencyMs: null,
+      packetLossPct: null,
+      uptimePct: null,
+    });
+  });
+
  test("returns null when no cloud site matches, never another account site", async () => {
    const { api } = createApi();

@@ -1,4 +1,5 @@
 import { ResponseError } from "@homarr/common/server";
+import type { RequestInit, Response } from "undici";
 import type { z } from "zod/v4";

 import type { UnifiOsClientCounts, UnifiOsDevice, UnifiOsDeviceStatistics, UnifiOsSite } from "./unifi-os-types";
@@ -13,6 +14,11 @@ import {
  unifiOsSiteSchema,
 } from "./unifi-os-types";

+/**
+ * Déclaré sur les types d'undici, pas sur ceux du DOM : le dépôt n'utilise que
+ * `undici` côté serveur. Aligner les deux évite les `as unknown as` qui
+ * désactivaient le contrôle de type à la frontière du transport.
+ */
 export type UnifiOsFetch = (url: string, init?: RequestInit & { timeout?: number }) => Promise<Response>;

 const INTEGRATION_PATH = "/proxy/network/integration/v1";
@@ -1,3 +1,5 @@
+import { fetch as platformFetch } from "undici";
+
 import { fetchWithTrustedCertificatesAsync } from "@homarr/core/infrastructure/http";

 import type { IntegrationTestingInput } from "../base/integration";
@@ -11,13 +13,37 @@ import { UnifiSmApi } from "./unifi-sm-api";
 import type { UnifiOsDeviceStatistics, UnifiOsSummary } from "./unifi-os-types";
 import { findGateway, summarizeDevices } from "./unifi-os-types";

+/**
+ * Transport du cloud Site Manager (§5.3).
+ *
+ * `fetchWithTrustedCertificatesAsync` ajoute **toutes** les autorités
+ * personnalisées téléversées par l'administrateur Homarr, et installe un
+ * `checkServerIdentity` qui saute la vérification du nom d'hôte pour les
+ * empreintes épinglées. Une autorité ajoutée pour faire accepter le certificat
+ * auto-signé de la console deviendrait alors une ancre de confiance valable
+ * pour `api.ui.com` aussi : son détenteur pourrait intercepter l'appel cloud et
+ * récupérer la clé Site Manager, qui lit tous les sites du compte Ubiquiti.
+ *
+ * `api.ui.com` présente un certificat public ordinaire : le `fetch` de la
+ * plateforme suffit, et c'est le seul qui soit sûr ici. Le `timeout` du client
+ * est traduit en `AbortSignal`, `fetch` ne connaissant pas cette option.
+ */
+const fetchCloudAsync: UnifiOsFetch = async (url, init) => {
+  const { timeout, ...requestInit } = init ?? {};
+
+  return await platformFetch(url, {
+    ...requestInit,
+    ...(timeout === undefined ? {} : { signal: AbortSignal.timeout(timeout) }),
+  });
+};
+
 export class UnifiOsIntegration extends Integration implements UnifiOsSummaryIntegration {
  public async getUnifiOsSummaryAsync(): Promise<UnifiOsSummary> {
-    return await this.buildSummaryAsync(fetchWithTrustedCertificatesAsync as unknown as UnifiOsFetch);
+    return await this.buildSummaryAsync(fetchWithTrustedCertificatesAsync);
  }

  protected async testingAsync({ fetchAsync }: IntegrationTestingInput): Promise<TestingResult> {
-    const api = this.createLocalApi(fetchAsync as unknown as UnifiOsFetch);
+    const api = this.createLocalApi(fetchAsync);

    try {
      await api.getInfoAsync();
@@ -69,7 +95,7 @@ export class UnifiOsIntegration extends Integration implements UnifiOsSummaryInt
      }
    }

-    const isp = await this.readIspMetricsAsync(fetchAsync, gatewayDevice?.macAddress ?? null);
+    const isp = await this.readIspMetricsAsync(gatewayDevice?.macAddress ?? null);

    return {
      site: { id: site.id, name: site.name, applicationVersion: info.applicationVersion },
@@ -99,12 +125,13 @@ export class UnifiOsIntegration extends Integration implements UnifiOsSummaryInt

  /**
   * Le cloud n'est jamais interrogé sans clé configurée : le module n'est même
-   * pas instancié, ce qui rend impossible un appel réseau accidentel.
+   * pas instancié, ce qui rend impossible un appel réseau accidentel. Il reçoit
+   * `fetchCloudAsync`, jamais le transport à certificats de confiance du local.
   */
-  private async readIspMetricsAsync(fetchAsync: UnifiOsFetch, gatewayMac: string | null) {
+  private async readIspMetricsAsync(gatewayMac: string | null) {
    if (!this.hasSecretValue("siteManagerApiKey")) return null;

-    const smApi = new UnifiSmApi(this.getSecretValue("siteManagerApiKey"), fetchAsync);
+    const smApi = new UnifiSmApi(this.getSecretValue("siteManagerApiKey"), fetchCloudAsync);
    return await smApi.getIspMetricsAsync(gatewayMac);
  }
 }
@@ -41,12 +41,6 @@ export const unifiOsDeviceStatisticsSchema = z.looseObject({
  uplink: z.looseObject({ txRateBps: z.number(), rxRateBps: z.number() }).optional(),
 });

-export const unifiOsVpnServerSchema = z.looseObject({
-  id: z.string(),
-  name: z.string().optional(),
-  enabled: z.boolean(),
-});
-
 /** Enveloppe de pagination commune à tous les endpoints de liste. */
 export const createUnifiOsPageSchema = <TItem extends z.ZodType>(item: TItem) =>
  z.looseObject({
@@ -129,10 +123,13 @@ export interface UnifiOsGateway {
 export interface UnifiOsIspMetrics {
  name: string;
  externalIp: string | null;
-  latencyMs: number;
-  maxLatencyMs: number;
-  packetLossPct: number;
-  uptimePct: number;
+  /** Nullables : le cloud omet ces mesures pour les périodes où le WAN
+   *  était coupé. Un `0` fabriqué s'afficherait comme « 0 ms » à côté d'un
+   *  voyant vert, au moment précis où la connexion est tombée. */
+  latencyMs: number | null;
+  maxLatencyMs: number | null;
+  packetLossPct: number | null;
+  uptimePct: number | null;
  /** Horodatage de la mesure : la donnée cloud accuse ~27 min de retard. */
  measuredAt: Date;
 }
@@ -216,6 +213,13 @@ export const findGateway = (devices: UnifiOsDevice[]): UnifiOsDevice | null =>
 * Une clé Site Manager voit tous les sites du compte, y compris ceux d'autres
 * consoles. Sans correspondance stricte sur le MAC du gateway, on afficherait
 * la latence d'un autre réseau. Renvoyer null est le seul comportement sûr.
+ *
+ * `normalizeMacAddress` retire tout caractère hors `[0-9a-f]` : `""`,
+ * `"unknown"` et `"null"` se réduisent donc à la même chaîne vide. Sans les deux
+ * gardes ci-dessous, un gateway local sans MAC exploitable correspondrait à
+ * n'importe quel site cloud dont le `gatewayMac` est un marqueur de la même
+ * forme — et le widget afficherait le FAI, l'IP publique et la latence du réseau
+ * d'un voisin de compte comme s'ils étaient ceux de l'utilisateur.
 */
 export const matchCloudSite = <TSite extends { meta: { gatewayMac?: string } }>(
  gatewayMac: string | null,
@@ -224,10 +228,14 @@ export const matchCloudSite = <TSite extends { meta: { gatewayMac?: string } }>(
  if (gatewayMac === null) return null;

  const normalized = normalizeMacAddress(gatewayMac);
+  if (normalized === "") return null;

  return (
-    sites.find(
-      (site) => site.meta.gatewayMac !== undefined && normalizeMacAddress(site.meta.gatewayMac) === normalized,
-    ) ?? null
+    sites.find((site) => {
+      if (site.meta.gatewayMac === undefined) return false;
+
+      const candidate = normalizeMacAddress(site.meta.gatewayMac);
+      return candidate !== "" && candidate === normalized;
+    }) ?? null
  );
 };
@@ -126,13 +126,17 @@ export class UnifiSmApi {
      .map((entry) => entry.externalIp)
      .find((ip) => ip !== undefined);

+    // Ubiquiti omet ces mesures pour les périodes où le WAN était coupé. Les
+    // remplacer par `0` fabriquerait un « 0 ms » rassurant à l'instant précis où
+    // la connexion est tombée : on laisse passer `null`, que le widget affiche
+    // comme une absence.
    return {
      name: wan.ispName ?? site.statistics?.ispInfo?.name ?? "",
      externalIp: externalIp ?? null,
-      latencyMs: wan.avgLatency ?? 0,
-      maxLatencyMs: wan.maxLatency ?? 0,
-      packetLossPct: wan.packetLoss ?? 0,
-      uptimePct: wan.uptime ?? site.statistics?.percentages?.wanUptime ?? 0,
+      latencyMs: wan.avgLatency ?? null,
+      maxLatencyMs: wan.maxLatency ?? null,
+      packetLossPct: wan.packetLoss ?? null,
+      uptimePct: wan.uptime ?? site.statistics?.percentages?.wanUptime ?? null,
      measuredAt: new Date(latest.metricTime),
    };
  }
@@ -4236,9 +4236,10 @@
      },
      "card": {
        "internet": "Internet",
-        "isp": "ISP",
        "latency": "{value} ms",
+        "packetLoss": "{value} % loss",
        "measuredAt": "Measured at {time}",
+        "stale": "Local data could not be refreshed, showing the last known values",
        "clients": "Clients",
        "clientBreakdown": "{total} · {wired} wired · {wireless} Wi-Fi",
        "devices": "Devices",
@@ -7,6 +7,7 @@ import dayjs from "dayjs";
 import relativeTime from "dayjs/plugin/relativeTime";

 import { clientApi } from "@homarr/api/client";
+import { useIntegrationConnected } from "@homarr/common";
 import { useI18n } from "@homarr/translation/client";

 import type { WidgetComponentProps } from "../definition";
@@ -15,6 +16,18 @@ dayjs.extend(relativeTime);

 const EMPTY_VALUE = "—";

+/**
+ * Le request-handler met les compteurs locaux en cache 60 s et le widget se
+ * rafraîchit toutes les 60 s : une donnée de moins de 2 min 30 est normale.
+ * Au-delà, la console n'a pas répondu et `fallbackToStaleOnError` sert la
+ * dernière valeur connue — ce que l'utilisateur doit voir, sans quoi la source
+ * périmée paraîtrait plus fraîche que le bloc cloud, qui affiche son âge.
+ */
+const LOCAL_STALE_AFTER_MS = 150_000;
+
+/** Identité stable : évite de relancer l'effet du hook à chaque rendu. */
+const NEVER_UPDATED = new Date(0);
+
 const formatBitsPerSecond = (bps: number): string => {
  const units = ["b/s", "Kb/s", "Mb/s", "Gb/s"];
  let value = bps;
@@ -40,6 +53,8 @@ export default function UnifiOsWidget({ integrationIds, options }: WidgetCompone
  const { data: results = [] } = clientApi.widget.unifiOs.summary.useQuery({ integrationIds });

  const summary = useMemo(() => results[0]?.summary ?? null, [results]);
+  const updatedAt = results[0]?.updatedAt ?? NEVER_UPDATED;
+  const isLocalFresh = useIntegrationConnected(updatedAt, { timeout: LOCAL_STALE_AFTER_MS });

  if (summary === null) {
    return (
@@ -52,6 +67,7 @@ export default function UnifiOsWidget({ integrationIds, options }: WidgetCompone
  }

  const wanColor = summary.wan.state === "online" ? "green" : summary.wan.state === "offline" ? "red" : "gray";
+  const isp = summary.wan.isp;

  return (
    <Stack h="100%" p="sm" gap="xs" justify="space-between">
@@ -62,6 +78,17 @@ export default function UnifiOsWidget({ integrationIds, options }: WidgetCompone
        <Text size="xs" c="dimmed" tt="uppercase" fw={600} truncate>
          {summary.site.name}
        </Text>
+        {/* Second rôle du badge de fraîcheur (§8) : une console injoignable est
+            servie depuis le cache périmé. Sans ce signal, les compteurs locaux
+            paraîtraient plus frais que le bloc cloud, qui affiche son âge.
+            Gris et discret : il ne doit pas concurrencer le bandeau WAN. */}
+        {!isLocalFresh && (
+          <Tooltip label={t("widget.unifiOs.card.stale")}>
+            <Badge size="xs" variant="light" color="gray" style={{ flexShrink: 0 }}>
+              {dayjs(updatedAt).fromNow()}
+            </Badge>
+          </Tooltip>
+        )}
      </Group>

      <Stack
@@ -83,40 +110,40 @@ export default function UnifiOsWidget({ integrationIds, options }: WidgetCompone
          </Text>
        </Group>

-        {options.showIsp && (
+        {/* Bloc ISP masqué quand il n'y a pas de donnée cloud (§8) : sans clé
+            Site Manager, cloud injoignable, ou aucun site correspondant au MAC
+            du gateway. Une absence que l'intégration a positivement établie ne
+            se déguise pas en valeur inconnue. */}
+        {options.showIsp && isp !== null && (
          <Group justify="space-between" wrap="nowrap" gap="xs">
-            {summary.wan.isp === null ? (
-              <>
-                <Text size="xs" c="dimmed">
-                  {t("widget.unifiOs.card.isp")}
+            <Text size="xs" c="dimmed" truncate>
+              {isp.name}
+              {isp.externalIp !== null ? ` · ${isp.externalIp}` : ""}
+            </Text>
+            <Group gap={6} wrap="nowrap">
+              {/* Le cloud omet la latence pour les périodes de coupure : afficher
+                  un tiret plutôt qu'un « 0 ms » qui rassurerait à tort. */}
+              <Text size="xs">
+                {isp.latencyMs === null ? EMPTY_VALUE : t("widget.unifiOs.card.latency", { value: `${isp.latencyMs}` })}
+              </Text>
+              {isp.packetLossPct !== null && (
+                <Text size="xs" c={isp.packetLossPct > 0 ? "yellow" : undefined}>
+                  {t("widget.unifiOs.card.packetLoss", { value: `${isp.packetLossPct}` })}
                </Text>
-                <Text size="xs" c="dimmed">
-                  {EMPTY_VALUE}
-                </Text>
-              </>
-            ) : (
-              <>
-                <Text size="xs" c="dimmed" truncate>
-                  {summary.wan.isp.name}
-                  {summary.wan.isp.externalIp !== null ? ` · ${summary.wan.isp.externalIp}` : ""}
-                </Text>
-                <Group gap={6} wrap="nowrap">
-                  <Text size="xs">{t("widget.unifiOs.card.latency", { value: `${summary.wan.isp.latencyMs}` })}</Text>
-                  {/* The cloud metrics behind this badge lag ~27 minutes: it must
-                      never look live, hence the relative-time badge instead of a
-                      bare number. */}
-                  <Tooltip
-                    label={t("widget.unifiOs.card.measuredAt", {
-                      time: dayjs(summary.wan.isp.measuredAt).format("HH:mm"),
-                    })}
-                  >
-                    <Badge size="xs" variant="light" color="yellow">
-                      {dayjs(summary.wan.isp.measuredAt).fromNow()}
-                    </Badge>
-                  </Tooltip>
-                </Group>
-              </>
-            )}
+              )}
+              {/* The cloud metrics behind this badge lag ~27 minutes: it must
+                  never look live, hence the relative-time badge instead of a
+                  bare number. */}
+              <Tooltip
+                label={t("widget.unifiOs.card.measuredAt", {
+                  time: dayjs(isp.measuredAt).format("HH:mm"),
+                })}
+              >
+                <Badge size="xs" variant="light" color="yellow">
+                  {dayjs(isp.measuredAt).fromNow()}
+                </Badge>
+              </Tooltip>
+            </Group>
          </Group>
        )}
      </Stack>