Repository: syncthing/syncthing (language: go)

Issue:
Improve error message about folder marker outside of folder
A folder marker point at the folder root or even outside of it (e.g. `../foo/bar`) is accepted. This is unsafe, as it breaks the check against fully disappeared folder contents. *If* that check should be possible to be disabled, then that should be an explicit option (I don't think it should be).


Discussion / hints:
If I try to use things like `../foo` or `..` for the folder marker in Windows, I get the following errors.

![image](https://user-images.githubusercontent.com/5626656/133009279-07f74f29-3f52-41e8-b7df-f6429ed0aa64.png)
![image](https://user-images.githubusercontent.com/5626656/133009281-662e9d15-ec5e-4bef-b30e-5851ae9cf554.png)

This is with the current Syncthing release already. The only "funky" marker that is accepted here seems to be `.`, but the ones located outside of the folder are not.

Does the behaviour differ between operating systems? Do other OSes accept `../foo/bar`? It doesn't seem that outside markers have ever been accepted, at least in Windows (e.g. see https://reddit.com/r/Syncthing/comments/9yhtnr/can_i_get_one_example_of_a_relative_marker_name from 2018).
(I haven't looked at this at all but I hope our FS implementation eats the dot-dots and so this isn't allowed.)
Yeah right. The issue has been about ".", the rest was an afterthought. Still doesn't to explicitly reject it from config, the error message will be nicer than "not a relative path" for something that is technically a relative path.
> Yeah right. The issue has been about ".", the rest was an afterthought.

No worries 🙂. How about changing the issue title a little bit to make it more about adding a proper error message instead?

Fix patch:
diff --git a/lib/fs/basicfs.go b/lib/fs/basicfs.go
index db5bb97cb1d..28392e70390 100644
--- a/lib/fs/basicfs.go
+++ b/lib/fs/basicfs.go
@@ -23,7 +23,6 @@ var (
 	errInvalidFilenameWindowsSpacePeriod  = errors.New("name is invalid, must not end in space or period on Windows")
 	errInvalidFilenameWindowsReservedName = errors.New("name is invalid, contains Windows reserved name (NUL, COM1, etc.)")
 	errInvalidFilenameWindowsReservedChar = errors.New("name is invalid, contains Windows reserved character (?, *, etc.)")
-	errNotRelative                        = errors.New("not a relative path")
 )
 
 type OptionJunctionsAsDirs struct{}
diff --git a/lib/fs/filesystem.go b/lib/fs/filesystem.go
index c19860dd47a..d2af30518fd 100644
--- a/lib/fs/filesystem.go
+++ b/lib/fs/filesystem.go
@@ -249,6 +249,11 @@ func IsInternal(file string) bool {
 	return false
 }
 
+var (
+	errPathInvalid           = errors.New("path is invalid")
+	errPathTraversingUpwards = errors.New("relative path traversing upwards (starting with ..)")
+)
+
 // Canonicalize checks that the file path is valid and returns it in the "canonical" form:
 // - /foo/bar -> foo/bar
 // - / -> "."
@@ -259,7 +264,7 @@ func Canonicalize(file string) (string, error) {
 		// The relative path may pretend to be an absolute path within
 		// the root, but the double path separator on Windows implies
 		// something else and is out of spec.
-		return "", errNotRelative
+		return "", errPathInvalid
 	}
 
 	// The relative path should be clean from internal dotdots and similar
@@ -268,10 +273,10 @@ func Canonicalize(file string) (string, error) {
 
 	// It is not acceptable to attempt to traverse upwards.
 	if file == ".." {
-		return "", errNotRelative
+		return "", errPathTraversingUpwards
 	}
 	if strings.HasPrefix(file, ".."+pathSep) {
-		return "", errNotRelative
+		return "", errPathTraversingUpwards
 	}
 
 	if strings.HasPrefix(file, pathSep) {


Test patch: