From 28de6050a02d82f586bb64694b9ec01b2df4a9b9 Mon Sep 17 00:00:00 2001 From: David Bond Date: Sun, 27 Sep 2026 22:32:28 +0100 Subject: [PATCH 1/2] OPS-157454: Upload test coverage to Codacy from CI Adds a coverage job to CI that runs the offline tests with Microsoft.Testing.Extensions.CodeCoverage and uploads the Cobertura report to Codacy, on the pdl-public runner. Excluded from CI (need credentials or a live service): nothing; all 19 tests run offline, so no filter is applied. Local line coverage with the CI filter, in a clean Linux container: 98.8%. REAL BUG FOUND, DO NOT MERGE YET: in the local run HashingTests.Hash_StringEmpty_Succeeds failed with CryptographicException 'Concurrent operations from multiple threads on this type are not supported'. EncryptionService holds a single 'private readonly static SHA256 _hashAlgorithm' (EncryptionService.cs line 17) that GetHash shares across threads; SHA256 instances are not thread-safe, so parallel callers (here, parallel tests) race. This is a genuine thread-safety defect in the library, intermittent, which is why the existing Test step on main passes most of the time. The fix is to use SHA256.HashData(...) (static, thread-safe) instead of the shared instance; that is a product change and is left for a human. Also sets UseMicrosoftTestingPlatformRunner in the test project, without which the xunit v3 test executable rejects --coverage. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/ci.yml | 62 +++++++++++++++++++ .../PanoramicData.Encryption.Test.csproj | 1 + coverage.config | 33 ++++++++++ 3 files changed, 96 insertions(+) create mode 100644 coverage.config diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e118710..d7789a6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -39,6 +39,68 @@ jobs: name: packages path: ./artifacts/*.nupkg + # Test coverage, uploaded to Codacy (OPS-157454). + # pdl-public, never a GitHub-hosted runner and never pdl-prod-cluster: this repository is public, + # so a fork pull request is untrusted code, and pdl-public is the hardened scale set that may run + # it (no dind, no privileged container, no hostPath, nothing shared between jobs). + coverage: + runs-on: [pdl-public] + + env: + DOTNET_NOLOGO: 1 + DOTNET_CLI_TELEMETRY_OPTOUT: 1 + # The actions-runner image does not offer the Node version several of these actions request + # by default; without this they fail before their first step runs. + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true + + steps: + - uses: actions/checkout@v7 + with: + fetch-depth: 0 # avoid shallow clone so nbgv can do its work. + + - name: Setup .NET + uses: actions/setup-dotnet@v6 + # The runner user cannot write /usr/share/dotnet, so install the SDK somewhere it can. + env: + DOTNET_INSTALL_DIR: ${{ runner.temp }}/dotnet + with: + dotnet-version: 10.0.x + + - name: Build tests + run: dotnet build PanoramicData.Encryption.Test/PanoramicData.Encryption.Test.csproj --configuration Debug + + # The test project is a Microsoft.Testing.Platform application, so coverage is collected by + # running the test executable directly (Microsoft.Testing.Extensions.CodeCoverage) rather than + # through `dotnet test`. On Linux the executable has no .exe extension. + # + # Every test in this repository runs offline with no credentials, so no filter is applied. + - name: Run tests with coverage + run: >- + ./PanoramicData.Encryption.Test/bin/Debug/net10.0/PanoramicData.Encryption.Test + --coverage + --coverage-settings "$GITHUB_WORKSPACE/coverage.config" + --coverage-output-format cobertura + --coverage-output coverage.cobertura.xml + + # continue-on-error: a Codacy outage must not turn a passing test run red. The trade-off is that + # an expired or missing project token fails silently, so check the log for + # "Coverage received successfully" when in doubt. + - name: Upload coverage to Codacy + continue-on-error: true + uses: codacy/codacy-coverage-reporter-action@v1 + with: + project-token: ${{ secrets.CODACY_PROJECT_TOKEN }} + coverage-reports: PanoramicData.Encryption.Test/bin/Debug/net10.0/TestResults/coverage.cobertura.xml + + - name: Upload coverage artifact + if: always() + continue-on-error: true + uses: actions/upload-artifact@v7 + with: + name: coverage + path: PanoramicData.Encryption.Test/bin/Debug/net10.0/TestResults/coverage.cobertura.xml + retention-days: 7 + publish: needs: build runs-on: ubuntu-latest diff --git a/PanoramicData.Encryption.Test/PanoramicData.Encryption.Test.csproj b/PanoramicData.Encryption.Test/PanoramicData.Encryption.Test.csproj index b1a3405..198f2e4 100644 --- a/PanoramicData.Encryption.Test/PanoramicData.Encryption.Test.csproj +++ b/PanoramicData.Encryption.Test/PanoramicData.Encryption.Test.csproj @@ -2,6 +2,7 @@ net10.0 + true latest-recommended false enable diff --git a/coverage.config b/coverage.config new file mode 100644 index 0000000..cf54802 --- /dev/null +++ b/coverage.config @@ -0,0 +1,33 @@ + + + + + + + .*Test\.dll$ + .*Tests\.dll$ + + + + + ^System\.Diagnostics\.CodeAnalysis\.ExcludeFromCodeCoverageAttribute$ + ^System\.Runtime\.CompilerServices\.CompilerGeneratedAttribute$ + ^System\.CodeDom\.Compiler\.GeneratedCodeAttribute$ + ^System\.ObsoleteAttribute$ + + + + + .*\\obj\\.* + + + False + True + + From 178fd6f5480587be98fe21b2ddae6834d5471d1a Mon Sep 17 00:00:00 2001 From: David Bond Date: Mon, 28 Sep 2026 13:14:59 +0100 Subject: [PATCH 2/2] OPS-157454: Make EncryptionService thread-safe EncryptionService held static shared SHA256, Aes and RandomNumberGenerator instances. HashAlgorithm is not thread-safe, so concurrent GetHash calls intermittently threw "Concurrent operations from multiple threads on this type are not supported" and could return wrong hashes. - GetHash now uses the stateless SHA256.HashData one-shot API and Convert.ToHexString (uppercase, identical to the previous "X2" output). - Salt generation uses the static RandomNumberGenerator.GetBytes(16). - Encrypt/Decrypt create and dispose their own Aes instance and ICryptoTransform per call instead of sharing a static Aes, which also fixes the undisposed transforms. Algorithm, mode, padding, key/IV handling, encoding and output format are unchanged. New tests pin known hash and cipher outputs (published SHA-256 vectors and independently computed AES-256-CBC values) and hash, encrypt and decrypt from many threads, asserting every result equals the single-threaded one. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../ConcurrencyTests.cs | 98 +++++++++++++++++++ .../PinnedOutputTests.cs | 72 ++++++++++++++ PanoramicData.Encryption/EncryptionService.cs | 29 +++--- 3 files changed, 184 insertions(+), 15 deletions(-) create mode 100644 PanoramicData.Encryption.Test/ConcurrencyTests.cs create mode 100644 PanoramicData.Encryption.Test/PinnedOutputTests.cs diff --git a/PanoramicData.Encryption.Test/ConcurrencyTests.cs b/PanoramicData.Encryption.Test/ConcurrencyTests.cs new file mode 100644 index 0000000..8ddde54 --- /dev/null +++ b/PanoramicData.Encryption.Test/ConcurrencyTests.cs @@ -0,0 +1,98 @@ +using System.Collections.Concurrent; + +namespace PanoramicData.Encryption.Test; + +/// +/// Regression tests for OPS-157454: EncryptionService previously shared a single static SHA256 +/// instance across threads, which intermittently threw "Concurrent operations from multiple +/// threads on this type are not supported" and could silently return wrong hashes. +/// Each test runs many operations concurrently and asserts every result equals the +/// single-threaded result. +/// +public class ConcurrencyTests +{ + private const int Iterations = 5000; + private const string EncryptionKey = "00112233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF"; + private const string Salt = "0f0e0d0c0b0a09080706050403020100"; + + private static readonly ParallelOptions Options = new() + { + MaxDegreeOfParallelism = Math.Max(8, Environment.ProcessorCount * 2) + }; + + private static readonly string[] Inputs = + [ + string.Empty, + "abc", + "The quick brown fox jumps over the lazy dog", + "café € 100 🔒", + new string('x', 10_000), + ]; + + /// + /// Hashing from many threads at once returns the single-threaded result every time. + /// + [Fact] + public void GetHash_Concurrent_MatchesSingleThreadedResult() + { + var expected = Inputs.Select(EncryptionService.GetHash).ToArray(); + var actual = new string[Iterations]; + + Parallel.For(0, Iterations, Options, i => actual[i] = EncryptionService.GetHash(Inputs[i % Inputs.Length])); + + for (var i = 0; i < Iterations; i++) + { + actual[i].Should().Be(expected[i % Inputs.Length], "iteration {0} must match the single-threaded hash", i); + } + } + + /// + /// Encrypting and decrypting from many threads at once, on one shared service instance and + /// across instances, returns the single-threaded result every time. + /// + [Fact] + public void EncryptDecrypt_Concurrent_MatchesSingleThreadedResult() + { + var shared = new EncryptionService(EncryptionKey); + var expected = Inputs.Select(input => shared.Encrypt(input, Salt).cipherText).ToArray(); + var cipherTexts = new string[Iterations]; + var plainTexts = new string[Iterations]; + + Parallel.For(0, Iterations, Options, i => + { + var service = i % 2 == 0 ? shared : new EncryptionService(EncryptionKey); + var input = Inputs[i % Inputs.Length]; + cipherTexts[i] = service.Encrypt(input, Salt).cipherText; + plainTexts[i] = service.Decrypt(cipherTexts[i], Salt); + }); + + for (var i = 0; i < Iterations; i++) + { + cipherTexts[i].Should().Be(expected[i % Inputs.Length], "iteration {0} must match the single-threaded cipher text", i); + plainTexts[i].Should().Be(Inputs[i % Inputs.Length], "iteration {0} must round-trip", i); + } + } + + /// + /// Randomly generated salts produced from many threads at once are well formed, distinct, + /// and round-trip. + /// + [Fact] + public void Encrypt_RandomSaltConcurrent_ProducesDistinctSaltsThatRoundTrip() + { + var service = new EncryptionService(EncryptionKey); + var salts = new ConcurrentBag(); + + Parallel.For(0, Iterations, Options, i => + { + var input = Inputs[i % Inputs.Length]; + var (cipherText, salt) = service.Encrypt(input); + service.Decrypt(cipherText, salt).Should().Be(input); + salts.Add(salt); + }); + + salts.Should().HaveCount(Iterations); + salts.Should().OnlyHaveUniqueItems(); + salts.Should().AllSatisfy(salt => salt.Should().MatchRegex("^[0-9a-f]{32}$")); + } +} diff --git a/PanoramicData.Encryption.Test/PinnedOutputTests.cs b/PanoramicData.Encryption.Test/PinnedOutputTests.cs new file mode 100644 index 0000000..e41d136 --- /dev/null +++ b/PanoramicData.Encryption.Test/PinnedOutputTests.cs @@ -0,0 +1,72 @@ +namespace PanoramicData.Encryption.Test; + +/// +/// Pins the exact output of the library for known inputs, so that any change to the input +/// encoding, the algorithm, the padding or the output format (hex, casing) fails a test. +/// The expected values are published SHA-256 test vectors where one exists, and otherwise were +/// computed independently with the .NET BCL (SHA256.HashData / Aes.EncryptCbc over UTF-8). +/// +public class PinnedOutputTests +{ + private const string EncryptionKey = "00112233445566778899AABBCCDDEEFF00112233445566778899AABBCCDDEEFF"; + private const string Salt = "0f0e0d0c0b0a09080706050403020100"; + + // "café € 100 🔒": two-, three- and four-byte UTF-8 sequences, including a surrogate pair. + private const string UnicodeText = "café € 100 🔒"; + + /// + /// GetHash returns the uppercase hex SHA-256 of the UTF-8 bytes of the input. + /// + [Theory] + [InlineData("", "E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855")] + [InlineData("abc", "BA7816BF8F01CFEA414140DE5DAE2223B00361A396177A9CB410FF61F20015AD")] + [InlineData("The quick brown fox jumps over the lazy dog", "D7A8FBB307D7809469CA9ABCB0082E4F8D5651E46D3CDB762D02D0BF37C9E592")] + [InlineData(UnicodeText, "91FD866D1271995C1AD7BC61B1970441991C32696B88F2ED75CB874B996D88F7")] + public void GetHash_KnownInput_ReturnsPinnedValue(string input, string expected) + => EncryptionService.GetHash(input).Should().Be(expected); + + /// + /// Encrypting with a fixed key and salt returns pinned lowercase hex cipher text (AES-256-CBC, + /// PKCS7 padding, UTF-8 plaintext), and the salt is returned unchanged in lowercase hex. + /// + [Theory] + [InlineData("", "2258814885f97e0e539cc0613346d8f4")] + [InlineData("Hello, World!", "3ce17eea6f106637d64736e7ed1280f1")] + [InlineData(UnicodeText, "08514b73e16093df81a1feea30eaae85efe306b05dc057f56172f0fbf88b3766")] + public void Encrypt_KnownInput_ReturnsPinnedValue(string plainText, string expectedCipherText) + { + var service = new EncryptionService(EncryptionKey); + + var (cipherText, salt) = service.Encrypt(plainText, Salt); + + cipherText.Should().Be(expectedCipherText); + salt.Should().Be(Salt); + service.Decrypt(cipherText, salt).Should().Be(plainText); + } + + /// + /// Decrypting pinned cipher text returns the original plaintext, so data encrypted by earlier + /// versions of the library remains readable. + /// + [Theory] + [InlineData("2258814885f97e0e539cc0613346d8f4", "")] + [InlineData("3ce17eea6f106637d64736e7ed1280f1", "Hello, World!")] + [InlineData("08514B73E16093DF81A1FEEA30EAAE85EFE306B05DC057F56172F0FBF88B3766", UnicodeText)] + public void Decrypt_PinnedCipherText_ReturnsPlainText(string cipherText, string expectedPlainText) + => new EncryptionService(EncryptionKey) + .Decrypt(cipherText, Salt) + .Should() + .Be(expectedPlainText); + + /// + /// A randomly generated salt is a 32 character lowercase hex string (16 bytes). + /// + [Fact] + public void Encrypt_RandomSalt_IsLowercaseHexOf16Bytes() + { + var (_, salt) = new EncryptionService(EncryptionKey).Encrypt("Hello, World!"); + + salt.Should().HaveLength(32); + salt.Should().MatchRegex("^[0-9a-f]{32}$"); + } +} diff --git a/PanoramicData.Encryption/EncryptionService.cs b/PanoramicData.Encryption/EncryptionService.cs index a3bb68d..6358f22 100644 --- a/PanoramicData.Encryption/EncryptionService.cs +++ b/PanoramicData.Encryption/EncryptionService.cs @@ -11,10 +11,11 @@ public class EncryptionService { private readonly byte[] _key; - private readonly static RandomNumberGenerator _random = RandomNumberGenerator.Create(); + // UTF8Encoding is stateless for GetBytes/GetString and is safe to share across threads. + // No HashAlgorithm, SymmetricAlgorithm, ICryptoTransform or RandomNumberGenerator instance is + // shared: those types are not thread-safe (OPS-157454), so each call uses a static one-shot + // API or its own short-lived instance. private readonly static UTF8Encoding _encoder = new(); - private readonly static Aes _aes = Aes.Create(); - private readonly static SHA256 _hashAlgorithm = SHA256.Create(); /// @@ -33,9 +34,7 @@ public EncryptionService(string encryptionKey) private static byte[] GenerateVector() { - var vector = new byte[16]; - _random.GetBytes(vector); - return vector; + return RandomNumberGenerator.GetBytes(16); } #pragma warning disable CA5401 // Symmetric encryption uses non-default initialization vector, which could be potentially repeatable @@ -47,7 +46,8 @@ private static byte[] GenerateVector() public (string cipherText, string salt) Encrypt(string unencrypted) { var vector = GenerateVector(); - var encryptor = _aes.CreateEncryptor(_key, vector); + using var aes = Aes.Create(); + using var encryptor = aes.CreateEncryptor(_key, vector); return (ByteArrayToHexString(Transform(_encoder.GetBytes(unencrypted), encryptor)), ByteArrayToHexString(vector)); } @@ -69,7 +69,8 @@ private static byte[] GenerateVector() } var vector = HexStringToByteArray(salt); - var encryptor = _aes.CreateEncryptor(_key, vector); + using var aes = Aes.Create(); + using var encryptor = aes.CreateEncryptor(_key, vector); return (ByteArrayToHexString(Transform(_encoder.GetBytes(unencrypted), encryptor)), ByteArrayToHexString(vector)); } #pragma warning restore CA5401 @@ -94,7 +95,8 @@ public string Decrypt(string encryptedString, string salt) var encrypted = HexStringToByteArray(encryptedString); - var decryptor = _aes.CreateDecryptor(_key, vector); + using var aes = Aes.Create(); + using var decryptor = aes.CreateDecryptor(_key, vector); var decrypt = _encoder.GetString(Transform(encrypted, decryptor)); return decrypt; } @@ -140,12 +142,9 @@ public static string GetHash(string inputString) { ArgumentNullException.ThrowIfNull(inputString, nameof(inputString)); - var sb = new StringBuilder(); - foreach (var @byte in _hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes(inputString))) { - sb.Append(@byte.ToString("X2", CultureInfo.InvariantCulture)); - } - var result = sb.ToString(); - return result; + // SHA256.HashData is a stateless one-shot API and is safe to call from any thread. + // Convert.ToHexString emits uppercase hex, identical to the previous "X2" formatting. + return Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(inputString))); } ///