fix code review findings

docs/backend/oobe-bootstrap-go-flow.md

@@ -84,14 +84,14 @@ serve identical scripts during the transition.
 # Rate limiting behind Front Door
 
 The endpoint self-limits per client IP (`bootstrap-script:{ip}`, 20/min via
-`ClientIpExtractor.GetTrustedClientIp`) — anonymous routes are skipped by
+`ClientIpExtractor.GetRateLimitClientIp`) — anonymous routes are skipped by
 `UserRateLimitMiddleware` and must bring their own limiter. Because traffic
-arrives through Front Door, the rightmost X-Forwarded-For hop may resolve to
-AFD egress IPs; whether buckets collapse (the shared-bucket 429 incident class)
-must be verified post-deploy from two networks. If they do, the fix is an
-AFD-aware extraction honoring `X-Azure-ClientIP` only when `X-Azure-FDID`
-matches our profile — NOT `GetClientEgressIp`, which trusts the header
-unconditionally and is documented as unfit for rate-limit keys.
+arrives through Front Door, the rightmost X-Forwarded-For hop resolves to AFD
+egress IPs and buckets would collapse (the shared-bucket 429 incident class);
+the AFD-aware extraction therefore honors `X-Azure-ClientIP` only when
+`X-Azure-FDID` matches the `FrontDoorProfileId` app setting (fail-closed to
+the trusted-hop path otherwise). Verified post-deploy 2026-07-29: real client
+IPs are keyed, not AFD egress.
 
 Front Door route caching is DISABLED for this route: the body inlines the
 bearer token, so a cache hit would replay one requester's token to another.

src/Backend/AutopilotMonitor.Functions.Tests/DistressValidationTests.cs

@@ -120,6 +120,77 @@ public class DistressValidationTests
         Assert.Equal("203.0.113.5", ClientIpExtractor.ExtractTrustedHop("10.0.0.1, 203.0.113.5, "));
     }
 
+    // =========================================================================
+    // ClientIpExtractor.GetHeaderFromBindingDataJson — Flex Consumption strips the
+    // X-Forwarded-* family from the proxied HTTP request; the host passes the
+    // ORIGINAL header set as trigger metadata (BindingData["Headers"], JSON object).
+    // Malformed metadata must fail soft (null), never throw into request handling.
+    // =========================================================================
+
+    [Fact]
+    public void GetHeaderFromBindingDataJson_XffPresent_ReturnsValue()
+    {
+        var json = """{"Host":"example.com","X-Forwarded-For":"1.1.1.1, 203.0.113.5:443"}""";
+        Assert.Equal(
+            "1.1.1.1, 203.0.113.5:443",
+            ClientIpExtractor.GetHeaderFromBindingDataJson(json, "X-Forwarded-For"));
+    }
+
+    [Fact]
+    public void GetHeaderFromBindingDataJson_HeaderNameCaseInsensitive()
+    {
+        var json = """{"x-forwarded-for":"203.0.113.5"}""";
+        Assert.Equal(
+            "203.0.113.5",
+            ClientIpExtractor.GetHeaderFromBindingDataJson(json, "X-Forwarded-For"));
+    }
+
+    [Theory]
+    [InlineData(null)]
+    [InlineData("")]
+    [InlineData("not json")]
+    [InlineData("[]")]                                // JSON but not an object
+    [InlineData("""{"Host":"example.com"}""")]        // header absent
+    [InlineData("""{"X-Forwarded-For":42}""")]        // non-string value
+    public void GetHeaderFromBindingDataJson_MissingOrMalformed_ReturnsNull(string? json)
+    {
+        Assert.Null(ClientIpExtractor.GetHeaderFromBindingDataJson(json, "X-Forwarded-For"));
+    }
+
+    // =========================================================================
+    // ClientIpExtractor.RemoteIpToString — transport-level fallback. On Flex the
+    // worker's TCP peer is the host's LOCAL proxy: loopback means "not a client"
+    // and must map to "unknown", never be stored as a forensic SourceIp
+    // (production 2026-07-29: distress reports carried SourceIp 127.0.0.1).
+    // =========================================================================
+
+    [Fact]
+    public void RemoteIpToString_Null_ReturnsUnknown()
+    {
+        Assert.Equal("unknown", ClientIpExtractor.RemoteIpToString(null));
+    }
+
+    [Theory]
+    [InlineData("127.0.0.1")]
+    [InlineData("::1")]
+    [InlineData("::ffff:127.0.0.1")]  // IPv4-mapped loopback
+    public void RemoteIpToString_Loopback_ReturnsUnknown(string ip)
+    {
+        Assert.Equal("unknown", ClientIpExtractor.RemoteIpToString(System.Net.IPAddress.Parse(ip)));
+    }
+
+    [Fact]
+    public void RemoteIpToString_PublicIpv4_ReturnsIp()
+    {
+        Assert.Equal("203.0.113.5", ClientIpExtractor.RemoteIpToString(System.Net.IPAddress.Parse("203.0.113.5")));
+    }
+
+    [Fact]
+    public void RemoteIpToString_Ipv4MappedIpv6_FoldsToIpv4()
+    {
+        Assert.Equal("203.0.113.5", ClientIpExtractor.RemoteIpToString(System.Net.IPAddress.Parse("::ffff:203.0.113.5")));
+    }
+
     // =========================================================================
     // Sanitize — Control character stripping and truncation
     // =========================================================================

src/Backend/AutopilotMonitor.Functions/Functions/Ingest/ReportDistressFunction.cs

@@ -118,19 +118,15 @@ namespace AutopilotMonitor.Functions.Functions.Ingest
                     return req.CreateResponse(HttpStatusCode.OK);
 
                 // Gate 4: Rate limiting (IP + tenant + global circuit breaker).
-                // ClientIpExtractor returns the rightmost X-Forwarded-For hop (the one
-                // App Service appended) — never the leftmost, which is caller-controlled
-                // and would let a single attacker rotate past the per-IP throttle.
-                var clientIp = ClientIpExtractor.GetTrustedClientIp(req);
+                // One extraction serves both the bucket key and the stored forensic
+                // SourceIp: rightmost X-Forwarded-For hop (host-appended, never the
+                // caller-controlled leftmost), with X-Azure-ClientIP honored only behind
+                // a verified Front Door FDID — direct callers cannot spoof either value.
+                var clientIp = ClientIpExtractor.GetRateLimitClientIp(req);
                 var rateLimitResult = _rateLimitService.Check(clientIp, tenantId);
                 if (!rateLimitResult.IsAllowed)
                     return req.CreateResponse(HttpStatusCode.OK);
 
-                // For the STORED forensic SourceIp, prefer the real client egress IP. The trusted
-                // hop above (used as the un-spoofable rate-limit key) is Front Door's own egress IP
-                // behind Front Door, not the device — which is why stored SourceIp was unusable.
-                var sourceIp = ClientIpExtractor.GetClientEgressIp(req);
-
                 // Gate 5: Tenant existence check (cached — cheap O(1) lookup)
                 var (_, exists) = await _tenantConfigService.TryGetConfigurationAsync(tenantId);
                 if (!exists)
@@ -207,7 +203,7 @@ namespace AutopilotMonitor.Functions.Functions.Ingest
                     Message         = message,
                     AgentTimestamp  = report.Timestamp,
                     IngestedAt      = DateTime.UtcNow,
-                    SourceIp        = sourceIp,
+                    SourceIp        = clientIp,
                     CertSourceState = certSourceState,
                     CertThumbprint  = certThumbprint,
                     CertSubject     = certSubject,
@@ -318,7 +314,7 @@ namespace AutopilotMonitor.Functions.Functions.Ingest
                     ["HttpStatusCode"]  = report.HttpStatusCode?.ToString() ?? string.Empty,
                     ["Message"]         = message ?? string.Empty,
                     ["AgentTimestamp"]  = report.Timestamp.ToString("O"),
-                    ["SourceIp"]        = sourceIp ?? string.Empty,
+                    ["SourceIp"]        = clientIp,
                     ["CertSourceState"] = certSourceState ?? string.Empty,
                     ["CertThumbprint"]  = certThumbprint ?? string.Empty,
                     ["CertSubject"]     = certSubject ?? string.Empty,

src/Backend/AutopilotMonitor.Functions/Security/ClientIpExtractor.cs

@@ -1,6 +1,8 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Net;
+using System.Text.Json;
 using Microsoft.Azure.Functions.Worker;
 using Microsoft.Azure.Functions.Worker.Http;
 
@@ -36,28 +38,94 @@ namespace AutopilotMonitor.Functions.Security
                 if (hop != Unknown) return hop;
             }
 
-            // Flex Consumption does not reliably surface X-Forwarded-For to the isolated
-            // worker (observed 2026-07-28: every anonymous caller keyed as "unknown",
-            // collapsing all per-IP rate-limit buckets into one). The transport-level
-            // peer address is unspoofable and equals what the rightmost XFF hop would
-            // carry when no proxy chain exists; behind a fronting proxy it is that
-            // proxy's egress — coarse but honest, per the model above.
+            // Flex Consumption strips the X-Forwarded-* family from the HTTP request the
+            // host proxies to the isolated worker (Azure/azure-functions-host#10253/#11254;
+            // observed 2026-07-28: every anonymous caller keyed as "unknown"). The host does
+            // pass the ORIGINAL header set out-of-band as trigger metadata, so recover the
+            // rightmost XFF hop from there — same trust model, different transport.
+            var bindingHop = ExtractTrustedHop(
+                GetHeaderFromBindingDataJson(TryGetBindingHeadersJson(req), "X-Forwarded-For"));
+            if (bindingHop != Unknown) return bindingHop;
+
+            // Last resort: the transport-level peer address — unspoofable, and equal to the
+            // rightmost XFF hop when no proxy chain exists. On Flex the worker's peer is the
+            // host's local proxy, so loopback means "not a client" and must not be stored or
+            // used as a bucket key (a stored 127.0.0.1 is forensically misleading).
             var remote = req.FunctionContext.GetHttpContext()?.Connection.RemoteIpAddress;
-            if (remote == null) return Unknown;
-            if (remote.IsIPv4MappedToIPv6) remote = remote.MapToIPv4();
-            return remote.ToString();
+            return RemoteIpToString(remote);
         }
 
         /// <summary>
-        /// Rate-limit key for endpoints reached through Azure Front Door (e.g.
-        /// go.autopilotmonitor.com → /api/bootstrap/go). Behind AFD the trusted hop is
-        /// AFD's egress IP, so all callers would share a handful of buckets — one abuser
-        /// could starve every legitimate technician. AFD carries the true client IP in
-        /// X-Azure-ClientIP; it is honored ONLY when the request provably traversed OUR
-        /// profile: X-Azure-FDID (which AFD overwrites on pass-through, so a client
-        /// cannot inject it across AFD) must equal the FrontDoorProfileId app setting.
-        /// Fail-closed: unset setting, missing/mismatched/multi-valued FDID, or empty
-        /// client IP all fall back to <see cref="GetTrustedClientIp"/>.
+        /// Normalizes a transport-level peer address into a client-IP string:
+        /// null and loopback (the Functions host's local proxy on Flex) yield
+        /// <see cref="Unknown"/>; IPv4-mapped IPv6 is folded back to IPv4.
+        /// </summary>
+        internal static string RemoteIpToString(IPAddress? remote)
+        {
+            if (remote == null) return Unknown;
+            if (remote.IsIPv4MappedToIPv6) remote = remote.MapToIPv4();
+            if (IPAddress.IsLoopback(remote)) return Unknown;
+            return remote.ToString();
+        }
+
+        private static string? TryGetBindingHeadersJson(HttpRequestData req)
+        {
+            try
+            {
+                return req.FunctionContext.BindingContext.BindingData.TryGetValue("Headers", out var raw)
+                    ? raw as string
+                    : null;
+            }
+            catch
+            {
+                // Fail-soft: absent/odd trigger metadata must never break request handling.
+                return null;
+            }
+        }
+
+        /// <summary>
+        /// Reads one header (case-insensitive) from the JSON object the Functions host
+        /// places in BindingData["Headers"] — the request headers as the HOST saw them,
+        /// before the worker-bound proxy hop drops the X-Forwarded-* family.
+        /// Returns null for missing/malformed input.
+        /// </summary>
+        internal static string? GetHeaderFromBindingDataJson(string? headersJson, string headerName)
+        {
+            if (string.IsNullOrEmpty(headersJson)) return null;
+            try
+            {
+                using var doc = JsonDocument.Parse(headersJson);
+                if (doc.RootElement.ValueKind != JsonValueKind.Object) return null;
+                foreach (var prop in doc.RootElement.EnumerateObject())
+                {
+                    if (string.Equals(prop.Name, headerName, StringComparison.OrdinalIgnoreCase)
+                        && prop.Value.ValueKind == JsonValueKind.String)
+                    {
+                        return prop.Value.GetString();
+                    }
+                }
+                return null;
+            }
+            catch (JsonException)
+            {
+                return null;
+            }
+        }
+
+        /// <summary>
+        /// Finest-grained trustworthy client IP — used both as the rate-limit key for
+        /// endpoints reached through Azure Front Door (e.g. go.autopilotmonitor.com →
+        /// /api/bootstrap/go) and as the stored forensic SourceIp (distress reports).
+        /// Behind AFD the trusted hop is AFD's egress IP, so all callers would share a
+        /// handful of buckets — one abuser could starve every legitimate technician. AFD
+        /// carries the true client IP in X-Azure-ClientIP; it is honored ONLY when the
+        /// request provably traversed OUR profile: X-Azure-FDID (which AFD overwrites on
+        /// pass-through, so a client cannot inject it across AFD) must equal the
+        /// FrontDoorProfileId app setting. The same gate keeps stored forensic IPs
+        /// unspoofable when the Function App is reached directly, where anyone could
+        /// send an X-Azure-ClientIP header. Fail-closed: unset setting,
+        /// missing/mismatched/multi-valued FDID, or empty client IP all fall back to
+        /// <see cref="GetTrustedClientIp"/>.
         /// </summary>
         public static string GetRateLimitClientIp(HttpRequestData? req)
         {
@@ -96,28 +164,6 @@ namespace AutopilotMonitor.Functions.Security
             return single != null && string.Equals(single, expected, StringComparison.OrdinalIgnoreCase);
         }
 
-        /// <summary>
-        /// Best-effort REAL client egress IP for forensic/diagnostic STORAGE (e.g. distress-report
-        /// SourceIp, audit fields). NOT for rate-limit keys — use <see cref="GetTrustedClientIp"/>
-        /// there, because the value below is spoofable when no trusted proxy populates it.
-        /// <para>
-        /// Behind Azure Front Door the trusted rightmost X-Forwarded-For hop is Front Door's own
-        /// egress IP, not the device — so a stored SourceIp taken from it is useless for identifying
-        /// origin. Front Door sets the true client IP in <c>X-Azure-ClientIP</c>; prefer that and
-        /// fall back to the trusted hop when absent (no Front Door in front).
-        /// </para>
-        /// </summary>
-        public static string GetClientEgressIp(HttpRequestData? req)
-        {
-            if (req == null) return Unknown;
-            if (req.Headers.TryGetValues("X-Azure-ClientIP", out var azValues))
-            {
-                var ip = ExtractTrustedHop(string.Join(",", azValues));
-                if (ip != Unknown) return ip;
-            }
-            return GetTrustedClientIp(req);
-        }
-
         /// <summary>
         /// Takes the rightmost non-empty entry from an X-Forwarded-For header value and
         /// strips any port suffix or IPv6 brackets. Returns <see cref="Unknown"/> when

src/Web/autopilot-monitor-web/lib/__tests__/hostRouting.guard.test.ts

@@ -0,0 +1,136 @@
+import { describe, it, expect } from "vitest";
+import { existsSync, readdirSync, readFileSync } from "node:fs";
+import { join } from "node:path";
+import { PUBLIC_PATH_PREFIXES, isPublicPath } from "../hostRouting";
+
+/**
+ * Guards the public/portal split in hostRouting.ts.
+ *
+ * Failure mode this pins (prod incident 2026-07-29): /sla was listed as a
+ * public path while its page renders ProtectedRoute. On the public host
+ * ProtectedRoute stands down, so the page waited forever for a sign-in —
+ * a silent dead-end no unit test covered.
+ *
+ *  (a) No route whose page (or any ancestor layout) renders ProtectedRoute
+ *      may ever satisfy isPublicPath.
+ *  (b) Every entry in PUBLIC_PATH_PREFIXES must exist somewhere real — an
+ *      unprotected app page, an app-root metadata file, a public/ asset, or
+ *      a staticwebapp.config.json route. Catches typos and stale entries.
+ *  (c) Behavioral pins for isPublicPath's matching rules.
+ */
+
+const WEB_ROOT = join(__dirname, "..", "..");
+const APP_DIR = join(WEB_ROOT, "app");
+const PUBLIC_DIR = join(WEB_ROOT, "public");
+
+interface AppPage {
+  route: string;
+  isProtected: boolean;
+}
+
+/** Walks the app router tree; a page is protected when its page.tsx or any
+ *  ancestor layout.tsx renders ProtectedRoute. Route groups "(...)" add no
+ *  path segment. */
+function collectPages(
+  dir: string,
+  segments: string[],
+  ancestorProtected: boolean,
+  acc: AppPage[],
+): AppPage[] {
+  const layoutPath = join(dir, "layout.tsx");
+  const chainProtected =
+    ancestorProtected ||
+    (existsSync(layoutPath) && readFileSync(layoutPath, "utf-8").includes("ProtectedRoute"));
+
+  const pagePath = join(dir, "page.tsx");
+  if (existsSync(pagePath)) {
+    acc.push({
+      route: segments.length === 0 ? "/" : "/" + segments.join("/"),
+      isProtected: chainProtected || readFileSync(pagePath, "utf-8").includes("ProtectedRoute"),
+    });
+  }
+
+  for (const entry of readdirSync(dir, { withFileTypes: true })) {
+    if (!entry.isDirectory()) continue;
+    const nextSegments =
+      entry.name.startsWith("(") && entry.name.endsWith(")")
+        ? segments
+        : [...segments, entry.name];
+    collectPages(join(dir, entry.name), nextSegments, chainProtected, acc);
+  }
+  return acc;
+}
+
+const pages = collectPages(APP_DIR, [], false, []);
+const pageByRoute = new Map(pages.map((p) => [p.route, p]));
+
+const swaRoutes: string[] = (
+  JSON.parse(readFileSync(join(WEB_ROOT, "staticwebapp.config.json"), "utf-8")) as {
+    routes: Array<{ route: string }>;
+  }
+).routes.map((r) => r.route);
+
+describe("hostRouting public/portal guard", () => {
+  it("detects the app router tree and the ProtectedRoute marker (self-check)", () => {
+    // If either collapses to zero the guards below would pass vacuously.
+    expect(pages.length).toBeGreaterThan(20);
+    const sla = pageByRoute.get("/sla");
+    expect(sla, "/sla page missing — incident pin needs updating").toBeTruthy();
+    expect(sla!.isProtected, "/sla no longer renders ProtectedRoute — update this guard").toBe(true);
+  });
+
+  it("no ProtectedRoute page is classified as public", () => {
+    const violations = pages
+      .filter((p) => p.isProtected && isPublicPath(p.route))
+      .map((p) => p.route);
+    expect(
+      violations,
+      `Portal page(s) listed as public — on www ProtectedRoute stands down and the page dead-ends (the /sla incident): ${violations.join(", ")}`,
+    ).toEqual([]);
+  });
+
+  it("every public prefix maps to something real and unprotected", () => {
+    const appRootFiles = readdirSync(APP_DIR);
+    for (const prefix of PUBLIC_PATH_PREFIXES) {
+      const page = pageByRoute.get(prefix);
+      if (page) {
+        expect(
+          page.isProtected,
+          `${prefix} is in PUBLIC_PATH_PREFIXES but its page renders ProtectedRoute`,
+        ).toBe(false);
+        continue;
+      }
+      // Not an app page: must be app-root metadata (robots.ts, icon.svg,
+      // opengraph-image.png, …), a public/ asset, or a SWA redirect/rewrite.
+      const base = prefix.slice(1);
+      const metadataName = base.split(".")[0];
+      const exists =
+        appRootFiles.some((f) => f.startsWith(metadataName + ".")) ||
+        existsSync(join(PUBLIC_DIR, base)) ||
+        swaRoutes.includes(prefix) ||
+        swaRoutes.includes(prefix + "/*");
+      expect(
+        exists,
+        `${prefix} is in PUBLIC_PATH_PREFIXES but resolves to no app page, metadata file, public/ asset, or staticwebapp.config.json route`,
+      ).toBe(true);
+    }
+  });
+
+  it("pins isPublicPath matching semantics", () => {
+    // The landing page is public; the incident route is portal.
+    expect(isPublicPath("/")).toBe(true);
+    expect(isPublicPath("/sla")).toBe(false);
+    expect(isPublicPath("/sla/anything")).toBe(false);
+    // Core portal surface stays portal.
+    for (const r of ["/dashboard", "/sessions", "/settings", "/admin/metrics", "/progress"]) {
+      expect(isPublicPath(r), `${r} must be portal`).toBe(false);
+    }
+    // Prefix matching: subpaths and generated asset variants match…
+    expect(isPublicPath("/about")).toBe(true);
+    expect(isPublicPath("/docs/setup")).toBe(true);
+    expect(isPublicPath("/icon-192.png")).toBe(true);
+    expect(isPublicPath("/opengraph-image.png")).toBe(true);
+    // …but sibling names that merely share a prefix string do not.
+    expect(isPublicPath("/aboutx")).toBe(false);
+  });
+});