Use sanitized path in dir redirect

Author: stephank

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,

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('?');

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,
         }
     }

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")]);