Skip to content

Commit

Permalink
Use sanitized path in dir redirect
Browse files Browse the repository at this point in the history
  • Loading branch information
stephank committed Dec 23, 2022
1 parent 2ff8d89 commit f12cadc
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 8 deletions.
18 changes: 16 additions & 2 deletions src/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ pub enum ResolveResult<F = File> {
/// The requested file could not be accessed.
PermissionDenied,
/// A directory was requested as a file.
IsDirectory,
IsDirectory {
/// Path to redirect to.
redirect_to: String,
},
/// The requested file was found.
Found(ResolvedFile<F>),
}
Expand Down Expand Up @@ -169,7 +172,18 @@ impl<O: FileOpener> Resolver<O> {

// We may have opened a directory for a file request, in which case we redirect.
if !is_dir_request && file.is_dir {
return Ok(ResolveResult::IsDirectory);
// Build the redirect path. On Windows, we can't just append the entire path, because
// it contains Windows path separators. Instead, append each component separately.
let mut target = String::with_capacity(path.as_os_str().len() + 2);
target.push('/');
for component in path.components() {
target.push_str(&component.as_os_str().to_string_lossy());
target.push('/');
}

return Ok(ResolveResult::IsDirectory {
redirect_to: target,
});
}

// If not a directory, serve this file.
Expand Down
7 changes: 4 additions & 3 deletions src/response_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ impl<'a> ResponseBuilder<'a> {
ResolveResult::PermissionDenied => HttpResponseBuilder::new()
.status(StatusCode::FORBIDDEN)
.body(Body::Empty),
ResolveResult::IsDirectory => {
let mut target = self.path.to_owned();
target.push('/');
ResolveResult::IsDirectory {
redirect_to: mut target,
} => {
// Preserve any query string from the original request.
if let Some(query) = self.query {
target.push('?');
target.push_str(query);
Expand Down
25 changes: 22 additions & 3 deletions tests/static.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,32 @@ async fn returns_404_if_file_not_found() {

#[tokio::test]
async fn redirects_if_trailing_slash_is_missing() {
let harness = Harness::new(vec![("dir/index.html", "this is index")]);
let harness = Harness::new(vec![("foo/bar/index.html", "this is index")]);

let res = harness.get("/dir").await.unwrap();
let res = harness.get("/foo/bar").await.unwrap();
assert_eq!(res.status(), StatusCode::MOVED_PERMANENTLY);

let url = res.headers().get(header::LOCATION).unwrap();
assert_eq!(url, "/dir/");
assert_eq!(url, "/foo/bar/");
}

#[tokio::test]
async fn redirects_to_sanitized_path() {
let harness = Harness::new(vec![("foo.org/bar/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("//foo.org/bar").await.unwrap();
assert_eq!(res.status(), StatusCode::MOVED_PERMANENTLY);

let url = res.headers().get(header::LOCATION).unwrap();
// TODO: The request path is apparently parsed differently on Windows, but at least the
// resulting redirect is still safe, and that's the important part.
if cfg!(target_os = "windows") {
assert_eq!(url, "/");
} else {
assert_eq!(url, "/foo.org/bar/");
}
}

#[tokio::test]
Expand Down

0 comments on commit f12cadc

Please sign in to comment.