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

[bugfix] Windows paths don’t work with Server #445

Merged
merged 2 commits into from
Jun 30, 2021

Conversation

puellanivis
Copy link
Collaborator

Change uses of direct paths in Server to use toLocalPath to deconvert encoded absolute paths.

For Windows:

  • We convert to slashes, then if it does not start with a slash, we add one at the start. So, when decoding paths, we have to check if after FromSlash removing the leading backslashes produces an absolute path. If it does, then we return that instead.
  • We switch to using filepath.Clean first before filepath.ToSlash so that clients work consistently. ( \\UNC\Path paths work on clients that do not FXP_REALPATH sanitize first, while for those that do, path.Clean would otherwise strip out the double slash start.)

For Plan9:

  • Some absolute paths (those that start with #) get prefixed with a slash in cleanPath, using filepath.Clean first should avoid accidentally dropping the #, and then we make sure a leading / will get removed if added from the “add a starting slash” encoding .
  • Plan9 wasn’t compiling because s_ISVTX was typoed as S_ISVTX

All other OSes:

  • toLocalPath just returns the path. There’s nothing further to be done for it.

@@ -288,9 +288,9 @@ func cleanPath(p string) string {
}

func cleanPathWithBase(base, p string) string {
p = filepath.ToSlash(p)
p = filepath.ToSlash(filepath.Clean(p))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The order here really doesn’t matter except for Windows, and only for UNC paths. “Let’s just not support UNC paths“ doesn’t work, because OpenSSH’s client does not sanitize the path first, so UNC paths would still work with that client anyways. 😭

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

linux-machine $ sftp -P 31337 windows-machine
sftp> ls //wsl$/openSUSE-Leap-15.3
//wsl$/openSUSE-Leap-15.3/bin           //wsl$/openSUSE-Leap-15.3/dev           //wsl$/openSUSE-Leap-15.3/etc           
//wsl$/openSUSE-Leap-15.3/home          //wsl$/openSUSE-Leap-15.3/init          //wsl$/openSUSE-Leap-15.3/lib           
//wsl$/openSUSE-Leap-15.3/lib64         //wsl$/openSUSE-Leap-15.3/lost+found    //wsl$/openSUSE-Leap-15.3/mnt           
//wsl$/openSUSE-Leap-15.3/opt           //wsl$/openSUSE-Leap-15.3/proc          //wsl$/openSUSE-Leap-15.3/root          
//wsl$/openSUSE-Leap-15.3/run           //wsl$/openSUSE-Leap-15.3/sbin          //wsl$/openSUSE-Leap-15.3/selinux       
//wsl$/openSUSE-Leap-15.3/srv           //wsl$/openSUSE-Leap-15.3/sys           //wsl$/openSUSE-Leap-15.3/tmp           
//wsl$/openSUSE-Leap-15.3/usr           //wsl$/openSUSE-Leap-15.3/var

🤷‍♀️ alright.

if !path.IsAbs(p) {
return path.Join(base, p)
}
return path.Clean(p)
return p
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some basic tests and all worked, anyway since path.Join calls Clean internally, I'll leave path.Clean(p) here too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm… the point here in removing this path.Clean() is that PuTTY sftp sanitizes names with FXP_REALPATH and this removes the double slash at the start of a //UNC/Path this means that while on my Linux machine, I could do ls //wsl$/openSUSE-Leap-15.3 attempting to do the same on my PuTTY sftp client, resulted in the path being cleaned in the FXP_REALPATH, and then failing with cannot open: /wsl$/openSUSE-Leap-15.3

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we could have the same issue if the path is not absolute and we hit return path.Join(base, p)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, but this should only apply in the case where the base being added is a UNC address. So, for example cleanPathWithBase("//wsl$/openSUSE-Leap-15.3", aPath)

I think in this case, we’re kind of a bit stuck on “having a non-default base paths is not particularly robust and a little bit of a hack”. 🤔 For this particular case, I’m a bit more willing to let the inconsistent behavior slide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Allowing non cleaned paths could break some usages, let's merge this patch and see if someone complaints. I'll try to do some tests myself using wsl (I never tested it) as soon as I have some free time, it could take a while

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, this is why I think keeping the path.Join which will auto-clean as well is the better choice for now, even though it would break UNC base paths. Getting some sort of logic to correctly handle all the nuances of properly rooting all reasonable base/path combos would be 😩 .

It’s at least pretty nice that we can do UNC paths with a “local” datastore with WSL, if I had to setup a remote SMB server to test any of this, I might very well tear my hair out. 😆

@puellanivis puellanivis merged commit 35cb1f0 into master Jun 30, 2021
@puellanivis puellanivis deleted the bugfix/windows-server-paths branch June 30, 2021 17:18
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