fix(oidc): reject backslash authority forms in relative redirects

The relative-redirect validation accepted /\evil.test because it only
rejected values starting with //. URL parsers normalize \ to /, turning
such a value into an external http://evil.test/ target (open redirect).

Add a shared is_safe_relative_redirect helper that also rejects any
backslash, and use it in the three validation sites: logout
verify_logout_params, sqlpage.oidc_logout_url, and validate_redirect_url.

Author: lovasoa

CHANGELOG.md

@@ -1,5 +1,9 @@
 # CHANGELOG.md
 
+## Unreleased
+
+- **Security (OIDC open redirect): logout and login redirect targets could point at an external site.** SQLPage accepted a relative redirect target if it started with `/` but not `//`. A value like `/\evil.test` passed that check, but the [WHATWG URL Standard](https://url.spec.whatwg.org/#url-parsing) treats `\` as `/` for http/https URLs, so SQLPage's own URL parser (the `url` crate) turns `/\evil.test` into `http://evil.test/` when it builds the absolute `post_logout_redirect_uri`. That makes it a server-side open redirect, independent of the client. The same WHATWG rule is implemented by current browsers (Chromium, Firefox, Safari), so a `Location: /\evil.test` is also followed to the external host. (Parsers that follow RFC 3986 instead, as many proxies do, leave the backslash alone, but SQLPage cannot rely on that.) You are affected only if you use OIDC and build a redirect target from user-controlled input, for example a `redirect_uri` passed to `sqlpage.oidc_logout_url`. SQLPage now rejects any redirect target containing a backslash, on top of the existing `//` check. Normal paths such as `/foo/bar?x=1` keep working; just upgrade.
+
 ## v0.44.0
 
 This release focuses on making production SQLPage apps easier to understand, debug, and operate. Most apps should keep working without SQL changes, but maintainers should review the notes about logging and uploaded-file permissions.

src/webserver/database/sqlpage_functions/functions.rs

@@ -1050,9 +1050,9 @@ async fn oidc_logout_url<'a>(
 
     let redirect_uri = redirect_uri.as_deref().unwrap_or("/");
 
-    if !redirect_uri.starts_with('/') || redirect_uri.starts_with("//") {
+    if !crate::webserver::oidc::is_safe_relative_redirect(redirect_uri) {
         anyhow::bail!(
-            "oidc_logout_url: redirect_uri must be a relative path starting with '/'. Got: {redirect_uri}"
+            "oidc_logout_url: redirect_uri must be a relative path starting with a single '/'. Got: {redirect_uri}"
         );
     }
 

src/webserver/oidc.rs

@@ -659,7 +659,7 @@ fn verify_logout_params(params: &LogoutParams, client_secret: &str) -> anyhow::R
         anyhow::bail!("Logout token timestamp is in the future");
     }
 
-    if !params.redirect_uri.starts_with('/') || params.redirect_uri.starts_with("//") {
+    if !is_safe_relative_redirect(&params.redirect_uri) {
         anyhow::bail!("Invalid redirect URI");
     }
 
@@ -1180,9 +1180,36 @@ impl AudienceVerifier {
     }
 }
 
+/// Returns true if the given value is a safe relative redirect target.
+///
+/// Only paths starting with a single `/` (not `//`) are accepted, and any value
+/// containing a backslash is rejected. The WHATWG URL Standard treats `\` as
+/// equivalent to `/` for special schemes (http/https), so `/\evil.test` parses
+/// to the authority `evil.test`, i.e. `http://evil.test/`. SQLPage itself uses a
+/// WHATWG parser (the `url` crate) when it builds the absolute
+/// `post_logout_redirect_uri`, so without this check a value classified as
+/// "relative" becomes an external open-redirect target on the server side,
+/// independent of the client. Browsers implementing the same standard (Chromium,
+/// Firefox, Safari) resolve a `Location: /\evil.test` the same way.
+pub(crate) fn is_safe_relative_redirect(uri: &str) -> bool {
+    // Reject any embedded backslash: the WHATWG URL parser used by SQLPage's
+    // `url` crate (and by browsers) treats `\` as `/`, so it can smuggle in an
+    // authority component.
+    if uri.contains('\\') {
+        return false;
+    }
+    let mut chars = uri.chars();
+    // Must start with `/`...
+    if chars.next() != Some('/') {
+        return false;
+    }
+    // ...but the second character must not be `/` (protocol-relative authority).
+    !matches!(chars.next(), Some('/'))
+}
+
 /// Validate that a redirect URL is safe to use (prevents open redirect attacks)
 fn validate_redirect_url(url: String, redirect_uri: &str) -> String {
-    if url.starts_with('/') && !url.starts_with("//") && !url.starts_with(redirect_uri) {
+    if is_safe_relative_redirect(&url) && !url.starts_with(redirect_uri) {
         return url;
     }
     log::warn!("Refusing to redirect to {url}");
@@ -1196,6 +1223,36 @@ mod tests {
     use actix_web::{cookie::Cookie, test::TestRequest};
     use openidconnect::url::Url;
 
+    #[test]
+    fn relative_redirect_rejects_authority_and_backslash_forms() {
+        // Legitimate relative paths must be accepted.
+        for ok in ["/", "/foo", "/foo/bar?x=1", "/a/b/c#frag"] {
+            assert!(
+                is_safe_relative_redirect(ok),
+                "expected {ok:?} to be accepted"
+            );
+        }
+        // Authority and backslash forms must all be rejected: SQLPage's `url`
+        // crate (a WHATWG URL parser) normalizes these into an external
+        // `http://evil.test/` target.
+        for bad in [
+            "//evil.test",
+            "/\\evil.test",
+            "/\\/evil.test",
+            "\\evil.test",
+            "\\\\evil.test",
+            "/foo\\bar",
+            "https://evil.test",
+            "evil.test",
+            "",
+        ] {
+            assert!(
+                !is_safe_relative_redirect(bad),
+                "expected {bad:?} to be rejected"
+            );
+        }
+    }
+
     #[test]
     fn login_redirects_use_see_other() {
         let response = build_redirect_response("/foo".to_string());