Rewrite PasswordHash.Parse to work with ReadOnlySpans

MediaBrowser.Common/Cryptography/PasswordHash.cs

@@ -1,4 +1,5 @@
 #pragma warning disable CS1591
+#nullable enable
 
 using System;
 using System.Collections.Generic;
@@ -30,6 +31,16 @@ namespace MediaBrowser.Common.Cryptography
 
         public PasswordHash(string id, byte[] hash, byte[] salt, Dictionary<string, string> parameters)
         {
+            if (id == null)
+            {
+                throw new ArgumentNullException(nameof(id));
+            }
+
+            if (id.Length == 0)
+            {
+                throw new ArgumentException("String can't be empty", nameof(id));
+            }
+
             Id = id;
             _hash = hash;
             _salt = salt;
@@ -59,58 +70,109 @@ namespace MediaBrowser.Common.Cryptography
         /// <value>Return the hashed password.</value>
         public ReadOnlySpan<byte> Hash => _hash;
 
-        public static PasswordHash Parse(string hashString)
+        public static PasswordHash Parse(ReadOnlySpan<char> hashString)
         {
-            // The string should at least contain the hash function and the hash itself
-            string[] splitted = hashString.Split('$');
-            if (splitted.Length < 3)
+            if (hashString.IsEmpty)
+            {
+                throw new ArgumentException("String can't be empty", nameof(hashString));
+            }
+
+            if (hashString[0] != '$')
             {
-                throw new ArgumentException("String doesn't contain enough segments", nameof(hashString));
+                throw new FormatException("Hash string must start with a $");
             }
 
-            // Start at 1, the first index shouldn't contain any data
-            int index = 1;
+            // Ignore first $
+            hashString = hashString[1..];
 
-            // Name of the hash function
-            string id = splitted[index++];
+            int nextSegment = hashString.IndexOf('$');
+            if (hashString.IsEmpty || nextSegment == 0)
+            {
+                throw new FormatException("Hash string must contain a valid id");
+            }
+            else if (nextSegment == -1)
+            {
+                return new PasswordHash(hashString.ToString(), Array.Empty<byte>());
+            }
+
+            ReadOnlySpan<char> id = hashString[..nextSegment];
+            hashString = hashString[(nextSegment + 1)..];
+            Dictionary<string, string>? parameters = null;
+
+            nextSegment = hashString.IndexOf('$');
 
             // Optional parameters
-            Dictionary<string, string> parameters = new Dictionary<string, string>();
-            if (splitted[index].IndexOf('=', StringComparison.Ordinal) != -1)
+            ReadOnlySpan<char> parametersSpan = nextSegment == -1 ? hashString : hashString[..nextSegment];
+            if (parametersSpan.Contains('='))
             {
-                foreach (string paramset in splitted[index++].Split(','))
+                while (!parametersSpan.IsEmpty)
                 {
-                    if (string.IsNullOrEmpty(paramset))
+                    ReadOnlySpan<char> parameter;
+                    int index = parametersSpan.IndexOf(',');
+                    if (index == -1)
+                    {
+                        parameter = parametersSpan;
+                        parametersSpan = ReadOnlySpan<char>.Empty;
+                    }
+                    else
                     {
-                        continue;
+                        parameter = parametersSpan[..index];
+                        parametersSpan = parametersSpan[(index + 1)..];
                     }
 
-                    string[] fields = paramset.Split('=');
-                    if (fields.Length != 2)
+                    int splitIndex = parameter.IndexOf('=');
+                    if (splitIndex == -1 || splitIndex == 0 || splitIndex == parameter.Length - 1)
                     {
-                        throw new InvalidDataException($"Malformed parameter in password hash string {paramset}");
+                        throw new FormatException($"Malformed parameter in password hash string");
                     }
 
-                    parameters.Add(fields[0], fields[1]);
+                    (parameters ??= new Dictionary<string, string>()).Add(
+                        parameter[..splitIndex].ToString(),
+                        parameter[(splitIndex + 1)..].ToString());
+                }
+
+                if (nextSegment == -1)
+                {
+                    // parameters can't be null here
+                    return new PasswordHash(id.ToString(), Array.Empty<byte>(), Array.Empty<byte>(), parameters!);
                 }
+
+                hashString = hashString[(nextSegment + 1)..];
+                nextSegment = hashString.IndexOf('$');
+            }
+
+            if (nextSegment == 0)
+            {
+                throw new FormatException($"Hash string contains an empty segment");
             }
 
             byte[] hash;
             byte[] salt;
 
-            // Check if the string also contains a salt
-            if (splitted.Length - index == 2)
+            if (nextSegment == -1)
             {
-                salt = Convert.FromHexString(splitted[index++]);
-                hash = Convert.FromHexString(splitted[index++]);
+                salt = Array.Empty<byte>();
+                hash = Convert.FromHexString(hashString);
             }
             else
             {
-                salt = Array.Empty<byte>();
-                hash = Convert.FromHexString(splitted[index++]);
+                salt = Convert.FromHexString(hashString[..nextSegment]);
+                hashString = hashString[(nextSegment + 1)..];
+                nextSegment = hashString.IndexOf('$');
+                if (nextSegment != -1)
+                {
+                    throw new FormatException("Hash string contains too many segments");
+                }
+
+                if (hashString.IsEmpty)
+                {
+                    throw new FormatException("Hash segment is empty");
+                }
+
+                hash = Convert.FromHexString(hashString);
             }
 
-            return new PasswordHash(id, hash, salt, parameters);
+            return new PasswordHash(id.ToString(), hash, salt, parameters ?? new Dictionary<string, string>());
         }
 
         private void SerializeParameters(StringBuilder stringBuilder)
@@ -147,8 +209,13 @@ namespace MediaBrowser.Common.Cryptography
                     .Append(Convert.ToHexString(_salt));
             }
 
-            return str.Append('$')
-                .Append(Convert.ToHexString(_hash)).ToString();
+            if (_hash.Length != 0)
+            {
+                str.Append('$')
+                    .Append(Convert.ToHexString(_hash));
+            }
+
+            return str.ToString();
         }
     }
 }

tests/Jellyfin.Common.Tests/Cryptography/PasswordHashTests.cs

@@ -0,0 +1,139 @@
+using System;
+using System.Collections.Generic;
+using MediaBrowser.Common.Cryptography;
+using Xunit;
+
+namespace Jellyfin.Common.Tests.Cryptography
+{
+    public static class PasswordHashTests
+    {
+        [Fact]
+        public static void Ctor_Null_ThrowsArgumentNullException()
+        {
+            Assert.Throws<ArgumentNullException>(() => new PasswordHash(null!, Array.Empty<byte>()));
+        }
+
+        [Fact]
+        public static void Ctor_Empty_ThrowsArgumentException()
+        {
+            Assert.Throws<ArgumentException>(() => new PasswordHash(string.Empty, Array.Empty<byte>()));
+        }
+
+        public static IEnumerable<object[]> Parse_Valid_TestData()
+        {
+            yield return new object[]
+            {
+                "$PBKDF2",
+                new PasswordHash("PBKDF2", Array.Empty<byte>())
+            };
+
+            yield return new object[]
+            {
+                "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D",
+                new PasswordHash(
+                    "PBKDF2",
+                    Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"),
+                    Array.Empty<byte>(),
+                    new Dictionary<string, string>()
+                    {
+                        { "iterations", "1000" }
+                    })
+            };
+
+            yield return new object[]
+            {
+                "$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D",
+                new PasswordHash(
+                    "PBKDF2",
+                    Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"),
+                    Array.Empty<byte>(),
+                    new Dictionary<string, string>()
+                    {
+                        { "iterations", "1000" },
+                        { "m", "120" }
+                    })
+            };
+
+            yield return new object[]
+            {
+                "$PBKDF2$iterations=1000,m=120$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D",
+                new PasswordHash(
+                    "PBKDF2",
+                    Convert.FromHexString("62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D"),
+                    Convert.FromHexString("69F420"),
+                    new Dictionary<string, string>()
+                    {
+                        { "iterations", "1000" },
+                        { "m", "120" }
+                    })
+            };
+
+            yield return new object[]
+            {
+                "$PBKDF2$iterations=1000,m=120",
+                new PasswordHash(
+                    "PBKDF2",
+                    Array.Empty<byte>(),
+                    Array.Empty<byte>(),
+                    new Dictionary<string, string>()
+                    {
+                        { "iterations", "1000" },
+                        { "m", "120" }
+                    })
+            };
+        }
+
+        [Theory]
+        [MemberData(nameof(Parse_Valid_TestData))]
+        public static void Parse_Valid_Success(string passwordHashString, PasswordHash expected)
+        {
+            var passwordHash = PasswordHash.Parse(passwordHashString);
+            Assert.Equal(expected.Id, passwordHash.Id);
+            Assert.Equal(expected.Parameters, passwordHash.Parameters);
+            Assert.Equal(expected.Salt.ToArray(), passwordHash.Salt.ToArray());
+            Assert.Equal(expected.Hash.ToArray(), passwordHash.Hash.ToArray());
+            Assert.Equal(expected.ToString(), passwordHash.ToString());
+        }
+
+        [Theory]
+        [InlineData("$PBKDF2")]
+        [InlineData("$PBKDF2$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
+        [InlineData("$PBKDF2$69F420$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
+        [InlineData("$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
+        [InlineData("$PBKDF2$iterations=1000,m=120$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
+        [InlineData("$PBKDF2$iterations=1000,m=120")]
+        public static void ToString_Roundtrip_Success(string passwordHash)
+        {
+            Assert.Equal(passwordHash, PasswordHash.Parse(passwordHash).ToString());
+        }
+
+        [Fact]
+        public static void Parse_Null_ThrowsArgumentException()
+        {
+            Assert.Throws<ArgumentException>(() => PasswordHash.Parse(null));
+        }
+
+        [Fact]
+        public static void Parse_Empty_ThrowsArgumentException()
+        {
+            Assert.Throws<ArgumentException>(() => PasswordHash.Parse(string.Empty));
+        }
+
+        [Theory]
+        [InlineData("$")] // No id
+        [InlineData("$$")] // Empty segments
+        [InlineData("PBKDF2$")] // Doesn't start with $
+        [InlineData("$PBKDF2$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty segment
+        [InlineData("$PBKDF2$iterations=1000$$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Empty salt segment
+        [InlineData("$PBKDF2$=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter
+        [InlineData("$PBKDF2$=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter
+        [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")] // Invalid parmeter
+        [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$")] // Ends on $
+        [InlineData("$PBKDF2$iterations=$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D$anotherone")] // Extra segment
+        [InlineData("$PBKDF2$69F420$")] // Empty hash
+        public static void Parse_InvalidFormat_ThrowsFormatException(string passwordHash)
+        {
+            Assert.Throws<FormatException>(() => PasswordHash.Parse(passwordHash));
+        }
+    }
+}

tests/Jellyfin.Common.Tests/PasswordHashTests.cs

@@ -1,31 +0,0 @@
-using System;
-using MediaBrowser.Common;
-using MediaBrowser.Common.Cryptography;
-using Xunit;
-
-namespace Jellyfin.Common.Tests
-{
-    public class PasswordHashTests
-    {
-        [Theory]
-        [InlineData(
-            "$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D",
-            "PBKDF2",
-            "",
-            "62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
-        public void ParseTest(string passwordHash, string id, string salt, string hash)
-        {
-            var pass = PasswordHash.Parse(passwordHash);
-            Assert.Equal(id, pass.Id);
-            Assert.Equal(salt, Convert.ToHexString(pass.Salt));
-            Assert.Equal(hash, Convert.ToHexString(pass.Hash));
-        }
-
-        [Theory]
-        [InlineData("$PBKDF2$iterations=1000$62FBA410AFCA5B4475F35137AB2E8596B127E4D927BA23F6CC05C067E897042D")]
-        public void ToStringTest(string passwordHash)
-        {
-            Assert.Equal(passwordHash, PasswordHash.Parse(passwordHash).ToString());
-        }
-    }
-}