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
This commit is contained in:
Dragan Spiridonov
parent
c72bbc15dd
commit
6ce50d5158
|
|
@ -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.
|
||||
|
||||
<details>
|
||||
<summary>Original text (no longer accurate)</summary>
|
||||
|
||||
`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.
|
||||
</details>
|
||||
|
||||
## 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=<their own validly signed
|
||||
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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -369,12 +369,214 @@ pub fn from_cookie_header(cookie_header: &str) -> Option<BrowserSession> {
|
|||
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(),
|
||||
|
|
|
|||
Loading…
Reference in New Issue