jwed
bc3f415647
Found while triaging every authenticated route that carries no authorisation
middleware. Of 91 such routes this was the ONLY one that proved nothing: 35 scope to
$request->user(), 51 check ownership in the controller, 4 carry a credential in the URL.
TWO DEFECTS.
1. Arbitrary device admission. Everything identifying the device is request-supplied —
$clientMac = $request->input('mac', $request->input('id', '')) — and this endpoint
WRITES network state: authorizeGuest() puts a MAC on the mapped VLAN and a
PortalSession row is created. So any signed-in account, a student or a parent
included, could call
GET /portal/wifi/callback?mac=<a MAC they do not own>&url=…
and admit that device, up to the mapping's max_devices. No signature, no nonce, no
check that the MAC was even present on the network.
Fixed HALFWAY, on purpose. A well-formed MAC is now required, which stops the
parameter smuggling junk into authorizeGuest and into the session row — but it
cannot prove the caller owns the device, and the comment says so rather than
implying the hole is closed.
The likely proper fix is already in this codebase: UnifiApiClient::isClientConnected()
exists and is used a few lines below for stale-session cleanup, so requiring the MAC
to be currently associated would bind it. I did NOT add it, because if a pre-auth
guest reports as not-connected that check breaks WiFi onboarding for the entire
school, and there is no hardware here to test against. It needs one deliberate test
on real APs. The alternative is having UniFi sign the redirect.
2. Open redirect. `return redirect($request->input('url', '/'))`, unvalidated, on an
authenticated route — a phishing primitive serving from this school's own domain.
A captive portal exists to forward people to an ARBITRARY external site they were
already trying to reach, so an allowlist is not available. safeRedirectTarget()
instead accepts only absolute http/https URLs and site-relative paths, rejecting
javascript:, data: and protocol-relative "//evil.example" values that redirect()
would otherwise follow off-site. A rejected value falls back to '/' rather than
erroring: the guest is already online by then, and a bad `url` should not look like
a failed login.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RDA3fQbsa3gaHCFoV1Q8Gg