Fix #104237: fix issue with drive-only paths on Windows

Fix an issue where a shared storage path on Linux, that maps via two-way
variables to a drive root on Windows, caused problems with the path
translation system.

Windows paths that consist only of a drive letter (`F:`) cannot just be
concatenated to a relative path, as that will result in `F:path\to\file`,
which is still a relative path of sorts. This is now handled correctly,
and should result in `F:\path\to\file`.

This fixes #104237.

internal/manager/api_impl/meta_test.go

@@ -143,6 +143,63 @@ func TestGetSharedStorage(t *testing.T) {
 
 }
 
+// Test shared storage sitting on /mnt/flamenco, where that's mapped to F:\ for Windows.
+func TestGetSharedStorageDriveLetterRoot(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	mf := newMockedFlamenco(mockCtrl)
+
+	conf := config.GetTestConfig(func(c *config.Conf) {
+		// Test with a Manager on Linux.
+		c.MockCurrentGOOSForTests("linux")
+
+		// Set up a two-way variable to do the mapping.
+		c.Variables["shared_storage_mapping"] = config.Variable{
+			IsTwoWay: true,
+			Values: []config.VariableValue{
+				{Value: "/mnt/flamenco", Platform: config.VariablePlatformLinux, Audience: config.VariableAudienceAll},
+				{Value: `F:\`, Platform: config.VariablePlatformWindows, Audience: config.VariableAudienceAll},
+			},
+		}
+	})
+	mf.config.EXPECT().Get().Return(&conf).AnyTimes()
+	mf.config.EXPECT().EffectiveStoragePath().Return(`/mnt/flamenco`).AnyTimes()
+
+	{ // Test user client on Linux.
+		mf.config.EXPECT().
+			NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformLinux).
+			DoAndReturn(conf.NewVariableExpander)
+		mf.shaman.EXPECT().IsEnabled().Return(false)
+
+		echoCtx := mf.prepareMockedRequest(nil)
+		err := mf.flamenco.GetSharedStorage(echoCtx, api.ManagerVariableAudienceUsers, "linux")
+		require.NoError(t, err)
+		assertResponseJSON(t, echoCtx, http.StatusOK, api.SharedStorageLocation{
+			Location: "/mnt/flamenco",
+			Audience: api.ManagerVariableAudienceUsers,
+			Platform: "linux",
+		})
+	}
+
+	{ // Test user client on Windows.
+		mf.config.EXPECT().
+			NewVariableExpander(config.VariableAudienceUsers, config.VariablePlatformWindows).
+			DoAndReturn(conf.NewVariableExpander)
+		mf.shaman.EXPECT().IsEnabled().Return(false)
+
+		echoCtx := mf.prepareMockedRequest(nil)
+		err := mf.flamenco.GetSharedStorage(echoCtx, api.ManagerVariableAudienceUsers, "windows")
+		require.NoError(t, err)
+		assertResponseJSON(t, echoCtx, http.StatusOK, api.SharedStorageLocation{
+			Location: `F:\`,
+			Audience: api.ManagerVariableAudienceUsers,
+			Platform: "windows",
+		})
+	}
+
+}
+
 func TestCheckSharedStoragePath(t *testing.T) {
 	mf, finish := metaTestFixtures(t)
 	defer finish()

internal/manager/config/variables.go

@@ -126,7 +126,7 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 		if !isValueMatch(expanded, managerValue) {
 			continue
 		}
-		expanded = targetValue + expanded[len(managerValue):]
+		expanded = ve.join(targetValue, expanded[len(managerValue):], ve.targetPlatform)
 
 		// Since two-way variables are meant for path replacement, we know this
 		// should be a path.
@@ -136,5 +136,20 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 	if isPathValue {
 		expanded = crosspath.ToPlatform(expanded, string(ve.targetPlatform))
 	}
+
 	return expanded
 }
+
+func (ve *VariableExpander) join(valueFromVariable, suffix string, platform VariablePlatform) string {
+	result := valueFromVariable + suffix
+
+	if platform == VariablePlatformWindows {
+		// 'result' may now be of the form `F:some\path\to\file`, where `F:` comes
+		// from `valueFromVariable` and the rest is the suffix. This is not an
+		// absolute path, and needs a separator between the drive letter and the
+		// rest of the path.
+		return crosspath.EnsureDriveAbsolute(result)
+	}
+
+	return result
+}

pkg/crosspath/crosspath.go

@@ -131,6 +131,10 @@ func ToPlatform(path, platform string) string {
 	pathsep := pathSepForPlatform(platform)
 	translated := strings.Join(components, pathsep)
 
+	if platform == "windows" {
+		return EnsureDriveAbsolute(translated)
+	}
+
 	return translated
 }
 
@@ -162,3 +166,28 @@ func TrimTrailingSep(path string) string {
 	}
 	return trimmed
 }
+
+// EnsureDriveAbsolute ensures that a Windows path that starts with a drive
+// letter also has an absolute path on that drive. For example, turns
+// `F:path\to\file` into `F:\path\to\file`.
+func EnsureDriveAbsolute(windowsPath string) string {
+	runes := []rune(windowsPath)
+	numRunes := len(runes)
+	if numRunes < 2 {
+		return windowsPath
+	}
+
+	if !validDriveLetter(runes[0]) || runes[1] != ':' {
+		return windowsPath
+	}
+
+	pathSep := pathSepForPlatform("windows")
+	if numRunes == 2 {
+		return windowsPath + pathSep
+	}
+	if string(runes[2]) == pathSep {
+		return windowsPath // Already F:\blabla
+	}
+
+	return string(runes[:2]) + pathSep + string(runes[2:])
+}

pkg/crosspath/crosspath_test.go

@@ -185,6 +185,7 @@ func TestIsRoot(t *testing.T) {
 		{"just letters", "just letters", false},
 		{"subdir of root", "/subdir", false},
 		{"subdir of drive", "C:\\subdir", false},
+		{"relative subdir of drive", "C:subdir", false},
 
 		{"indirectly root", "/subdir/..", false},
 		{"UNC notation", `\\NAS\Share\`, false},
@@ -221,7 +222,14 @@ func TestToPlatform(t *testing.T) {
 		{"mixed-lnx", args{`F:/mixed/path\to\file.blend`, "linux"}, `F:/mixed/path/to/file.blend`},
 		{"absolute-win", args{`F:/absolute/path`, "windows"}, `F:\absolute\path`},
 		{"absolute-lnx", args{`/absolute/path`, "linux"}, `/absolute/path`},
-		{"drive-relative-win", args{`/absolute/path`, "windows"}, `\absolute\path`},
+		{"relative-win", args{`/absolute/path`, "windows"}, `\absolute\path`},
+
+		// Trailing path separators should not be removed if it's only a drive
+		// letter, as concatenation rules are tricky there. `F:path` is not the same
+		// as `F:\path`.
+		{"drive-root-win", args{`F:\`, "windows"}, `F:\`},
+		{"trailing-win", args{`F:\directory\`, "linux"}, `F:/directory`},
+		{"trailing-lnx", args{`/dir/path/`, "windows"}, `\dir\path`},
 
 		// UNC notation should survive, even when it no longer makes sense (like on Linux).
 		{"unc-win", args{`\\NAS\share\path`, "windows"}, `\\NAS\share\path`},
@@ -267,3 +275,34 @@ func TestTrimTrailingSep(t *testing.T) {
 		})
 	}
 }
+
+func TestEnsureDriveAbsolute(t *testing.T) {
+	tests := []struct {
+		name      string
+		inputPath string
+		want      string
+	}{
+		// Windows paths expected to change:
+		{"windows-drive-relative", `F:path\to\file`, `F:\path\to\file`},
+		{"windows-drive-relative-mixed", "F:path/to/file", `F:\path/to/file`},
+		{"windows-drive-only", "F:", `F:\`},
+
+		// No-op paths:
+		{"empty", "", ""},
+		{"linux-root", "/", "/"},
+		{"linux-path", "/some/path", "/some/path"},
+		{"linux-unicode-path", "/söme/path", "/söme/path"},
+		{"one-letter", "F", "F"},
+		{"windows-drive-invalid", `©:path\to\thing`, `©:path\to\thing`},
+		{"windows-unc", `\\NAS\Flamenco\path`, `\\NAS\Flamenco\path`},
+		{"unicode-one-letter", "€", "€"},
+		{"windows-drive-absolute", `F:\some\path`, `F:\some\path`},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := EnsureDriveAbsolute(tt.inputPath); got != tt.want {
+				t.Errorf("EnsureDriveAbsolute() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}