Reintroduce Path Sanitization #198
Conversation
I've documented the drawbacks of this strategy to make sure users are aware of the tradeoff being made.
Plecra
force-pushed
the
path-sanitization
branch
from
7a21da8
to
d0e905a
Compare
When cfg(unix), the `outpatj` meeded to last until the `set_permissions` call, but it can't exist during the `io::copy`
|
Thanks, this is great! |
src/read.rs
Outdated
| @@ -310,6 +311,49 @@ impl<R: Read + io::Seek> ZipArchive<R> { | |||
| comment: footer.zip_file_comment, | |||
| }) | |||
| } | |||
| /// Extract a Zip archive into a directory. | |||
| /// | |||
| /// Malformed and malicious paths are rejected so that they cannot escape | |||
There was a problem hiding this comment.
This should probably say "Malformed or potentially malicious paths".
Perhaps we should make reference to name_as_child docs to enumerate the types of paths that will be rejected.
src/read.rs
Outdated
|
|
||
| let outpath = directory.as_ref().join(filepath); | ||
|
|
||
| if (file.name()).ends_with('/') { |
There was a problem hiding this comment.
Why the () around file.name()?
src/read.rs
Outdated
| /// Malformed and malicious paths are rejected so that they cannot escape | ||
| /// the given directory. | ||
| /// | ||
| /// This bails on the first error and does not attempt cleanup. |
There was a problem hiding this comment.
Perhaps we should also mention that this will override existing files if they have the same path as a file in the zip archive.
src/read.rs
Outdated
| /// This will read well-formed ZIP files correctly, and is resistant | ||
| /// to path-based exploits. It is recommended over | ||
| /// [`ZipFile::mangled_name`]. | ||
| pub fn name_as_child(&self) -> Option<&Path> { |
There was a problem hiding this comment.
Not sure how I feel about the name of the method. Perhaps relative_name or checked_relative_name. Nothing stands out as clearly better.
|
Nice points @rylev. I've renamed the API to |
Closes #181
I've kept the old behaviour available through
mangled_name, for users who actively decided on it. I took the opportunity to give a stronger warning in the documentation, and provide an alternative which I think is more suitable.This also means we can bring back the
ZipArchive::extractAPI, which I now see as effectively complete for 0.5.Are these API changes enough @jamesmcm ?
(And since you've shown interest in the old API, I'd also like to know which strategy you prefer @aspen 馃榾 )