mirror of
https://github.com/ruvnet/RuView
synced 2026-07-24 17:43:20 +00:00
fix(auth): close /api/field bypass, two fail-opens, and a self-disarming test
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
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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))
|
||||
);
|
||||
}
|
||||
|
||||
+72
-8
@@ -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', () => {
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -60,14 +60,24 @@ fn path_or_default(p: Option<PathBuf>) -> 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());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -86,18 +86,78 @@ pub const DEFAULT_CLIENT_ID: &str = "ruview";
|
||||
fn allowed_client_ids() -> Vec<String> {
|
||||
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<String> = 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" }))
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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<Self> {
|
||||
///
|
||||
/// 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", &[]),
|
||||
|
||||
Reference in New Issue
Block a user