From 6ce50d51587533a00261578886c44b30818204e1 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 12:08:13 +0200 Subject: [PATCH] test(auth): cover browser sign-in; ADR-271/272 corrections and remediation plans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../ADR-271-cognitum-oauth-resource-server.md | 162 +++++++++++++- ...DR-272-websocket-authentication-tickets.md | 22 +- .../src/bearer_auth.rs | 123 +++++++++++ .../src/browser_session.rs | 202 ++++++++++++++++++ 4 files changed, 502 insertions(+), 7 deletions(-) diff --git a/docs/adr/ADR-271-cognitum-oauth-resource-server.md b/docs/adr/ADR-271-cognitum-oauth-resource-server.md index a89ec047..6736deeb 100644 --- a/docs/adr/ADR-271-cognitum-oauth-resource-server.md +++ b/docs/adr/ADR-271-cognitum-oauth-resource-server.md @@ -170,7 +170,18 @@ take no HTTP dependency at all. - A new dependency, `jsonwebtoken` — the same crate, same major version, that identity itself uses to sign these tokens. -## Known incomplete: the browser cannot obtain an OAuth token +## ~~Known incomplete: the browser cannot obtain an OAuth token~~ — CLOSED 2026-07-23 + +> **Superseded within this same PR.** The text below described the state when +> this ADR was first written. It is retained because the reasoning still +> explains *why* the browser half was built, but every factual claim in it is +> now false — in particular `grep -ril "oauth|cognitum|pkce" ui/` now returns +> `ui/sw.js`, `ui/sw.test.mjs` and `ui/utils/quick-settings.js`. An adversarial +> review caught the ADR still asserting the old state; see "Browser sign-in" +> below for what actually ships. + +
+Original text (no longer accurate) `wifi-densepose login` writes to `~/.ruview/credentials.json` — a file a browser cannot read. The UI's `ws-ticket.js` reads a bearer from @@ -184,9 +195,152 @@ browsers" is today only exercisable with the legacy static shared secret that OAuth was meant to replace. The server-side gating is correct and complete; the browser half of the story these ADRs tell is not built. -Deliberately recorded rather than left implied, because the ADRs read as though -the browser path exists. Closing it needs a UI sign-in flow that puts an OAuth -access token where the page can reach it — a separate piece of work. +
+ +## Browser sign-in + +`/oauth/start`, `/oauth/callback`, `/oauth/logout` and `/oauth/status`, plus a +"Cognitum Account" panel in QuickSettings. The server runs the authorization +code + PKCE flow itself and hands the browser a **signed session cookie** — +never the access token. The browser gets an assertion that this server already +verified a token, which is nothing replayable anywhere else. + +Three things about it are load-bearing and were each found the hard way: + +- **The cookie carries the granted scope**, and the gate re-checks it per + request. A `sensing:read` session cannot delete a model. +- **`__Host-` is deliberately NOT used.** That prefix requires `Secure`, and + RuView is routinely reached over plain HTTP on a LAN; a cookie the browser + refuses to set is worse than one without the prefix. The cost is real and is + recorded as P3 under "Open problems" below. +- **The service worker must never cache `/oauth/*` or authenticated `/api/*`.** + The Cache API is not the HTTP cache and ignores `Cache-Control` entirely, so + a cached `/oauth/status` froze sign-in until a hard reload, and cached API + responses could be replayed to a different user after sign-out. `ui/sw.js` is + now deny-by-default with an allowlist. + +### Still incomplete + +`redirect_uri` defaults to `http://127.0.0.1:8080/oauth/callback` and is +overridden only by `RUVIEW_PUBLIC_BASE_URL`. Browser sign-in therefore works +only on a host reached at exactly that origin: an operator browsing +`http://localhost:8080` or `http://192.168.1.50:8080` cannot complete the flow +(PKCE keeps the code unexchangeable, so this is a broken flow, not a token +leak). Deriving it from the request is the fix; deferred deliberately, since +deriving a redirect URI from attacker-controllable headers is its own class of +bug and deserves its own decision. + +The credential `wifi-densepose login` stores is also **not yet consumed by any +shipped client** — no CLI subcommand, MCP server or Python client reads +`~/.ruview/credentials.json`. The token is obtainable and verifiable; wiring the +clients to send it is separate work. + +## Open problems and proposed remediation + +Two findings from the 2026-07-23 adversarial review are **not fixed in this +work**. Both are recorded here with a proposed design rather than patched in a +hurry, because each changes a runtime property that deserves its own decision. + +### P1 — the JWKS fetch blocks a tokio worker, and the stale path is unbounded + +`verify.rs:182` calls `JwksCache::decoding_key_for`, which performs a blocking +`ureq` request (`jwks.rs:181`, 3s connect + 3s read) directly on the async +worker running `require_bearer`. The same codebase already knows this is wrong: +`main.rs:9265` wraps the token exchange in `spawn_blocking`, commenting "the +same mistake this codebase had to fix in `jwks.rs`". The hot verification path +did not get the same treatment. + +Worse, the rate limiter does not cover the case that matters. +`state.fetched_at` is updated **only on success** (`jwks.rs:188`); the error arm +leaves it untouched. So once the TTL elapses after the last *successful* fetch, +`fresh` is permanently `false`, the `may_force` guard at `:170` is never +consulted, and **every** request performs its own blocking fetch attempt. + +This fires with no attacker present. On a Pi that loses WAN — the documented +deployment reality — 300 seconds later every API call and every UI poll starts a +blocking outbound attempt, and with few tokio workers the whole server stalls, +including `/health`. An attacker can reach the same state deliberately by +flooding tokens carrying an unknown `kid`. + +**Proposed fix, in dependency order:** + +1. **Rate-limit attempts, not successes.** Add `last_attempt_at`, recorded + before the fetch regardless of outcome, and consult it on the stale path too. + This alone converts "every request fetches" into "one request per interval". +2. **Get the blocking call off the runtime.** Either wrap the call in + `spawn_blocking` at the `verify` boundary, or give `JwksCache` an async + transport behind the existing transport seam. The seam already exists — + `JwksCache::new` takes a boxed transport — so this is an added + implementation, not a redesign. +3. **Single-flight the refresh.** Concurrent misses for the same `kid` should + await one shared fetch rather than each issuing their own. +4. **Refresh ahead of expiry** from a background task, so the request path + normally never fetches at all. + +Steps 1 and 2 are the ones that remove the stall; 3 and 4 are optimisations. +The test that must accompany this: a transport whose fetch blocks on a barrier, +asserting that a second concurrent verification is not serialised behind it — +the current suite is entirely single-threaded and could not observe a +reintroduction (`jwks::tests` contains no concurrency primitive at all). + +### P2 — a 15-minute access token becomes a 12-hour session + +`issue()` sets `exp: now() + SESSION_TTL_SECS` with `SESSION_TTL_SECS = 12 * +3600`, deliberately not inheriting the access token's ~15-minute lifetime. The +session cookie is an assertion that this server verified a token, so it is not +*wrong* for it to outlive the token — but 12 hours is a long time to hold an +authority that cannot be revoked. Cognitum publishes no introspection endpoint +(see "Facts about the tokens"), so RuView has no way to ask whether the grant +behind a session still stands. A disabled account keeps sensing access, and +`sensing:admin` if it had it, until the cookie expires on its own. + +Capping the session at `sensing:read` was considered and **rejected**: the +dashboard genuinely performs admin operations (`model.service.js:136` issues +`DELETE /api/v1/models/{id}`), so that would break shipped functionality. + +**Three options, with the tradeoff each carries:** + +| Option | Effect | Cost | +|---|---|---| +| **A. Shorten the TTL** (e.g. 12h → 4h) | Bounds exposure by a factor of 3, one constant | Re-auth is a full-page navigation, which interrupts a live streaming dashboard. Mostly silent while the Cognitum session is alive, but not free. | +| **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. + +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. + +### P3 — dropping `__Host-` costs cookie origin-integrity, not just `Secure` + +The decision above frames omitting `__Host-` as trading away a `Secure` +requirement that RuView cannot meet on a plain-HTTP LAN. That framing is +incomplete: `__Host-` also guarantees the cookie was set by *this* origin with +`Path=/` and no `Domain`. Without it, cookies are not port-scoped and are not +integrity-protected against a same-host writer. + +`read_cookie` returns the **first** match in the header, and RFC 6265 §5.4 sends +longer-`Path` cookies first. So an attacker who can set a cookie on the same +host — any other service on any port on that appliance, or a plain-HTTP MITM +injecting `Set-Cookie` — can plant `ruview_session=; Path=/ui`. The victim's browser then sends both, the attacker's first, +and it verifies correctly because it *is* genuinely signed. The victim ends up +operating inside the attacker's session; `/oauth/status` reports the attacker's +account, and anything the victim records is attributed to them. + +Note the shape: the signature is doing its job. Forgery was never the threat +`__Host-` addresses, so "the signature is what protects the value" does not +answer this. + +**Proposed fix (cheap, no prefix needed):** have `read_cookie` collect *all* +values for the name and accept only if exactly one verifies — or, more strictly, +reject outright when more than one `ruview_session` is present, since a browser +should never legitimately send two. Add `Secure` and the `__Host-` prefix +conditionally when the server knows it is behind TLS, keeping the plain-HTTP LAN +case working. ## Alternatives considered diff --git a/docs/adr/ADR-272-websocket-authentication-tickets.md b/docs/adr/ADR-272-websocket-authentication-tickets.md index 48a0bead..8b5a5024 100644 --- a/docs/adr/ADR-272-websocket-authentication-tickets.md +++ b/docs/adr/ADR-272-websocket-authentication-tickets.md @@ -50,9 +50,25 @@ 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 send a normal -`Authorization: Bearer` on the handshake. Routing them through a ticket would -add a round-trip and a second credential path for no benefit. +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 diff --git a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs index 34fc339b..045f8ca7 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs @@ -1155,6 +1155,129 @@ mod oauth_tests { .status() } + /// Same as [`call`] but presents a browser session cookie instead of a + /// bearer. The cookie is minted through `browser_session`'s real signing + /// path, so this exercises genuine verification. + async fn call_with_session(auth: AuthState, method: &str, path: &str, cookie: &str) -> StatusCode { + let req = Request::builder() + .method(method) + .uri(path) + .header(axum::http::header::COOKIE, cookie); + app(auth) + .oneshot(req.body(Body::empty()).unwrap()) + .await + .unwrap() + .status() + } + + fn session_cookie(scope_claim: &str, ttl: i64) -> String { + format!( + "ruview_session={}", + crate::browser_session::test_cookie_value("sub-b", "acct-b", scope_claim, ttl) + ) + } + + // ── browser session cookie as a credential ──────────────────────── + // + // The session cookie authorizes /api/v1/* and WebSocket upgrades, and had + // no test presenting one at any level — the newest credential in the system + // was the one with no executable evidence behind it. + + #[tokio::test] + async fn a_read_scoped_browser_session_can_read() { + let c = session_cookie(scope::SENSING_READ, 3600); + assert_eq!( + call_with_session(oauth_only(), "GET", "/api/v1/models", &c).await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn a_read_scoped_browser_session_cannot_delete_or_train() { + // MUTANT THIS KILLS: replacing `session.has_scope(required)` with + // `true` in the cookie branch. Without this, anyone signed in through + // the browser — the least-bound credential in the system, host-only + // with no proof-of-possession — could delete models and recordings and + // start training, regardless of what they consented to. + let c = session_cookie(scope::SENSING_READ, 3600); + for (method, path) in [ + ("DELETE", "/api/v1/models/m1"), + ("DELETE", "/api/v1/recording/r1"), + ("POST", "/api/v1/train/start"), + ] { + assert_eq!( + call_with_session(oauth_only(), method, path, &c).await, + StatusCode::UNAUTHORIZED, + "{method} {path} accepted a read-only browser session" + ); + } + } + + #[tokio::test] + async fn an_admin_scoped_browser_session_can_delete() { + // The negative above must not pass merely because cookies never work. + let c = session_cookie( + &format!("{} {}", scope::SENSING_READ, scope::SENSING_ADMIN), + 3600, + ); + assert_eq!( + call_with_session(oauth_only(), "DELETE", "/api/v1/models/m1", &c).await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn an_expired_browser_session_is_refused() { + let c = session_cookie(scope::SENSING_READ, -1); + assert_eq!( + call_with_session(oauth_only(), "GET", "/api/v1/models", &c).await, + StatusCode::UNAUTHORIZED + ); + } + + #[tokio::test] + async fn a_bad_bearer_beats_a_good_cookie_rather_than_falling_back() { + // Pins REAL precedence, which is not what the ordering comment in + // `require_bearer` implies. When an Authorization header is present the + // OAuth step returns on BOTH arms, so the cookie branch that follows it + // is unreachable — a browser holding a valid session that also sends a + // stale bearer gets 401 rather than falling back to its cookie. + // + // Verified empirically: mutating that branch's `has_scope` to `true` + // changes no test outcome, while mutating the one in + // `session_or_unauthorized` fails `a_read_scoped_browser_session_ + // cannot_delete_or_train`. + // + // This fails CLOSED, so it is not a hole — but it was undocumented and + // untested, and "try the next credential" is what the code reads like. + // Pinned here so changing it is a decision rather than an accident. + let c = session_cookie(scope::SENSING_READ, 3600); + let req = Request::builder() + .method("GET") + .uri("/api/v1/models") + .header(AUTHORIZATION, "Bearer not-a-valid-token") + .header(axum::http::header::COOKIE, &c); + let status = app(oauth_only()) + .oneshot(req.body(Body::empty()).unwrap()) + .await + .unwrap() + .status(); + assert_eq!( + status, + StatusCode::UNAUTHORIZED, + "a presented bearer is authoritative; the cookie is not consulted after it fails" + ); + } + + #[tokio::test] + async fn a_forged_browser_session_is_refused() { + let c = "ruview_session=Zm9yZ2Vk.bm90LWEtdmFsaWQtbWFj"; + assert_eq!( + call_with_session(oauth_only(), "GET", "/api/v1/models", c).await, + StatusCode::UNAUTHORIZED + ); + } + fn oauth_only() -> AuthState { AuthState { token: None, 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 6e67e6b9..ff86c559 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -369,12 +369,214 @@ pub fn from_cookie_header(cookie_header: &str) -> Option { session.is_live().then_some(session) } +/// Install a usable signing secret for tests in this crate. +/// +/// Idempotent: `SECRET` is a `OnceLock`, so whichever test gets there first +/// wins and the rest reuse it. Nothing here depends on the secret's VALUE, only +/// on the process having one, so the race is benign. +#[cfg(test)] +pub(crate) fn init_secret_for_tests() { + let _ = SECRET.set(Some("crate-test-session-secret".to_string())); +} + +/// Mint a session cookie VALUE (not a `Set-Cookie` header) for tests elsewhere +/// in this crate — `bearer_auth`, which needs to present one. +/// +/// Deliberately goes through the same `sign` path as [`issue`], so a test that +/// presents this is exercising the real verification path rather than a +/// test-only bypass. `ttl` may be negative to forge an already-expired session. +#[cfg(test)] +pub(crate) fn test_cookie_value(subject: &str, account_id: &str, scope: &str, ttl: i64) -> String { + init_secret_for_tests(); + let secret = secret().expect("secret installed above"); + let session = BrowserSession { + subject: subject.to_string(), + account_id: account_id.to_string(), + scope: scope.to_string(), + exp: now() + ttl, + }; + sign(&serde_json::to_vec(&session).expect("serializes"), &secret) +} + #[cfg(test)] mod tests { use super::*; const SECRET: &str = "test-secret-value"; + /// Pull a cookie's value out of a `Set-Cookie` header, as a browser would + /// when later sending it back in a `Cookie:` header. + fn value_of(set_cookie: &str) -> String { + set_cookie + .split(';') + .next() + .and_then(|kv| kv.split_once('=')) + .map(|(_, v)| v.to_string()) + .expect("Set-Cookie has name=value") + } + + fn query_param(url: &str, key: &str) -> String { + url.split(['?', '&']) + .find_map(|p| p.strip_prefix(&format!("{key}="))) + .unwrap_or_else(|| panic!("{key} missing from {url}")) + .to_string() + } + + // ── public API: the surface that had no tests at all ────────────── + // + // Every test below this line covers a PUBLIC function. The tests that + // already existed all targeted private helpers (sign, unsign, cookie, + // read_cookie, is_live, has_scope), so `issue`, `from_cookie_header`, + // `begin`, `verifier_for_callback` and `is_configured` — the entire + // browser sign-in flow — had no executable evidence behind them. + + #[test] + fn a_server_with_a_secret_reports_browser_sign_in_as_available() { + init_secret_for_tests(); + assert!(is_configured(), "sign-in must be offered once a secret exists"); + } + + #[test] + fn begin_produces_an_authorize_url_carrying_every_required_parameter() { + init_secret_for_tests(); + let (url, set_cookie) = + begin("https://auth.cognitum.one/", "ruview", "sensing:read", false).unwrap(); + + assert!(url.starts_with("https://auth.cognitum.one/oauth/authorize?"), "{url}"); + assert!(url.contains("response_type=code"), "{url}"); + assert!(url.contains("client_id=ruview"), "{url}"); + // S256 only — the AS rejects `plain`, so getting this wrong is a + // sign-in that always fails. + assert!(url.contains("code_challenge_method=S256"), "{url}"); + assert!(!query_param(&url, "code_challenge").is_empty(), "{url}"); + assert!(!query_param(&url, "state").is_empty(), "{url}"); + // The scope's space must survive encoding or the AS sees one scope. + let (u2, _) = begin("https://a.example", "ruview", "sensing:read sensing:admin", false).unwrap(); + assert!(u2.contains("sensing%3Aread%20sensing%3Aadmin"), "{u2}"); + + // The verifier must never be in the URL — only its S256 hash. + assert!(set_cookie.starts_with(TXN_COOKIE), "{set_cookie}"); + assert!(set_cookie.contains("HttpOnly"), "the verifier must not be script-readable"); + } + + #[test] + fn the_callback_returns_the_verifier_when_the_state_matches() { + init_secret_for_tests(); + let (url, set_cookie) = begin("https://a.example", "ruview", "sensing:read", false).unwrap(); + let state = query_param(&url, "state"); + let header = format!("{TXN_COOKIE}={}", value_of(&set_cookie)); + + let verifier = verifier_for_callback(&header, &state).expect("matching state"); + assert!(verifier.len() >= 43, "PKCE verifier looks too short: {}", verifier.len()); + } + + #[test] + fn a_callback_whose_state_does_not_match_is_refused() { + // MUTANT THIS KILLS: deleting the `state` comparison in + // `verifier_for_callback`. Without it the callback accepts a code from + // a flow the user never started — login CSRF: an attacker completes + // their own authorization, feeds the victim the resulting callback URL, + // and the victim's browser silently ends up in the ATTACKER's session. + init_secret_for_tests(); + let (_url, set_cookie) = begin("https://a.example", "ruview", "sensing:read", false).unwrap(); + let header = format!("{TXN_COOKIE}={}", value_of(&set_cookie)); + + assert!(matches!( + verifier_for_callback(&header, "state-from-a-different-flow"), + Err(SessionError::StateMismatch) + )); + // Empty is the degenerate case a naive comparison lets through. + assert!(matches!( + verifier_for_callback(&header, ""), + Err(SessionError::StateMismatch) + )); + } + + #[test] + fn a_callback_with_no_transaction_or_a_forged_one_is_refused() { + init_secret_for_tests(); + // No cookie at all. + assert!(matches!( + verifier_for_callback("other=1", "any"), + Err(SessionError::InvalidTransaction) + )); + // Present but not signed by us: an attacker choosing their own verifier + // would defeat PKCE entirely. + assert!(matches!( + verifier_for_callback(&format!("{TXN_COOKIE}=bm90LXNpZ25lZA.deadbeef"), "any"), + Err(SessionError::InvalidTransaction) + )); + } + + #[test] + fn an_expired_transaction_is_refused_even_with_the_right_state() { + init_secret_for_tests(); + let secret = secret().unwrap(); + let txn = Transaction { + state: "s".into(), + verifier: "v".into(), + exp: now() - 1, + }; + let header = format!( + "{TXN_COOKIE}={}", + sign(&serde_json::to_vec(&txn).unwrap(), &secret) + ); + assert!(matches!( + verifier_for_callback(&header, "s"), + Err(SessionError::InvalidTransaction) + )); + } + + #[test] + fn an_issued_session_round_trips_with_its_subject_account_and_scope() { + init_secret_for_tests(); + let raw = test_cookie_value("sub-1", "acct-1", "sensing:read", 3600); + let session = from_cookie_header(&format!("{SESSION_COOKIE}={raw}")) + .expect("a freshly issued session must be recoverable"); + + assert_eq!(session.subject, "sub-1"); + assert_eq!(session.account_id, "acct-1"); + assert!(session.has_scope("sensing:read")); + assert!(!session.has_scope("sensing:admin"), "scope must not be widened in transit"); + } + + #[test] + fn an_expired_session_cookie_does_not_authenticate() { + // MUTANT THIS KILLS: `session.is_live().then_some(session)` -> + // `Some(session)` in `from_cookie_header`. `is_live` IS unit-tested, + // but nothing asserted that the caller consults it — the recurring + // "tested in isolation, call site untested" shape. Without this, a + // signed cookie authenticates forever and the session TTL is decorative. + init_secret_for_tests(); + let raw = test_cookie_value("sub-1", "acct-1", "sensing:read", -1); + assert!(from_cookie_header(&format!("{SESSION_COOKIE}={raw}")).is_none()); + } + + #[test] + fn a_session_signed_with_another_secret_does_not_authenticate() { + init_secret_for_tests(); + // Forged with a different key: the payload is well-formed and unexpired, + // so only the MAC stands between it and a valid session. + let forged = sign( + &serde_json::to_vec(&BrowserSession { + subject: "attacker".into(), + account_id: "acct-attacker".into(), + scope: "sensing:admin".into(), + exp: now() + 3600, + }) + .unwrap(), + "a-different-secret", + ); + assert!(from_cookie_header(&format!("{SESSION_COOKIE}={forged}")).is_none()); + } + + #[test] + fn clearing_cookies_expires_them_immediately() { + for c in [clear_session(false), clear_transaction(false)] { + assert!(c.contains("Max-Age=0"), "{c}"); + } + } + fn session(exp: i64) -> BrowserSession { BrowserSession { subject: "user-1".into(),