-
Notifications
You must be signed in to change notification settings - Fork 587
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
Do not double-prefix symlink paths with base directory when they already contain volume names #2051
Conversation
Fixes anchore#1950 Co-authored-by: Joseph Palermo <jpalermo@vmware.com> Signed-off-by: Joseph Palermo <jpalermo@vmware.com> Signed-off-by: Chris Selzo <cselzo@vmware.com> Co-authored-by: Chris Selzo <cselzo@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix -- I don't see a good way to get in a test for this given that the expectations would be different for various platforms (and that can't be specified in testing since it's behind filepath package).
@wagoodman I'm going to give this a look through on my Windows machine |
I'm going to debug through this on Windows before merging
Great! Yes, I couldn't see a good place to add a test for this. I did verify that it works on a running Windows Server 2019 VM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did debug through this and verified absolute links work on windows with this change, albeit not bounded to a base directory. Thanks a lot @selzoc 👍
…ames (anchore#2051) Signed-off-by: Joseph Palermo <jpalermo@vmware.com> Signed-off-by: Chris Selzo <cselzo@vmware.com> Co-authored-by: Joseph Palermo <jpalermo@vmware.com>
We observed that on Windows, symlinks from the
C:\
base directory such asC:\Users
were being resolved asC:\C:\Users
. In this PR, we do not join the base directory to the resolved symlink if it is already there.On Linux, having a path such as
//usr/local/bin
does not cause any issues (and the//
is converted to/
infilepath.Clean
) butC:\C:\
does not work, and is not converted toC:\
infilepath.Clean
(go playground example)We also noticed that if there is a symlink from the
C:\
drive to another drive such asD:\
, this code correctly resolves the path, and does not have a path that starts withC:\D:\
Fixes #1950 (we think)