You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
OPS-157454: Upload test coverage to Codacy from CI - #15
Fixes a thread-safety bug in EncryptionService (the reason this PR was held).
Adds a coverage job to CI that uploads test coverage to Codacy.
1. Thread-safety fix
Root cause.EncryptionService held private readonly static SHA256 _hashAlgorithm, and every GetHash call on every thread used that one instance. HashAlgorithm instances are not thread-safe. When xUnit ran hashing tests in parallel, Hash_StringEmpty_Succeeds / Hash_Text_Succeeds intermittently threw CryptographicException: Concurrent operations from multiple threads on this type are not supported. On a runtime without that guard, the same race can quietly return a wrong hash.
The same problem elsewhere in the class. The audit found two more shared crypto objects:
static Aes _aes, used by Encrypt/Decrypt from any thread through CreateEncryptor/CreateDecryptor. SymmetricAlgorithm is not documented as thread-safe. The ICryptoTransforms it created were also never disposed.
static RandomNumberGenerator _random = RandomNumberGenerator.Create(), shared and never disposed.
static UTF8Encoding _encoder is stateless for GetBytes/GetString, so it is safe to share and was left as it was.
Fix.
GetHash now returns Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(input))). That is the stateless one-shot API, and its uppercase hex output is identical to the old per-byte "X2" output.
Salt/IV generation now uses the static RandomNumberGenerator.GetBytes(16).
Encrypt/Decrypt now create their own Aes and dispose it after each call, and do the same for the ICryptoTransform.
Unchanged: the algorithm (AES-256-CBC, PKCS7), the key and IV handling, UTF-8 encoding, and the output format (lowercase hex for cipher text and salt, uppercase hex for hashes). Data encrypted by earlier versions still decrypts.
Tests (19 → 33):
PinnedOutputTests pins the exact outputs. GetHash is checked against published SHA-256 vectors ("", "abc", "The quick brown fox...") plus a multi-byte UTF-8 string. Encrypt/Decrypt are checked with a fixed key and salt against AES-256-CBC values computed separately with the BCL. The salt format is checked too. Every pinned test passed against the old, unfixed code, which shows the output did not change.
ConcurrencyTests runs 5,000 iterations under Parallel.For of GetHash, of fixed-salt Encrypt+Decrypt (on both shared and separate service instances), and of random-salt Encrypt. It asserts that every result equals the single-threaded result and that the random salts are distinct. Against the old code the GetHash test failed on 3 of 3 runs. With the fix the suite passed 8 of 8 times on Windows and 6 of 6 times in a clean mcr.microsoft.com/dotnet/sdk:10.0.401 Linux container, with 0 warnings.
2. Codacy coverage job
The job builds the test project and runs the Microsoft.Testing.Platform test executable with Microsoft.Testing.Extensions.CodeCoverage (--coverage --coverage-output-format cobertura). It uploads the Cobertura report to Codacy with codacy/codacy-coverage-reporter-action, with continue-on-error: true so a Codacy outage cannot turn a green run red, and keeps the report as a 7-day artifact. It runs on pdl-public and never on a GitHub-hosted runner. coverage.config follows the estate's reference (PanoramicData.NugetManagement) and excludes test assemblies and generated code. The test project also sets UseMicrosoftTestingPlatformRunner, which the xunit v3 executable needs before it will accept --coverage.
All tests run offline with no credentials, so no test filter is applied. Local line coverage in a clean Linux container: 98.7%.
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) <noreply@anthropic.com>
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%
AI Reviewer: run a review on demand. To trigger the first review automatically, go to your organization or repository integration settings. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
EncryptionService(the reason this PR was held).coveragejob to CI that uploads test coverage to Codacy.1. Thread-safety fix
Root cause.
EncryptionServiceheldprivate readonly static SHA256 _hashAlgorithm, and everyGetHashcall on every thread used that one instance.HashAlgorithminstances are not thread-safe. When xUnit ran hashing tests in parallel,Hash_StringEmpty_Succeeds/Hash_Text_Succeedsintermittently threwCryptographicException: Concurrent operations from multiple threads on this type are not supported. On a runtime without that guard, the same race can quietly return a wrong hash.The same problem elsewhere in the class. The audit found two more shared crypto objects:
static Aes _aes, used byEncrypt/Decryptfrom any thread throughCreateEncryptor/CreateDecryptor.SymmetricAlgorithmis not documented as thread-safe. TheICryptoTransforms it created were also never disposed.static RandomNumberGenerator _random = RandomNumberGenerator.Create(), shared and never disposed.static UTF8Encoding _encoderis stateless forGetBytes/GetString, so it is safe to share and was left as it was.Fix.
GetHashnow returnsConvert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(input))). That is the stateless one-shot API, and its uppercase hex output is identical to the old per-byte"X2"output.RandomNumberGenerator.GetBytes(16).Encrypt/Decryptnow create their ownAesand dispose it after each call, and do the same for theICryptoTransform.Unchanged: the algorithm (AES-256-CBC, PKCS7), the key and IV handling, UTF-8 encoding, and the output format (lowercase hex for cipher text and salt, uppercase hex for hashes). Data encrypted by earlier versions still decrypts.
Tests (19 → 33):
PinnedOutputTestspins the exact outputs.GetHashis checked against published SHA-256 vectors ("","abc","The quick brown fox...") plus a multi-byte UTF-8 string.Encrypt/Decryptare checked with a fixed key and salt against AES-256-CBC values computed separately with the BCL. The salt format is checked too. Every pinned test passed against the old, unfixed code, which shows the output did not change.ConcurrencyTestsruns 5,000 iterations underParallel.ForofGetHash, of fixed-saltEncrypt+Decrypt(on both shared and separate service instances), and of random-saltEncrypt. It asserts that every result equals the single-threaded result and that the random salts are distinct. Against the old code theGetHashtest failed on 3 of 3 runs. With the fix the suite passed 8 of 8 times on Windows and 6 of 6 times in a cleanmcr.microsoft.com/dotnet/sdk:10.0.401Linux container, with 0 warnings.2. Codacy coverage job
The job builds the test project and runs the Microsoft.Testing.Platform test executable with
Microsoft.Testing.Extensions.CodeCoverage(--coverage --coverage-output-format cobertura). It uploads the Cobertura report to Codacy withcodacy/codacy-coverage-reporter-action, withcontinue-on-error: trueso a Codacy outage cannot turn a green run red, and keeps the report as a 7-day artifact. It runs onpdl-publicand never on a GitHub-hosted runner.coverage.configfollows the estate's reference (PanoramicData.NugetManagement) and excludes test assemblies and generated code. The test project also setsUseMicrosoftTestingPlatformRunner, which the xunit v3 executable needs before it will accept--coverage.All tests run offline with no credentials, so no test filter is applied. Local line coverage in a clean Linux container: 98.7%.
Tracked on OPS-157454.
🤖 Generated with Claude Code