From 4db4afb811c553bc3d54a01a9985b9e6dfc5a115 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?St=C3=A9phan=20Kochen?= <git@stephank.nl>
Date: Fri, 23 Dec 2022 09:44:40 +0100
Subject: [PATCH] Use sanitized path in dir redirect

---
 src/resolve.rs             |  7 +++++--
 src/response_builder.rs    | 17 +++++++++++++++--
 src/util/requested_path.rs | 12 ++++--------
 tests/static.rs            | 13 +++++++++++++
 4 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/src/resolve.rs b/src/resolve.rs
index 6ccd5b2..c3dbeb2 100644
--- a/src/resolve.rs
+++ b/src/resolve.rs
@@ -74,9 +74,12 @@ pub async fn resolve_path(
     request_path: &str,
 ) -> Result<ResolveResult, IoError> {
     let RequestedPath {
-        mut full_path,
+        sanitized,
         is_dir_request,
-    } = RequestedPath::resolve(root, request_path);
+    } = RequestedPath::resolve(request_path);
+
+    let mut full_path = root.into();
+    full_path.extend(&sanitized);
 
     let (file, metadata) = match open_with_metadata(&full_path).await {
         Ok(pair) => pair,
diff --git a/src/response_builder.rs b/src/response_builder.rs
index a4ba7f2..797adfb 100644
--- a/src/response_builder.rs
+++ b/src/response_builder.rs
@@ -1,5 +1,5 @@
 use crate::resolve::ResolveResult;
-use crate::util::FileResponseBuilder;
+use crate::util::{FileResponseBuilder, RequestedPath};
 use http::response::Builder as HttpResponseBuilder;
 use http::{header, HeaderMap, Method, Request, Response, Result, StatusCode, Uri};
 use hyper::Body;
@@ -84,7 +84,20 @@ impl<'a> ResponseBuilder<'a> {
                 .status(StatusCode::FORBIDDEN)
                 .body(Body::empty()),
             ResolveResult::IsDirectory => {
-                let mut target = self.path.to_owned();
+                // NOTE: We are doing an origin-relative redirect, but need to use the sanitized
+                // path in order to prevent a malicious redirect to `//foo` (schema-relative).
+                // With the current API, we have no other option here than to do sanitization
+                // again, but a future version may reuse the earlier sanitization result.
+                let resolved = RequestedPath::resolve(self.path);
+                let path = resolved.sanitized.to_string_lossy();
+
+                let mut target_len = path.len() + 2;
+                if let Some(ref query) = self.query {
+                    target_len += query.len() + 1;
+                }
+                let mut target = String::with_capacity(target_len);
+                target.push('/');
+                target.push_str(&path);
                 target.push('/');
                 if let Some(query) = self.query {
                     target.push('?');
diff --git a/src/util/requested_path.rs b/src/util/requested_path.rs
index e5163f7..8343f9d 100644
--- a/src/util/requested_path.rs
+++ b/src/util/requested_path.rs
@@ -31,23 +31,19 @@ fn normalize_path(path: &Path) -> PathBuf {
 
 /// Resolved request path.
 pub struct RequestedPath {
-    /// Fully resolved filesystem path of the request.
-    pub full_path: PathBuf,
+    /// Sanitized path of the request.
+    pub sanitized: PathBuf,
     /// Whether a directory was requested. (`original` ends with a slash.)
     pub is_dir_request: bool,
 }
 
 impl RequestedPath {
     /// Resolve the requested path to a full filesystem path, limited to the root.
-    pub fn resolve(root_path: impl Into<PathBuf>, request_path: &str) -> Self {
+    pub fn resolve(request_path: &str) -> Self {
         let is_dir_request = request_path.as_bytes().last() == Some(&b'/');
         let request_path = PathBuf::from(decode_percents(request_path));
-
-        let mut full_path = root_path.into();
-        full_path.extend(&normalize_path(&request_path));
-
         RequestedPath {
-            full_path,
+            sanitized: normalize_path(&request_path),
             is_dir_request,
         }
     }
diff --git a/tests/static.rs b/tests/static.rs
index 94a52bc..4eaa43c 100644
--- a/tests/static.rs
+++ b/tests/static.rs
@@ -112,6 +112,19 @@ async fn redirects_if_trailing_slash_is_missing() {
     assert_eq!(url, "/dir/");
 }
 
+#[tokio::test]
+async fn redirects_to_sanitized_path() {
+    let harness = Harness::new(vec![("dir/index.html", "this is index")]);
+
+    // Previous versions would base the redirect on the request path, but that is user input, and
+    // the user could construct a schema-relative redirect this way.
+    let res = harness.get("//dir").await.unwrap();
+    assert_eq!(res.status(), StatusCode::MOVED_PERMANENTLY);
+
+    let url = res.headers().get(header::LOCATION).unwrap();
+    assert_eq!(url, "/dir/");
+}
+
 #[tokio::test]
 async fn decodes_percent_notation() {
     let harness = Harness::new(vec![("has space.html", "file with funky chars")]);