fix(oidc): bind logout URLs to the current session (forced-logout CSRF)

Logout URLs were signed only over the redirect target and a timestamp, so any
valid, unexpired logout URL would clear whoever's cookies followed it. The
logout signature now also covers the caller's sqlpage_auth cookie, so a logout
URL only logs out the session it was issued for. Generation and verification
select the same cookie (the last of any duplicates, matching how RequestInfo
merges them) so the check stays consistent.

Author: lovasoa

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - **Download filenames can no longer inject extra `Content-Disposition` parameters.** The `csv` and `download` components now build the `Content-Disposition` header with a properly quoted and escaped filename instead of plain string concatenation. Before this fix, a filename containing characters such as `;`, `"`, or `=` could add a second header parameter (for example a `filename*=...` value), and some browsers prefer that injected value over the intended one. You are affected if your app puts untrusted data (such as a user-provided name or a value from the database) into the `filename` of a `csv` or `download` component. No SQL change is required: the supplied filename now always appears as a single, safely quoted `filename` value.
 - **Security fix: reserved and private files could be served directly over HTTP after a trusted page loaded them.** Files that SQLPage normally refuses to serve to direct HTTP requests (anything under the reserved `sqlpage/` prefix such as migrations, configuration, and database connection details, as well as dotfiles, parent-directory traversal paths like `../secret.sql`, and absolute paths) could briefly become reachable. This happened only after a trusted page loaded that same file with `sqlpage.run_sql(...)`, which loads files with elevated privileges and caches the parsed result. While that cache entry was fresh, a direct request such as `GET /sqlpage/secret.sql` (or the extensionless alias `GET /sqlpage/secret`) returned `200 OK` and executed the private SQL instead of returning `403 Forbidden`. Worst case, an attacker who can reach your site could read or execute internal SQL that was meant to stay private, including migration and configuration logic. You are affected if your app calls `sqlpage.run_sql()` on files inside `sqlpage/` (or on dotfiles or paths outside the web root) and is reachable by untrusted users. The fix enforces the unprivileged path guard on every direct HTTP request regardless of the cache, so these paths now always return `403 Forbidden`. Upgrade to get the fix; no configuration change is required. Legitimate `sqlpage.run_sql()` includes of such files from your own pages keep working as before.
+- **Security: OIDC logout links are now bound to the session that requested them.** Previously, a logout URL produced by `sqlpage.oidc_logout_url` only signed the redirect target and a timestamp, so any visitor who opened a still-valid logout link had their SQLPage auth cookies cleared. This allowed a forced logout via CSRF (a logout link made for one user could log out another, or be replayed). It did NOT allow account takeover or access to anyone's data. Logout links are now tied to the current `sqlpage_auth` cookie, so following one only logs out the browser it was generated for. You are affected only if you use built-in OIDC authentication together with `sqlpage.oidc_logout_url`. No action is needed: the normal "click your own logout link" flow keeps working, and old links simply stop being honored for other sessions.
 
 ## v0.44.0
 

src/webserver/database/sqlpage_functions/functions.rs

@@ -1061,7 +1061,19 @@ async fn oidc_logout_url<'a>(
         );
     }
 
-    let logout_url = oidc_state.config.create_logout_url(redirect_uri);
+    // Bind the logout URL to the current session so that it can only log out
+    // the browser it was generated for, never a different user's session. Use
+    // the first cookie value, matching how verification reads the auth cookie
+    // (HttpRequest::cookie returns the first cookie of that name); signing a
+    // JSON array of duplicate cookies here would never match verification.
+    let session_token = request
+        .cookies
+        .get("sqlpage_auth")
+        .map(SingleOrVec::first_str);
+
+    let logout_url = oidc_state
+        .config
+        .create_logout_url(redirect_uri, session_token);
 
     Ok(Some(logout_url))
 }

src/webserver/oidc.rs

@@ -154,11 +154,22 @@ impl OidcConfig {
             .set_other_audience_verifier_fn(self.additional_audience_verifier.as_fn())
     }
 
-    /// Creates a logout URL with the given redirect URI
+    /// Creates a logout URL with the given redirect URI.
+    ///
+    /// The signature is bound to the current session (the value of the
+    /// [`SQLPAGE_AUTH_COOKIE_NAME`] cookie, if any) so that a logout URL
+    /// issued for one browser cannot be used to forcibly log out a different
+    /// browser. `session_token` is the current value of the auth cookie, or
+    /// `None`/empty if the caller is not authenticated.
     #[must_use]
-    pub fn create_logout_url(&self, redirect_uri: &str) -> String {
+    pub fn create_logout_url(&self, redirect_uri: &str, session_token: Option<&str>) -> String {
         let timestamp = chrono::Utc::now().timestamp();
-        let signature = compute_logout_signature(redirect_uri, timestamp, &self.client_secret);
+        let signature = compute_logout_signature(
+            redirect_uri,
+            timestamp,
+            session_token.unwrap_or_default(),
+            &self.client_secret,
+        );
         let query = form_urlencoded::Serializer::new(String::new())
             .append_pair("redirect_uri", redirect_uri)
             .append_pair("timestamp", &timestamp.to_string())
@@ -564,15 +575,38 @@ fn parse_logout_params(query: &str) -> anyhow::Result<LogoutParams> {
         .map(Query::into_inner)
 }
 
+/// Selects the `sqlpage_auth` cookie that a logout URL is bound to. When a
+/// request carries duplicate `sqlpage_auth` cookies, this returns the last one,
+/// matching how `RequestInfo` merges duplicate cookies (last value wins) and
+/// therefore what `sqlpage.oidc_logout_url` signs. `ServiceRequest::cookie`
+/// would return the first instead, which made legitimate logout URLs fail with
+/// a 400 whenever duplicate cookies were present.
+fn logout_session_cookie(request: &ServiceRequest) -> Option<Cookie<'static>> {
+    request.cookies().ok().and_then(|cookies| {
+        cookies
+            .iter()
+            .rev()
+            .find(|c| c.name() == SQLPAGE_AUTH_COOKIE_NAME)
+            .cloned()
+    })
+}
+
 fn process_oidc_logout(
     oidc_state: &OidcState,
     request: &ServiceRequest,
 ) -> anyhow::Result<HttpResponse> {
     let params = parse_logout_params(request.query_string())?;
 
-    verify_logout_params(&params, &oidc_state.config.client_secret)?;
+    let id_token_cookie = logout_session_cookie(request);
+    let session_token = id_token_cookie
+        .as_ref()
+        .map(Cookie::value)
+        .unwrap_or_default();
+
+    // The signature is bound to the session it was issued for, so a logout URL
+    // cannot be used to forcibly log out a different browser (CSRF).
+    verify_logout_params(&params, session_token, &oidc_state.config.client_secret)?;
 
-    let id_token_cookie = request.cookie(SQLPAGE_AUTH_COOKIE_NAME);
     let id_token = id_token_cookie
         .as_ref()
         .map(|c| OidcToken::from_str(c.value()))
@@ -620,7 +654,12 @@ fn process_oidc_logout(
     Ok(response)
 }
 
-fn compute_logout_signature(redirect_uri: &str, timestamp: i64, client_secret: &str) -> String {
+fn compute_logout_signature(
+    redirect_uri: &str,
+    timestamp: i64,
+    session_token: &str,
+    client_secret: &str,
+) -> String {
     use base64::Engine;
     use hmac::{Hmac, KeyInit, Mac};
     use sha2::Sha256;
@@ -629,15 +668,28 @@ fn compute_logout_signature(redirect_uri: &str, timestamp: i64, client_secret: &
         .expect("HMAC accepts any key size");
     mac.update(redirect_uri.as_bytes());
     mac.update(&timestamp.to_be_bytes());
+    // Bind the signature to the session so a logout URL can only log out the
+    // session it was issued for. The length prefix prevents ambiguity between
+    // the redirect_uri and session_token fields.
+    mac.update(&(session_token.len() as u64).to_be_bytes());
+    mac.update(session_token.as_bytes());
     let signature = mac.finalize().into_bytes();
     base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(signature)
 }
 
-fn verify_logout_params(params: &LogoutParams, client_secret: &str) -> anyhow::Result<()> {
+fn verify_logout_params(
+    params: &LogoutParams,
+    session_token: &str,
+    client_secret: &str,
+) -> anyhow::Result<()> {
     use base64::Engine;
 
-    let expected_signature =
-        compute_logout_signature(&params.redirect_uri, params.timestamp, client_secret);
+    let expected_signature = compute_logout_signature(
+        &params.redirect_uri,
+        params.timestamp,
+        session_token,
+        client_secret,
+    );
 
     let provided_signature = base64::engine::general_purpose::URL_SAFE_NO_PAD
         .decode(&params.signature)
@@ -1240,14 +1292,82 @@ mod tests {
             redirect_uri: format!("https://example.com{SQLPAGE_REDIRECT_URI}"),
             logout_uri: format!("https://example.com{SQLPAGE_LOGOUT_URI}"),
         };
-        let generated = config.create_logout_url("/after");
+        let generated = config.create_logout_url("/after", Some("session-token"));
 
         let parsed = Url::parse(&generated).expect("generated URL should be valid");
         assert_eq!(parsed.path(), SQLPAGE_LOGOUT_URI);
 
         let params = parse_logout_params(parsed.query().expect("query string is present"))
             .expect("generated URL should parse");
-        verify_logout_params(&params, secret).expect("generated URL should validate");
+        verify_logout_params(&params, "session-token", secret)
+            .expect("generated URL should validate for the session it was issued for");
+    }
+
+    /// A logout URL is bound to the session it was issued for: presenting it
+    /// from a different browser (a different `sqlpage_auth` cookie), or with
+    /// no session at all, must NOT validate. This is what prevents a forced
+    /// logout CSRF where a logout URL generated for one user logs out another.
+    #[test]
+    fn logout_url_is_bound_to_the_issuing_session() {
+        let secret = "super_secret_key";
+        let config = OidcConfig {
+            issuer_url: IssuerUrl::new("https://example.com".to_string()).unwrap(),
+            client_id: "test_client".to_string(),
+            client_secret: secret.to_string(),
+            protected_paths: vec![],
+            public_paths: vec![],
+            app_host: "example.com".to_string(),
+            scopes: vec![],
+            additional_audience_verifier: AudienceVerifier::new(None),
+            site_prefix: "https://example.com".to_string(),
+            redirect_uri: format!("https://example.com{SQLPAGE_REDIRECT_URI}"),
+            logout_uri: format!("https://example.com{SQLPAGE_LOGOUT_URI}"),
+        };
+
+        // A logout URL issued for victim's session.
+        let victim_session = "victim-auth-token";
+        let generated = config.create_logout_url("/after", Some(victim_session));
+        let parsed = Url::parse(&generated).expect("generated URL should be valid");
+        let params = parse_logout_params(parsed.query().expect("query string is present"))
+            .expect("generated URL should parse");
+
+        // It validates for the session it was issued for.
+        verify_logout_params(&params, victim_session, secret)
+            .expect("must validate for the issuing session");
+
+        // It must NOT validate when presented by a different session...
+        verify_logout_params(&params, "attacker-auth-token", secret)
+            .expect_err("must reject a different session's auth cookie");
+        // ...nor when presented with no session at all (replay/CSRF).
+        verify_logout_params(&params, "", secret)
+            .expect_err("must reject a request with no auth cookie");
+    }
+
+    #[test]
+    fn logout_session_cookie_uses_last_duplicate_like_request_info() {
+        // Two sqlpage_auth cookies (e.g. set with different Path attributes).
+        // actix's ServiceRequest::cookie returns the first, but RequestInfo (and
+        // thus sqlpage.oidc_logout_url, which signs the logout URL) keeps the
+        // last after merging duplicates. Verification must use the same value
+        // the URL was signed with, otherwise a legitimate logout URL is 400ed.
+        let request = TestRequest::default()
+            .insert_header(("cookie", "sqlpage_auth=first; sqlpage_auth=second"))
+            .to_srv_request();
+        assert_eq!(
+            request
+                .cookie(SQLPAGE_AUTH_COOKIE_NAME)
+                .as_ref()
+                .map(Cookie::value),
+            Some("first"),
+            "actix selects the first duplicate cookie"
+        );
+        assert_eq!(
+            logout_session_cookie(&request)
+                .as_ref()
+                .map(Cookie::value),
+            Some("second"),
+            "logout binding must use the last duplicate, matching RequestInfo and the signed URL"
+        );
     }
 
     #[test]

src/webserver/single_or_vec.rs

@@ -83,6 +83,18 @@ impl SingleOrVec {
             SingleOrVec::Vec(v) => Cow::Owned(serde_json::to_string(v).unwrap()),
         }
     }
+
+    /// Returns the first value. This mirrors how `actix_web::HttpRequest::cookie`
+    /// selects the first cookie of a given name, so callers that need to agree
+    /// with that selection (such as OIDC logout session binding) stay consistent
+    /// instead of accidentally using a JSON array of merged duplicate values.
+    #[must_use]
+    pub fn first_str(&self) -> &str {
+        match self {
+            SingleOrVec::Single(x) => x,
+            SingleOrVec::Vec(v) => v.first().map_or("", String::as_str),
+        }
+    }
 }
 
 #[cfg(test)]
@@ -110,6 +122,18 @@ mod single_or_vec_tests {
         );
     }
 
+    #[test]
+    fn first_str_returns_first_value() {
+        assert_eq!(SingleOrVec::Single("a".to_string()).first_str(), "a");
+        // Duplicate values (e.g. two cookies of the same name) yield the first,
+        // matching HttpRequest::cookie, not a JSON array.
+        assert_eq!(
+            SingleOrVec::Vec(vec!["a".to_string(), "b".to_string()]).first_str(),
+            "a"
+        );
+        assert_eq!(SingleOrVec::Vec(vec![]).first_str(), "");
+    }
+
     #[test]
     fn displays_single_value() {
         let single = SingleOrVec::Single("hello".to_string());

tests/oidc/mod.rs

@@ -547,7 +547,7 @@ async fn test_oidc_logout_uses_correct_scheme() {
         .as_ref()
         .unwrap()
         .config
-        .create_logout_url("/logged_out");
+        .create_logout_url("/logged_out", None);
     let app = test::init_service(create_app(Data::new(app_state))).await;
     // make sure the logout path includes the configured domain
     assert!(logout_path.starts_with("/sqlpage/oidc_logout"));
@@ -653,3 +653,105 @@ async fn test_slow_token_endpoint_does_not_freeze_server() {
         .unwrap();
     assert_eq!(resp.status(), StatusCode::SEE_OTHER);
 }
+
+/// A logout URL is bound to the session it was issued for. A logout URL
+/// generated for one session must NOT clear a different browser's auth cookie
+/// (forced-logout CSRF), while the legitimate logout of the issuing session
+/// must keep working.
+#[actix_web::test]
+async fn test_oidc_logout_is_session_bound() {
+    use sqlpage::{
+        AppState,
+        app_config::{AppConfig, test_database_url},
+    };
+
+    crate::common::init_log();
+    let provider = FakeOidcProvider::new().await;
+
+    let db_url = test_database_url();
+    let config_json = format!(
+        r#"{{
+        "database_url": "{db_url}",
+        "oidc_issuer_url": "{}",
+        "oidc_client_id": "{}",
+        "oidc_client_secret": "{}",
+        "oidc_protected_paths": ["/"],
+        "host": "localhost:1"
+    }}"#,
+        provider.issuer_url, provider.client_id, provider.client_secret
+    );
+
+    let config: AppConfig = serde_json::from_str(&config_json).unwrap();
+    let app_state = AppState::init(&config).await.unwrap();
+    let oidc_state = app_state.oidc_state.clone().unwrap();
+    let app = test::init_service(create_app(Data::new(app_state))).await;
+
+    // Complete a full login as the victim.
+    let mut cookies: Vec<Cookie<'static>> = Vec::new();
+    let resp = request_with_cookies!(app, test::TestRequest::get().uri("/"), cookies);
+    assert_eq!(resp.status(), StatusCode::SEE_OTHER);
+    let auth_url = Url::parse(resp.headers().get("location").unwrap().to_str().unwrap()).unwrap();
+    let state = get_query_param(&auth_url, "state");
+    let nonce = get_query_param(&auth_url, "nonce");
+    let redirect_uri = get_query_param(&auth_url, "redirect_uri");
+    provider.store_auth_code("test_auth_code".to_string(), nonce);
+    let callback_uri = format!(
+        "{}?code=test_auth_code&state={}",
+        Url::parse(&redirect_uri).unwrap().path(),
+        state
+    );
+    let callback_resp =
+        request_with_cookies!(app, test::TestRequest::get().uri(&callback_uri), cookies);
+    assert_eq!(callback_resp.status(), StatusCode::SEE_OTHER);
+
+    let victim_auth = cookies
+        .iter()
+        .find(|c| c.name() == "sqlpage_auth")
+        .expect("victim should be authenticated")
+        .value()
+        .to_string();
+    assert!(!victim_auth.is_empty());
+
+    // A logout URL bound to a DIFFERENT session must not clear the victim's
+    // cookie when the victim's browser follows it.
+    let foreign_logout_url = oidc_state
+        .config
+        .create_logout_url("/", Some("some-other-sessions-token"));
+    let mut req = test::TestRequest::get().uri(&foreign_logout_url);
+    for cookie in &cookies {
+        req = req.cookie(cookie.clone());
+    }
+    let resp = test::call_service(&app, req.to_request()).await;
+    assert_eq!(
+        resp.status(),
+        StatusCode::BAD_REQUEST,
+        "a logout URL bound to another session must be rejected"
+    );
+    let cleared = extract_set_cookies(resp.headers())
+        .iter()
+        .any(|c| c.name() == "sqlpage_auth" && c.value().is_empty());
+    assert!(
+        !cleared,
+        "the victim's auth cookie must not be cleared by a foreign logout URL"
+    );
+
+    // The victim's own logout URL (bound to the victim's session) must work.
+    let own_logout_url = oidc_state.config.create_logout_url("/", Some(&victim_auth));
+    let mut req = test::TestRequest::get().uri(&own_logout_url);
+    for cookie in &cookies {
+        req = req.cookie(cookie.clone());
+    }
+    let resp = test::call_service(&app, req.to_request()).await;
+    assert_eq!(
+        resp.status(),
+        StatusCode::SEE_OTHER,
+        "the user's own logout must keep working"
+    );
+    let cleared = extract_set_cookies(resp.headers())
+        .iter()
+        .any(|c| c.name() == "sqlpage_auth" && c.value().is_empty());
+    assert!(
+        cleared,
+        "the user's own logout must clear their auth cookie"
+    );
+}