central-unifi/docs/superpowers/reviews/2026-08-21-plan1-review.md

# Plan 1 review findings — BYOD Wi-Fi access model

**Date:** 2026-08-21
**Reviewed:** the six commits implementing
`docs/superpowers/plans/2026-08-21-byod-wifi-access-model.md`
(`central-unifi` `a74818b`..`a8d7b34`, `dashboard-shell` `f3de4aa`..HEAD)
**Verdict:** CHANGES NEEDED. Nothing needs redesigning; the list below needs fixing.
**Suite at time of review:** 48 passed in `tests/Feature/Unifi/`; full suite
6293 passed / 10 failed, all in the unrelated pre-existing `Logs\ScriptRunsTest`.

Recorded here because these findings outlived the session that produced them and
every one of them is about code that is already committed.

---

## 0. The request IP is trusted, and the app has no opinion of its own

`dashboard-shell/bootstrap/app.php:14` — `$middleware->trustProxies(at: '*')`.
Laravel routes `'*'` to `setTrustedProxyIpAddressesToTheCallingIp()`, so **the
immediate peer is unconditionally trusted as a proxy** and its `X-Forwarded-For`
becomes `$request->ip()`. `docker/nginx/default.conf` sets no `real_ip_header` or
`X-Forwarded-For` of its own, so a client-supplied header reaches PHP verbatim.

`DeviceResolver::resolve()` reads `$request->ip()` and derives the MAC from it, so
if a client can deliver a forged header the resolver identifies a device of the
attacker's choosing. The AP check does not help: `ap` is also a request parameter,
so an attacker forges both.

### What is NOT established

An earlier session demonstrated forgery by sending `X-Forwarded-For` **directly to
nginx on `:80`**. That path is not available to a real client: the Wi-Fi VLAN is
firewalled off from the server and can reach only Traefik. So that demonstration
does **not** show a live exploit, and it should not be cited as one.

### What IS established: the legitimate value arrives correctly

Confirmed by the operator on 2026-08-21 from the production Logged-in Users list,
served through Traefik: sessions show **real per-client addresses** —
`10.22.5.254` and `10.22.5.154` for on-site staff, and a public address for an
off-site phone. So `TrustProxies` plus Traefik does yield the true client IP, and
UniFi can map an on-site `10.x` address to a MAC.

That settles two things:

- **The IP → MAC approach is viable.** This was the operator's original question and
  the answer is yes, with production evidence.
- **Spec §E had the release-gate risk backwards.** It called "whether TrustProxies
  yields the true client IP" the thing that gates release, fearing a fail-closed
  outage where nobody can get online. That fear is unfounded — it demonstrably
  works. Remove it as a blocker.

Note the off-site public address: a portal request only ever originates on-site, so
a request whose IP is public will correctly find no UniFi client and refuse. That is
the right behaviour, not a gap.

### The open question, which is narrower than it first appeared

**A legitimate value arriving correctly and a forged value being rejected are
different properties.** The evidence above establishes the first. It does not
establish the second, because nothing in that list would look wrong if Traefik were
also passing through client-supplied headers.

So one question remains, and it is a one-line check

Whether a client can deliver a forged header depends entirely on Traefik's
`forwardedHeaders` for the entry point, on the Traefik host (162.223.102.234 —
a separate LXC, not reachable from the dev box):

- `forwardedHeaders.insecure: true`, or `trustedIPs` covering client ranges
  → client headers are passed through → **the resolver's guarantee is void**
- unset (Traefik's default) → Traefik replaces client-supplied forwarded headers
  → the design works as intended

**Treat this as the release gate.** Narrowing `trustProxies` from `*` to the
Traefik network is worth doing either way as defence in depth: as written, the
application delegates the entire question to whoever happens to be talking to it.

### The consequence that was missed, if it is forgeable

Not a quota bypass — quotas key on `user_id`, so an attacker cannot exceed their
own limit. It is worse in a different way. `updateOrCreate` writes a row keyed
`(attacker_user_id, victim_mac)`, making the attacker the legitimate **owner** of a
session pointing at somebody else's device. `disconnectSession` then passes its
owner check and runs `kickClient` + `unauthorizeGuest` on the victim's MAC.

The spec declared `disconnectSession` "already correct and not being changed". That
was true of the function and false of its inputs — the one component held out of
scope as safe is what turns this into an eviction primitive. Worth remembering as a
general lesson about scoping something out on the strength of its internals.

Also note the three distinct refusal reasons (`no_client` / `ap_mismatch` / success)
form an oracle: iterating candidate IPs enumerates the live client table with MAC
and AP. They were added for operability and double as reconnaissance.

**Nothing in the six commits is defective on this point.** `DeviceResolver` is a
correct implementation of "derive the MAC from the source IP" sitting on a platform
assumption. Spec §E named this exact dependency as the release gate and got the
direction backwards — it worried `TrustProxies` would be too strict (a visible
outage) and never considered it being too permissive (silent fail-open). The
blind-spot note in `b59b5bf` reasons carefully and still only considers the strict
direction.

**Exposure is wider than this feature.** Every consumer of `$request->ip()` in the
shell inherits it — session client IP, the Logged-in Users list, anything keyed on
IP. File against `dashboard-shell`, not `central-unifi`.

### What would not help

Assessed and rejected, all reducing to trusting the same value:

- **Re-checking the client's reported IP** — circular; the record was *selected* by
  that IP, so the check cannot fail.
- **Requiring the client to be unauthorized** — breaks the renewal path and with it
  the spec's headline promise, while still leaving first-time connectors targetable.
- **Association recency** — narrows harvesting to a race the attacker wins by
  polling, and wrongly refuses a device revisiting after a long window expires.

The durable fix is to stop making the request IP load-bearing: have the controller
vouch for the MAC (a signed redirect / HMAC over `mac, ap, timestamp`). That
survives any proxy misconfiguration. Whether this controller supports it is a
question for the hardware session — **add it as a fourth item to spec §E**, ranked
above the health line in Plan 2.

---

## 1. The reconciler would revoke every guest authorization on the whole site

`src/Console/ReconcilePortal.php` — `array_diff($authorized, $live)` then
`unauthorizeGuest` on each, where `$authorized` comes from `getAuthorizedGuestMacs()`
reading **site-wide** `/stat/guest`. Nothing scopes it to authorizations this portal
created.

A second guest SSID, UniFi's own voucher portal, front-desk visitors, contractors,
a gym rental, or anything an admin authorized by hand: all revoked on the first
scheduled run, and again every run. Inversely, a device an admin deliberately
un-authorized in the UniFi UI gets re-authorized — the dashboard silently
overriding the network admin, forever.

Fix: only revoke a MAC that appears in `unifi_portal_sessions` at all, or filter by
the portal's SSID/VLAN. **This is why "not scheduled" is currently load-bearing
rather than merely cautious.**

Unverifiable without hardware, and worth adding to §E: what window does
`GET /stat/guest` return with no `within`, and does the controller rewrite `end`
when an authorization is revoked? If not, job 3 silently stops re-asserting.

## 2. The pre-DHCP retry cannot succeed, and costs a worker 1.5s per failure

`DeviceResolver` retries three times, but `findClientByIp()` → `getActiveClients()`
is `Cache::remember('unifi:clients', 30s)`. All three attempts read one snapshot.

Worse than a no-op: the first attempt **warms** the cache with a snapshot lacking
the connecting device, so "try again" stays refused for the rest of the TTL, for
that pupil and everyone else. Meanwhile each failure holds a PHP worker for 1.5s of
`usleep` — the exact cost the comment claims to bound, with none of the benefit. A
class of 25 arriving together is 25 workers asleep on one stale array.

`UnifiApiClient`'s own comment justifies the cache on the grounds that "the portal
calls this once per connection attempt". The resolver calls it three times.

Fix: `Cache::forget('unifi:clients')` between retries, or an uncached read path.

## 3a. Device type should come from the User-Agent, not from UniFi

Raised by the operator on 2026-08-21, from the same production screenshot: the
Logged-in Users list already labels sessions "Safari · iPhone" and
"Chrome · macOS", and does it well.

That is parsed from the **User-Agent**, by a hand-rolled private method
`ActiveSessionController::device(string $ua)` — no library involved. Its OS table
already distinguishes `iPhone`, `iPad`, `Android` from `Windows`, `macOS`,
`Linux`, `CrOS`, which is precisely the phone-versus-laptop distinction the
no-phones-for-students policy needs.

**This is a better source than UniFi's fingerprint for this purpose**, and it
removes a dependency the spec could never verify:

- **Available at authorization time.** A portal request comes *from the device*, so
  its User-Agent is the device's own. UniFi's `dev_cat` is inferred from observed
  traffic and does not exist yet at that moment — the reason §C had to push capture
  into the reconciler at all.
- **Known to be populated.** `unifi_client_snapshots` has zero rows on dev, so
  whether `dev_cat` is ever filled in this environment was never established. The
  User-Agent is always present.
- **It fixes finding 3 with a value that is actually there.** Replacing the three
  always-NULL `$request->input(...)` writes with the parsed label gives the denial
  page real device names, which is the single case that list exists for.

Recommendation: extract `device()` into a shared support class (e.g.
`App\Support\UserAgent::describe()`) and have both the Logged-in Users controller
and the portal call it. Small refactor, one owner for the parsing, no duplication.

**The spoofing caveat stands and does not weaken this.** A mobile browser's
"Request Desktop Site" toggle changes the User-Agent, so this is sound for
*reporting* and unsound for *enforcement* — which is exactly decision 6
(report-only). Keep UniFi's `dev_cat` capture in the reconciler as a corroborating
second opinion: it is not client-controlled, so a disagreement between the two is
itself a useful signal if enforcement is ever wanted.

## 3. Every portal visit blanks the fingerprint the reconciler filled in

`PortalController` writes `device_hostname`, `device_os` and `device_type` from
`$request->input('hostname'/'os'/'dev_cat')`. UniFi's redirect sends `id`, `ap`,
`t`, `url`, `ssid` — the controller's own comment says so — so all three are
written NULL on every authorization.

The exact mirror of the bug `captureFingerprints()` goes out of its way to prevent.
Consequence: the denial page's device list always reads "Unknown device", so a pupil
at their limit chooses between two unlabelled MACs — the single case that list
exists for. Plan 2's Wi-Fi Devices page inherits it.

`$device->client` already carries UniFi's `hostname`/`os_name`/`dev_cat` and is
never read. Use it. These are also the last request-supplied values written to a
session, so `?hostname=Principal's Laptop` is shown to admins as fact.

## 4. Windows slide, contradicting the spec

`expires_at => now()->addMinutes($minutes)` on the renewal branch grants a fresh
full window on every visit. With `session_minutes` set to a term, anyone who
bookmarks the callback never ages out — which is what §C's fixed-window rule exists
to prevent. Either preserve `expires_at` on renewal, or amend the spec and say why
in a comment; today there is no comment where a reader would ask.

## 5. Two routes to a wrong quota

No unique index on `(user_id, mac_address)` (confirmed: only PRIMARY,
`(user_id,is_active)`, `(mac_address)`) and no lock around read-then-authorize.

- **Over quota:** two devices of a 1-device pupil hit the callback concurrently,
  both read zero live sessions, both are authorized. The reconciler cannot correct
  it — both rows are live and both authorized.
- **Phantom slots:** two concurrent renewals of the same MAC each miss the other and
  insert twice. One phone, two live rows, permanently. The reconciler's
  `keyBy(mac)` silently collapses the pair so neither direction sees it, and
  self-disconnect deactivates only the row that was clicked.

Fix: unique index plus `lockForUpdate` in a transaction. Consider a reconciler step
trimming a user's live rows to their quota, newest first — nothing currently
enforces the quota against our own rows.

## 6. Denial copy contradicts itself on the commonest path

Controller says "Still connecting — wait a moment and try again"; `PortalDenied.vue`
then prints "Please contact the school office or IT for help getting online". Two
instructions in one view, on the most frequent denial. `resolveFailure` is passed
and never declared as a prop.

---

## Tests that pass for the wrong reason

1. **The headline quota-leak regression test** asserts `component('Unifi/PortalDenied')`
   and a live count — but four branches render that component. It passes with no
   role and no mapping at all, via "No WiFi access configured". Fix: assert the
   `reason` string.
2. **The proxy-outage test** sets `X-Forwarded-For` and `REMOTE_ADDR` to the same
   value, so its condition holds by construction; mutating the production
   condition to `($forwarded)` alone still passes. (Note: under
   `trustProxies(at: '*')`, `$ip === $raw` only holds when no XFF was sent, so
   `proxy_untrusted` is unreachable in this environment — a consequence of #0.)
3. **The reconciler re-assert test** constrains neither MAC nor minutes. A sign
   error in `now()->diffInMinutes($expires_at, false)` would clamp every
   re-assertion to one minute and the test would not notice — and Carbon 3.13.1
   changed that method's return type and default `$absolute`.
4. **The `sort_order` precedence test** inserts the low-`sort_order` row first, so
   replacing `orderBy('sort_order')` with `all()` still passes. It also never
   asserts the user holds `parent`.

Untested: the unreachable-controller guard — the fail-safe between a network blip
and mass revocation. Worth a test now, and more so once #1 is fixed.

---

## Smaller notes

- `PortalDeniedTest`'s docblock explains why a device "must look CONNECTED, or the
  controller treats the session as stale, frees the slot and authorises" —
  documenting behaviour this work **deleted**. Most confidently-worded comment in
  the suite and now false. The dead `isClientConnected` stub goes with it.
- `ResolvedDevice::$client` is never read (see #3). `$macParameterDisagreed` is
  asserted in a test and consumed nowhere — surface it or drop it and keep the log.
- The disagreement test asserts the flag, not that `Log::warning` fired, which is
  what §B actually asks for.
- Unused `use ...DB;` in two test files, copied from the plan.
- Three resolver tests burn 1.5s of real `usleep` each; injecting the pause would
  pay for itself.
- `UnifiApiClient::isClientConnected()` is now referenced nowhere in `src/`.
  Correct per spec (display-only, Plan 2) — say so in a comment or the next
  dead-code sweep deletes it.

## Release sequencing — not a finding, but it changes the plan

`Pages/Unifi/Portal.vue` still offers "Default (catch-all)", which now 422s, and
offers **no `role` option**. Wi-Fi quotas are therefore *unconfigurable* today:
the reachable match types are `ou`/`group`/`email_domain`. Spec §F step 3
("configure `role` → student / staff") cannot be carried out until Plan 2, which
makes **Plan 2 a hard prerequisite for release rather than a follow-up**.

---

## What is good and should not be disturbed

- **`scopeLive`** is the right shape in the right place: the model owns the
  definition of a consumed slot, a NULL expiry is not live, and the *why* is in the
  docblock. Leak 2 is fixed at the query level and does not depend on the
  unscheduled reconciler. The most valuable thing in the six commits, and untouched
  by #0.
- **`?mac=` really is dead.** Verified end to end: with a supplied MAC present,
  `authorizeGuest` receives the derived one and no other row is written. No
  fallback anywhere; an absent `?ap=` fails closed as `ap_mismatch` rather than
  skipping the check. The fail-closed discipline is genuine and thorough — it is
  the input that betrays it, not the logic.
- **`DeviceResolver` split out of the controller** — four named refusal reasons, no
  quota knowledge, no Inertia. When the platform is fixed this is the right object
  in the right place, and where an eventual HMAC check belongs.
- **`captureFingerprints()`' fill-only rule** and its reasoning. #3 is the
  controller violating this rule, not this code being wrong.
- **The Mockery declaration-order docblock** in the reconciler test will save the
  next person an hour.
- **Removing `default` while `unifi_vlan_mappings` was empty**, and recording in
  code why doing it later would be worse.