fix(oidc): apply protected/public path rules to the percent-decoded path (#1310)

An unauthenticated request could encode a byte of a protected prefix (e.g.
/%70rotected/, which the router decodes to /protected/ and serves) to bypass
oidc_protected_paths. is_public_path now percent-decodes both the request path
and the configured prefixes before the plain string-prefix comparison, so
encoded and plain URLs are classified identically, including when site_prefix
contains percent-encoded characters such as a space. Plain string matching
preserves the documented /public vs /public/ distinction.

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 fix: OIDC protected paths could be bypassed with percent-encoded URLs.** When `oidc_protected_paths` used a non-default prefix such as `["/protected"]`, an unauthenticated visitor could percent-encode a byte of the prefix (for example requesting `/%70rotected/`, where `%70` is `p`) to make the OIDC middleware treat the page as public and serve it without logging in, even though SQLPage then decoded the URL and ran the protected SQL file. You are affected if you rely on `oidc_protected_paths` or `oidc_public_paths` with prefixes other than the default `/`. SQLPage now percent-decodes the request path and the configured prefixes before applying the rules, so encoded and plain URLs are classified identically (including when `site_prefix` contains characters such as a space). No configuration change is required; just upgrade.
 
 ## v0.44.0
 

src/webserver/oidc.rs

@@ -140,8 +140,28 @@ impl TryFrom<&AppConfig> for OidcConfig {
 impl OidcConfig {
     #[must_use]
     pub fn is_public_path(&self, path: &str) -> bool {
-        !self.protected_paths.iter().any(|p| path.starts_with(p))
-            || self.public_paths.iter().any(|p| path.starts_with(p))
+        // Percent-decode both the request path and the configured prefixes
+        // before comparing. Otherwise an unauthenticated request could encode a
+        // byte of a protected prefix (e.g. `/%70rotected/`, which the router
+        // decodes to `/protected/` and serves) to dodge the rule. The prefixes
+        // are decoded too because a `site_prefix` such as `/my app/` is stored
+        // percent-encoded (`/my%20app/...`); decoding only one side would never
+        // match and would wrongly make protected pages public. Matching stays a
+        // plain string prefix, preserving the documented `/public` vs `/public/`
+        // distinction (those resolve to different files).
+        fn decode(s: &str) -> std::borrow::Cow<'_, str> {
+            percent_encoding::percent_decode_str(s).decode_utf8_lossy()
+        }
+        let decoded = decode(path);
+        let path = decoded.as_ref();
+        !self
+            .protected_paths
+            .iter()
+            .any(|p| path.starts_with(decode(p).as_ref()))
+            || self
+                .public_paths
+                .iter()
+                .any(|p| path.starts_with(decode(p).as_ref()))
     }
 
     /// Creates a custom ID token verifier that supports multiple issuers
@@ -1250,6 +1270,83 @@ mod tests {
         verify_logout_params(&params, secret).expect("generated URL should validate");
     }
 
+    fn test_oidc_config_with_paths(
+        protected_paths: Vec<String>,
+        public_paths: Vec<String>,
+    ) -> OidcConfig {
+        OidcConfig {
+            issuer_url: IssuerUrl::new("https://example.com".to_string()).unwrap(),
+            client_id: "test_client".to_string(),
+            client_secret: "secret".to_string(),
+            protected_paths,
+            public_paths,
+            app_host: "example.com".to_string(),
+            scopes: vec![],
+            additional_audience_verifier: AudienceVerifier::new(None),
+            site_prefix: "/".to_string(),
+            redirect_uri: SQLPAGE_REDIRECT_URI.to_string(),
+            logout_uri: SQLPAGE_LOGOUT_URI.to_string(),
+        }
+    }
+
+    #[test]
+    fn percent_encoded_protected_path_is_not_public() {
+        let config = test_oidc_config_with_paths(vec!["/protected".to_string()], vec![]);
+        assert!(
+            !config.is_public_path("/protected/"),
+            "/protected/ must be protected"
+        );
+        // Encoding a byte of the prefix (%70 == 'p') must not bypass protection:
+        // the router percent-decodes the path and serves the protected file.
+        assert!(
+            !config.is_public_path("/%70rotected/"),
+            "/%70rotected/ decodes to /protected/ and must stay protected"
+        );
+    }
+
+    #[test]
+    fn percent_encoded_public_path_stays_public() {
+        let config = test_oidc_config_with_paths(
+            vec!["/protected".to_string()],
+            vec!["/protected/public".to_string()],
+        );
+        assert!(
+            config.is_public_path("/protected/%70ublic/page.sql"),
+            "/protected/%70ublic/... decodes to a public path"
+        );
+    }
+
+    #[test]
+    fn encoded_site_prefix_keeps_protected_paths_protected() {
+        // With a site_prefix like `/my app/`, the configured prefixes are stored
+        // encoded (`/my%20app/...`). Decoding only the request path would never
+        // match, so protected pages must not become public.
+        let config = test_oidc_config_with_paths(vec!["/my%20app/protected".to_string()], vec![]);
+        assert!(!config.is_public_path("/my%20app/protected/page.sql"));
+        assert!(!config.is_public_path("/my%20app/%70rotected/"));
+        assert!(
+            config.is_public_path("/my%20app/login.sql"),
+            "a non-protected page under the site prefix stays public"
+        );
+    }
+
+    #[test]
+    fn trailing_slash_public_prefix_does_not_expose_sibling_file() {
+        // A public exception for the `/public/` directory must not also make the
+        // extensionless `/public` (which the router resolves to public.sql)
+        // public. Plain string-prefix matching keeps them distinct.
+        let config =
+            test_oidc_config_with_paths(vec!["/".to_string()], vec!["/public/".to_string()]);
+        assert!(
+            config.is_public_path("/public/page.sql"),
+            "files under /public/ are public"
+        );
+        assert!(
+            !config.is_public_path("/public"),
+            "/public (the file public.sql) stays protected"
+        );
+    }
+
     #[test]
     fn evicts_excess_tmp_login_flow_state_cookies() {
         let request = (0..MAX_OIDC_PARALLEL_LOGIN_FLOWS)