Bind OIDC logout URLs to the requesting session

OIDC logout URLs signed only the redirect target and a timestamp, so any
visitor who opened a still-valid logout link had their sqlpage_auth cookies
cleared. A logout link issued for one user could log out another, or be
replayed (forced-logout CSRF). It did not allow account takeover.

The logout signature now also covers the current sqlpage_auth cookie value,
and the logout handler verifies the signature against the auth cookie
presented with the request. A logout link is therefore only honored for the
browser it was generated for. The normal authenticated logout flow is
unchanged.

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,16 @@ 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.
+    let session_token = request
+        .cookies
+        .get("sqlpage_auth")
+        .map(SingleOrVec::as_json_str);
+
+    let logout_url = oidc_state
+        .config
+        .create_logout_url(redirect_uri, session_token.as_deref());
 
     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())
@@ -570,9 +581,16 @@ fn process_oidc_logout(
 ) -> anyhow::Result<HttpResponse> {
     let params = parse_logout_params(request.query_string())?;
 
-    verify_logout_params(&params, &oidc_state.config.client_secret)?;
-
     let id_token_cookie = request.cookie(SQLPAGE_AUTH_COOKIE_NAME);
+    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 = id_token_cookie
         .as_ref()
         .map(|c| OidcToken::from_str(c.value()))
@@ -620,7 +638,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 +652,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 +1276,55 @@ 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]

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"
+    );
+}