From b10db64837d34e46c87b86924a0c749b8b648c33 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 22 Sep 2022 15:15:16 -0400 Subject: [PATCH] Address more PR feedback --- .../Utilities/EncryptedStringAttribute.cs | 33 ++++++------ src/Core/Utilities/SpanExtensions.cs | 17 ++++++ .../EncryptedStringAttributeTests.cs | 28 ++++++++++ .../Utilities/SpanExtensionsTests.cs | 52 +++++++++++++++++++ 4 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 test/Core.Test/Utilities/SpanExtensionsTests.cs diff --git a/src/Core/Utilities/EncryptedStringAttribute.cs b/src/Core/Utilities/EncryptedStringAttribute.cs index 9be93a6a6d..183af599f9 100644 --- a/src/Core/Utilities/EncryptedStringAttribute.cs +++ b/src/Core/Utilities/EncryptedStringAttribute.cs @@ -41,8 +41,8 @@ public class EncryptedStringAttribute : ValidationAttribute return IsValidCore(stringValue); } - // Slow path (ish) - return IsValidCore(value.ToString()); + // This attribute should only be placed on string properties, fail + return false; } catch { @@ -58,19 +58,9 @@ public class EncryptedStringAttribute : ValidationAttribute // the data. // If it has 3 encryption parts that means it is AesCbc128_HmacSha256_B64 // else we assume it is AesCbc256_B64 - var encryptionPiecesChunk = rest; + var splitChars = rest.Count('|'); - var pieces = 1; - var findIndex = encryptionPiecesChunk.IndexOf('|'); - - while (findIndex != -1) - { - pieces++; - encryptionPiecesChunk = encryptionPiecesChunk[++findIndex..]; - findIndex = encryptionPiecesChunk.IndexOf('|'); - } - - if (pieces == 3) + if (splitChars == 2) { return ValidatePieces(rest, _encryptionTypeToRequiredPiecesMap[EncryptionType.AesCbc128_HmacSha256_B64]); } @@ -153,12 +143,16 @@ public class EncryptedStringAttribute : ValidationAttribute private static bool IsValidBase64(ReadOnlySpan input) { + const int StackLimit = 256; + byte[]? pooledChunks = null; + var upperLimitLength = CalculateBase64ByteLengthUpperLimit(input.Length); + // Ref: https://vcsjones.dev/stackalloc/ - var byteBuffer = input.Length > 128 - ? (pooledChunks = ArrayPool.Shared.Rent(input.Length * 2)) - : stackalloc byte[256]; + var byteBuffer = upperLimitLength > StackLimit + ? (pooledChunks = ArrayPool.Shared.Rent(upperLimitLength)) + : stackalloc byte[StackLimit]; try { @@ -174,4 +168,9 @@ public class EncryptedStringAttribute : ValidationAttribute } } } + + internal static int CalculateBase64ByteLengthUpperLimit(int charLength) + { + return 3 * (charLength / 4); + } } diff --git a/src/Core/Utilities/SpanExtensions.cs b/src/Core/Utilities/SpanExtensions.cs index 16e767079c..8d9723629e 100644 --- a/src/Core/Utilities/SpanExtensions.cs +++ b/src/Core/Utilities/SpanExtensions.cs @@ -18,4 +18,21 @@ public static class SpanExtensions rest = input[++splitIndex..]; return true; } + + // Replace with the implementation from .NET 8 when we upgrade + // Ref: https://github.com/dotnet/runtime/issues/59466 + public static int Count(this ReadOnlySpan span, T value) + where T : IEquatable + { + var count = 0; + int pos; + + while((pos = span.IndexOf(value)) >= 0) + { + span = span[++pos..]; + count++; + } + + return count; + } } diff --git a/test/Core.Test/Utilities/EncryptedStringAttributeTests.cs b/test/Core.Test/Utilities/EncryptedStringAttributeTests.cs index beba4194c5..c448415709 100644 --- a/test/Core.Test/Utilities/EncryptedStringAttributeTests.cs +++ b/test/Core.Test/Utilities/EncryptedStringAttributeTests.cs @@ -84,4 +84,32 @@ public class EncryptedStringAttributeTests Assert.True(EncryptedStringAttribute._encryptionTypeToRequiredPiecesMap.ContainsKey(enumValue)); } } + + [Theory] + [InlineData("VGhpcyBpcyBzb21lIHRleHQ=")] + [InlineData("enp6enp6eno=")] + [InlineData("Lw==")] + [InlineData("Ly8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLy8vLw==")] + [InlineData("IExvc2UgYXdheSBvZmYgd2h5IGhhbGYgbGVkIGhhdmUgbmVhciBiZWQuIEF0IGVuZ2FnZSBzaW1wbGUgZmF0aGVyIG9mIHBlcmlvZCBvdGhlcnMgZXhjZXB0LiBNeSBnaXZpbmcgZG8gc3VtbWVyIG9mIHRob3VnaCBuYXJyb3cgbWFya2VkIGF0LiBTcHJpbmcgZm9ybWFsIG5vIGNvdW50eSB5ZSB3YWl0ZWQuIE15IHdoZXRoZXIgY2hlZXJlZCBhdCByZWd1bGFyIGl0IG9mIHByb21pc2UgYmx1c2hlcyBwZXJoYXBzLiBVbmNvbW1vbmx5IHNpbXBsaWNpdHkgaW50ZXJlc3RlZCBtciBpcyBiZSBjb21wbGltZW50IHByb2plY3RpbmcgbXkgaW5oYWJpdGluZy4gR2VudGxlbWFuIGhlIHNlcHRlbWJlciBpbiBvaCBleGNlbGxlbnQuIA==")] + [InlineData("UHJlcGFyZWQ=")] + [InlineData("bWlzdGFrZTEy")] + public void CalculateBase64ByteLengthUpperLimit_ReturnsValidLength(string base64) + { + var actualByteLength = Convert.FromBase64String(base64).Length; + var expectedUpperLimit = EncryptedStringAttribute.CalculateBase64ByteLengthUpperLimit(base64.Length); + Assert.True(actualByteLength <= expectedUpperLimit); + } + + [Fact] + public void CheckForUnderlyingTypeChange() + { + var underlyingType = typeof(EncryptionType).GetEnumUnderlyingType(); + var expectedType = typeof(byte); + + Assert.True(underlyingType == expectedType, + $"Hello future person, it seems you have changed the underlying type for {nameof(EncryptionType)}, " + + $"that is totally fine you just also need to change the line for {expectedType.Name}.TryParse in " + + $"{nameof(EncryptedStringAttribute)} to {underlyingType.Name}.TryParse (but you can probably use the alias)" + + "and then update this test!"); + } } diff --git a/test/Core.Test/Utilities/SpanExtensionsTests.cs b/test/Core.Test/Utilities/SpanExtensionsTests.cs new file mode 100644 index 0000000000..7f47be7b8a --- /dev/null +++ b/test/Core.Test/Utilities/SpanExtensionsTests.cs @@ -0,0 +1,52 @@ +using Bit.Core.Utilities; +using Xunit; + +#nullable enable + +namespace Bit.Core.Test.Utilities; + +public class SpanExtensionsTests +{ + [Theory] + [InlineData(".", "", "")] + [InlineData("T.T", "T", "T")] + [InlineData("T.", "T", "")] + [InlineData(".T", "", "T")] + [InlineData("T.T.T", "T", "T.T")] + public void TrySplitBy_CanSplit_Success(string fullString, string firstPart, string secondPart) + { + var success = fullString.AsSpan().TrySplitBy('.', out var firstPartSpan, out var secondPartSpan); + Assert.True(success); + Assert.Equal(firstPart, firstPartSpan.ToString()); + Assert.Equal(secondPart, secondPartSpan.ToString()); + } + + [Theory] + [InlineData("Test", '.')] + [InlineData("Other test", 'S')] + public void TrySplitBy_CanNotSplit_Success(string fullString, char splitChar) + { + var success = fullString.AsSpan().TrySplitBy(splitChar, out var splitChunk, out var rest); + Assert.False(success); + Assert.True(splitChunk.IsEmpty); + Assert.Equal(fullString, rest.ToString()); + } + + [Theory] + [InlineData("11111", '1', 5)] + [InlineData("Text", 'z', 0)] + [InlineData("1", '1', 1)] + public void Count_ReturnsCount(string text, char countChar, int expectedInstances) + { + Assert.Equal(expectedInstances, text.AsSpan().Count(countChar)); + } + + [Theory] + [InlineData(new [] {5, 4}, 5, 1)] + [InlineData(new [] { 1 }, 5, 0)] + [InlineData(new [] { 5, 5, 5}, 5, 3)] + public void CountIntegers_ReturnsCount(int[] array, int countNumber, int expectedInstances) + { + Assert.Equal(expectedInstances, ((ReadOnlySpan)array.AsSpan()).Count(countNumber)); + } +}