From 56327d0931e2749a8186e88f1e16a7198979b621 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 14:47:31 +0200 Subject: [PATCH] decide: browser sessions are read-only permanently; drop the dead step-up client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decision: browser-side admin is not wanted. `BROWSER_SIGNIN_SCOPE` stays `sensing:read`, and the escalate-on-demand design sketched while this was open is not being built. Destructive operations — training, model delete, recording delete — keep their home in the CLI, where `--admin` is explicit and typed by a person. Routing them through a browser would mean either asking every user to consent to delete capability in order to watch a stream, or building a second consent flow to avoid that. Consequences, now settled rather than open: - The UI's admin controls are unreachable from a Cognitum browser session, and that is intended. The manual token-paste field is unchanged and still carries whatever authority the pasted token has, so nothing that worked before stops working. - REMOVED the client-side step-up redirect from ui/services/api.service.js. It caught an RFC 6750 challenge that can never be issued to a browser, and it ended in `return new Promise(() => {})` — so if any other 401 had ever grown that header, every caller awaiting it would have hung forever with no error and no timeout. Dead code with a trap in it is worse than no code. - KEPT ADMIN_REVERIFY_SECS as a server-side backstop. Fail-closed and free, so if the requested scope is ever widened the freshness requirement is already in place. Documented at its definition as a backstop specifically so nobody reads its passing tests as evidence the control is exercised — the tests reach it through a crate-internal seam that mints an admin cookie the real flow does not produce. ADR-271 also stops hedging on the session TTL: chosen is A at one hour. Option B (server-side refresh-token store) is not built, and the ADR now names the residual instead of implying it is closed — within one hour a revoked Cognitum grant still reads sensing data through an existing browser session. There is no introspection endpoint, so nothing short of B closes that, and one hour is the size of the hole we accepted. Adds `the_cookie_max_age_matches_the_session_expiry`, which the previous ADR revision asked for and nobody had written: Max-Age and the payload's `exp` are two independent expressions of one lifetime, and drift means either the browser presents a session we reject or we hold authority the browser discarded. Verified: workspace 176 suites clean, UI 22, api.service.js parses. Co-Authored-By: Ruflo & AQE --- .../ADR-271-cognitum-oauth-resource-server.md | 60 ++++++++++++++----- ui/services/api.service.js | 35 ++++------- .../src/browser_session.rs | 43 ++++++++++--- 3 files changed, 94 insertions(+), 44 deletions(-) diff --git a/docs/adr/ADR-271-cognitum-oauth-resource-server.md b/docs/adr/ADR-271-cognitum-oauth-resource-server.md index 7c50da64..e02a8d8a 100644 --- a/docs/adr/ADR-271-cognitum-oauth-resource-server.md +++ b/docs/adr/ADR-271-cognitum-oauth-resource-server.md @@ -324,14 +324,37 @@ to trip over: the right time for the guard to already exist, but it is not evidence that the control is exercised today. -If browser-side admin is wanted, the coherent design is **escalate on demand**: -keep `sensing:read` as the default, and have the RFC 6750 challenge send the -user back through `/oauth/start` with `sensing:admin` requested, returning a -session that holds admin AND a fresh `auth_time`. That preserves least privilege -by default, makes the challenge meaningful, and is the only option that does not -ask every user to consent to delete capability just to watch a stream. It needs -a scope parameter on `/oauth/start` and a UI affordance, so it is deliberately -not bundled into this change. +### Decision, 2026-07-23: the browser is read-only, permanently + +**Browser-side admin is not wanted.** `BROWSER_SIGNIN_SCOPE` stays +`sensing:read`, and the escalate-on-demand design sketched while this was still +open is **not** being built. + +The reasoning holds up on its own terms rather than being a concession to +scope: the destructive operations — training, model delete, recording delete — +already have a home in the CLI, where `--admin` is explicit, typed by a person, +and scoped to the session that needed it. Routing them through a browser would +mean either asking every user to consent to delete capability in order to watch +a stream, or building a second consent flow to avoid that. Neither is worth it +for operations that are administrative by nature and rare by frequency. + +What this settles: + +- **The UI's admin controls are unreachable from a Cognitum browser session** + and that is now intended, not a gap. `model.service.js` issuing + `DELETE /api/v1/models/{id}` returns 401. The manual token-paste field still + works and carries whatever authority the pasted token has, so nothing that + worked before this change stops working. +- **The client-side step-up redirect has been removed** from + `ui/services/api.service.js`. It caught a challenge that can never be issued, + and it ended in a promise that never settles — so had any other 401 ever grown + that header, every caller would have hung forever. Dead code with a trap in it + is worse than no code. +- **`ADMIN_REVERIFY_SECS` stays as a server-side backstop.** It is fail-closed + and costs nothing, so if the requested scope is ever widened the freshness + requirement is already there rather than something to remember. It is + documented at its definition as a backstop, so nobody mistakes its passing + tests for evidence that it is exercised. **Three options, with the tradeoff each carries:** @@ -341,13 +364,22 @@ not bundled into this change. | **B. Server-side session store** with the refresh token, revalidated periodically | Real revocation: a disabled grant fails at the next refresh | The server now stores refresh tokens — a new and higher-value secret at rest — and refresh rotates with reuse detection, so a bug logs users out. | | **C. Re-verify on privileged operations only** | `sensing:admin` requires a fresh token; reads keep the long session | Best blast-radius-per-unit-cost, but needs a UI affordance for step-up auth that does not exist. | -**Recommendation: A now, C next.** A is a one-line change that bounds the -window immediately; C is the design that actually matches the risk, since the -damage a stale session can do is concentrated in the mutating routes. B is only -worth it if RuView later needs true cross-device sign-out. +**Chosen: A, at one hour** — `SESSION_TTL_SECS` is 3600, down from 12 hours. -Whichever is chosen, `SESSION_TTL_SECS` should be pinned by a test asserting the -issued cookie's `Max-Age` matches the session's `exp`, so the two cannot drift. +C was implemented too, and then the browser-read-only decision above made it a +backstop rather than an active control: with no browser session holding +`sensing:admin`, there is no privileged operation to re-verify. It is kept +because it is fail-closed and free, not because it is doing work today. + +B is not built. It is only worth its cost — storing refresh tokens at rest, +against an authorization server that rotates them with reuse detection — if +RuView later needs true cross-device sign-out. Shortening the window addresses +the same risk for a fraction of the exposure. + +That leaves a residual this ADR should not pretend away: **within one hour, a +revoked Cognitum grant still reads sensing data through an existing browser +session.** Cognitum publishes no introspection endpoint, so nothing short of B +closes that, and one hour is the size of the hole we accepted. ### P3 — dropping `__Host-` costs cookie origin-integrity, not just `Secure` — **FIXED** diff --git a/ui/services/api.service.js b/ui/services/api.service.js index b9052b4a..49695af9 100644 --- a/ui/services/api.service.js +++ b/ui/services/api.service.js @@ -86,31 +86,20 @@ export class ApiService { // Process response through interceptors const processedResponse = await this.processResponse(response, url); - // Step-up re-authentication (ADR-271 P2). + // NOTE: there is deliberately no step-up re-authentication branch here. // - // A browser session outlives the ~15-minute access token that created it, - // and Cognitum publishes no introspection endpoint, so the server refuses - // PRIVILEGED actions from a session older than a few minutes. That is a - // 401, but it means something different from "you are not signed in" — - // the user IS signed in, and the fix is to prove it again. The server - // marks it with an RFC 6750 error code so the two are distinguishable. + // An earlier revision caught the server's RFC 6750 "reauthentication + // required" challenge and redirected to /oauth/start. That challenge can + // never be issued to a browser: browser sign-in requests `sensing:read` + // only and always will (see BROWSER_SIGNIN_SCOPE), so no browser session + // holds `sensing:admin`, so the freshness gate the challenge announces is + // never reached. Admin work goes through the CLI or a pasted bearer. // - // Without this branch a stale-session delete surfaces as a generic - // "Request failed" and the user has no way to know that signing in again - // resolves it. - if (processedResponse.status === 401) { - const challenge = processedResponse.headers.get('WWW-Authenticate') || ''; - if (challenge.includes('reauthentication required')) { - // Full-page redirect: the flow ends by setting a cookie, which an - // XHR cannot do usefully. Returns here with a fresh auth_time and, - // while the Cognitum session is alive, no prompt. - window.location.href = '/oauth/start'; - // Never settles — the navigation is already underway, and resolving - // would let the caller render an error for an operation that is - // simply being retried after sign-in. - return new Promise(() => {}); - } - } + // Removed rather than left inert, because it was not merely dead — it + // ended in a promise that never settles. If any other 401 ever grew that + // header, every caller awaiting this would hang forever with no error. + // The server-side guard stays as a fail-closed backstop; the client has + // nothing to do about a flow that does not exist. // Handle errors if (!processedResponse.ok) { diff --git a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs index 917fc8f1..4a008041 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -76,9 +76,14 @@ pub const SESSION_TTL_SECS: i64 = 3600; /// rather than making every user re-authenticate hourly for a dashboard whose /// primary use is watching a live stream. /// -/// When a session is too old for an admin action the request is refused, and -/// the UI sends the user back through `/oauth/start` — which returns with a -/// fresh `auth_time` and, if their Cognitum session is live, no prompt. +/// **This is a backstop, not an active control.** Browser sign-in requests +/// `sensing:read` only and always will ([`BROWSER_SIGNIN_SCOPE`]), so no browser +/// session holds `sensing:admin` and this branch is never reached in production. +/// It is kept because it is cheap and fail-closed: if the requested scope is +/// ever widened, the freshness requirement is already in place rather than +/// something someone has to remember to add. Its tests exercise it through a +/// crate-internal seam that mints an admin cookie the real flow does not +/// produce — do not read them as evidence the control is exercised. pub const ADMIN_REVERIFY_SECS: i64 = 300; /// The scope `/oauth/start` requests. Read-only, deliberately. @@ -96,10 +101,10 @@ pub const ADMIN_REVERIFY_SECS: i64 = 300; /// constant grows, which is the right ordering — but do not mistake its /// passing tests for evidence that the control is exercised. /// -/// Widening this to include `sensing:admin` would make every browser sign-in -/// consent to delete capability just to watch a stream. The coherent way to add -/// browser-side admin is escalate-on-demand: keep this read-only and let the -/// RFC 6750 challenge send the user back through `/oauth/start` asking for more. +/// **Decided 2026-07-23: browser-side admin is not wanted.** This stays +/// read-only. Widening it would make every browser sign-in consent to delete +/// capability just to watch a stream, and the destructive operations have a +/// deliberate home — the CLI, where `--admin` is explicit and typed. pub const BROWSER_SIGNIN_SCOPE: &str = ruview_auth::scope::SENSING_READ; fn now() -> i64 { @@ -806,6 +811,30 @@ mod tests { )); } + #[test] + fn the_cookie_max_age_matches_the_session_expiry() { + // Two independent expressions of the same lifetime: the cookie's + // Max-Age (when the browser stops sending it) and the payload's `exp` + // (when we stop accepting it). If they drift, one silently wins — + // a longer Max-Age means the browser keeps presenting a session we + // reject, a shorter one means we hold authority the browser discards. + init_secret_for_tests(); + let raw = test_cookie_value("s", "a", "sensing:read", SESSION_TTL_SECS); + let session = from_cookie_header(&format!("{SESSION_COOKIE}={raw}")).unwrap(); + + let set_cookie = cookie(SESSION_COOKIE, &raw, SESSION_TTL_SECS, false); + assert!( + set_cookie.contains(&format!("Max-Age={SESSION_TTL_SECS}")), + "{set_cookie}" + ); + // Same lifetime, allowing a second for the clock ticking between them. + assert!( + (session.exp - now() - SESSION_TTL_SECS).abs() <= 1, + "cookie Max-Age and session exp disagree: exp-now={}, Max-Age={SESSION_TTL_SECS}", + session.exp - now() + ); + } + #[test] fn browser_sign_in_stays_read_only_until_someone_decides_otherwise() { // Pins the decision documented on BROWSER_SIGNIN_SCOPE. Widening it is