lib/versioner: support tilde shortcut in external versioner command

[xduugu]

Purpose

The tilde character is commonly used as a reference to the user's home directory and already supported in other parts of syncthing. This change allows to configure portable external versioner commands that call scripts located in the user's home directory.

Testing

I built the syncthing binary, configured an external versioner command to start with a tilde instead of my actual home directory and deleted a file on another device.

Additionally, I started syncthing with STTRACE=versioner and verified that the leading ~ in the external versioner command was replaced with my home directory path in the debug output.

Documentation

I will create the corresponding pull request if the PR gets approved and you think it is necessary.

[tomasz1986]

I don't do this anymore, but in the past I used to embed the whole Recycle Bin script from https://docs.syncthing.net/users/versioning.html#move-to-the-recycle-bin-using-powershell in the external versioner command (i.e. using powershell -Command "& {…}"). The script includes the literal ~ as it is used for Syncthing's temporary files (in Windows). If someone else has albo been embedding the whole script in Syncthing itself like that, this change will break it.

[xduugu]

I don't think this is the case, @tomasz1986. fs.ExpandTilde only replaces the leading ~, and only if it is followed by a path separator.

[tomasz1986]

You're right, I didn't know that 😳.

Does this mean that it won't work with the path inside double quotes?

Using another example from the Docs,

"C:\Users\mfrnd\Scripts\onlylatest.bat" "%FOLDER_PATH%" "%FILE_PATH%"

could be written as

~\Scripts\onlylatest.bat "%FOLDER_PATH%" "%FILE_PATH%"

but not

"~\Scripts\onlylatest.bat" "%FOLDER_PATH%" "%FILE_PATH%"

Is this correct? If yes, then it would probably be useful to document this expansion at https://docs.syncthing.net/users/versioning with the caveats, as especially on Windows, people often have spaces in their usernames, and because of that double quotes are a necessity, so ~ will not work there.

[xduugu]

I actually haven't thought about quoting, but I don't expect it to work if the first word is quoted.

Maybe it would be better then to expand a leading tilde in words[0] just before the command is executed here. This would also correctly deal with home paths that contain spaces which would currently break the command. The only oddity would be that on linux/unix shells, a quoted ~ usually does not have the same meaning as the unquoted one.

[tomasz1986]

Just a second thought, but is the expansion really needed in this specific command? The reason is that you can use your standard environment variables, so on Windows, you can just do "%USERPROFILE%\Scripts\onlylatest.bat" "%FOLDER_PATH%" "%FILE_PATH%". I believe that the standard $HOME variable should work on macOS and Linux as well, although I haven't tested it myself.

This is different than using ~ in other places in Syncthing, e.g. folder paths, where OS variables are not allowed.

[xduugu]

That's what I tried first, but unfortunately neither $HOME/path/to/command nor "$HOME/path/to/command" works. I'm surprised that it works on windows, because at the top of os/exec, it explicitly states:

Unlike the "system" library call from C and other languages, the os/exec package intentionally does not invoke the system shell and does not expand any glob patterns or handle other expansions, pipelines, or redirections typically done by shells. The package behaves more like C's "exec" family of functions. To expand glob patterns, either call the shell directly, taking care to escape any dangerous input, or use the path/filepath package's Glob function. To expand environment variables, use package os's ExpandEnv.

(emphasis added by me)

edit: If you dislike the tilde expansion, adding another variable %HOME% (or something else if this conflicts with a windows variable) would be fine for me, too. ~ is just the common shortcut for HOME in paths on linux.

[tomasz1986]

That's what I tried first, but unfortunately neither $HOME/path/to/command nor "$HOME/path/to/command" works. I'm surprised that it works on windows, because at the top of os/exec, it explicitly states:

Unlike the "system" library call from C and other languages, the os/exec package intentionally does not invoke the system shell and does not expand any glob patterns or handle other expansions, pipelines, or redirections typically done by shells. The package behaves more like C's "exec" family of functions. To expand glob patterns, either call the shell directly, taking care to escape any dangerous input, or use the path/filepath package's Glob function. To expand environment variables, use package os's ExpandEnv.

(emphasis added by me)

Yeah, so I've tested this again, and the variable doesn't actually expand. However, there is also some kind of a bug because, e.g. if you put "%USERPROFILE%\Scripts\onlylatest.bat" in the command, then the script always succeeds, with the GUI showing the file as deleted, yet in reality it remains in the folder. This happens regardless if the Batch file even exists or not.

I'm now wondering if it would be harmful in any way to just allow using OS environment variables in this particular command only?

[calmh]

Not expanding inside quotes is the expected behavior as far as shells go.

jb@ok:~ % echo ~
/Users/jb
jb@ok:~ % echo "~"
~

This change seems harmless and possibly beneficial to me.

[imsodin]

fs.ExpandTilde calls filepath.FromSlash - that seems to conflict with whatever the if build.IsWindows block here is trying to achieve. Also that call might lead to complications/unintended modifications if the command is not just a path, but also has arguments.

[xduugu]

Oh, that's bad. Thanks for pointing out, @imsodin.

I pushed the fixup that I already prepared to solve the quoting issue @tomasz1986 mentioned^1. This should prevent the possible unintended modifications as it only calls fs.ExpandTilde on the first word of the command, but has the drawback that it behaves differently than a shell. A quoted and an unquoted (leading) tilde are treated the same, because the quotes were already removed by shellquote.Split at this point in the code.

[imsodin]

I still don't think converting from slashes to native path separators is a good idea here (what fs.ExpandTilde does), especially not if done on just one part of the argument. Not because I know/believe that particular action is problematic, but because changing the user input at all just has the potential for unexpected behaviour. We don't know what the user does/intends with the input. Thus I'd prefer not to dabble in trying to emulate shell behaviour, but instead use the already existing template variable handling by adding %HOME%, as was already suggested somewhere. Tilde may still be used by explicitly calling a shell in the external versioner comment. If we for some reason need tilde expansion, I'd rather have a dedicated check here for the first character being a tilde, and followed by any path separator, and then replacing just that tilde.

[xduugu]

Thanks for coming back to this, @imsodin.

As I wrote before, I'm fine with using %HOME% instead of the tilde and would be happy to implement it if there is a consensus.

edit: If you dislike the tilde expansion, adding another variable %HOME% (or something else if this conflicts with a windows variable) would be fine for me, too. ~ is just the common shortcut for HOME in paths on linux.

As I already wrote, my only concern is that %HOME% is a too generic term and could conflict with a windows environment variable. They use the the same syntax and there is no possibility to prevent the replacement of %HOME%, because it's a simple string replacement operation and not a template string which would support escaping.

[calmh]

Frankly, just not specifying a path to script and making sure it's in $PATH seems like the easiest solution here.

[xduugu]

Frankly, just not specifying a path to script and making sure it's in $PATH seems like the easiest solution here.

The script is actually in "my" PATH, but unfortunately, this is not the same PATH as the one used by systemd services. This PATH is usually something like /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin, so it does not include any user-writable directories. It is possible to adjust the PATH variable for a specific systemd service, but you cannot simply pre- or append a directory to it, you have to hardcode the complete PATH variable.

Eventually, it "splits" the syncthing configuration into two files as it requires an additional configuration file at a totally different location in the home directory. And if you run syncthing inside a flatpak^1, you need a different hack to make syncthing aware of the user script.

What actually works, is calling a shell to execute the script (bash -c 'script "%FOLDER_PATH%/%FILE_PATH%"'), but it feels like a hack to use a shell to get the path of a script which I already know before. It also looks like it will open the door for code injections via file name. And finally, even it is okay in my case, I don't think it should be necessary to add a script to your PATH that must only be executed by syncthing, not an user.

[calmh]

Yet another option, since you're looking to be portable, would be to have the script in the synced folder.

[xduugu]

With "portable" I meant that I would like to avoid to hardcode the home directory in a config file, not that I necessarily want to use the same versioner command on a different syncthing device.

Putting the "script", which in my case is actually a rust binary, into the folder is not a good solution for me, because I use the same versioner command as default for all my folder. I would have to put the binary into all folders and also have to find a way to handle different cpu architectures.

My use case is actually the "Move deleted file to system trash" one. I saw several discussions in the issues about this topic, but since it is not supported by syncthing, I have to use the external versioner.

[xduugu]

I've added a commit which adds an ExpandTilde function without the slash to path separator conversion. When I looked at my syncthing config, I noticed that the main reason, why I used the tilde initially was, that I also use it in the folder paths. Aside from that, the user home directory is identical for every deleted file, so it does not make sense to handle it in the same way as, for example, %FOLDER_PATH% (which is a bad example, because it is also static and could be replaced only once per folder in newExternal).

By the way, I tried to replace the tilde in the command in newExternal, but unfortunately, that's not easy to implement, because the first word could be quoted. Does somebody know why shellquote.Split is executed in Archive for every single file and not only once in newExternal when the folder is instantiated?

[imsodin]

Does somebody know why shellquote.Split is executed in Archive for every single file and not only once in newExternal when the folder is instantiated?

I don't know, and agree with the sentiment that it should be equivalent/better to do it in newExternal. Also all the template expansion stuff.

[calmh]

Looks like this ran into difficulties and there are workarounds. I'm open for having the change though if the challenges are sorted out, in which case please reopen.