lib/fs, lib/model: Cover more windowsyness sanitizing paths (fixes #7075) (#7158)

lib/fs/util.go

@@ -12,6 +12,7 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
+	"unicode"
 )
 
 func ExpandTilde(path string) (string, error) {
@@ -72,16 +73,9 @@ func WindowsInvalidFilename(name string) error {
 			// Names ending in space or period are not valid.
 			return errInvalidFilenameWindowsSpacePeriod
 		}
-		upperCased := strings.ToUpper(part)
+		if windowsIsReserved(part) {
-		for _, disallowed := range windowsDisallowedNames {
-			if upperCased == disallowed {
 			return errInvalidFilenameWindowsReservedName
 		}
-			if strings.HasPrefix(upperCased, disallowed+".") {
-				// nul.txt.jpg is also disallowed
-				return errInvalidFilenameWindowsReservedName
-			}
-		}
 	}
 
 	// The path must not contain any disallowed characters
@@ -92,6 +86,63 @@ func WindowsInvalidFilename(name string) error {
 	return nil
 }
 
+// SanitizePath takes a string that might contain all kinds of special
+// characters and makes a valid, similar, path name out of it.
+//
+// Spans of invalid characters, whitespace and/or non-UTF-8 sequences are
+// replaced by a single space. The result is always UTF-8 and contains only
+// printable characters, as determined by unicode.IsPrint.
+//
+// Invalid characters are non-printing runes, things not allowed in file names
+// in Windows, and common shell metacharacters. Even if asterisks and pipes
+// and stuff are allowed on Unixes in general they might not be allowed by
+// the filesystem and may surprise the user and cause shell oddness. This
+// function is intended for file names we generate on behalf of the user,
+// and surprising them with odd shell characters in file names is unkind.
+//
+// We include whitespace in the invalid characters so that multiple
+// whitespace is collapsed to a single space. Additionally, whitespace at
+// either end is removed.
+//
+// If the result is a name disallowed on windows, a hyphen is prepended.
+func SanitizePath(path string) string {
+	var b strings.Builder
+
+	disallowed := `<>:"'/\|?*[]{};:!@$%&^#` + windowsDisallowedCharacters
+	prev := ' '
+	for _, c := range path {
+		if !unicode.IsPrint(c) || c == unicode.ReplacementChar ||
+			strings.ContainsRune(disallowed, c) {
+			c = ' '
+		}
+
+		if !(c == ' ' && prev == ' ') {
+			b.WriteRune(c)
+		}
+		prev = c
+	}
+
+	path = strings.TrimSpace(b.String())
+	if windowsIsReserved(path) {
+		path = "-" + path
+	}
+	return path
+}
+
+func windowsIsReserved(part string) bool {
+	upperCased := strings.ToUpper(part)
+	for _, disallowed := range windowsDisallowedNames {
+		if upperCased == disallowed {
+			return true
+		}
+		if strings.HasPrefix(upperCased, disallowed+".") {
+			// nul.txt.jpg is also disallowed
+			return true
+		}
+	}
+	return false
+}
+
 // IsParent compares paths purely lexicographically, meaning it returns false
 // if path and parent aren't both absolute or relative.
 func IsParent(path, parent string) bool {

lib/fs/util_test.go

@@ -7,8 +7,11 @@
 package fs
 
 import (
+	"math/rand"
 	"runtime"
 	"testing"
+	"unicode"
+	"unicode/utf8"
 )
 
 func TestCommonPrefix(t *testing.T) {
@@ -70,3 +73,47 @@ func TestWindowsInvalidFilename(t *testing.T) {
 		}
 	}
 }
+
+func TestSanitizePath(t *testing.T) {
+	cases := [][2]string{
+		{"", ""},
+		{"foo", "foo"},
+		{`\*/foo\?/bar[{!@$%^&*#()}]`, "foo bar ()"},
+		{"Räksmörgås", "Räksmörgås"},
+		{`Räk \/ smörgås`, "Räk smörgås"},
+		{"هذا هو *\x07?اسم الملف", "هذا هو اسم الملف"},
+		{`../foo.txt`, `.. foo.txt`},
+		{"  \t \n filename in  \t space\r", "filename in space"},
+		{"你\xff好", `你 好`},
+		{"\000 foo", "foo"},
+	}
+
+	for _, tc := range cases {
+		res := SanitizePath(tc[0])
+		if res != tc[1] {
+			t.Errorf("SanitizePath(%q) => %q, expected %q", tc[0], res, tc[1])
+		}
+	}
+}
+
+// Fuzz test: SanitizePath must always return strings of printable UTF-8
+// characters when fed random data.
+//
+// Note that space is considered printable, but other whitespace runes are not.
+func TestSanitizePathFuzz(t *testing.T) {
+	buf := make([]byte, 128)
+
+	for i := 0; i < 100; i++ {
+		rand.Read(buf)
+		path := SanitizePath(string(buf))
+		if !utf8.ValidString(path) {
+			t.Errorf("SanitizePath(%q) => %q, not valid UTF-8", buf, path)
+			continue
+		}
+		for _, c := range path {
+			if !unicode.IsPrint(c) {
+				t.Errorf("non-printable rune %q in sanitized path", c)
+			}
+		}
+	}
+}

lib/model/model.go

@@ -19,7 +19,6 @@ import (
 	"strings"
 	stdsync "sync"
 	"time"
-	"unicode"
 
 	"github.com/pkg/errors"
 	"github.com/thejerf/suture/v4"
@@ -1467,8 +1466,8 @@ func (m *model) handleAutoAccepts(deviceID protocol.DeviceID, folder protocol.Fo
 		defaultPath := m.cfg.Options().DefaultFolderPath
 		defaultPathFs := fs.NewFilesystem(fs.FilesystemTypeBasic, defaultPath)
 		pathAlternatives := []string{
-			sanitizePath(folder.Label),
+			fs.SanitizePath(folder.Label),
-			sanitizePath(folder.ID),
+			fs.SanitizePath(folder.ID),
 		}
 		for _, path := range pathAlternatives {
 			if _, err := defaultPathFs.Lstat(path); !fs.IsNotExist(err) {
@@ -2751,42 +2750,6 @@ func (m *syncMutexMap) Get(key string) sync.Mutex {
 	return v.(sync.Mutex)
 }
 
-// sanitizePath takes a string that might contain all kinds of special
-// characters and makes a valid, similar, path name out of it.
-//
-// Spans of invalid characters, whitespace and/or non-UTF-8 sequences are
-// replaced by a single space. The result is always UTF-8 and contains only
-// printable characters, as determined by unicode.IsPrint.
-//
-// Invalid characters are non-printing runes, things not allowed in file names
-// in Windows, and common shell metacharacters. Even if asterisks and pipes
-// and stuff are allowed on Unixes in general they might not be allowed by
-// the filesystem and may surprise the user and cause shell oddness. This
-// function is intended for file names we generate on behalf of the user,
-// and surprising them with odd shell characters in file names is unkind.
-//
-// We include whitespace in the invalid characters so that multiple
-// whitespace is collapsed to a single space. Additionally, whitespace at
-// either end is removed.
-func sanitizePath(path string) string {
-	var b strings.Builder
-
-	prev := ' '
-	for _, c := range path {
-		if !unicode.IsPrint(c) || c == unicode.ReplacementChar ||
-			strings.ContainsRune(`<>:"'/\|?*[]{};:!@$%&^#`, c) {
-			c = ' '
-		}
-
-		if !(c == ' ' && prev == ' ') {
-			b.WriteRune(c)
-		}
-		prev = c
-	}
-
-	return strings.TrimSpace(b.String())
-}
-
 type deviceIDSet map[protocol.DeviceID]struct{}
 
 func (s deviceIDSet) add(ids []protocol.DeviceID) {

lib/model/model_test.go

@@ -25,8 +25,6 @@ import (
 	"sync/atomic"
 	"testing"
 	"time"
-	"unicode"
-	"unicode/utf8"
 
 	"github.com/syncthing/syncthing/lib/config"
 	"github.com/syncthing/syncthing/lib/db"
@@ -3283,50 +3281,6 @@ func TestRequestLimit(t *testing.T) {
 	}
 }
 
-func TestSanitizePath(t *testing.T) {
-	cases := [][2]string{
-		{"", ""},
-		{"foo", "foo"},
-		{`\*/foo\?/bar[{!@$%^&*#()}]`, "foo bar ()"},
-		{"Räksmörgås", "Räksmörgås"},
-		{`Räk \/ smörgås`, "Räk smörgås"},
-		{"هذا هو *\x07?اسم الملف", "هذا هو اسم الملف"},
-		{`../foo.txt`, `.. foo.txt`},
-		{"  \t \n filename in  \t space\r", "filename in space"},
-		{"你\xff好", `你 好`},
-		{"\000 foo", "foo"},
-	}
-
-	for _, tc := range cases {
-		res := sanitizePath(tc[0])
-		if res != tc[1] {
-			t.Errorf("sanitizePath(%q) => %q, expected %q", tc[0], res, tc[1])
-		}
-	}
-}
-
-// Fuzz test: sanitizePath must always return strings of printable UTF-8
-// characters when fed random data.
-//
-// Note that space is considered printable, but other whitespace runes are not.
-func TestSanitizePathFuzz(t *testing.T) {
-	buf := make([]byte, 128)
-
-	for i := 0; i < 100; i++ {
-		rand.Read(buf)
-		path := sanitizePath(string(buf))
-		if !utf8.ValidString(path) {
-			t.Errorf("sanitizePath(%q) => %q, not valid UTF-8", buf, path)
-			continue
-		}
-		for _, c := range path {
-			if !unicode.IsPrint(c) {
-				t.Errorf("non-printable rune %q in sanitized path", c)
-			}
-		}
-	}
-}
-
 // TestConnCloseOnRestart checks that there is no deadlock when calling Close
 // on a protocol connection that has a blocking reader (blocking writer can't
 // be done as the test requires clusterconfigs to go through).