From 6399f7e3db1645d6d9ea4a2cf3c083a77169d1c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 3 Sep 2025 08:59:33 +0200 Subject: [PATCH 1/4] Add failing test case --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 33 +++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 62e7a80435..66e286cf4f 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -294,6 +294,39 @@ namespace osu.Game.Tests.Skins.IO #endregion + /// + /// Note that this test passing / failing is platform / OS-specific (if it is to fail, it'll fail on windows). + /// + [Test] + public async Task TestExternallyMountingImportWithInvalidFilename() + { + using (HeadlessGameHost host = new CleanRunHeadlessGameHost()) + { + try + { + var osu = LoadOsuIntoHost(host); + + var zipStream = new MemoryStream(); + using var zip = ZipArchive.Create(); + zip.AddEntry("test?.png", new MemoryStream(new byte[] { 0xDE, 0xAD, 0xBE, 0xEF })); + zip.SaveTo(zipStream); + + var import = await loadSkinIntoOsu(osu, new ImportTask(zipStream, "test skin.osk")); + + var skinManager = osu.Dependencies.Get(); + var externalEdit = await skinManager.BeginExternalEditing(import.PerformRead(s => s.Detach())); // should not fail + + Task finishTask = Task.CompletedTask; + host.UpdateThread.Scheduler.Add(() => finishTask = externalEdit.Finish()); + await finishTask; + } + finally + { + host.Exit(); + } + } + } + private void assertCorrectMetadata(Live import1, string name, string creator, decimal version, OsuGameBase osu) { import1.PerformRead(i => From 51e75934462d78bb83122d67b54b6e2d258c2a66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 3 Sep 2025 08:59:57 +0200 Subject: [PATCH 2/4] Fix external edit operations failing due to invalid filenames --- osu.Game/Database/RealmArchiveModelImporter.cs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/osu.Game/Database/RealmArchiveModelImporter.cs b/osu.Game/Database/RealmArchiveModelImporter.cs index a3cdc2dc77..0f9832578b 100644 --- a/osu.Game/Database/RealmArchiveModelImporter.cs +++ b/osu.Game/Database/RealmArchiveModelImporter.cs @@ -208,7 +208,16 @@ namespace osu.Game.Database foreach (var realmFile in model.Files) { string sourcePath = Files.Storage.GetFullPath(realmFile.File.GetStoragePath()); - string destinationPath = Path.Join(mountedPath, realmFile.Filename); + // there are edge cases where externalising an imported model to the filesystem could fail due to invalid filenames. + // one scenario where this happens goes something like this: + // - stable user exports an archive, which contains filenames that get mangled by stable's default zip encoding codepage (Shift-JIS) + // - said archive is imported to lazer, but the invalid filename is not actually an issue due to lazer file store structure + // (the file is stored under a filename correspondent to its SHA instead, and its real filename is only stored in realm) + // - however attempts to externally edit the model fail as the external edit attempts and fails to produce the file's "real" filename in the mounted path + // to prevent this bricking external edit, strip invalid characters on external edit. + // the presumption here is that whatever produced the mangled archive is primarily at fault here, and we're just trying to trudge on locally as best as possible. + // if there are further troubles related to similar issues, reevaluate moving this sort of check to the import side instead (sanitising filenames on import from archive). + string destinationPath = Path.Join(mountedPath, realmFile.Filename.GetValidFilename()); Directory.CreateDirectory(Path.GetDirectoryName(destinationPath)!); From 5c66998c57850993b671ab1f17185908f91721ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 3 Sep 2025 09:32:43 +0200 Subject: [PATCH 3/4] Replace local copy of `GetValidFilename()` with direct usage Noticed in passing. --- osu.Game/Beatmaps/WorkingBeatmapCache.cs | 26 +++++------------------- osu.Game/Extensions/ModelExtensions.cs | 1 + 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/osu.Game/Beatmaps/WorkingBeatmapCache.cs b/osu.Game/Beatmaps/WorkingBeatmapCache.cs index 30bbbbc1fe..9957935977 100644 --- a/osu.Game/Beatmaps/WorkingBeatmapCache.cs +++ b/osu.Game/Beatmaps/WorkingBeatmapCache.cs @@ -20,6 +20,7 @@ using osu.Framework.Platform; using osu.Framework.Statistics; using osu.Game.Beatmaps.Formats; using osu.Game.Database; +using osu.Game.Extensions; using osu.Game.IO; using osu.Game.Skinning; using osu.Game.Storyboards; @@ -341,27 +342,10 @@ namespace osu.Game.Beatmaps { // Matches stable implementation, because it's probably simpler than trying to do anything else. // This may need to be reconsidered after we begin storing storyboards in the new editor. - return windowsFilenameStrip( - (metadata.Artist.Length > 0 ? metadata.Artist + @" - " + metadata.Title : Path.GetFileNameWithoutExtension(metadata.AudioFile)) - + (metadata.Author.Username.Length > 0 ? @" (" + metadata.Author.Username + @")" : string.Empty) - + @".osb"); - - string windowsFilenameStrip(string entry) - { - // Inlined from Path.GetInvalidFilenameChars() to ensure the windows characters are used (to match stable). - char[] invalidCharacters = - { - '\x00', '\x01', '\x02', '\x03', '\x04', '\x05', '\x06', '\x07', - '\x08', '\x09', '\x0A', '\x0B', '\x0C', '\x0D', '\x0E', '\x0F', '\x10', '\x11', '\x12', - '\x13', '\x14', '\x15', '\x16', '\x17', '\x18', '\x19', '\x1A', '\x1B', '\x1C', '\x1D', - '\x1E', '\x1F', '\x22', '\x3C', '\x3E', '\x7C', ':', '*', '?', '\\', '/' - }; - - foreach (char c in invalidCharacters) - entry = entry.Replace(c.ToString(), string.Empty); - - return entry; - } + string baseFilename = (metadata.Artist.Length > 0 ? metadata.Artist + @" - " + metadata.Title : Path.GetFileNameWithoutExtension(metadata.AudioFile)) + + (metadata.Author.Username.Length > 0 ? @" (" + metadata.Author.Username + @")" : string.Empty) + + @".osb"; + return baseFilename.GetValidFilename(); } } } diff --git a/osu.Game/Extensions/ModelExtensions.cs b/osu.Game/Extensions/ModelExtensions.cs index 18c991297a..7c9d929999 100644 --- a/osu.Game/Extensions/ModelExtensions.cs +++ b/osu.Game/Extensions/ModelExtensions.cs @@ -175,6 +175,7 @@ namespace osu.Game.Extensions /// DO NOT CHANGE THE SEMANTICS OF THIS METHOD unless you know well what you are doing. /// /// + /// public static string GetValidFilename(this string filename) { foreach (char c in invalid_filename_chars) From a840e55977aed7510ee8610035b34b670a87f7f4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 3 Sep 2025 17:14:45 +0900 Subject: [PATCH 4/4] Check exported filenames for added safety --- osu.Game.Tests/Skins/IO/ImportSkinTest.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 66e286cf4f..f909638333 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -316,6 +316,13 @@ namespace osu.Game.Tests.Skins.IO var skinManager = osu.Dependencies.Get(); var externalEdit = await skinManager.BeginExternalEditing(import.PerformRead(s => s.Detach())); // should not fail + Assert.That(Directory.Exists(externalEdit.MountedPath)); + Assert.That(new DirectoryInfo(externalEdit.MountedPath).GetFiles().Select(f => f.Name), Is.EquivalentTo(new[] + { + "skin.ini", + "test.png" + })); + Task finishTask = Task.CompletedTask; host.UpdateThread.Scheduler.Add(() => finishTask = externalEdit.Finish()); await finishTask;