From 4322d07b40b11b3fa49b87aa3c10430ccec0b45d Mon Sep 17 00:00:00 2001
From: jwed <jwedemire@vancouverchristian.org>
Date: Fri, 21 Aug 2026 13:18:30 -0400
Subject: [PATCH] The portal derives the device MAC instead of trusting the
 caller

---
 src/Http/Controllers/PortalController.php |  36 ++++----
 src/Services/DeviceResolver.php           | 104 ++++++++++++++++++++++
 src/Services/UnifiApiClient.php           |  18 ++++
 src/Support/MacAddress.php                |  31 +++++++
 4 files changed, 168 insertions(+), 21 deletions(-)
 create mode 100644 src/Services/DeviceResolver.php
 create mode 100644 src/Support/MacAddress.php

diff --git a/src/Http/Controllers/PortalController.php b/src/Http/Controllers/PortalController.php
index 96fdd94..e4d2c84 100644
--- a/src/Http/Controllers/PortalController.php
+++ b/src/Http/Controllers/PortalController.php
@@ -133,30 +133,24 @@ class PortalController extends Controller
             return redirect()->route('login');
         }
 
-        $clientMac = strtolower($request->input('mac', $request->input('id', '')));
-        $ssid      = $request->input('ssid', '');
-        $apMac     = $request->input('ap', '');
+        $apMac = (string) $request->input('ap', '');
+        $ssid  = (string) $request->input('ssid', '');
 
-        if (! $clientMac) {
-            return response('Missing MAC address', 400);
+        $device = app(\Dashboard\Unifi\Services\DeviceResolver::class)->resolve($request, $apMac);
+
+        if (! $device->ok) {
+            return Inertia::render('Unifi/PortalDenied', [
+                'reason' => match ($device->reason) {
+                    'ap_mismatch'     => 'That device is not connected to this access point. Reconnect and try again.',
+                    'proxy_untrusted' => 'Wi-Fi sign-in is misconfigured and cannot identify your device. Please tell IT.',
+                    default           => 'Still connecting — wait a moment and try again.',
+                },
+                'resolveFailure' => $device->reason,
+            ]);
         }
 
-        // SECURITY: everything identifying the device is request-supplied, and this
-        // endpoint WRITES network state (authorizeGuest puts a MAC on the mapped VLAN).
-        // Before this check any signed-in account — a student, a parent — could call
-        //   GET /portal/wifi/callback?mac=<someone else's MAC>&url=…
-        // and admit a device they do not own, up to max_devices.
-        //
-        // A well-formed MAC is the floor, not a fix: it stops the parameter being used
-        // to smuggle junk into authorizeGuest and into the PortalSession row, but it
-        // cannot prove the caller owns the device. Properly binding it needs the AP
-        // handshake — UniFi can sign the redirect, or we ask the controller whether this
-        // MAC is CURRENTLY associated to $apMac before admitting it. Both need testing
-        // against real hardware, so they are deliberately not guessed at here.
-        $clientMac = str_replace('-', ':', $clientMac);
-        if (! preg_match('/^([0-9a-f]{2}:){5}[0-9a-f]{2}$/', $clientMac)) {
-            return response('Malformed MAC address', 400);
-        }
+        $clientMac    = $device->mac;
+        $isRandomized = $device->isRandomized;
 
         // Find the VLAN mapping for this user's group/OU
         $mapping = $this->resolveMapping($user);
diff --git a/src/Services/DeviceResolver.php b/src/Services/DeviceResolver.php
new file mode 100644
index 0000000..3dbd624
--- /dev/null
+++ b/src/Services/DeviceResolver.php
@@ -0,0 +1,104 @@
+<?php
+
+namespace Dashboard\Unifi\Services;
+
+use Dashboard\Unifi\Support\MacAddress;
+use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Log;
+
+/** The outcome of trying to identify the device making a portal request. */
+class ResolvedDevice
+{
+    public function __construct(
+        public bool $ok,
+        public ?string $mac = null,
+        public ?string $reason = null,
+        public bool $isRandomized = false,
+        public bool $macParameterDisagreed = false,
+        public array $client = [],
+    ) {}
+}
+
+/**
+ * Identify the device that made a captive-portal request, or refuse.
+ *
+ * The `?mac=` parameter is NOT trusted. It is read only so a disagreement with
+ * the derived value can be logged — that disagreement is somebody trying to
+ * authorize a device they do not own, and before this nothing anywhere noticed.
+ *
+ * Every path refuses rather than guessing. The cost of that is real: if
+ * TrustProxies is misconfigured, `proxy_untrusted` is returned for everyone and
+ * nobody gets online, which is why it is a distinct reason the portal page can
+ * report instead of a generic miss.
+ */
+class DeviceResolver
+{
+    public function __construct(private UnifiApiClient $unifi) {}
+
+    /** Attempts, and the pause between them, for a device that has just associated. */
+    private const ATTEMPTS   = 3;
+    private const PAUSE_US   = 750_000;
+
+    public function resolve(Request $request, string $apMac): ResolvedDevice
+    {
+        $ip = $request->ip();
+
+        /*
+         * A device that has only just associated may have no IP yet, so UniFi's
+         * client table does not know it. Retry briefly rather than telling a
+         * pupil their device is not connected when it is about to be.
+         *
+         * Bounded on purpose: this runs inside a web request, and a longer wait
+         * would hold a worker per connecting device at exactly the moment a
+         * class of them arrives at once.
+         */
+        $client = null;
+        for ($attempt = 1; $attempt <= self::ATTEMPTS; $attempt++) {
+            $client = $this->unifi->findClientByIp($ip);
+            if ($client) {
+                break;
+            }
+            if ($attempt < self::ATTEMPTS) {
+                usleep(self::PAUSE_US);
+            }
+        }
+
+        if (! $client) {
+            // A forwarded header present while the request IP is still the raw
+            // socket address means TrustProxies is not applying, so every lookup
+            // here is doomed for the same reason. Say which failure this is.
+            $forwarded = $request->headers->get('X-Forwarded-For');
+            $raw       = $request->server('REMOTE_ADDR');
+            $reason    = ($forwarded && $ip === $raw) ? 'proxy_untrusted' : 'no_client';
+
+            return new ResolvedDevice(ok: false, reason: $reason);
+        }
+
+        $mac = MacAddress::normalise($client['mac'] ?? null);
+        if (! $mac) {
+            return new ResolvedDevice(ok: false, reason: 'no_client');
+        }
+
+        $clientAp = MacAddress::normalise($client['ap_mac'] ?? null);
+        if (! $clientAp || $clientAp !== MacAddress::normalise($apMac)) {
+            return new ResolvedDevice(ok: false, reason: 'ap_mismatch');
+        }
+
+        $supplied   = MacAddress::normalise($request->input('mac'));
+        $disagreed  = $supplied !== null && $supplied !== $mac;
+
+        if ($disagreed) {
+            Log::warning('unifi.portal: supplied MAC disagreed with the derived one', [
+                'derived' => $mac, 'supplied' => $supplied, 'ip' => $ip, 'ap' => $apMac,
+            ]);
+        }
+
+        return new ResolvedDevice(
+            ok: true,
+            mac: $mac,
+            isRandomized: MacAddress::isRandomized($mac),
+            macParameterDisagreed: $disagreed,
+            client: $client,
+        );
+    }
+}
diff --git a/src/Services/UnifiApiClient.php b/src/Services/UnifiApiClient.php
index fe965dc..3953b4d 100644
--- a/src/Services/UnifiApiClient.php
+++ b/src/Services/UnifiApiClient.php
@@ -253,6 +253,24 @@ class UnifiApiClient
         return ! empty($data);
     }
 
+    /**
+     * The client currently holding this IP, or null.
+     *
+     * Reads the cached client list rather than querying per IP: the portal calls
+     * this once per connection attempt, and a class arriving at once would
+     * otherwise be one controller round trip each.
+     */
+    public function findClientByIp(string $ip): ?array
+    {
+        foreach ($this->getActiveClients() as $client) {
+            if (($client['ip'] ?? null) === $ip) {
+                return $client;
+            }
+        }
+
+        return null;
+    }
+
     public function kickClient(string $mac): array
     {
         Cache::forget('unifi:clients');
diff --git a/src/Support/MacAddress.php b/src/Support/MacAddress.php
new file mode 100644
index 0000000..93f3975
--- /dev/null
+++ b/src/Support/MacAddress.php
@@ -0,0 +1,31 @@
+<?php
+
+namespace Dashboard\Unifi\Support;
+
+/**
+ * Pure MAC helpers. No dependencies, so they are cheap to test and safe to call
+ * from anywhere.
+ */
+class MacAddress
+{
+    /** Lower-case, colon-separated, or null if it is not a MAC at all. */
+    public static function normalise(?string $mac): ?string
+    {
+        $mac = strtolower(str_replace('-', ':', trim((string) $mac)));
+
+        return preg_match('/^([0-9a-f]{2}:){5}[0-9a-f]{2}$/', $mac) ? $mac : null;
+    }
+
+    /**
+     * Is this a private (randomized) address rather than a real manufacturer one?
+     *
+     * The locally-administered bit is the second-least-significant bit of the
+     * first octet. iOS "Private Wi-Fi Address" and Android per-network
+     * randomization both set it. This is why OUI is useless for identifying
+     * device types: a randomized MAC has no real manufacturer prefix.
+     */
+    public static function isRandomized(string $mac): bool
+    {
+        return (bool) (hexdec(substr($mac, 0, 2)) & 0b10);
+    }
+}