Add fork-specific CVars + cleanup

RELEASE-NOTES.md

@@ -35,7 +35,7 @@ END TEMPLATE-->
 
 ### Breaking changes
 
-*None yet*
+* CVar.FORK is now a flag if you wish to save / load CVars to a folder specific to your Fork ID. Configs will now also save in a Config subfolder with the base config and all fork-specific configs located within.
 
 ### New features
 
@@ -45,6 +45,8 @@ END TEMPLATE-->
 ### Bugfixes
 
 * Fix ValueList TryPop not clearing element references.
+* The client will no longer save server CVars and vice versa.
+* Fix ConfigurationManager doing raw File calls over using the UserData provider.
 
 ### Other
 

Robust.Client/GameController/GameController.cs

@@ -406,17 +406,52 @@ internal bool StartupSystemSplash(
 
             if (Options.LoadConfigAndUserData)
             {
-                var configFile = Path.Combine(userDataDir, Options.ConfigFileName);
-                if (File.Exists(configFile))
+                var userData = new WritableDirProvider(Directory.CreateDirectory(userDataDir), hideRootDir: true);
+                var configPath = new ResPath(Options.ConfigFileName).ToRootedPath();
+                var legacyConfigPath = new ResPath("/client_config.toml");
+                if (userData.Exists(configPath))
                 {
                     // Load config from user data if available.
-                    _configurationManager.LoadFromFile(configFile);
+                    _configurationManager.LoadFromFile(userData, configPath);
+                    _configurationManager.SetSaveFile(userData, configPath);
+                }
+                else if (userData.Exists(legacyConfigPath))
+                {
+                    // Load the old root-level config for backwards compatibility, but save to the new location.
+                    _logManager.GetSawmill("cfg").Info(
+                        "Migrating client configuration from '{0}' to '{1}'.",
+                        legacyConfigPath,
+                        configPath);
+                    _configurationManager.LoadFromFile(userData, legacyConfigPath);
+                    _configurationManager.SetSaveFile(userData, configPath);
+                    _configurationManager.SaveToFile();
+
+                    if (userData.Exists(configPath))
+                    {
+                        _logManager.GetSawmill("cfg").Info(
+                            "Removing migrated legacy client configuration file '{0}'.",
+                            legacyConfigPath);
+                        userData.Delete(legacyConfigPath);
+                    }
                 }
                 else
                 {
                     // Else we just use code-defined defaults and let it save to file when the user changes things.
-                    _configurationManager.SetSaveFile(configFile);
+                    _configurationManager.SetSaveFile(userData, configPath);
                 }
+
+                var forkId = _configurationManager.GetCVar(CVars.BuildForkId);
+                var forkConfigName = string.IsNullOrWhiteSpace(forkId)
+                    ? "unspecified"
+                    : ResPath.SanitizeFilename(forkId);
+                var forkConfigPath = new ResPath($"/Config/{forkConfigName}_config.toml");
+                if (userData.Exists(forkConfigPath))
+                {
+                    _configurationManager.LoadFromFile(userData, forkConfigPath);
+                    _configurationManager.SetSaveFile(userData, configPath);
+                }
+
+                _configurationManager.SetForkSaveFile(userData, forkConfigPath);
             }
 
             _configurationManager.OverrideConVars(EnvironmentVariables.GetEnvironmentCVars());

Robust.Client/GameControllerOptions.cs

@@ -24,7 +24,7 @@ public sealed class GameControllerOptions
         /// <summary>
         ///     Name of the configuration file in the user data directory.
         /// </summary>
-        public string ConfigFileName { get; init; } = "client_config.toml";
+        public string ConfigFileName { get; init; } = "Config/client_config.toml";
 
         // TODO: Define engine branding from json file in resources.
         /// <summary>

Robust.Client/Replays/Commands/ReplayLoadCommand.cs

@@ -43,7 +43,15 @@ public override void Execute(IConsoleShell shell, string argStr, string[] args)
             return;
         }
 
-        var file = new ResPath(_cfg.GetCVar(CVars.ReplayDirectory)) / args[0];
+        var replayFile = new ResPath(args[0]);
+        var file = new ResPath(_cfg.GetCVar(CVars.ReplayDirectory)) / replayFile;
+
+        if (!file.IsValidFilePath(rooted: false))
+        {
+            shell.WriteError(Loc.GetString("cmd-error-file-not-found", ("file", args[0])));
+            return;
+        }
+
         if (!_resMan.UserData.Exists(file))
         {
             shell.WriteError(Loc.GetString("cmd-error-file-not-found", ("file", file)));

Robust.Shared.IntegrationTests/Configuration/ConfigurationManagerTest.cs

@@ -1,12 +1,14 @@
 using Moq;
 using NUnit.Framework;
-using Robust.Server.Configuration;
+using Robust.Shared;
 using Robust.Shared.Configuration;
+using Robust.Shared.ContentPack;
 using Robust.Shared.IoC;
 using Robust.Shared.Log;
 using Robust.Shared.Network;
 using Robust.Shared.Replays;
 using Robust.Shared.Timing;
+using Robust.Shared.Utility;
 
 namespace Robust.Shared.IntegrationTests.Configuration
 {
@@ -137,18 +139,116 @@ public void TestOverrideDefaultValue()
             Assert.That(mgr.GetCVar<int>("foo.bar"), Is.EqualTo(7));
         }
 
+        [Test]
+        public void TestClientSaveDoesNotSerializeServerCVars()
+        {
+            var mgr = MakeCfgInternal(isServer: false);
+            var userData = new VirtualWritableDirProvider();
+            var path = new ResPath("/Config/client_config.toml");
+
+            mgr.RegisterCVar("test.client", 0, CVar.ARCHIVE);
+            mgr.RegisterCVar("test.server", 0, CVar.ARCHIVE | CVar.SERVER);
+            mgr.RegisterCVar("test.server_only", 0, CVar.ARCHIVE | CVar.SERVERONLY);
+            mgr.SetCVar("test.client", 1);
+            mgr.SetCVar("test.server", 2, force: true);
+            mgr.SetSaveFile(userData, path);
+            mgr.SaveToFile();
+
+            var text = userData.ReadAllText(path);
+            Assert.That(text, Does.Contain("client = 1"));
+            Assert.That(text, Does.Not.Contain("server = 2"));
+            Assert.That(text, Does.Not.Contain("server_only"));
+        }
+
+        [Test]
+        public void TestForkCVarsSaveToForkConfig()
+        {
+            var mgr = MakeCfgInternal(isServer: false);
+            var userData = new VirtualWritableDirProvider();
+            var configPath = new ResPath("/Config/client_config.toml");
+            var forkConfigPath = new ResPath("/Config/test-fork_config.toml");
+
+            mgr.RegisterCVar(CVars.BuildForkId.Name, "", CVar.NONE);
+            mgr.LoadCVarsFromType(typeof(TestForkCVars));
+            mgr.SetCVar(CVars.BuildForkId.Name, "test-fork");
+            mgr.SetCVar(TestForkCVars.Shared.Name, 5);
+            mgr.SetCVar(TestForkCVars.ForkSpecific.Name, 7);
+            mgr.SetSaveFile(userData, configPath);
+            mgr.SetForkSaveFile(userData, forkConfigPath);
+            mgr.SaveToFile();
+
+            var sharedText = userData.ReadAllText(configPath);
+            var forkText = userData.ReadAllText(forkConfigPath);
+            Assert.That(sharedText, Does.Contain("shared = 5"));
+            Assert.That(sharedText, Does.Not.Contain("fork_specific"));
+            Assert.That(forkText, Does.Contain("fork_specific = 7"));
+            Assert.That(forkText, Does.Not.Contain("shared"));
+        }
+
+        [Test]
+        public void TestForkCVarsSaveToUnspecifiedForkConfig()
+        {
+            var mgr = MakeCfgInternal(isServer: false);
+            var userData = new VirtualWritableDirProvider();
+            var configPath = new ResPath("/Config/client_config.toml");
+            var forkConfigPath = new ResPath("/Config/unspecified_config.toml");
+
+            mgr.LoadCVarsFromType(typeof(TestForkCVars));
+            mgr.SetCVar(TestForkCVars.ForkSpecific.Name, 7);
+            mgr.SetSaveFile(userData, configPath);
+            mgr.SetForkSaveFile(userData, forkConfigPath);
+            mgr.SaveToFile();
+
+            var sharedText = userData.ReadAllText(configPath);
+            var forkText = userData.ReadAllText(forkConfigPath);
+            Assert.That(sharedText, Does.Not.Contain("fork_specific"));
+            Assert.That(forkText, Does.Contain("fork_specific = 7"));
+        }
+
+        [Test]
+        public void TestUserDataConfigSaveRejectsTraversal()
+        {
+            var mgr = MakeCfgInternal(isServer: false);
+            var userData = new VirtualWritableDirProvider();
+
+            Assert.That(
+                () => mgr.SetSaveFile(userData, new ResPath("/Config/../client_config.toml")),
+                Throws.ArgumentException);
+
+            Assert.That(
+                () => mgr.SetForkSaveFile(userData, new ResPath("/Config/../fork_config.toml")),
+                Throws.ArgumentException);
+        }
+
         private IConfigurationManager MakeCfg()
+        {
+            return MakeCfgInternal(isServer: true);
+        }
+
+        private ConfigurationManager MakeCfgInternal(bool isServer)
         {
             var collection = new DependencyCollection();
             collection.RegisterInstance<IReplayRecordingManager>(new Mock<IReplayRecordingManager>().Object);
             collection.RegisterInstance<INetManager>(new Mock<INetManager>().Object);
-            collection.Register<ConfigurationManager, ServerNetConfigurationManager>();
-            collection.Register<IServerNetConfigurationManager, ServerNetConfigurationManager>();
+            collection.Register<ConfigurationManager, ConfigurationManager>();
+            collection.Register<IConfigurationManager, ConfigurationManager>();
             collection.Register<IGameTiming, GameTiming>();
             collection.Register<ILogManager, LogManager>();
             collection.BuildGraph();
 
-            return collection.Resolve<ConfigurationManager>();
+            var cfg = collection.Resolve<ConfigurationManager>();
+            cfg.Initialize(isServer);
+            return cfg;
+        }
+
+        [CVarDefs]
+        private static class TestForkCVars
+        {
+            public static readonly CVarDef<int> Shared =
+                CVarDef.Create("test.shared", 0, CVar.ARCHIVE);
+
+            public static readonly CVarDef<int> ForkSpecific =
+                CVarDef.Create("test.fork_specific", 0, CVar.ARCHIVE | CVar.FORK);
         }
     }
 }

Robust.Shared.Tests/Resources/WritableDirProviderTest.cs

@@ -59,5 +59,55 @@ public void TestParentAccessClamped()
             // ../ should get clamped to /.
             Assert.That(_dirProvider.ReadAllText(new ResPath("/../dummy")), Is.EqualTo("pranked"));
         }
+
+        [Test]
+        public void TestVirtualGetFullPath()
+        {
+            var provider = new VirtualWritableDirProvider();
+
+            Assert.That(provider.GetFullPath(new ResPath("/../foo/./bar")), Is.EqualTo(new ResPath("/foo/bar")));
+            Assert.That(() => provider.GetFullPath(new ResPath("foo/bar")), Throws.ArgumentException);
+        }
+
+        [Test]
+        public void TestVirtualParentAccessClamped()
+        {
+            var provider = new VirtualWritableDirProvider();
+
+            provider.WriteAllText(new ResPath("/dummy"), "pranked");
+
+            Assert.That(provider.ReadAllText(new ResPath("/../dummy")), Is.EqualTo("pranked"));
+        }
+
+        [Test]
+        public void TestVirtualFileMethodsUseFullPath()
+        {
+            var provider = new VirtualWritableDirProvider();
+
+            provider.CreateDir(new ResPath("/foo/../bar"));
+            provider.WriteAllText(new ResPath("/bar/file.txt"), "contents");
+
+            Assert.That(provider.Exists(new ResPath("/foo/../bar/file.txt")), Is.True);
+            Assert.That(provider.IsDir(new ResPath("/foo/../bar")), Is.True);
+            Assert.That(provider.DirectoryEntries(new ResPath("/foo/../bar")), Is.EquivalentTo(new[] {"file.txt"}));
+
+            provider.Rename(new ResPath("/foo/../bar/file.txt"), new ResPath("/foo/../bar/renamed.txt"));
+            Assert.That(provider.ReadAllText(new ResPath("/bar/renamed.txt")), Is.EqualTo("contents"));
+
+            provider.Delete(new ResPath("/foo/../bar/renamed.txt"));
+            Assert.That(provider.Exists(new ResPath("/bar/renamed.txt")), Is.False);
+        }
+
+        [Test]
+        public void TestVirtualOpenSubdirectoryUsesFullPath()
+        {
+            var provider = new VirtualWritableDirProvider();
+            provider.CreateDir(new ResPath("/bar"));
+
+            var subdirectory = provider.OpenSubdirectory(new ResPath("/foo/../bar"));
+            subdirectory.WriteAllText(new ResPath("/file.txt"), "contents");
+
+            Assert.That(provider.ReadAllText(new ResPath("/bar/file.txt")), Is.EqualTo("contents"));
+        }
     }
 }

Robust.Shared.Tests/Utility/ResPathTest.cs

@@ -126,6 +126,65 @@ public void RootedConversionsTest()
         Assert.That(relative.ToRootedPath(), Is.EqualTo(path));
     }
 
+    [Test]
+    [TestCase("/..", ExpectedResult = "/")]
+    [TestCase("/../..", ExpectedResult = "/")]
+    [TestCase("/../../a", ExpectedResult = "/a")]
+    [TestCase("/a/../../b", ExpectedResult = "/b")]
+    [TestCase("/a/b/../../../c", ExpectedResult = "/c")]
+    [TestCase("/a/./b//../c", ExpectedResult = "/a/c")]
+    public string CleanRootedTraversalDoesNotEscapeRoot(string input)
+    {
+        var cleaned = new ResPath(input).Clean();
+        Assert.That(cleaned.IsRooted, Is.True);
+        Assert.That(cleaned.CanonPath, Does.Not.StartWith("/.."));
+        return cleaned.ToString();
+    }
+
+    [Test]
+    [TestCase("..", ExpectedResult = "..")]
+    [TestCase("../a", ExpectedResult = "../a")]
+    [TestCase("a/../..", ExpectedResult = "..")]
+    [TestCase("a/../../b", ExpectedResult = "../b")]
+    public string CleanRelativeTraversalPreservesEscapes(string input)
+    {
+        return new ResPath(input).Clean().ToString();
+    }
+
+    [Test]
+    [TestCase("/Config/client_config.toml", true, ExpectedResult = true)]
+    [TestCase("/Config/../client_config.toml", true, ExpectedResult = false)]
+    [TestCase("client_config.toml", true, ExpectedResult = false)]
+    [TestCase("replay.zip", false, ExpectedResult = true)]
+    [TestCase("replays/replay.zip", false, ExpectedResult = true)]
+    [TestCase("../replay.zip", false, ExpectedResult = false)]
+    [TestCase("/replay.zip", false, ExpectedResult = false)]
+    [TestCase(".", false, ExpectedResult = false)]
+    [TestCase("/", true, ExpectedResult = false)]
+    public bool IsValidFilePathTest(string input, bool rooted)
+    {
+        return new ResPath(input).IsValidFilePath(rooted);
+    }
+
+    [Test]
+    [TestCase("fork", ExpectedResult = "fork")]
+    [TestCase("../bad\\fork", ExpectedResult = ".._bad_fork")]
+    [TestCase("bad/fork", ExpectedResult = "bad_fork")]
+    [TestCase("bad\\fork", ExpectedResult = "bad_fork")]
+    [TestCase("", ExpectedResult = "_")]
+    [TestCase(".", ExpectedResult = "_")]
+    [TestCase("..", ExpectedResult = "_")]
+    public string SanitizeFilenameTest(string input)
+    {
+        var sanitized = ResPath.SanitizeFilename(input);
+
+        Assert.That(ResPath.IsValidFilename(sanitized), Is.True);
+        Assert.That(sanitized, Does.Not.Contain('/'));
+        Assert.That(sanitized, Does.Not.Contain('\\'));
+
+        return sanitized;
+    }
+
     [Test]
     [TestCase("/a/b", "/a", ExpectedResult = "b")]
     [TestCase("/a", "/", ExpectedResult = "a")]

Robust.Shared/Configuration/CVar.cs

@@ -77,5 +77,10 @@ public enum CVar : short
         ///     Only the client can change this variable.
         /// </summary>
         CLIENT = 512,
+
+        /// <summary>
+        ///     Non-default values are saved to a fork-specific configuration file.
+        /// </summary>
+        FORK = 1024,
     }
 }

Robust.Shared/Configuration/CVarDef.cs

@@ -27,7 +27,6 @@ public abstract class CVarDef
         ///     The description of this CVar.
         /// </summary>
         public string? Desc { get; }
-
         private protected CVarDef(string name, object defaultValue, CVar flags, string? desc)
         {
             Name = name;