From f7cc68bd5c9a446ff61f6c02fb7dc450fe53c1e2 Mon Sep 17 00:00:00 2001 From: Dragan Spiridonov Date: Thu, 23 Jul 2026 14:03:54 +0200 Subject: [PATCH] fix(auth): check scope before step-up, so read-only callers are not sent in a circle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by an empirical route sweep during the pre-merge pass, not by a test. The step-up gate fired BEFORE the scope check, so a caller holding only `sensing:read` who attempted a privileged action with a session older than ADMIN_REVERIFY_SECS received the RFC 6750 "reauthentication required" challenge. `api.service.js` acts on that by redirecting through /oauth/start — and the user returns with exactly the same scopes and is refused again. One wasted round trip, and a reason for the refusal that was simply untrue: they were not refused for staleness, they were refused for capability. Now the scope check comes first, so only a caller who actually HOLDS `sensing:admin` is ever asked to prove the session is fresh. Not a loop before (the second refusal is a plain 401 with no challenge) and not a security issue either way — both paths refuse. It was misleading, and it cost a redirect. Also confirms the deny-by-default gate does not over-reach. Probed the real binary with auth off vs on: /, /ui/*, /health, /health/{live,ready,metrics,version}, /oauth/{status,start} unchanged /api/field, /api/v1/* 200 -> 401 (intended) /metrics, /favicon.ico 404 -> 401 (never existed; now uniform) Nothing that returned 2xx before returns 401 now, which is the property that matters for existing deployments. Tests: +2. `a_read_only_user_is_not_sent_to_reauthenticate_pointlessly` fails against the old ordering with the challenge header present; its counterpart `an_admin_holder_with_a_stale_session_does_get_the_challenge` ensures the fix does not simply disable the signal. Verified: workspace 176 suites clean, ruview-auth 62+25+2 --all-features. Co-Authored-By: Ruflo & AQE --- .../src/bearer_auth.rs | 87 ++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) 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 891b4fb7..783ed7c1 100644 --- a/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs +++ b/v2/crates/wifi-densepose-sensing-server/src/bearer_auth.rs @@ -667,7 +667,16 @@ pub async fn require_bearer( // token that created it and Cognitum offers no introspection, // so a stale session is authority we cannot revoke — bounded // here to the routes where that authority actually does damage. - if required == scope::SENSING_ADMIN && !session.recently_authenticated() { + // + // Ordered AFTER the scope check on purpose. Asking someone who + // does not hold `sensing:admin` to re-authenticate sends them + // through a redirect that cannot possibly help: they come back + // with the same scopes and are refused again. Only a caller who + // actually holds the capability is asked to prove it is fresh. + if session.has_scope(required) + && required == scope::SENSING_ADMIN + && !session.recently_authenticated() + { tracing::debug!( sub = %session.subject, path = %path.as_str(), @@ -716,7 +725,16 @@ async fn session_or_unauthorized(auth: &AuthState, request: Request, next: Next) // token that created it and Cognitum offers no introspection, // so a stale session is authority we cannot revoke — bounded // here to the routes where that authority actually does damage. - if required == scope::SENSING_ADMIN && !session.recently_authenticated() { + // + // Ordered AFTER the scope check on purpose. Asking someone who + // does not hold `sensing:admin` to re-authenticate sends them + // through a redirect that cannot possibly help: they come back + // with the same scopes and are refused again. Only a caller who + // actually holds the capability is asked to prove it is fresh. + if session.has_scope(required) + && required == scope::SENSING_ADMIN + && !session.recently_authenticated() + { tracing::debug!( sub = %session.subject, path = %path.as_str(), @@ -1347,6 +1365,71 @@ mod oauth_tests { } } + #[tokio::test] + async fn a_read_only_user_is_not_sent_to_reauthenticate_pointlessly() { + // The scope check must come FIRST. A caller without `sensing:admin` + // cannot be helped by re-authenticating — they return with the same + // scopes and are refused again — so sending the step-up challenge would + // cost them a redirect and tell them something untrue about why they + // were refused. Only a caller who actually HOLDS the capability is + // asked to prove it is fresh. + let stale_read_only = format!( + "ruview_session={}", + crate::browser_session::test_cookie_value_aged( + "sub-b", + "acct-b", + scope::SENSING_READ, + 3600, + crate::browser_session::ADMIN_REVERIFY_SECS + 60, + ) + ); + let req = Request::builder() + .method("DELETE") + .uri("/api/v1/models/m1") + .header(axum::http::header::COOKIE, &stale_read_only); + let resp = app(oauth_only()) + .oneshot(req.body(Body::empty()).unwrap()) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let challenge = resp + .headers() + .get(axum::http::header::WWW_AUTHENTICATE) + .and_then(|v| v.to_str().ok()) + .unwrap_or_default(); + assert!( + !challenge.contains("reauthentication required"), + "a read-only caller must not be told to re-authenticate: {challenge:?}" + ); + } + + #[tokio::test] + async fn an_admin_holder_with_a_stale_session_does_get_the_challenge() { + // The counterpart: the signal must actually fire for the caller it can + // help, or the UI never learns to send them back through /oauth/start. + let c = aged_admin_cookie(crate::browser_session::ADMIN_REVERIFY_SECS + 60); + let req = Request::builder() + .method("DELETE") + .uri("/api/v1/models/m1") + .header(axum::http::header::COOKIE, &c); + let resp = app(oauth_only()) + .oneshot(req.body(Body::empty()).unwrap()) + .await + .unwrap(); + + assert_eq!(resp.status(), StatusCode::UNAUTHORIZED); + let challenge = resp + .headers() + .get(axum::http::header::WWW_AUTHENTICATE) + .and_then(|v| v.to_str().ok()) + .unwrap_or_default(); + assert!( + challenge.contains("reauthentication required"), + "an admin holder with a stale session must be told to re-authenticate: {challenge:?}" + ); + } + #[tokio::test] async fn a_freshly_authenticated_session_can_delete() { // The negative above must not pass merely because admin never works.