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

Do not double-prefix symlink paths with base directory when they already contain volume names #2051

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

selzoc
Copy link
Contributor

@selzoc selzoc commented Aug 22, 2023

We observed that on Windows, symlinks from the C:\ base directory such as C:\Users were being resolved as C:\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 / in filepath.Clean) but C:\C:\ does not work, and is not converted to C:\ in filepath.Clean (go playground example)

We also noticed that if there is a symlink from the C:\ drive to another drive such as D:\, this code correctly resolves the path, and does not have a path that starts with C:\D:\

Fixes #1950 (we think)

@selzoc selzoc marked this pull request as draft August 22, 2023 23:35
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>
@selzoc selzoc changed the title Do not double-prefix symlink paths with base directory Do not double-prefix symlink paths with base directory when they already contain volume names Aug 23, 2023
@selzoc selzoc marked this pull request as ready for review August 23, 2023 00:07
wagoodman
wagoodman previously approved these changes Aug 23, 2023
Copy link
Contributor

@wagoodman wagoodman left a 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).

@kzantow
Copy link
Contributor

kzantow commented Aug 23, 2023

@wagoodman I'm going to give this a look through on my Windows machine

@kzantow kzantow dismissed wagoodman’s stale review August 23, 2023 21:12

I'm going to debug through this on Windows before merging

@selzoc
Copy link
Contributor Author

selzoc commented Aug 23, 2023

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.

@wagoodman
Copy link
Contributor

Thanks @selzoc @kzantow 🙏

Copy link
Contributor

@kzantow kzantow left a 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 👍

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.

Errors when handling symlinks on Windows with syft v0.85.0
3 participants