Merge pull request #38 from bdach/string-length-limits

Add user-facing validation for DB-enforced length limits of string properties

Author: peppy

osu.Server.BeatmapSubmission.Tests/BeatmapSubmissionControllerTest.cs

@@ -1136,6 +1136,31 @@ public async Task TestUploadFullPackage_FailsIfNonUnicodeMetadataHasMetadataChar
             Assert.Contains("Romanised title contains disallowed characters.", (await response.Content.ReadFromJsonAsync<ErrorResponse>())!.Error);
         }
 
+        [Fact]
+        public async Task TestUploadFullPackage_FailsIfMetadataTooLong()
+        {
+            using var db = await DatabaseAccess.GetConnectionAsync();
+            await db.ExecuteAsync("INSERT INTO `phpbb_users` (`user_id`, `username`, `username_clean`, `country_acronym`, `user_permissions`, `user_sig`, `user_occ`, `user_interests`) VALUES (1000, 'test', 'test', 'JP', '', '', '', '')");
+
+            await db.ExecuteAsync(@"INSERT INTO `osu_beatmapsets` (`beatmapset_id`, `user_id`, `creator`, `approved`, `thread_id`, `active`, `submit_date`) VALUES (241526, 1000, 'test user', -1, 0, -1, CURRENT_TIMESTAMP)");
+
+            foreach (uint beatmapId in new uint[] { 557810 })
+                await db.ExecuteAsync(@"INSERT INTO `osu_beatmaps` (`beatmap_id`, `user_id`, `beatmapset_id`, `approved`) VALUES (@beatmapId, 1000, 241526, -1)", new { beatmapId = beatmapId });
+
+            var request = new HttpRequestMessage(HttpMethod.Put, "/beatmapsets/241526");
+
+            using var content = new MultipartFormDataContent($"{Guid.NewGuid()}----");
+            using var stream = TestResources.GetResource("metadata-too-long.osz")!;
+            content.Add(new StreamContent(stream), "beatmapArchive", osz_filename);
+            request.Content = content;
+            request.Headers.Add(HeaderBasedAuthenticationHandler.USER_ID_HEADER, "1000");
+
+            var response = await Client.SendAsync(request);
+            Assert.False(response.IsSuccessStatusCode);
+            Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode);
+            Assert.Contains("Beatmap difficulty names must not exceed 80 characters.", (await response.Content.ReadFromJsonAsync<ErrorResponse>())!.Error);
+        }
+
         [Fact]
         public async Task TestPatchPackage()
         {

osu.Server.BeatmapSubmission.Tests/Resources/metadata-too-long.osz

@@ -0,0 +1,14 @@
+// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System.ComponentModel.DataAnnotations;
+using osu.Game.Beatmaps;
+
+namespace osu.Server.BeatmapSubmission.Models.Database.Validation
+{
+    public class RomanisedAttribute : ValidationAttribute
+    {
+        public override bool IsValid(object? value)
+            => value is string s && MetadataUtils.IsRomanised(s);
+    }
+}

osu.Server.BeatmapSubmission/Models/Database/Validation/RomanisedAttribute.cs

@@ -3,12 +3,16 @@
 
 // ReSharper disable InconsistentNaming
 
+using System.ComponentModel.DataAnnotations;
+
 namespace osu.Server.BeatmapSubmission.Models.Database
 {
     public class beatmapset_version_file
     {
         public ulong file_id { get; set; }
         public ulong version_id { get; set; }
+
+        [MaxLength(500, ErrorMessage = "Filenames cannot exceed 500 characters.")]
         public string filename { get; set; } = string.Empty;
     }
 }

osu.Server.BeatmapSubmission/Models/Database/beatmapset_version_file.cs

@@ -3,7 +3,9 @@
 
 // ReSharper disable InconsistentNaming
 
+using System.ComponentModel.DataAnnotations;
 using osu.Game.Beatmaps;
+using osu.Server.BeatmapSubmission.Models.Database.Validation;
 
 namespace osu.Server.BeatmapSubmission.Models.Database
 {
@@ -12,9 +14,16 @@ public class osu_beatmap
         public uint beatmap_id { get; set; }
         public uint? beatmapset_id { get; set; }
         public uint user_id { get; set; }
+
+        [MaxLength(150, ErrorMessage = "Beatmap difficulty filenames must not exceed 150 characters.")]
         public string? filename { get; set; }
+
         public string? checksum { get; set; }
+
+        [MaxLength(80, ErrorMessage = "Beatmap difficulty names must not exceed 80 characters.")]
+        [Romanised(ErrorMessage = "Difficulty name contains disallowed characters.")]
         public string version { get; set; } = string.Empty;
+
         public uint total_length { get; set; }
         public uint hit_length { get; set; }
         public uint countTotal { get; set; }

osu.Server.BeatmapSubmission/Models/Database/osu_beatmap.cs

@@ -4,7 +4,9 @@
 // ReSharper disable InconsistentNaming
 // ReSharper disable CollectionNeverQueried
 
+using System.ComponentModel.DataAnnotations;
 using osu.Game.Beatmaps;
+using osu.Server.BeatmapSubmission.Models.Database.Validation;
 
 namespace osu.Server.BeatmapSubmission.Models.Database
 {
@@ -13,13 +15,30 @@ public class osu_beatmapset
         public uint beatmapset_id { get; set; }
         public uint user_id { get; set; }
         public uint thread_id { get; set; }
+
+        [MaxLength(80, ErrorMessage = "Romanised artist cannot exceed 80 characters.")]
+        [Romanised(ErrorMessage = "Romanised artist contains disallowed characters.")]
         public string artist { get; set; } = string.Empty;
+
+        [MaxLength(80, ErrorMessage = "Artist cannot exceed 80 characters.")]
         public string? artist_unicode { get; set; }
+
+        [MaxLength(80, ErrorMessage = "Romanised title cannot exceed 80 characters.")]
+        [Romanised(ErrorMessage = "Romanised title contains disallowed characters.")]
         public string title { get; set; } = string.Empty;
+
+        [MaxLength(80, ErrorMessage = "Title cannot exceed 80 characters.")]
         public string? title_unicode { get; set; }
+
+        [MaxLength(80, ErrorMessage = "Author username cannot exceed 80 characters.")]
         public string creator { get; set; } = string.Empty;
+
+        [MaxLength(200, ErrorMessage = "Source cannot exceed 200 characters.")]
         public string source { get; set; } = string.Empty;
+
+        [MaxLength(1000, ErrorMessage = "Tags cannot exceed 1000 characters.")]
         public string tags { get; set; } = string.Empty;
+
         public bool video { get; set; }
         public bool storyboard { get; set; }
         public bool epilepsy { get; set; }

osu.Server.BeatmapSubmission/Models/Database/osu_beatmapset.cs

@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.ComponentModel.DataAnnotations;
 using System.Security.Cryptography;
 using osu.Framework.Extensions;
 using osu.Game.Beatmaps;
@@ -57,12 +58,9 @@ public BeatmapPackageParseResult Parse(uint beatmapSetId, ArchiveReader archiveR
 
                     if (beatmapContent.Beatmap.BeatmapInfo.BeatmapSet.OnlineID != beatmapSetId)
                         throw new InvariantException($"Beatmap has invalid beatmap set ID inside ({filename})");
-
-                    if (!MetadataUtils.IsRomanised(beatmapContent.Beatmap.BeatmapInfo.DifficultyName))
-                        throw new InvariantException($"Difficulty name \"{beatmapContent.Beatmap.BeatmapInfo.DifficultyName}\" contains disallowed characters.");
                 }
 
-                files.Add(new PackageFile(
+                var file = new PackageFile(
                     new beatmapset_file
                     {
                         sha2_hash = SHA256.HashData(stream),
@@ -73,7 +71,13 @@ public BeatmapPackageParseResult Parse(uint beatmapSetId, ArchiveReader archiveR
                         filename = filename,
                     },
                     beatmapContent
-                ));
+                );
+
+                var errors = new List<ValidationResult>();
+                if (!Validator.TryValidateObject(file.VersionFile, new ValidationContext(file.VersionFile), errors, validateAllProperties: true))
+                    throw new InvariantException(string.Join(Environment.NewLine, errors.Select(r => r.ErrorMessage)));
+
+                files.Add(file);
             }
 
             var beatmapSetRow = constructDatabaseRowForBeatmapset(beatmapSetId, archiveReader,
@@ -118,11 +122,9 @@ private osu_beatmapset constructDatabaseRowForBeatmapset(uint beatmapSetId, Arch
             if (creator != expectedCreator)
                 throw new InvariantException("At least one difficulty has a specified creator that isn't the beatmap host's username.");
 
-            if (!MetadataUtils.IsRomanised(result.artist))
-                throw new InvariantException("Romanised artist contains disallowed characters.");
-
-            if (!MetadataUtils.IsRomanised(result.title))
-                throw new InvariantException("Romanised title contains disallowed characters.");
+            var errors = new List<ValidationResult>();
+            if (!Validator.TryValidateObject(result, new ValidationContext(result), errors, validateAllProperties: true))
+                throw new InvariantException(string.Join(Environment.NewLine, errors.Select(r => r.ErrorMessage)));
 
             // TODO: maybe unnecessary?
             result.displaytitle = $"[bold:0,size:20]{result.artist_unicode}|{result.title_unicode}";
@@ -186,6 +188,11 @@ public osu_beatmap GetDatabaseRow()
             };
 
             countObjectsByType(Beatmap, result);
+
+            var errors = new List<ValidationResult>();
+            if (!Validator.TryValidateObject(result, new ValidationContext(result), errors, validateAllProperties: true))
+                throw new InvariantException(string.Join(Environment.NewLine, errors.Select(r => r.ErrorMessage)));
+
             return result;
         }