Normalize the request path #90
Conversation
| @@ -13,15 +14,27 @@ fn decode_percents(string: &&str) -> String { | |||
| percent_decode(string.as_bytes()).decode_utf8().unwrap().into_owned() | |||
| } | |||
|
|
|||
| fn normalize_path(path: &Path) -> PathBuf { | |||
There was a problem hiding this comment.
I have tried to find an API to normalize path, but it looks Rust does not have such an API.
ref. https://github.com/rust-lang/rfcs/blob/master/text/0474-path-reform.md
| impl RequestedPath { | ||
| pub fn new<P: AsRef<Path>>(root_path: P, request: &Request) -> RequestedPath { | ||
| let decoded_req_path = PathBuf::from_iter(request.url.path().iter().map(decode_percents)); |
There was a problem hiding this comment.
You can just join the request path with the root here, and check if the resulting path is a subpath of the root.
There was a problem hiding this comment.
Oh and a call to Path.canonicalize.
There was a problem hiding this comment.
Thank you for the review.
Doesn't Path.canonicalize() touch an underlying filesystem? It seems to call stat()-ish system call. If we don't have a permission to a intermediate directory, I think it would fail. Is that okay?
This PR's approach is inspired by Python's simple builtin httpserver, available via python3 -m http.server
See https://github.com/python/cpython/blob/c30098c8c6014f3340a369a31df9c74bdbacc269/Lib/http/server.py#L756
There was a problem hiding this comment.
Something like this: https://play.rust-lang.org/?gist=ce54238276ab1a9058a0e4fffcde560e&version=stable&backtrace=0
This also fails if the file doesn't exist, which is fine in our case.
There was a problem hiding this comment.
If we don't have permission for a file it is nonexistent for our purposes IMO.
(BTW the behavior for "Permission Denied" is wrong IMO, bad file permissions rather implies a 500 Internal Server Error. But 404 is fine too I think)
There was a problem hiding this comment.
Ah, I meant:
Suppose we have the following file system:
- /web-server-root
- /web-server-root/index.html
- /web-server-root/directory-i-do-not-have-permission-to-access/
... and then serve static files from "/web-server-root".
In this case, the request, directory-i-do-not-have-permission-to-access/xxx/../../index.html (please replace / with %2f) should be successful, I thought. It looks Python3's builtin web server allows it, at least.
I do not have a strong opinion either. :)
There was a problem hiding this comment.
According to servo/rust-url#229 rust-url should normalize such path segments. Haven't tried it though.
There was a problem hiding this comment.
Thank you. The problem here is that %2f is ignored in rust-url's path segments, I think. As far as I confirmed, a segment, like a ..%2f..%2f, can be passed to a staticfile handler, as-is, without being decoded nor being normalized.
That's the reason we have to normalize path again here after percent-decoding, I am afraid.
|
Thanks! |
Fixes #89.