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

entry: relax symlink target constraints #90

Merged
merged 1 commit into from
Jan 9, 2017

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Dec 29, 2016

This allows the creation and extraction of symlinks pointing to targets
outside of the destination directory, including absolute paths and ../
chains.

Path traversal is now checked at unpack time, by canonicalizing the target
path and ensuring that it is nested below destination path.

Fixes #87

@lucab lucab force-pushed the to-upstream/link-creation branch 14 times, most recently from 5fcb8f5 to f2e48a1 Compare December 29, 2016 14:25
return Err(other("symlink destination is empty"))
}

println!("{:?} {:?}", actual_src, dst);
println!("{:?} {:?}", src, dst);
Copy link
Owner

Choose a reason for hiding this comment

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

oh oops this is actually a stray println and should get removed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I re-introduced it later in order to get some clues about the appveyor issue.

@@ -130,5 +180,8 @@ fn good_parent_paths_ok() {

let td = t!(TempDir::new("tar"));
t!(ar.unpack(td.path()));
t!(File::open(td.path().join("foo/bar")));
t!(td.path().join("foo").join("bar").read_link());
let dst = t!(td.path().join("foo").join("bar").canonicalize());
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this line is failing on AppVeyor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I'm puzzled. I expanded the previous test into path-joining, readlink(), and canonicalize() in order to understand what's going on, but the original one was also failing. It looks like windows doesn't like the ../bar symlink. This crate is converting \ to / in symlinks, so ..\bar becomes ../bar. This may be part of the issue, but it seems to be on purpose (and actually covered by unit testing). I'm not familiar with windows api, how do relative symlinks and separators work there?

Copy link
Owner

Choose a reason for hiding this comment

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

Hm to be honest I'm surprised that symlinks work at all on Windows, they're supposed to be gated by default...

Want to just #[cfg(unix)] the symlink tests?

@alexcrichton
Copy link
Owner

Looks great to me, thanks! I'm fine merging once AppVeyor is green

This allows the creation and extraction of symlinks pointing to targets
outside of the destination directory, including absolute paths and `../`
chains.

Path traversal is now checked at unpack time, by canonicalizing the target
path and ensuring that it is nested below destination path.
@lucab
Copy link
Contributor Author

lucab commented Jan 7, 2017

As suggested, moved the last test to unix-only. PTAL.

@alexcrichton alexcrichton merged commit 28b18c0 into alexcrichton:master Jan 9, 2017
@alexcrichton
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants