mirror of https://github.com/ruvnet/RuView synced 2026-07-24 17:43:20 +00:00

Files

T

Dragan Spiridonov 6ce50d5158 test(auth): cover browser sign-in; ADR-271/272 corrections and remediation plans

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

2026-07-23 12:08:13 +02:00

9.1 KiB

Raw Permalink Blame History

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.

9.1 KiB Raw Permalink Blame History