diff --git a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs index 62e7a80435..f909638333 100644 --- a/osu.Game.Tests/Skins/IO/ImportSkinTest.cs +++ b/osu.Game.Tests/Skins/IO/ImportSkinTest.cs @@ -294,6 +294,46 @@ 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 + + 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; + } + finally + { + host.Exit(); + } + } + } + private void assertCorrectMetadata(Live import1, string name, string creator, decimal version, OsuGameBase osu) { import1.PerformRead(i => 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/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)!); 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)