Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(static_file_handler): Support for blank spaces and percent encodi… #457

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ serde_json = "1.0"
time = "0.1"
typemap = "0.3"
url = "1.0"
percent-encoding = "1.0.0"

[dev-dependencies]
serde_derive = "1.0"
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ extern crate url;
extern crate mustache;
extern crate groupable;
extern crate modifier;
extern crate percent_encoding;

#[macro_use] extern crate log;
#[macro_use] extern crate lazy_static;
Expand Down
31 changes: 27 additions & 4 deletions src/static_files_handler.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::path::{Path, PathBuf};
use std::io::ErrorKind::NotFound;
use std::fs;
use std::borrow::Cow;
use std::str::Utf8Error;

use hyper::method::Method::{Get, Head};

Expand Down Expand Up @@ -45,13 +47,15 @@ impl StaticFilesHandler {
}
}

fn extract_path<'a, D>(&self, req: &'a mut Request<D>) -> Option<&'a str> {
fn extract_path<'a, D>(&self, req: &'a mut Request<D>) -> Option<String> {
req.path_without_query().map(|path| {
debug!("{:?} {:?}{:?}", req.origin.method, self.root_path.display(), path);
let percent_decoded_path = percent_decode_path(path).unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about losing track of this. I am a little concerned about the unwrap here. If that gets triggered, the current thread will be killed. That may be an opening for a DOS, where a series of ill formed URLs exhausts all the threads. We should probably change how we are handling threads to avoid that, but I want to defer that until after I get the async migration completed.

This should probably be changed to return None if the path cannot be decoded.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, will do. I might take some time to do the change since im busy with some personal stuff, but i'll make sure to do the change as soon as i have some free time. Thanks!


debug!("{:?} {:?}{:?}", req.origin.method, self.root_path.display(), percent_decoded_path);

match path {
"/" => "index.html",
path => &path[1..],
"/" => String::from("index.html"),
_path => percent_decoded_path[1..].to_string(),
}
})
}
Expand Down Expand Up @@ -92,6 +96,10 @@ fn safe_path<P: AsRef<Path>>(path: P) -> bool {
})
}

fn percent_decode_path(path: &str) -> Result<Cow<str>, Utf8Error> {
percent_encoding::percent_decode(path.as_bytes()).decode_utf8()
}

#[test]
fn bad_paths() {
let bad_paths = &[
Expand Down Expand Up @@ -121,3 +129,18 @@ fn valid_paths() {
assert!(safe_path(path), "expected {:?} to not be suspicious", path);
}
}

#[test]
fn percent_decoded_paths() {
let paths_to_match = &[
("file-without-percent", "file-without-percent"),
("file%20with%20percent", "file with percent"),
("folder%20with%20percent/file", "folder with percent/file"),
("folder%20with%20percent/file%20with%20percent", "folder with percent/file with percent"),
("folder-without-percent/file%20with%20percent", "folder-without-percent/file with percent"),
];

for &path in paths_to_match {
assert_eq!(percent_decode_path(path.0).unwrap(), path.1, "expected {:?} to be decoded to {:?}", path.0, path.1);
}
}