Merge pull request #34900 from bdach/invalid-filenames-in-imported-skins

Fix external edit operations failing due to invalid filenames

osu.Game.Tests/Skins/IO/ImportSkinTest.cs

@@ -294,6 +294,46 @@ namespace osu.Game.Tests.Skins.IO
 
         #endregion
 
+        /// <remarks>
+        /// Note that this test passing / failing is platform / OS-specific (if it is to fail, it'll fail on windows).
+        /// </remarks>
+        [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<SkinManager>();
+                    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<SkinInfo> import1, string name, string creator, decimal version, OsuGameBase osu)
         {
             import1.PerformRead(i =>

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();
             }
         }
     }

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)!);
 

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.
         /// </para>
         /// </remarks>
+        /// <seealso href="https://github.com/peppy/osu-stable-reference/blob/67795dba3c308e7d0493b296149dcb073ca47ecb/osu!common/Helpers/GeneralHelper.cs#L41-L46"/>
         public static string GetValidFilename(this string filename)
         {
             foreach (char c in invalid_filename_chars)