Properly validate posts added to the database from the API

BantFlags/Controllers/FlagsController.cs

@@ -33,23 +33,16 @@ namespace BantFlags.Controllers
         [ProducesResponseType(StatusCodes.Status400BadRequest)]
         public async Task<IActionResult> Get([FromForm]string post_nrs, [FromForm]string board, [FromForm]int? version)
         {
-            try // We only care if the post if valid.
-            {
-                int ver = version ?? 0;
+            int ver = version ?? 0;
 
-                if (ver > 1)
-                {
-                    // Improved data structuring, see Docs/GetPosts
-                    return Json(await Database.GetPosts_V2(post_nrs));
-                }
-                else
-                {
-                    return Json(await Database.GetPosts_V1(post_nrs));
-                }
+            if (ver > 1)
+            {
+                // Improved data structuring, see Docs/GetPosts
+                return Json(await Database.GetPosts_V2(post_nrs));
             }
-            catch (Exception e)
+            else
             {
-                return Problem(ErrorMessage(e), statusCode: StatusCodes.Status400BadRequest);
+                return Json(await Database.GetPosts_V1(post_nrs));
             }
         }
 
@@ -67,77 +60,23 @@ namespace BantFlags.Controllers
         [ProducesResponseType(StatusCodes.Status400BadRequest)]
         public async Task<IActionResult> Post([FromForm]string post_nr, [FromForm]string board, [FromForm]string regions, [FromForm]int? version)
         {
-            try // We only care if the post if valid.
-            {
-                string[] flags;
-                int ver = version ?? 0;
-
-                if (ver > 1)
-                {
-                    flags = regions.Split(",");
-                }
-                else
-                {
-                    flags = regions.Split("||");
-                }
-
-                // TODO: Currently we skip over invalid flags. Should we error instead?
-                // We can't easily format it like in the current bantflags - we really should continue to
-                // return "empty, or there were errors. Re-set your flags.", for compatibility, but we'd
-                // have to store that as a flag in the database and perform an expensive string comparison
-                // to stop people selecting it.
-                // Do we care if people select the broken flag?
-                var validFlags = flags.Where(x => Database.KnownFlags().Contains(x));
-
-                for (int i = 0; i < flags.Length; i++)
-                {
-                    if (!Database.KnownFlags().Contains(flags[i]))
-                    {
-                        flags[i] = "empty, or there were errors. Re-set your flags.";
-                    }
-                }
-
-                var numberOfFlags = validFlags.Count();
-                if (numberOfFlags <= 0 || numberOfFlags > 25)
-                {
-                    throw new ArgumentException("Your post didn't include any flags, or your flags were invalid.");
-                }
-
-                FlagModel post = new FlagModel
-                {
-                    PostNumber = int.TryParse(post_nr, out int temp) ? temp : throw new ArgumentException("Invalid post number."),
-                    Board = board == "bant" ? "bant" : throw new ArgumentException("Board parameter wasn't formatted correctly."),
-                    Flags = validFlags
-                };
+            string splitFlag = (version ?? 0) > 1 ? "," : "||"; // comma for v2+, else || for backwards compatibility.
 
-                await Database.InsertPost(post);
+            (bool isValid, FlagModel post, string errorMessage) = FlagModel.Create(post_nr, board, regions, splitFlag, Database.KnownFlags);
 
-                return Ok(post);
-            }
-            catch (Exception e)
+            if (!isValid)
             {
-                return Problem(detail: ErrorMessage(e), statusCode: StatusCodes.Status400BadRequest);
+                return Problem(errorMessage, statusCode: StatusCodes.Status400BadRequest);
             }
+
+            await Database.InsertPost(post);
+
+            return Ok(post);
         }
 
         [HttpGet]
         [Route("flags")]
         [ProducesResponseType(StatusCodes.Status200OK)]
-        public IActionResult Flags() => Ok(Database.FlagList());
-
-        /// <summary>
-        /// Creates an error mesage to send in case of 400 bad request, without giving away too much information.
-        /// </summary>
-        /// <param name="exception">Raw exception to be filtered.</param>
-        private string ErrorMessage(Exception exception) =>
-            exception switch
-            {
-                NullReferenceException _ => "Some data wasn't initialised. Are you sending everything?",
-                DbException _ => "Internal database error.",
-                ArgumentNullException _ => "No regions sent",
-                ArgumentException e => e.Message, // We create all arguement exceptions here, we can just pass the message on.
-                Exception e => e.Message, // Don't do this.
-                _ => "how in the hell"
-            }; // This needs more testing.
+        public IActionResult Flags() => Ok(Database.FlagList);
     }
 }

BantFlags/Data/Database/DatabaseService.cs

@@ -12,30 +12,24 @@ namespace BantFlags.Data.Database
     {
         private MySqlConnectionPool ConnectionPool { get; }
 
-        private string Flags { get; set; }
+        public string FlagList { get; private set; }
 
-        private HashSet<string> FlagsHash { get; set; }
+        public HashSet<string> KnownFlags { get; private set; }
 
         public DatabaseService(DatabaseServiceConfig dbConfig)
         {
             ConnectionPool = new MySqlConnectionPool(dbConfig.ConnectionString, dbConfig.PoolSize);
 
-            var flags = GetFlags().Result; // It's okay to error here since it's only initialised at startup.
-
-            Flags = string.Join("\n", flags);
-            FlagsHash = flags.ToHashSet();
+            UpdateKnownFlags().Wait(); // It's okay to deadlock here since it's only initialised at startup.
         }
 
-        public string FlagList() => Flags;
-
-        public HashSet<string> KnownFlags() => FlagsHash;
-
         public async Task UpdateKnownFlags()
         {
             var flags = await GetFlags();
+            flags.Remove("empty, or there were errors. Re-set your flags.");
 
-            Flags = string.Join("\n", flags);
-            FlagsHash = flags.ToHashSet();
+            FlagList = string.Join("\n", flags);
+            KnownFlags = flags.ToHashSet();
         }
 
         public async Task DeleteFlagsAsync(List<FormFlag> flags)
@@ -46,8 +40,6 @@ namespace BantFlags.Data.Database
             flags.ForEach(async f =>
                 await query.SetParam("@flag", f.Name)
                     .ExecuteNonQueryAsync(reuse: true));
-
-            return;
         }
 
         public async Task RenameFlagsAsync(List<RenameFlag> flags)
@@ -79,8 +71,6 @@ namespace BantFlags.Data.Database
                         .ExecuteNonQueryAsync(reuse: true));
                 }
             }
-
-            return;
         }
 
         /// <summary>
@@ -106,8 +96,6 @@ namespace BantFlags.Data.Database
             flags.ForEach(async f =>
                 await query.SetParam("@flag", f.Name)
                 .ExecuteNonQueryAsync(reuse: true));
-
-            return;
         }
     }
 

BantFlags/Data/Database/GetPosts.cs

@@ -31,7 +31,6 @@ namespace BantFlags.Data.Database
             List<Dictionary<string, string>> posts = new List<Dictionary<string, string>>();
 
             var x = await GetPosts(input);
-
             x.ForEach(x => posts.Add(new Dictionary<string, string>
                 {
                     {"post_nr", x.Key.ToString() },

BantFlags/Data/FlagModel.cs

@@ -1,13 +1,52 @@
 using System.Collections.Generic;
+using System.Linq;
 
 namespace BantFlags.Data
 {
     public class FlagModel
     {
-        public int PostNumber { get; set; }
+        public int PostNumber { get; private set; }
 
-        public string Board { get; set; }
+        public string Board { get; private set; }
 
-        public IEnumerable<string> Flags { get; set; }
+        public string[] Flags { get; private set; }
+
+        private FlagModel(int post_nr, string board, string[] flags)
+        {
+            PostNumber = post_nr;
+            Board = board;
+            Flags = flags;
+        }
+
+        /// <summary>
+        /// A wrapper around post validation so it's all in one place.
+        /// </summary>
+        public static (bool, FlagModel, string) Create(string post_nr, string board, string regions, string splitFlag, HashSet<string> knownFlags)
+        {
+            if (!int.TryParse(post_nr, out int postNumber))
+                return (false, default, "Invalid post number.");
+
+            if (board != "bant")
+                return (false, default, "Invalid board parameter.");
+
+            if (regions == null)
+                regions = "somebrokenflagstringsothatwegettheemptyflagwhenweshould";
+
+            var flags = regions.Split(splitFlag);
+
+            if (flags.Count() > 30)
+                return (false, default, "Too many flags.");
+
+            foreach (string flag in flags)
+            {
+                if (!knownFlags.Contains(flag)) // Not ideal but it's better than doing it in the controller / passing the database here.
+                {
+                    flags = new string[] { "empty, or there were errors. Re-set your flags." };
+                    break;
+                }
+            }
+
+            return (true, new FlagModel(postNumber, board, flags), default);
+        }
     }
 }

BantFlags/Data/Staging.cs

@@ -1,4 +1,5 @@
 using System.Collections.Generic;
+using System.Linq;
 
 namespace BantFlags.Data
 {
@@ -12,8 +13,15 @@ namespace BantFlags.Data
         public List<FormFlag> DeletedFlags { get; set; }
         public List<FormFlag> AddedFlags { get; set; }
 
+        /// <summary>
+        /// The current list of resolved flags including changes currently in Staging.
+        /// Exists here since it's a singleton.
+        /// </summary>
         public List<string> Flags { get; set; }
 
+        /// <summary>
+        /// Used for commiting and unstaging staged flags.
+        /// </summary>
         public string Password { get; }
 
         public Staging(string password)

BantFlags/Pages/Index.cshtml

@@ -10,4 +10,8 @@
 
 <a href="~/bantflags.user.js">Install Bantflags</a>
 <br />
-<a href="https://nineball.party/srsbsn/3521">Official Thread</a>
+<br  />
+<a href="https://nineball.party/srsbsn/3521">Official Thread</a>
+<br />
+<br />
+<a asp-page="Upload">Upload Flags</a>

BantFlags/Pages/Upload.cshtml

@@ -49,6 +49,8 @@
 <form method="post" asp-page-handler="Unstage">
     <label>Password:</label>
     <input type="text" name="password" />
+    <br />
+    <br />
     <button type="submit" onclick="window.confirm('Really unstage the selected flags?')">unstage</button>
     @if (Model.staging.AddedFlags.Any())
     {
@@ -115,17 +117,19 @@
     }
 </form>
 
+@section Head {
+    <link rel="stylesheet" href="~/upload.css" />
+}
+
 @section Scripts {
     @* Place flag image inside the <select> because ASP removes "invalid HTML" *@
     <script>
-        let x = document.getElementsByTagName('select')[0].children
-        Array.prototype.slice.call(x).forEach(function (e) {
-            var name = e.innerHTML;
-            e.innerHTML = "<img src=\"https://flags.plum.moe/flags/" + name + ".png\">" + name
-        });
+        window.addEventListener('load', function (e) {
+            let x = document.getElementsByTagName('select')[0].children
+            Array.prototype.slice.call(x).forEach(function (y) {
+                var name = y.innerHTML;
+                y.innerHTML = "<img src=\"https://flags.plum.moe/flags/" + name + ".png\">" + name
+            });
+        }, { once: true });
     </script>
-}
-
-@section Head {
-    <link rel="stylesheet" href="~/form.css" />
 }

BantFlags/Pages/Upload.cshtml.cs

@@ -14,6 +14,7 @@ using System.Threading.Tasks;
 
 namespace BantFlags
 {
+    // I don't know if I need these anymore.
     [RequestFormLimits(ValueCountLimit = 5000)]
     [IgnoreAntiforgeryToken(Order = 2000)]
     public class UploadModel : PageModel
@@ -21,7 +22,7 @@ namespace BantFlags
         private IWebHostEnvironment Env { get; }
         private DatabaseService Database { get; set; }
 
-        private readonly byte[] pngHeader = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
+        private readonly byte[] PNGHeader = new byte[] { 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A };
 
         private string FlagsPath { get; set; }
 
@@ -183,7 +184,7 @@ namespace BantFlags
                     return Page();
                 }
 
-                // TODO: maybe there's something no releasing memory here.
+                // TODO: maybe there's something no releasing memory here. - can't Directory.Move().
 
                 using var memoryStream = new MemoryStream();
                 await Upload.CopyToAsync(memoryStream);
@@ -215,7 +216,7 @@ namespace BantFlags
             }
             catch (Exception e)
             {
-                Message = $"Something went bang.\n{e.Message}";
+                Message = $"Something went bang.\n\n\n{e.Message}";
 
                 return Page();
             }
@@ -254,7 +255,7 @@ namespace BantFlags
                 {
                     reader.BaseStream.Position = 0;
 
-                    return reader.ReadBytes(pngHeader.Length).SequenceEqual(pngHeader);
+                    return reader.ReadBytes(PNGHeader.Length).SequenceEqual(PNGHeader);
                 }
             }
         }
@@ -268,8 +269,12 @@ namespace BantFlags
         private bool FileNameIsValid(string fileName) =>
             !(fileName == null
                 || fileName.Contains("||")
-                || Database.KnownFlags().Contains(fileName)
+                || fileName.Contains(",")
+                || Database.KnownFlags.Contains(fileName)
                 || staging.AddedFlags.Select(x => x.Name).Contains(fileName)
+                || staging.DeletedFlags.Select(x => x.Name).Contains(fileName)
+                || staging.RenamedFlags.Select(x => x.Name).Contains(fileName)
+                || staging.RenamedFlags.Select(x => x.NewName).Contains(fileName)
                 || fileName.Length > 100);
     }
 }

BantFlags/wwwroot/upload.css

@@ -19,6 +19,6 @@ h3 {
 .flag {
     margin: auto;
     width: 12%;
-    display: block;
+    display: inline-block;
     padding-bottom: 10px;
 }

bantflags.user.js

@@ -22,7 +22,7 @@
 
 // This script specifically targets ECMAScript 2015 (const, let, arrow functions). Update your hecking browser.
 
-// Change this if you want verbose debug information in the console.
+// Change this if you want verbose debuging information in the console.
 const debugMode = false;
 
 //
@@ -41,7 +41,7 @@ const api_get = 'api/get';
 const api_post = 'api/post';
 
 // If you increase this the server will ignore your post.
-const max_flags = 24;
+const max_flags = 30;
 
 var regions = []; // The flags we have selected.
 var postNrs = []; // all post numbers in the thread.
@@ -50,10 +50,10 @@ var postNrs = []; // all post numbers in the thread.
 // DO NOT EDIT ANYTHING IN THIS SCRIPT DIRECTLY - YOUR FLAGS SHOULD BE CONFIGURED USING THE CONFIGURATION BOXES
 //
 
-let elementsInClass = x => document.getElementsByClassName(x);
-let sliceCall = x => Array.prototype.slice.call(x);
-let firstChildInClass = (parent, className) => parent.getElementsByClassName(className)[0];
-let createAndAssign = (element, source) => Object.assign(document.createElement(element), source);
+const elementsInClass = x => document.getElementsByClassName(x);
+const sliceCall = x => Array.prototype.slice.call(x);
+const firstChildInClass = (parent, className) => parent.getElementsByClassName(className)[0];
+const createAndAssign = (element, source) => Object.assign(document.createElement(element), source);
 
 function addGlobalStyle(css) {
     let head = document.getElementsByTagName('head')[0];
@@ -93,7 +93,7 @@ function MakeRequest(method, url, data, func) {
 
 function retry(func, resp) {
     console.log("[BantFlags] Could not fetch flags, status: " + resp.status);
-    console.log(resp.statusText); // TODO: surely ASP.NET can return something more useful?
+    console.log(resp.statusText);
     setTimeout(func, requestRetryInterval);
 }
 
@@ -135,14 +135,13 @@ var nsetup = { // not anymore a clone of the original setup
                 flagLoad.style.display = 'none';
                 flagSelect.style.display = 'inline-block';
                 nsetup.flagsLoaded = true;
-                flagLoad.removeEventListener('click', nsetup.fillHtml);
             });
     },
     save: function (v) {
         GM_setValue(nsetup.namespace, v);
         regions = GM_getValue(nsetup.namespace);
     },
-    setFlag: function (flag) { // place a flag from the selector to the flags array variable and create an element in the flags_container div
+    setFlag: function (flag) {
         let UID = Math.random().toString(36).substring(7);
         let flagName = flag ? flag : document.getElementById("flagSelect").value;
         let flagContainer = document.getElementById("bantflags_container");
@@ -154,16 +153,18 @@ var nsetup = { // not anymore a clone of the original setup
             className: 'bantflags_flag'
         }));
 
-        if (flagContainer.children.length > max_flags) {
-            nsetup.ToggleFlagButton('off');
+        if (flagContainer.children.length >= max_flags) {
+            nsetup.toggleFlagButton('off');
         }
 
         document.getElementById(UID).addEventListener("click", function () {
+            console.log("removing flag");
             let flagToRemove = document.getElementById(UID);
 
             flagToRemove.parentNode.removeChild(flagToRemove);
             nsetup.toggleFlagButton('on');
             nsetup.save(nsetup.parse());
+            console.log("flag removed");
         });
 
         if (!flag) {
@@ -191,7 +192,7 @@ var nsetup = { // not anymore a clone of the original setup
         document.getElementById('append_flag_button').addEventListener('click',
             () => nsetup.flagsLoaded ? nsetup.setFlag() : alert('Load flags before adding them.'));
 
-        document.getElementById('flagLoad').addEventListener('click', nsetup.fillHtml);
+        document.getElementById('flagLoad').addEventListener('click', nsetup.fillHtml, { once: true });
     },
     parse: function () {
         let flagsArray = [];
@@ -203,7 +204,7 @@ var nsetup = { // not anymore a clone of the original setup
 
         return flagsArray;
     },
-    ToggleFlagButton: state => document.getElementById('append_flag_button').disabled = state === 'off' ? true : false
+    toggleFlagButton: state => document.getElementById('append_flag_button').disabled = state === 'off' ? true : false
 };
 
 /** Prompt to set region if regions is empty  */