mirror of
https://github.com/ruvnet/RuView
synced 2026-07-28 18:21:42 +00:00
Dragan Spiridonov
6ce50d5158
Browser sign-in was the newest security surface in this PR and had no
executable evidence behind it: browser_session.rs was 534 lines with 13 tests,
every one of which hit a private helper (sign, unsign, cookie, read_cookie,
is_live, has_scope). No test called issue, from_cookie_header, begin,
verifier_for_callback or is_configured, and no test anywhere presented a session
cookie to the gate.
+10 tests in browser_session, +6 in bearer_auth. Three mutants the adversarial
review named, each now verified dead by actually applying the mutation:
(a) delete the `state` comparison in verifier_for_callback
-> a_callback_whose_state_does_not_match_is_refused FAILED
Without it the callback accepts a code from a flow the user never
started: login CSRF, victim silently lands in the attacker's session.
(b) `session.is_live().then_some(session)` -> `Some(session)`
-> an_expired_session_cookie_does_not_authenticate FAILED
-> an_expired_browser_session_is_refused FAILED
`is_live` was already unit-tested; nothing asserted the CALLER consults
it. Same "tested in isolation, call site untested" shape as the earlier
refresh-never-invoked defect.
(c) `session.has_scope(required)` -> `true`
-> a_read_scoped_browser_session_cannot_delete_or_train FAILED
Without it any browser session could delete models and start training.
Mutant (c) initially appeared to SURVIVE. It did not — there are two
has_scope call sites and the first substitution only hit one. Mutating the
one in `session_or_unauthorized` kills the test. That accident confirmed a
separate finding: the cookie branch inside require_bearer is unreachable when
an Authorization header is present, because the OAuth step returns on both
arms. It fails closed, so it is not a hole, but "try the next credential" is
what the code reads like. Pinned by
a_bad_bearer_beats_a_good_cookie_rather_than_falling_back.
Adds two crate-internal test seams (init_secret_for_tests, test_cookie_value).
test_cookie_value signs through the same path as `issue`, so tests presenting a
cookie exercise real verification rather than a test-only bypass.
ADR-271:
- The "browser cannot obtain an OAuth token" section asserted
`grep -ril "oauth|cognitum|pkce" ui/` returns nothing. It now returns three
files, invalidated by commits in this same PR. Marked superseded, original
retained under a fold, replaced with what actually ships.
- Records the two deferred decisions with designs rather than patches: P1 the
blocking JWKS fetch on a tokio worker (whose rate limiter is bypassed on
exactly the stale path that matters, because fetched_at updates only on
success — so after the TTL every request fetches, and a Pi that loses WAN
stalls itself with no attacker present); P2 the 12-hour session from a
15-minute token, with three costed options. Capping the session to
sensing:read was considered and rejected: the dashboard genuinely issues
DELETE /api/v1/models/{id}.
- P3 records that dropping `__Host-` costs origin-integrity, not just Secure —
read_cookie takes the FIRST match and cookies are not port-scoped, so a
same-host writer can shadow a session. Forgery was never the threat that
prefix addresses.
- Documents redirect_uri's hardcoded default and the unconsumed CLI credential
as known-incomplete, per decision to leave both as-is.
ADR-272: corrected a claim that would mislead users into a 401. It stated the
Python client DOES send Authorization: Bearer on the handshake; ws.py passes no
headers at all (zero occurrences of extra_headers or Authorization), so every
published client 401s once auth is enabled. Server-side decision unchanged.
Verified: sensing-server 566 + 179 + 7 + 5 + 8 + 4 + 16 pass under CI flags,
ruview-auth 61 + 25 + 2 with --all-features, auth_wiring 7.
Co-Authored-By: Ruflo & AQE
186 lines
9.1 KiB
Markdown
186 lines
9.1 KiB
Markdown
# ADR-272: WebSocket authentication tickets
|
|
|
|
- **Status**: accepted
|
|
- **Date**: 2026-07-22
|
|
- **Deciders**: RuView maintainers
|
|
- **Tags**: auth, websocket, security, sensing-server
|
|
- **Related**: ADR-271 (Cognitum OAuth resource server), ADR-055 (integrated sensing server), PR #1313 (the exemption this supersedes), cognitum-one/dashboard ADR-060
|
|
|
|
## Context
|
|
|
|
`bearer_auth` gates `/api/v1/*`. WebSocket upgrade endpoints were exempt, for a
|
|
real reason: a browser's `WebSocket` constructor cannot attach an
|
|
`Authorization` header to the handshake, so a gated socket is simply
|
|
unreachable from page JavaScript. `/ws/sensing` and `/ws/introspection` sat
|
|
outside `PROTECTED_PREFIX` entirely; `/api/v1/stream/pose` was added to an
|
|
explicit `EXEMPT_PATHS` list by PR #1313.
|
|
|
|
The reasoning was sound. The consequence was not, and it was measured rather
|
|
than argued. On a server with `RUVIEW_API_TOKEN` set — an operator who believes
|
|
authentication is ON — a real WebSocket handshake carrying **no credential at
|
|
all**:
|
|
|
|
```
|
|
/ws/sensing -> 101 Switching Protocols
|
|
/ws/introspection -> 101 Switching Protocols
|
|
/api/v1/stream/pose -> 101 Switching Protocols
|
|
/api/v1/models -> 401 Unauthorized (control)
|
|
```
|
|
|
|
**The control plane was locked and the data plane was open.** `/ws/sensing`
|
|
carries the live sensing output — presence, pose, breathing and heart rate.
|
|
`/ws/introspection` exposes internal pipeline state. For the ADR-055 desktop
|
|
topology (server bundled in the app, loopback only) that is bounded. For the
|
|
LAN/hub deployment RuView also supports, anyone who can reach the port can
|
|
watch the sensor.
|
|
|
|
ADR-271 sharpened the contrast rather than causing it: the REST surface is now
|
|
genuinely strong — offline-verified Cognitum tokens, scope-separated
|
|
destructive routes — which makes an ungated data plane the obvious way in.
|
|
|
|
*Precision about the evidence:* the handshake completing was verified. A
|
|
payload frame was not captured in that window, so the finding is "the
|
|
connection is established without a credential", not "data was read".
|
|
|
|
## Decision
|
|
|
|
Gate every WebSocket upgrade. Accept **either** of two credentials, chosen to
|
|
match what each kind of client can actually do.
|
|
|
|
### 1. Native clients send a bearer on the upgrade
|
|
|
|
The Python client, the Rust CLI and the TypeScript MCP client are not browsers
|
|
and have never been subject to the header limitation. They **can** send a normal
|
|
`Authorization: Bearer` on the handshake, so the server accepts one there;
|
|
routing them through a ticket would add a round-trip and a second credential
|
|
path for no benefit.
|
|
|
|
> **Correction, 2026-07-23.** This section previously stated that those clients
|
|
> **do** send a bearer. The published Python client does not:
|
|
> `python/wifi_densepose/client/ws.py` calls `websockets.connect(url,
|
|
> ping_interval, ping_timeout, max_size)` and passes no headers at all — the
|
|
> file contains zero occurrences of `extra_headers` or `Authorization`. So every
|
|
> `wifi-densepose[client]` consumer **401s the moment an operator enables
|
|
> auth**, and this ADR told them they would be fine.
|
|
>
|
|
> The server side of the decision stands — a bearer on the upgrade is accepted,
|
|
> and that is the right contract for a non-browser client. What is missing is
|
|
> the client implementing it, tracked as ruvnet/RuView#1395. Until then the only
|
|
> remedy available to those users is
|
|
> `RUVIEW_WS_LEGACY_UNAUTHENTICATED=1`, which reopens the exposure this ADR
|
|
> exists to close — so it is a migration aid with a deadline, not an answer.
|
|
|
|
### 2. Browsers exchange their credential for a single-use ticket
|
|
|
|
`POST /api/v1/ws-ticket` is an ordinary authenticated request — where headers
|
|
*do* work — and returns an opaque ticket the page appends as
|
|
`?ticket=<value>` on the socket URL.
|
|
|
|
**A credential in a URL is normally a mistake.** URLs reach access logs,
|
|
`Referer` headers and browser history. Three properties bound this one, and all
|
|
three are load-bearing:
|
|
|
|
| Property | Why it matters |
|
|
|---|---|
|
|
| **Single use** — consumed on the first upgrade attempt, valid or not | A ticket found in a log is already spent |
|
|
| **~30 second TTL** | Long enough to open a socket; not long enough to harvest |
|
|
| **Not the credential** — authorizes one WebSocket | Cannot be replayed against `/api/v1/*`, cannot be refreshed, carries no reusable identity |
|
|
|
|
The long-lived bearer token is still never placed in a URL.
|
|
|
|
A ticket **inherits the issuing principal's scopes**, so a `sensing:read`
|
|
session cannot mint one that outranks itself, and a ticket from a token without
|
|
`sensing:read` is refused at the upgrade.
|
|
|
|
### 3. WebSocket paths are matched by **prefix**, not by an allowlist
|
|
|
|
Anything under `/ws/` is treated as an upgrade path, plus the one endpoint that
|
|
lives outside it (`/api/v1/stream/pose`).
|
|
|
|
This is the most important detail in the ADR. An allowlist means every
|
|
WebSocket route added later is ungated until someone remembers to extend it —
|
|
the same bug, reintroduced on a delay. It is not hypothetical:
|
|
`/ws/train/progress` (ADR-186, arriving with PR #1387) is already referenced by
|
|
`ui/services/training.service.js` and would have shipped unauthenticated under
|
|
an allowlist. Prefix matching gates it on arrival.
|
|
|
|
New WebSocket routes should live under `/ws/` and inherit gating for free.
|
|
|
|
### 4. A migration escape hatch, deliberately uncomfortable
|
|
|
|
`RUVIEW_WS_LEGACY_UNAUTHENTICATED=1` restores the previous behaviour. Gating
|
|
these paths **breaks a browser UI that has not yet been updated to fetch a
|
|
ticket**, and not every deployment can update server and UI in lockstep.
|
|
|
|
It is a migration aid, not a supported configuration:
|
|
|
|
- It logs a warning on every boot naming the actual exposure — "the live
|
|
sensing stream — presence, pose and vital signs — is readable by anyone who
|
|
can reach this port" — rather than something an operator can skim past.
|
|
- Its blast radius is exactly the WebSocket paths. A test pins that it does not
|
|
weaken `/api/v1/*`.
|
|
- It is read **once at construction**, so changing the environment cannot
|
|
silently open the paths on a running server.
|
|
|
|
The alternative — a clean break with no hatch — was considered and rejected as
|
|
sequencing, not principle: a hard break tempts an operator into turning auth off
|
|
entirely, which is strictly worse than a narrow, loudly-announced exception.
|
|
The hatch should be removed once the shipped UI fetches tickets.
|
|
|
|
### 5. Deployments with auth off are unchanged
|
|
|
|
No credential configured ⇒ the middleware is the same no-op it has always been.
|
|
Pinned by a test.
|
|
|
|
## Consequences
|
|
|
|
- The measured hole is closed: all three paths now return `401` to a
|
|
credential-less handshake, while a bearer or a valid ticket returns `101`.
|
|
- Browser UIs need updating. Shipped in the same change for
|
|
`sensing.service.js`, `websocket-client.js` and `observatory/js/main.js` via
|
|
a shared `withWsTicket()` helper; a ticket is minted per connection attempt
|
|
and never cached, because it is single-use and short-lived.
|
|
- A UI running against a server that predates this ADR still works: the helper
|
|
treats `404` from `/api/v1/ws-ticket` as "no ticket needed".
|
|
- One more round-trip before a browser opens a socket. Negligible against a
|
|
stream that then runs for minutes.
|
|
- Tickets live in memory, capped at 512 outstanding and self-healing as they
|
|
expire, so an authenticated but misbehaving caller cannot grow the store
|
|
without bound. In-memory is correct rather than convenient: a ticket
|
|
surviving a restart would outlive the server that vouched for it.
|
|
|
|
## Supersedes
|
|
|
|
PR #1313's `enabled_exempts_pose_stream_websocket`, which asserted the
|
|
exemption. Its premise about browsers was correct and is preserved here; its
|
|
conclusion is replaced. The test was renamed and inverted rather than deleted,
|
|
with the history in its doc comment, and the half that still matters — the
|
|
WebSocket rule must not leak to other `/api/v1/*` paths — is kept.
|
|
|
|
## Deliberately not done
|
|
|
|
- **`/health*` stays ungated.** Orchestrator probes hit it anonymously, and
|
|
that is the point of a liveness endpoint. `/health/metrics` is included in
|
|
that exemption; if metrics ever carry occupancy-derived values this should be
|
|
revisited, because that would make them sensing data wearing an ops label.
|
|
- **`/ui/*` stays ungated.** It is static assets; the data behind them is
|
|
gated.
|
|
- **No revocation of an issued ticket.** It expires in seconds and is
|
|
single-use; a revocation path would be more machinery than the exposure
|
|
justifies.
|
|
- **No ticket for native clients.** They can send a header, so they should.
|
|
|
|
## Implementation
|
|
|
|
`v2/crates/wifi-densepose-sensing-server/src/ws_ticket.rs` (store),
|
|
`src/bearer_auth.rs` (gating), `src/main.rs` (`POST /api/v1/ws-ticket`),
|
|
`ui/services/ws-ticket.js` plus the three call sites.
|
|
|
|
Tests: 12 store, 9 gating, 4 path-matching. Store coverage includes single-use
|
|
enforcement, replay refusal, expiry refusal *and* pruning, 256-bit
|
|
unpredictability, cap enforcement and self-healing, and `?myticket=x` not being
|
|
read as `?ticket=x`. Gating coverage includes every known WS path refusing an
|
|
unauthenticated upgrade, bearer acceptance, ticket single-use, a ticket being
|
|
useless against REST, the escape hatch working *and* not weakening REST, and
|
|
auth-off behaviour unchanged.
|