From c72bbc15dd7ebd9e3a525aaa5cb152b0b3fc799c Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 11:48:59 +0200 Subject: [PATCH] fix(auth): close /api/field bypass, two fail-opens, and a self-disarming test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings from a qe-court adversarial round (4 prosecutors across 2 vendors). Each was verified against the code before being accepted; the ones below reproduced, the rest are reported in the PR thread rather than acted on. FATAL — `/api/field` was reachable with no credential, on both listeners. The gate protected `/api/v1/*` by prefix. `/api/field` is the REST sibling of `/ws/field` and serves the same signed FieldEvent stream — live presence, pose, vitals. `/ws/field` was gated in this PR; its twin one path segment over was not. Measured with RUVIEW_API_TOKEN set and no credential supplied: /api/v1/models 401 (control) /ws/field 401 (gated by this PR) /api/field 200 on :8080 AND :8765 Fixed by inverting the gate to deny-by-default with an explicit anonymous allowlist (`/`, `/ui`, `/health`, `/oauth/`). A route added at a new path is now gated because nobody exposed it, rather than exposed because nobody protected it — the same inversion already applied to the scope gate. FATAL — the wiring test disarmed itself exactly when it mattered. `Server::start` returned an Option that all five tests turned into `return`, so a server that failed to boot produced "5 passed" with zero assertions run, and cargo swallows the skip line without --nocapture. The one test that observes real wiring — the guard against both shipped bypasses — was silent for any change that breaks startup, including a boot panic in the auth path. It now panics with the child's stderr. MAJOR — a malformed client-id list silently disabled the audience check. An empty allowlist is the opt-out sentinel in verify.rs. `RUVIEW_OAUTH_CLIENT_IDS=","` is non-empty, passes the guard, then filters to an empty Vec — turning the audience boundary off with no log and admitting a token minted for any other Cognitum product. Only a literal `*` may opt out now; anything else that parses to nothing warns and falls back to the default. Same fail-open shape as the scope denylist this PR already had to invert. MAJOR — credentials were world-readable for a window on every refresh. `fs::write` creates at 0666 & !umask (0644 by default), and both writers chmodded afterwards. The existing permissions test asserted on the FINAL file and passed throughout. Affected the CLI refresh token (rotated with reuse detection — a thief who presents it first takes the session family) and the browser session secret (the HMAC key for every session; stealing it forges any account at any scope). Both now create with mode 0600 via OpenOptions. MAJOR — ui/sw.js cached authenticated API responses. Closing the /oauth/ leg left the /api/ leg open. `networkFirst` cached every successful response, keyed by URL alone, purged by nothing at sign-out: sign in as A, load sensing data, sign out, sign in as B, lose the network, and B is served A's data with no authorization check. API responses are now network-only — which is also the correct behaviour for a live sensing dashboard, where replaying a stale reading can show a room occupied after the person left — plus a cache purge on sign-out. CI — 40 of ruview-auth's 87 tests never ran. The workspace runs --no-default-features, which switches off the `login` and `pkce` features. Measured: 47 tests vs 87. The whole interactive sign-in path — credential storage, single-flight refresh, the file lock, the loopback callback — was green locally and never executed in CI. Added an --all-features step. (Checked the sensing-server for the same problem and did NOT find it: bearer_auth's 49 and browser_session's 15 do run under CI flags.) Tests: +2 wiring tests (one fails against the old gate with "http port served /api/field to an anonymous caller", passes after), +3 UI service-worker tests, +3 CLI scope tests, +1 temp-file permission test, +1 client-id parsing test. wifi-densepose-cli/src/auth.rs had zero tests and builds its own scope string, so the library's least-privilege test said nothing about what the CLI requests. Verified: ruview-auth 61+25+2 pass (--all-features), sensing-server bearer_auth 50, browser_session 15, auth_wiring 7, workspace 25 suites clean, UI 22. Co-Authored-By: Ruflo & AQE --- .github/workflows/ci.yml | 13 ++ ui/sw.js | 61 +++++++-- ui/sw.test.mjs | 80 +++++++++-- v2/crates/ruview-auth/src/login/store.rs | 65 ++++++++- v2/crates/wifi-densepose-cli/src/auth.rs | 52 +++++++- .../src/bearer_auth.rs | 109 ++++++++++++++- .../src/browser_session.rs | 21 ++- .../tests/auth_wiring.rs | 126 +++++++++++++----- 8 files changed, 459 insertions(+), 68 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 58900321..2bbdc260 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -171,6 +171,19 @@ jobs: - name: ADR-135 calibration witness proof (determinism guard) run: bash scripts/verify-calibration-proof.sh + # The workspace runs with --no-default-features, which switches OFF + # ruview-auth's `login` and `pkce` features. That silently excluded 40 of + # its 87 tests — the whole interactive sign-in path: credential storage, + # single-flight refresh, the advisory file lock, the loopback callback, and + # PKCE generation. They were green locally and never executed here. + # Measured: 47 tests with --no-default-features, 87 with --all-features. + - name: Run ruview-auth tests with all features (ADR-271 login path) + working-directory: v2 + env: + CARGO_PROFILE_DEV_DEBUG: "0" + CARGO_PROFILE_TEST_DEBUG: "0" + run: cargo test -p ruview-auth --all-features + # Browser-facing JavaScript. # # These run the dashboard's own modules in Node with stubbed browser globals. diff --git a/ui/sw.js b/ui/sw.js index 8e2bfa3f..f47d00cc 100644 --- a/ui/sw.js +++ b/ui/sw.js @@ -97,7 +97,16 @@ self.addEventListener('fetch', (event) => { // Credentialed endpoints: hands off entirely. Not networkFirst — that still // writes a copy into the cache, which would be replayed the moment the server // is briefly unreachable, silently reinstating a stale sign-in state. - if (NEVER_CACHE_PREFIXES.some((prefix) => url.pathname.startsWith(prefix))) return; + if (NEVER_CACHE_PREFIXES.some((prefix) => url.pathname.startsWith(prefix))) { + // Signing out is the one moment we know cached data belongs to a session + // that is ending. Observed, not intercepted — the request itself still goes + // straight to the network. `waitUntil` keeps the worker alive for the purge + // even though the navigation is what the browser is really waiting on. + if (url.pathname === '/oauth/logout') { + event.waitUntil(purgeNonShell()); + } + return; + } // API calls: network-first with cache fallback if (url.pathname.startsWith('/api/') || url.pathname.startsWith('/health/')) { @@ -140,20 +149,54 @@ async function cacheFirst(request) { } } +/** + * Network-only, with an explicit offline signal. + * + * This used to cache every successful `/api/` response and replay it whenever + * the network failed. Two things are wrong with that now: + * + * 1. **Authorization.** API responses are per-user once auth is on, but the + * cache is keyed by URL alone and nothing purges it at sign-out. Sign in as + * A, load sensing data, sign out, sign in as B, lose the network — B is + * served A's data with no authorization check at all. That is the same + * defect class as the cached `/oauth/status`: the Cache API happily outlives + * the session that produced its contents. + * 2. **Correctness.** This is a live sensing dashboard. Replaying a stale pose + * or presence reading as if it were current is its own defect — it can show + * a room as occupied after the person has left. + * + * The offline shell (HTML/CSS/JS) is still cached; only the data is not. If + * offline data replay is wanted back, it needs a per-session cache key and a + * purge on sign-out, not a URL-keyed shared cache. + */ async function networkFirst(request) { try { - const response = await fetch(request); - if (response.ok) { - const cache = await caches.open(CACHE_NAME); - cache.put(request, response.clone()); - } - return response; + return await fetch(request); } catch { - const cached = await caches.match(request); - if (cached) return cached; return new Response(JSON.stringify({ error: 'offline' }), { status: 503, headers: { 'Content-Type': 'application/json' } }); } } + +/** + * Drop everything except the static shell. + * + * Called when the user signs out. Belt-and-braces: nothing user-specific should + * be in the cache after the `networkFirst` change above, but a cache populated + * by an OLDER worker on this browser can still hold API responses, and that + * worker's entries survive into this one under the same name. + */ +async function purgeNonShell() { + const cache = await caches.open(CACHE_NAME); + const keys = await cache.keys(); + await Promise.all( + keys + .filter((req) => { + const p = new URL(req.url).pathname; + return p.startsWith('/api/') || p.startsWith('/health/'); + }) + .map((req) => cache.delete(req)) + ); +} diff --git a/ui/sw.test.mjs b/ui/sw.test.mjs index cbe42f90..7a2f59ff 100644 --- a/ui/sw.test.mjs +++ b/ui/sw.test.mjs @@ -33,10 +33,18 @@ function loadServiceWorker() { const listeners = {}; const cachePuts = []; + // A real in-memory cache, so purge and put behaviour can be observed rather + // than assumed. + const entries = new Map(); const cacheStub = { addAll: async () => {}, - put: async (req, res) => { cachePuts.push(String(req.url ?? req)); return undefined; }, - keys: async () => [], + put: async (req, res) => { + const url = String(req.url ?? req); + cachePuts.push(url); + entries.set(url, res); + }, + keys: async () => Array.from(entries.keys()).map((url) => ({ url })), + delete: async (req) => entries.delete(String(req.url ?? req)), match: async () => undefined, }; @@ -63,7 +71,7 @@ function loadServiceWorker() { vm.createContext(sandbox); vm.runInContext(SW_SOURCE, sandbox); - return { listeners, cachePuts, sandbox }; + return { listeners, cachePuts, sandbox, entries }; } /** @@ -72,17 +80,28 @@ function loadServiceWorker() { * request goes to the network untouched, which is the only safe outcome for a * credentialed endpoint. */ -function route(path, { method = 'GET', mode = 'cors', headers = {} } = {}) { - const { listeners } = loadServiceWorker(); +function route(path, opts = {}) { + return dispatch(path, opts).handled; +} + +/** Route one request and expose everything the worker did with it. */ +function dispatch(path, { method = 'GET', mode = 'cors', headers = {}, sw = null } = {}) { + const worker = sw ?? loadServiceWorker(); let handled = false; + let responded = null; + const waited = []; const request = { url: `${ORIGIN}${path}`, method, mode, headers: { get: (k) => headers[k] ?? headers[k.toLowerCase()] ?? null }, }; - listeners.fetch({ request, respondWith: () => { handled = true; } }); - return handled; + worker.listeners.fetch({ + request, + respondWith: (p) => { handled = true; responded = p; }, + waitUntil: (p) => { waited.push(p); }, + }); + return { handled, responded, waited, worker }; } let sw; @@ -123,7 +142,8 @@ test('a navigation request is still served cache-first', () => { assert.equal(route('/ui/', { mode: 'navigate' }), true); }); -test('API paths are still handled, so offline fallback survives', () => { +test('API paths are still routed through the worker', () => { + // Handled, but network-only — see the "not written to the cache" test below. assert.equal(route('/api/v1/models'), true); assert.equal(route('/health/live'), true); }); @@ -153,6 +173,50 @@ test('cross-origin requests are ignored', () => { assert.equal(handled, false); }); +// --- authenticated API responses must not be retained ------------------------ +// Filed by the cross-vendor prosecutor in the qe-court round after the +// /oauth/status fix: closing the /oauth/ leg left the /api/ leg open. + +test('a successful API response is NOT written to the cache', async () => { + // The leak: cache keys are URLs, nothing partitions them by session, and + // nothing purged them at sign-out. Sign in as A, fetch sensing data, sign + // out, sign in as B, lose the network -> B is served A's data. + const { responded, worker } = dispatch('/api/v1/sensing/latest'); + await responded; + assert.deepEqual(worker.cachePuts, [], 'API responses must not be cached'); +}); + +test('an API request with no network returns 503 rather than stale data', async () => { + // Also a correctness property, not only an authorization one: replaying a + // stale pose reading as current can show a room occupied after the person + // has left. + const worker = loadServiceWorker(); + worker.sandbox.fetch = async () => { throw new Error('offline'); }; + worker.entries.set(`${ORIGIN}/api/v1/sensing/latest`, { stale: true }); + + const { responded } = dispatch('/api/v1/sensing/latest', { sw: worker }); + const res = await responded; + assert.equal(res.status, 503); + assert.match(String(res.body), /offline/); +}); + +test('signing out purges cached API data but keeps the offline shell', async () => { + const worker = loadServiceWorker(); + worker.entries.set(`${ORIGIN}/api/v1/sensing/latest`, {}); + worker.entries.set(`${ORIGIN}/health/live`, {}); + worker.entries.set(`${ORIGIN}/app.js`, {}); + + const { handled, waited } = dispatch('/oauth/logout', { sw: worker }); + // Observed, not intercepted — the logout request itself must still reach the + // server, or signing out would not actually sign anyone out. + assert.equal(handled, false, '/oauth/logout must still go to the network'); + assert.equal(waited.length, 1, 'the purge must be kept alive via waitUntil'); + await Promise.all(waited); + + const left = Array.from(worker.entries.keys()); + assert.deepEqual(left, [`${ORIGIN}/app.js`], 'only the static shell should survive'); +}); + // --- cache hygiene ----------------------------------------------------------- test('the cache name is bumped so clients holding the poisoned v1 evict it', () => { diff --git a/v2/crates/ruview-auth/src/login/store.rs b/v2/crates/ruview-auth/src/login/store.rs index 17a058a6..80e360e3 100644 --- a/v2/crates/ruview-auth/src/login/store.rs +++ b/v2/crates/ruview-auth/src/login/store.rs @@ -228,11 +228,19 @@ pub fn save(path: &Path, creds: &StoredCredentials) -> Result<(), StoreError> { let json = serde_json::to_vec_pretty(creds).expect("credentials serialize"); let tmp = path.with_extension("tmp"); - std::fs::write(&tmp, &json).map_err(|source| StoreError::Unwritable { + // Create with 0600 ALREADY SET, rather than write-then-chmod. + // + // `fs::write` creates at `0666 & !umask` — 0644 on a default umask — so the + // refresh token was world-readable at a predictable path for the window + // between the write and the chmod. `save` runs on every silent refresh + // (REFRESH_SKEW_SECS against a 15-minute token), so that window recurred + // every few minutes, and the refresh token is the highest-value credential + // here: identity rotates with reuse detection, so a thief who presents it + // first takes the session family and logs the real user out. + write_private(&tmp, &json).map_err(|source| StoreError::Unwritable { path: tmp.clone(), source, })?; - restrict_permissions(&tmp)?; std::fs::rename(&tmp, path).map_err(|source| StoreError::Unwritable { path: path.to_path_buf(), source, @@ -240,6 +248,29 @@ pub fn save(path: &Path, creds: &StoredCredentials) -> Result<(), StoreError> { Ok(()) } +/// Write `bytes` to a file that is never readable by anyone else, at any point. +/// +/// `create_new` also means a pre-existing `.tmp` — a symlink planted by a local +/// attacker, or a leftover from a crash — is an error rather than a target. +#[cfg(unix)] +fn write_private(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let _ = std::fs::remove_file(path); // clear our own leftover, not a race + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path)?; + f.write_all(bytes)?; + f.sync_all() +} + +#[cfg(not(unix))] +fn write_private(path: &Path, bytes: &[u8]) -> std::io::Result<()> { + std::fs::write(path, bytes) +} + #[cfg(unix)] fn restrict_permissions(path: &Path) -> Result<(), StoreError> { use std::os::unix::fs::PermissionsExt; @@ -557,6 +588,36 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn the_temp_file_is_never_world_readable_even_for_an_instant() { + // The test above checks the FINAL file. It passed while `save` wrote via + // `fs::write` (0644 under a default umask) and chmodded afterwards — so + // the refresh token sat world-readable at a predictable path in between, + // on every silent refresh. Asserting on the destination could never see + // that; this asserts on the temp file `save` actually creates. + use std::os::unix::fs::PermissionsExt; + let dir = std::env::temp_dir().join(format!("ruview-auth-tmpperm-{}", std::process::id())); + let path = dir.join("credentials.json"); + let tmp = path.with_extension("tmp"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + write_private(&tmp, b"secret").unwrap(); + let mode = std::fs::metadata(&tmp).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "temp file must be created 0600, got {mode:o}"); + assert_eq!(mode & 0o077, 0, "group/other must have no access at all"); + + // A leftover from a crashed run is cleared and replaced, and the + // replacement is 0600 too — the mode must not be inherited from + // whatever was there before. + let _ = std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o666)); + write_private(&tmp, b"replacement").unwrap(); + let mode = std::fs::metadata(&tmp).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "a replaced temp file must also be 0600, got {mode:o}"); + assert_eq!(std::fs::read(&tmp).unwrap(), b"replacement", "must replace, not append"); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn loading_a_missing_file_says_not_logged_in_rather_than_erroring_obscurely() { let path = std::env::temp_dir().join("ruview-auth-definitely-absent.json"); diff --git a/v2/crates/wifi-densepose-cli/src/auth.rs b/v2/crates/wifi-densepose-cli/src/auth.rs index 080cb6c2..eb97b4b8 100644 --- a/v2/crates/wifi-densepose-cli/src/auth.rs +++ b/v2/crates/wifi-densepose-cli/src/auth.rs @@ -60,14 +60,24 @@ fn path_or_default(p: Option) -> PathBuf { p.unwrap_or_else(login::default_credentials_path) } -pub async fn login_cmd(args: LoginArgs) -> anyhow::Result<()> { - let scope = if args.admin { +/// What `login` asks the authorization server for. +/// +/// Extracted so it is testable on its own. `LoginOptions::default()` has its own +/// least-privilege test in the library, but this command does NOT go through +/// that default — it builds the scope string itself, so the library test says +/// nothing about what the CLI actually requests. +fn requested_scope(admin: bool) -> String { + if admin { // Admin implies read: there is no scope hierarchy server-side, so a // session that needs both must consent to both explicitly. format!("{} {}", scope::SENSING_READ, scope::SENSING_ADMIN) } else { scope::SENSING_READ.to_string() - }; + } +} + +pub async fn login_cmd(args: LoginArgs) -> anyhow::Result<()> { + let scope = requested_scope(args.admin); let opts = LoginOptions { credentials_path: path_or_default(args.credentials_path), @@ -136,3 +146,39 @@ pub async fn whoami_cmd(args: WhoamiArgs) -> anyhow::Result<()> { } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_plain_login_asks_for_read_only() { + // The whole point of splitting the scopes (ADR-060) is that streaming + // poses must not carry the capability to delete recordings. If this + // ever returns admin by default, every session silently becomes + // destructive-capable and nothing else in the suite would notice. + let s = requested_scope(false); + assert_eq!(s, scope::SENSING_READ); + assert!(!s.contains(scope::SENSING_ADMIN), "read-only login leaked admin: {s}"); + } + + #[test] + fn admin_login_asks_for_both_because_there_is_no_hierarchy() { + // The authorization server grants exactly what is requested; admin does + // not imply read. Asking for admin alone would produce a session that + // cannot stream. + let s = requested_scope(true); + assert!(s.split_whitespace().any(|x| x == scope::SENSING_READ), "{s}"); + assert!(s.split_whitespace().any(|x| x == scope::SENSING_ADMIN), "{s}"); + } + + #[test] + fn an_explicit_credentials_path_is_honoured_over_the_default() { + // `--credentials-path` also carries the RUVIEW_CREDENTIALS_PATH env + // binding; silently ignoring it would write credentials somewhere the + // operator did not choose. + let p = PathBuf::from("/tmp/ruview-cli-explicit-credentials.json"); + assert_eq!(path_or_default(Some(p.clone())), p); + assert_eq!(path_or_default(None), login::default_credentials_path()); + } +} 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 dd4e5b7d..34fc339b 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs @@ -86,18 +86,78 @@ pub const DEFAULT_CLIENT_ID: &str = "ruview"; fn allowed_client_ids() -> Vec { match std::env::var(OAUTH_CLIENT_IDS_ENV) { Ok(v) if v.trim() == "*" => Vec::new(), // explicit opt-out - Ok(v) if !v.trim().is_empty() => v - .split(',') - .map(|c| c.trim().to_string()) - .filter(|c| !c.is_empty()) - .collect(), + Ok(v) if !v.trim().is_empty() => { + let parsed: Vec = v + .split(',') + .map(|c| c.trim().to_string()) + .filter(|c| !c.is_empty()) + .collect(); + // An empty Vec is the OPT-OUT sentinel downstream: `verify.rs` skips + // the audience check entirely when the allowlist is empty. So a value + // that is non-empty but parses to nothing — `","`, `" , "`, a stray + // trailing comma — would silently disable the audience boundary and + // admit a token minted for any other Cognitum product. Only the + // literal `*` may turn that check off. + if parsed.is_empty() { + tracing::warn!( + value = %v, + "{OAUTH_CLIENT_IDS_ENV} is set but lists no client id; falling back to \ + {DEFAULT_CLIENT_ID}. Use `*` if you really mean to accept any client." + ); + return vec![DEFAULT_CLIENT_ID.to_string()]; + } + parsed + } _ => vec![DEFAULT_CLIENT_ID.to_string()], } } /// Path prefix the middleware protects when auth is enabled. +/// +/// Retained because it names the bulk of the protected surface and is asserted +/// in tests, but it is NO LONGER the rule. The gate is [`is_anonymous`] — +/// deny-by-default. See that function for why. pub const PROTECTED_PREFIX: &str = "/api/v1/"; +/// Paths that stay reachable with no credential when auth is enabled. +/// +/// # Why an allowlist +/// +/// The gate used to be the inverse: protect `/api/v1/*`, let everything else +/// through. That is exposure-by-default, and it leaked. `/api/field` — the REST +/// sibling of `/ws/field`, serving the same signed `FieldEvent` stream of live +/// presence, pose and vitals — sits at `/api/field`, not `/api/v1/`, so it +/// returned `200` with no credential on BOTH listeners while `/api/v1/models` +/// correctly returned `401`. `/ws/field` was gated in this same PR; its REST +/// twin one path segment over was not. +/// +/// Measured, with `RUVIEW_API_TOKEN` set and no credential supplied: +/// `/api/v1/models` -> 401, `/ws/field` -> 401, `/api/field` -> 200 on :8080 +/// and :8765. +/// +/// With an allowlist, a route added at a new path is gated because nobody +/// remembered to expose it, rather than exposed because nobody remembered to +/// protect it. That is the same inversion already applied to the scope gate in +/// [`required_scope_for`]. +const ANONYMOUS_PREFIXES: &[&str] = &[ + // Orchestrator and load-balancer probes. Documented exemption (ADR-272), + // pinned by `health_stays_anonymous_on_both_listeners`. + "/health", + // Sign-in cannot require being signed in. + "/oauth/", +]; + +/// Is this path reachable without a credential? See [`ANONYMOUS_PREFIXES`]. +pub fn is_anonymous(path: &str) -> bool { + // The dashboard shell itself, mounted with `nest_service("/ui", …)`. It has + // to load for the user to reach the sign-in button at all; the data it then + // fetches is what is protected. + if path == "/" || path == "/ui" || path.starts_with("/ui/") { + return true; + } + ANONYMOUS_PREFIXES.iter().any(|p| path.starts_with(p)) +} + /// WebSocket upgrade endpoints. Previously ungated — `/ws/*` sat outside /// [`PROTECTED_PREFIX`] and `/api/v1/stream/pose` was an explicit exemption — /// because a browser's `WebSocket` constructor cannot attach an @@ -489,7 +549,7 @@ pub async fn require_bearer( } // No ticket: fall through to the bearer path below, which is how a // native (non-browser) client authenticates a WebSocket. - } else if !path.starts_with(PROTECTED_PREFIX) { + } else if is_anonymous(&path) { return next.run(request).await; } @@ -655,6 +715,43 @@ mod tests { }; use tower::ServiceExt; + /// ONE test, not three, because `allowed_client_ids` reads process-global + /// env and cargo runs tests on parallel threads — three tests mutating + /// `RUVIEW_OAUTH_CLIENT_IDS` race and fail intermittently. + #[test] + fn client_id_allowlist_parsing_never_silently_opts_out() { + let read = |v: &str| { + std::env::set_var(OAUTH_CLIENT_IDS_ENV, v); + let ids = allowed_client_ids(); + std::env::remove_var(OAUTH_CLIENT_IDS_ENV); + ids + }; + + // An empty allowlist is the OPT-OUT sentinel in `verify.rs` — it skips + // the audience check entirely. So a value that is non-empty but parses + // to nothing (a stray trailing comma, `","`) would silently turn the + // boundary off and admit a token minted for any other Cognitum product. + // Same fail-open shape as the scope denylist this PR already inverted. + for bad in [",", " , ", ",,,", " "] { + let ids = read(bad); + assert!( + !ids.is_empty(), + "{bad:?} produced an empty allowlist, which disables the audience check" + ); + assert_eq!(ids, vec![DEFAULT_CLIENT_ID.to_string()]); + } + + // The deliberate escape hatch must keep working, or the fix above would + // be a behaviour change wearing a security label. + assert!(read("*").is_empty(), "`*` must remain the way to accept any client"); + + // And a real list must still parse, trailing comma and all. + assert_eq!( + read(" ruview , musica ,"), + vec!["ruview".to_string(), "musica".to_string()] + ); + } + fn ok_handler() -> Router { Router::new() .route("/health", get(|| async { "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 4146834f..6e67e6b9 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/browser_session.rs @@ -214,13 +214,26 @@ fn write_secret(path: &Path, value: &str) -> std::io::Result<()> { std::fs::create_dir_all(dir)?; } let tmp = path.with_extension("tmp"); - std::fs::write(&tmp, value)?; + // Created 0600, not written-then-chmodded. `fs::write` creates at + // `0666 & !umask`, so this file — the HMAC key for EVERY browser session — + // was world-readable for the window before the chmod. Anyone who read it + // could forge a session cookie for any account with any scope, including + // `sensing:admin`, which is strictly worse than stealing one session. #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - // Restrict BEFORE publishing the path, as with the CLI's credentials. - std::fs::set_permissions(&tmp, std::fs::Permissions::from_mode(0o600))?; + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + let _ = std::fs::remove_file(&tmp); + let mut f = std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(&tmp)?; + f.write_all(value.as_bytes())?; + f.sync_all()?; } + #[cfg(not(unix))] + std::fs::write(&tmp, value)?; std::fs::rename(&tmp, path) } diff --git a/v2/crates/wifi-densepose-sensing-server/tests/auth_wiring.rs b/v2/crates/wifi-densepose-sensing-server/tests/auth_wiring.rs index 66f26da3..6dbf9633 100644 --- a/v2/crates/wifi-densepose-sensing-server/tests/auth_wiring.rs +++ b/v2/crates/wifi-densepose-sensing-server/tests/auth_wiring.rs @@ -57,7 +57,15 @@ impl Drop for Server { impl Server { /// Spawn the real binary. `env` lets a test choose the auth configuration. - fn start(env: &[(&str, &str)]) -> Option { + /// + /// PANICS rather than returning `None` on failure. This used to return an + /// `Option` that every test turned into `return`, which meant all five + /// assertions were skipped precisely when the server was broken — including + /// broken BY an auth change. A boot failure printed one line that `cargo + /// test` swallows without `--nocapture` and reported `5 passed`. The only + /// test in the suite that observes real wiring disarmed itself exactly when + /// it mattered; a boot-time panic in the auth path would have shipped green. + fn start(env: &[(&str, &str)]) -> Self { let (http, ws, udp) = (free_port(), free_port(), free_port()); let mut cmd = Command::new(env!("CARGO_BIN_EXE_sensing-server")); cmd.args([ @@ -74,27 +82,45 @@ impl Server { .env_remove("RUVIEW_OAUTH_ISSUER") .env_remove("RUVIEW_WS_LEGACY_UNAUTHENTICATED") .stdout(Stdio::null()) - .stderr(Stdio::null()); + // Captured, not discarded: if the server dies at boot, its stderr is the + // only thing that says why, and the panic below reproduces it. + .stderr(Stdio::piped()); for (k, v) in env { cmd.env(k, v); } - let child = cmd.spawn().ok()?; - let server = Server { child, http, ws }; - server.await_ready().then_some(server) + let mut child = cmd.spawn().expect("spawn sensing-server"); + let http_port = http; + let ws_port = ws; + if !await_ready(http_port, ws_port) { + let mut err = String::new(); + if let Some(mut s) = child.stderr.take() { + let _ = s.read_to_string(&mut err); + } + let _ = child.kill(); + let _ = child.wait(); + panic!( + "sensing-server did not become ready on :{http_port} (http) and :{ws_port} (ws) \ + within 30s. This is a FAILURE, not a skip — the wiring assertions below cannot \ + run, and a boot-time break in the auth path is exactly what they exist to catch.\n\ + --- server stderr ---\n{err}" + ); + } + Server { child, http: http_port, ws: ws_port } } - fn await_ready(&self) -> bool { - let deadline = Instant::now() + Duration::from_secs(30); - while Instant::now() < deadline { - if TcpStream::connect(("127.0.0.1", self.http)).is_ok() - && TcpStream::connect(("127.0.0.1", self.ws)).is_ok() - { - return true; - } - std::thread::sleep(Duration::from_millis(200)); +} + +fn await_ready(http: u16, ws: u16) -> bool { + let deadline = Instant::now() + Duration::from_secs(30); + while Instant::now() < deadline { + if TcpStream::connect(("127.0.0.1", http)).is_ok() + && TcpStream::connect(("127.0.0.1", ws)).is_ok() + { + return true; } - false + std::thread::sleep(Duration::from_millis(200)); } + false } /// One raw HTTP/1.1 request; returns the status code. @@ -154,12 +180,52 @@ fn ws_upgrade(port: u16, path: &str, bearer: Option<&str>) -> u16 { /// Every WebSocket path, on every listener. This list is the point of the test. const WS_PATHS: &[&str] = &["/ws/sensing", "/ws/introspection", "/api/v1/stream/pose", "/ws/field"]; +/// Non-WebSocket routes that carry sensing data and must be gated. +/// +/// `/api/field` is here because it was NOT gated: it serves the same signed +/// `FieldEvent` stream as `/ws/field`, but sits outside `/api/v1/`, and the gate +/// protected `/api/v1/*` by prefix. `/ws/field` was gated in this PR and its +/// REST twin, one path segment over, returned 200 to an anonymous caller on +/// both listeners. That is why the gate is now deny-by-default. +const PROTECTED_REST_PATHS: &[&str] = &["/api/field", "/api/v1/models"]; + +#[test] +fn with_auth_on_no_listener_serves_sensing_data_anonymously() { + let server = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]); + + for (label, port) in [("http", server.http), ("ws", server.ws)] { + for path in PROTECTED_REST_PATHS { + assert_eq!( + status(port, "GET", path, &[]), + 401, + "{label} port served {path} to an anonymous caller" + ); + // And the credential must actually work, or the assertion above + // could pass because the route simply does not exist. + let ok = status(port, "GET", path, &[("Authorization", &format!("Bearer {TOKEN}"))]); + assert_ne!(ok, 401, "{label} port {path} rejected a VALID bearer"); + } + } +} + +#[test] +fn the_dashboard_shell_and_sign_in_stay_reachable_when_auth_is_on() { + // Deny-by-default must not lock the user out of the page that renders the + // sign-in button, or of sign-in itself. This is the other half of the + // allowlist: too tight is as broken as too loose, just louder. + let server = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]); + for path in ["/health", "/oauth/status"] { + assert_ne!( + status(server.http, "GET", path, &[]), + 401, + "{path} must stay anonymous — sign-in depends on it" + ); + } +} + #[test] fn with_auth_on_no_listener_accepts_an_unauthenticated_websocket() { - let Some(server) = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]) else { - eprintln!("skipping: sensing-server did not start"); - return; - }; + let server = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]); // Control first: if REST is not gated, the server is misconfigured and the // WebSocket assertions below would pass for the wrong reason. @@ -188,10 +254,7 @@ fn with_auth_on_no_listener_accepts_an_unauthenticated_websocket() { #[test] fn a_bearer_on_the_upgrade_is_accepted_on_both_listeners() { - let Some(server) = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]) else { - eprintln!("skipping: sensing-server did not start"); - return; - }; + let server = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]); // Native clients (Python, CLI, MCP) are not browser-constrained and must be // able to authenticate a WebSocket without the ticket round-trip. for (label, port) in [("http", server.http), ("ws", server.ws)] { @@ -206,10 +269,7 @@ fn a_bearer_on_the_upgrade_is_accepted_on_both_listeners() { #[test] fn with_auth_off_both_listeners_stay_open() { // The compatibility promise: an unconfigured deployment sees no change. - let Some(server) = Server::start(&[]) else { - eprintln!("skipping: sensing-server did not start"); - return; - }; + let server = Server::start(&[]); assert_eq!(status(server.http, "GET", "/api/v1/models", &[]), 200); for (label, port) in [("http", server.http), ("ws", server.ws)] { assert_eq!( @@ -222,13 +282,10 @@ fn with_auth_off_both_listeners_stay_open() { #[test] fn the_legacy_escape_hatch_opens_websockets_without_weakening_rest() { - let Some(server) = Server::start(&[ + let server = Server::start(&[ ("RUVIEW_API_TOKEN", TOKEN), ("RUVIEW_WS_LEGACY_UNAUTHENTICATED", "1"), - ]) else { - eprintln!("skipping: sensing-server did not start"); - return; - }; + ]); // The hatch is scoped to WebSockets on purpose. If it ever widened to REST // it would be a bypass wearing a migration label. assert_eq!( @@ -249,10 +306,7 @@ fn the_legacy_escape_hatch_opens_websockets_without_weakening_rest() { fn health_stays_anonymous_on_both_listeners() { // Documented exemption (ADR-272): orchestrator probes are anonymous by // design. Pinned so it is a decision, not an accident nobody re-checks. - let Some(server) = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]) else { - eprintln!("skipping: sensing-server did not start"); - return; - }; + let server = Server::start(&[("RUVIEW_API_TOKEN", TOKEN)]); for (label, port) in [("http", server.http), ("ws", server.ws)] { assert_eq!( status(port, "GET", "/health", &[]),