diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index cb28b9ac8..9c2f3e9ef 100644 --- a/RELEASE-NOTES.md +++ b/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 diff --git a/Robust.Client/GameController/GameController.cs b/Robust.Client/GameController/GameController.cs index 1071ea0df..5cfd401cc 100644 --- a/Robust.Client/GameController/GameController.cs +++ b/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()); diff --git a/Robust.Client/GameControllerOptions.cs b/Robust.Client/GameControllerOptions.cs index d386b3906..ee11b36db 100644 --- a/Robust.Client/GameControllerOptions.cs +++ b/Robust.Client/GameControllerOptions.cs @@ -24,7 +24,7 @@ public sealed class GameControllerOptions /// /// Name of the configuration file in the user data directory. /// - 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. /// diff --git a/Robust.Client/Replays/Commands/ReplayLoadCommand.cs b/Robust.Client/Replays/Commands/ReplayLoadCommand.cs index 0b1390b2d..9a814ecca 100644 --- a/Robust.Client/Replays/Commands/ReplayLoadCommand.cs +++ b/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))); diff --git a/Robust.Shared.IntegrationTests/Configuration/ConfigurationManagerTest.cs b/Robust.Shared.IntegrationTests/Configuration/ConfigurationManagerTest.cs index ef84ada71..a774f6121 100644 --- a/Robust.Shared.IntegrationTests/Configuration/ConfigurationManagerTest.cs +++ b/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("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(new Mock().Object); collection.RegisterInstance(new Mock().Object); - collection.Register(); - collection.Register(); + collection.Register(); + collection.Register(); collection.Register(); collection.Register(); collection.BuildGraph(); - return collection.Resolve(); + var cfg = collection.Resolve(); + cfg.Initialize(isServer); + return cfg; + } + + [CVarDefs] + private static class TestForkCVars + { + public static readonly CVarDef Shared = + CVarDef.Create("test.shared", 0, CVar.ARCHIVE); + + public static readonly CVarDef ForkSpecific = + CVarDef.Create("test.fork_specific", 0, CVar.ARCHIVE | CVar.FORK); } } } diff --git a/Robust.Shared.Tests/Resources/WritableDirProviderTest.cs b/Robust.Shared.Tests/Resources/WritableDirProviderTest.cs index 892836231..63ce51536 100644 --- a/Robust.Shared.Tests/Resources/WritableDirProviderTest.cs +++ b/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")); + } } } diff --git a/Robust.Shared.Tests/Utility/ResPathTest.cs b/Robust.Shared.Tests/Utility/ResPathTest.cs index bb46211fc..4ce9b5d11 100644 --- a/Robust.Shared.Tests/Utility/ResPathTest.cs +++ b/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")] diff --git a/Robust.Shared/Configuration/CVar.cs b/Robust.Shared/Configuration/CVar.cs index 479c55eba..f6ccf2f7c 100644 --- a/Robust.Shared/Configuration/CVar.cs +++ b/Robust.Shared/Configuration/CVar.cs @@ -77,5 +77,10 @@ public enum CVar : short /// Only the client can change this variable. /// CLIENT = 512, + + /// + /// Non-default values are saved to a fork-specific configuration file. + /// + FORK = 1024, } } diff --git a/Robust.Shared/Configuration/CVarDef.cs b/Robust.Shared/Configuration/CVarDef.cs index d9bb66707..0330cfd36 100644 --- a/Robust.Shared/Configuration/CVarDef.cs +++ b/Robust.Shared/Configuration/CVarDef.cs @@ -27,7 +27,6 @@ public abstract class CVarDef /// The description of this CVar. /// public string? Desc { get; } - private protected CVarDef(string name, object defaultValue, CVar flags, string? desc) { Name = name; diff --git a/Robust.Shared/Configuration/ConfigurationManager.cs b/Robust.Shared/Configuration/ConfigurationManager.cs index 72844ae3d..a6e781dc6 100644 --- a/Robust.Shared/Configuration/ConfigurationManager.cs +++ b/Robust.Shared/Configuration/ConfigurationManager.cs @@ -8,6 +8,7 @@ using System.Threading; using Nett; using Robust.Shared.Collections; +using Robust.Shared.ContentPack; using Robust.Shared.IoC; using Robust.Shared.Log; using Robust.Shared.Timing; @@ -27,6 +28,7 @@ internal partial class ConfigurationManager : IConfigurationManagerInternal private const char TABLE_DELIMITER = '.'; protected readonly Dictionary _configVars = new(); private ConfigFileStorage? _configFile; + private ConfigFileStorage? _forkConfigFile; protected bool _isServer; protected readonly ReaderWriterLockSlim Lock = new(); @@ -55,6 +57,7 @@ public virtual void Shutdown() _configVars.Clear(); _configFile = null; + _forkConfigFile = null; } /// @@ -223,14 +226,56 @@ public HashSet LoadFromFile(string configFile) } } + public HashSet LoadFromFile(IWritableDirProvider provider, ResPath path) + { + if (!path.IsValidFilePath(rooted: true)) + throw new ArgumentException("Config load path must be rooted, point to a file, and not contain relative traversal.", nameof(path)); + + try + { + HashSet result; + using (var file = provider.OpenRead(path)) + { + result = LoadFromTomlStream(file); + } + + SetSaveFile(provider, path); + ApplyRollback(); + _sawmill.Info("Configuration loaded from file '{0}'.", path); + return result; + } + catch (Exception e) + { + _sawmill.Error("Unable to load configuration file '{0}':\n{1}", path, e); + return new HashSet(0); + } + } + public void SetSaveFile(string configFile) { _configFile = new ConfigFileStorageDisk { Path = configFile }; } + public void SetSaveFile(IWritableDirProvider provider, ResPath path) + { + if (!path.IsValidFilePath(rooted: true)) + throw new ArgumentException("Config save path must be rooted, point to a file, and not contain relative traversal.", nameof(path)); + + _configFile = new ConfigFileStorageWritableDir { Provider = provider, Path = path }; + } + + public void SetForkSaveFile(IWritableDirProvider provider, ResPath path) + { + if (!path.IsValidFilePath(rooted: true)) + throw new ArgumentException("Config save path must be rooted, point to a file, and not contain relative traversal.", nameof(path)); + + _forkConfigFile = new ConfigFileStorageWritableDir { Provider = provider, Path = path }; + } + public void SetVirtualConfig() { _configFile = new ConfigFileStorageVirtual(); + _forkConfigFile = new ConfigFileStorageVirtual(); } public void CheckUnusedCVars() @@ -356,33 +401,30 @@ public void SaveToFile() // Always write if it was present when reading from the config file, otherwise: // Don't write if Archive flag is not set. // Don't write if the cVar is the default value. - var cvars = _configVars.Where(x => x.Value.ConfigModified - || ((x.Value.Flags & CVar.ARCHIVE) != 0 && x.Value.Value != null && - !x.Value.Value.Equals(x.Value.DefaultValue))).Select(x => x.Key); + var cvars = _configVars + .Where(x => ShouldSaveCVar(x.Value) && (x.Value.Flags & CVar.FORK) == 0) + .Select(x => x.Key) + .ToArray(); // Write in-memory to avoid bulldozing config file on exception. var memoryStream = new MemoryStream(); SaveToTomlStream(memoryStream, cvars); memoryStream.Position = 0; - switch (_configFile) + SaveStreamToStorage(_configFile, memoryStream); + + if (_forkConfigFile != null) { - case ConfigFileStorageDisk disk: - { - using var file = File.Create(disk.Path); - memoryStream.CopyTo(file); - break; - } - case ConfigFileStorageVirtual @virtual: - { - @virtual.Stream.SetLength(0); - memoryStream.CopyTo(@virtual.Stream); - break; - } - default: - { - throw new UnreachableException(); - } + var forkCvars = _configVars + .Where(x => ShouldSaveCVar(x.Value) + && (x.Value.Flags & CVar.FORK) != 0) + .Select(x => x.Key) + .ToArray(); + + var forkMemoryStream = new MemoryStream(); + SaveToTomlStream(forkMemoryStream, forkCvars); + forkMemoryStream.Position = 0; + SaveStreamToStorage(_forkConfigFile, forkMemoryStream); } _sawmill.Info($"config saved to '{_configFile}'."); @@ -569,6 +611,58 @@ public void LoadCVarsFromType(Type containingType) } } + private bool ShouldSaveCVar(ConfigVar cVar) + { + if (!cVar.Registered) + return false; + + if (!_isServer && (cVar.Flags & (CVar.SERVER | CVar.SERVERONLY)) != 0) + return false; + + if (_isServer && (cVar.Flags & (CVar.CLIENT | CVar.CLIENTONLY)) != 0) + return false; + + return cVar.ConfigModified + || ((cVar.Flags & CVar.ARCHIVE) != 0 + && cVar.Value != null + && !cVar.Value.Equals(cVar.DefaultValue)); + } + + private static void SaveStreamToStorage(ConfigFileStorage storage, MemoryStream memoryStream) + { + switch (storage) + { + case ConfigFileStorageDisk disk: + { + var directory = Path.GetDirectoryName(disk.Path); + if (!string.IsNullOrEmpty(directory)) + Directory.CreateDirectory(directory); + + using var file = File.Create(disk.Path); + memoryStream.CopyTo(file); + break; + } + case ConfigFileStorageVirtual @virtual: + { + // Trim it just in case. + @virtual.Stream.SetLength(0); + memoryStream.CopyTo(@virtual.Stream); + break; + } + case ConfigFileStorageWritableDir writable: + { + writable.Provider.CreateDir(writable.Path.Directory); + using var file = writable.Provider.OpenWrite(writable.Path); + memoryStream.CopyTo(file); + break; + } + default: + { + throw new UnreachableException(); + } + } + } + /// public bool IsCVarRegistered(string name) { @@ -1051,6 +1145,17 @@ public override string ToString() } } + private sealed class ConfigFileStorageWritableDir : ConfigFileStorage + { + public required IWritableDirProvider Provider; + public required ResPath Path; + + public override string ToString() + { + return Path.ToString(); + } + } + private sealed class ConfigFileStorageVirtual : ConfigFileStorage { // I did not realize when adding this class that there is currently no way to *load* this data again. diff --git a/Robust.Shared/Configuration/IConfigurationManagerInternal.cs b/Robust.Shared/Configuration/IConfigurationManagerInternal.cs index f603cdd99..6f232b085 100644 --- a/Robust.Shared/Configuration/IConfigurationManagerInternal.cs +++ b/Robust.Shared/Configuration/IConfigurationManagerInternal.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Reflection; +using Robust.Shared.ContentPack; +using Robust.Shared.Utility; namespace Robust.Shared.Configuration { @@ -29,11 +31,28 @@ internal interface IConfigurationManagerInternal : IConfigurationManager /// the full name of the config file. HashSet LoadFromFile(string configFile); + /// + /// Sets up the ConfigurationManager and loads a TOML configuration file from a writable directory. + /// + /// The writable directory provider to load from. + /// The rooted path to the config file. + HashSet LoadFromFile(IWritableDirProvider provider, ResPath path); + /// /// Specifies the location where the config file should be saved, without trying to load from it. /// void SetSaveFile(string configFile); + /// + /// Specifies the virtual location where the config file should be saved, without trying to load from it. + /// + void SetSaveFile(IWritableDirProvider provider, ResPath path); + + /// + /// Specifies the virtual location where fork-specific CVars should be saved, without trying to load from it. + /// + void SetForkSaveFile(IWritableDirProvider provider, ResPath path); + /// /// Check the list of CVars to make sure there's no unused CVars set, which might indicate a typo or such. /// diff --git a/Robust.Shared/ContentPack/VirtualWritableDirProvider.cs b/Robust.Shared/ContentPack/VirtualWritableDirProvider.cs index 534029786..a976cda3d 100644 --- a/Robust.Shared/ContentPack/VirtualWritableDirProvider.cs +++ b/Robust.Shared/ContentPack/VirtualWritableDirProvider.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.IO; +using Robust.Shared.Log; using Robust.Shared.Utility; namespace Robust.Shared.ContentPack @@ -30,12 +31,8 @@ private VirtualWritableDirProvider(DirectoryNode node) public void CreateDir(ResPath path) { - if (!path.IsRooted) - { - throw new ArgumentException("Path must be rooted", nameof(path)); - } - - path = path.Clean(); + path = GetFullPath(path); + LogVfs("Creating virtual directory '{0}'.", path); var directory = _rootDirectoryNode; @@ -61,12 +58,8 @@ public void CreateDir(ResPath path) public void Delete(ResPath path) { - if (!path.IsRooted) - { - throw new ArgumentException("Path must be rooted", nameof(path)); - } - - path = path.Clean(); + path = GetFullPath(path); + LogVfs("Deleting virtual node '{0}'.", path); var pathParent = path.Directory; @@ -104,10 +97,8 @@ public bool IsDir(ResPath path) public Stream Open(ResPath path, FileMode fileMode, FileAccess access, FileShare share) { - if (!path.IsRooted) - { - throw new ArgumentException("Path must be rooted", nameof(path)); - } + path = GetFullPath(path); + LogVfs("Opening virtual file '{0}' with mode {1}, access {2}, share {3}.", path, fileMode, access, share); var parentPath = path.Directory; if (!TryGetNodeAt(parentPath, out var parent) || !(parent is DirectoryNode parentDir)) @@ -206,11 +197,9 @@ public Stream Open(ResPath path, FileMode fileMode, FileAccess access, FileShare public void Rename(ResPath oldPath, ResPath newPath) { - if (!oldPath.IsRooted) - throw new ArgumentException("Path must be rooted", nameof(oldPath)); - - if (!newPath.IsRooted) - throw new ArgumentException("Path must be rooted", nameof(newPath)); + oldPath = GetFullPath(oldPath); + newPath = GetFullPath(newPath); + LogVfs("Renaming virtual node '{0}' to '{1}'.", oldPath, newPath); if (!TryGetNodeAt(oldPath.Directory, out var parent) || parent is not DirectoryNode sourceDir) throw new ArgumentException("Source directory does not exist."); @@ -237,12 +226,7 @@ public void OpenOsWindow(ResPath path) private bool TryGetNodeAt(ResPath path, [NotNullWhen(true)] out INode? node) { - if (!path.IsRooted) - { - throw new ArgumentException("Path must be rooted", nameof(path)); - } - - path = path.Clean(); + path = GetFullPath(path); if (path == ResPath.Root) { @@ -281,6 +265,28 @@ public IWritableDirProvider OpenSubdirectory(ResPath path) return new VirtualWritableDirProvider(dirNode); } + public ResPath GetFullPath(ResPath path) + { + if (!path.IsRooted) + { + throw new ArgumentException("Path must be rooted", nameof(path)); + } + + return path.Clean(); + } + + private static void LogVfs(string message, params object?[] args) + { + try + { + Logger.GetSawmill("res.vfs").Info(message, args); + } + catch + { + // VirtualWritableDirProvider is usable in tests and tools without an IoC log manager. + } + } + private interface INode { } diff --git a/Robust.Shared/ContentPack/WritableDirProvider.cs b/Robust.Shared/ContentPack/WritableDirProvider.cs index 8f1632c88..16acadcd3 100644 --- a/Robust.Shared/ContentPack/WritableDirProvider.cs +++ b/Robust.Shared/ContentPack/WritableDirProvider.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.IO; using System.Threading; +using Robust.Shared.Log; using Robust.Shared.Utility; namespace Robust.Shared.ContentPack @@ -34,6 +35,7 @@ public WritableDirProvider(DirectoryInfo rootDir, bool hideRootDir) public void CreateDir(ResPath path) { var fullPath = GetFullPath(path); + LogVfs("Creating directory '{0}'.", path); Directory.CreateDirectory(fullPath); } @@ -43,10 +45,12 @@ public void Delete(ResPath path) var fullPath = GetFullPath(path); if (Directory.Exists(fullPath)) { + LogVfs("Deleting directory '{0}'.", path); Directory.Delete(fullPath, true); } else if (File.Exists(fullPath)) { + LogVfs("Deleting file '{0}'.", path); File.Delete(fullPath); } } @@ -115,6 +119,7 @@ public bool IsDir(ResPath path) public Stream Open(ResPath path, FileMode fileMode, FileAccess access, FileShare share) { var fullPath = GetFullPath(path); + LogVfs("Opening file '{0}' with mode {1}, access {2}, share {3}.", path, fileMode, access, share); return File.Open(fullPath, fileMode, access, share); } @@ -132,6 +137,7 @@ public void Rename(ResPath oldPath, ResPath newPath) { var fullOldPath = GetFullPath(oldPath); var fullNewPath = GetFullPath(newPath); + LogVfs("Renaming file '{0}' to '{1}'.", oldPath, newPath); File.Move(fullOldPath, fullNewPath); } @@ -187,5 +193,17 @@ public string GetFullPath(ResPath path) return PathHelpers.SafeGetResourcePath(RootDir, path); } + + private static void LogVfs(string message, params object?[] args) + { + try + { + Logger.GetSawmill("res.vfs").Info(message, args); + } + catch + { + // WritableDirProvider is usable in tests and tools without an IoC log manager. + } + } } } diff --git a/Robust.Shared/Replays/SharedReplayRecordingManager.cs b/Robust.Shared/Replays/SharedReplayRecordingManager.cs index aff88e9ca..a621326db 100644 --- a/Robust.Shared/Replays/SharedReplayRecordingManager.cs +++ b/Robust.Shared/Replays/SharedReplayRecordingManager.cs @@ -170,7 +170,9 @@ public virtual bool TryStartRecording( UpdateWriteTasks(); name ??= DefaultReplayFileName(); - var filePath = new ResPath(name).Clean(); + var filePath = new ResPath(name); + if (!filePath.IsValidFilePath(rooted: false)) + throw new ArgumentException("Replay file name must be relative, point to a file, and not contain relative traversal.", nameof(name)); if (filePath.Extension != "zip") filePath = filePath.WithName(filePath.Filename + ".zip"); diff --git a/Robust.Shared/Utility/ResPath.cs b/Robust.Shared/Utility/ResPath.cs index 1dc2126f0..d63b2c030 100644 --- a/Robust.Shared/Utility/ResPath.cs +++ b/Robust.Shared/Utility/ResPath.cs @@ -92,6 +92,27 @@ public static bool IsValidFilename([NotNullWhen(true)] string? filename) && filename != "." && filename != ".."; + /// + /// Converts arbitrary text into a single safe filename segment for use in resource paths. + /// Does NOT validate against traversal, only consider filesystem naming paths. + /// + public static string SanitizeFilename(string filename) + { + var builder = new StringBuilder(filename.Length); + + foreach (var c in filename) + { + builder.Append(c is '/' or '\\' || Array.IndexOf(Path.GetInvalidFileNameChars(), c) != -1 + ? '_' + : c); + } + + var sanitized = builder.ToString(); + return IsValidFilename(sanitized) + ? sanitized + : "_"; + } + /// /// Returns true if the path is equal to "." /// @@ -673,4 +694,31 @@ public static string[] EnumerateSegments(this ResPath path) ? path.CanonPath[1..].Split(ResPath.Separator) : path.CanonPath.Split(ResPath.Separator); } + + /// + /// Checks whether this path points to a file and does not contain relative traversal. + /// + /// Path to validate. + /// If true, the path must be rooted. If false, the path must be relative. + public static bool IsValidFilePath(this ResPath path, bool rooted) + { + if (rooted != path.IsRooted) + return false; + + if (path == ResPath.Root || path == ResPath.Self || path == ResPath.Empty) + return false; + + if (path.Clean() != path) + return false; + + // Clean() intentionally preserves base-level relative traversal like "../foo". + // Those paths are normalized, but still not safe file paths. + foreach (var segment in path.EnumerateSegments()) + { + if (segment == "..") + return false; + } + + return path.Filename is not ("." or ".."); + } }