diff --git a/CHANGELOG.md b/CHANGELOG.md index 26f77b8..41e7298 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,23 @@ This project follows Semantic Versioning using the format `MAJOR.MINOR.PATCH`. * Completed the AsiBackbone 6.0 and Learning 1.0 alignment review, updated current terminology, and confirmed that NCAT remains compatible with its 2.x public and generated-template contracts. +* **Behavior change:** audit value canonicalization used by the `Hash`, + `HmacSha256`, and `Truncate` dispositions now renders `byte[]` values as hex, + collections as JSON arrays of canonical elements, and `DateTime` / + `DateTimeOffset` values in round-trip (`O`) format. Digests for these types + differ from earlier releases; string and numeric digests are unchanged. +* Clarified that the `Hash` disposition is an unkeyed integrity digest with no + confidentiality for low-entropy values; use `HmacSha256` instead. + +### Fixed + +* Audit reconciliation runs now persist findings in a single transaction inside + the execution strategy (or join a caller-owned transaction), guard every + finding update with its `ConcurrencyStamp`, and insert findings only when the + key is absent. A concurrent writer now causes a `DbUpdateConcurrencyException` + and a full rollback instead of a last-writer-wins or partially applied run. +* `byte[]` audit values no longer canonicalize to the literal `System.Byte[]`, + which made every binary value hash and truncate to the same constant. ## 2.10.0 - 2026-09-19 diff --git a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditReconciler.cs b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditReconciler.cs index 4a05bb4..a7e2221 100644 --- a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditReconciler.cs +++ b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditReconciler.cs @@ -438,22 +438,74 @@ private async Task PersistCandidatesAsync( IReadOnlyCollection completionEntries, DateTime now, CancellationToken cancellationToken) + { + // When the caller already owns an EF Core transaction, join it and let the caller decide whether + // the reconciliation writes commit. + if (_dbContext.Database.CurrentTransaction is not null) + { + await PersistCandidatesCoreAsync(candidates, auditBatchIds, completionEntries, now, cancellationToken) + .ConfigureAwait(false); + return; + } + + // Otherwise persist every finding change in one transaction inside the execution strategy, so a run + // applies all of its inserts and guarded updates or none of them, and a retrying provider replays + // the reads together with the writes. + IExecutionStrategy executionStrategy = _dbContext.Database.CreateExecutionStrategy(); + + await executionStrategy.ExecuteAsync( + async strategyCancellationToken => + { + await using IDbContextTransaction transaction = await _dbContext.Database + .BeginTransactionAsync(strategyCancellationToken) + .ConfigureAwait(false); + + await PersistCandidatesCoreAsync( + candidates, + auditBatchIds, + completionEntries, + now, + strategyCancellationToken) + .ConfigureAwait(false); + + await transaction.CommitAsync(strategyCancellationToken).ConfigureAwait(false); + }, + cancellationToken) + .ConfigureAwait(false); + } + + private async Task PersistCandidatesCoreAsync( + IReadOnlyCollection candidates, + IReadOnlyCollection auditBatchIds, + IReadOnlyCollection completionEntries, + DateTime now, + CancellationToken cancellationToken) { string[] keys = [.. candidates.Select(candidate => candidate.FindingKey)]; + string[] scopeBatchIds = [.. auditBatchIds + .Concat(completionEntries.Select(entry => entry.MutationBatchId)) + .Where(batchId => !string.IsNullOrWhiteSpace(batchId)) + .Distinct(StringComparer.Ordinal)]; + + // Read the active and the resolvable findings in one round trip. Each row's stamp guards its update. List existing = await _dbContext .ApplicationAuditReconciliationFindings .AsNoTracking() - .Where(finding => keys.Contains(finding.FindingKey)) + .Where(finding => keys.Contains(finding.FindingKey) || + (scopeBatchIds.Contains(finding.MutationBatchId) && + finding.RemediationStatus != ApplicationAuditReconciliationRemediationStatuses.Resolved)) .ToListAsync(cancellationToken) .ConfigureAwait(false); var existingByKey = existing .ToDictionary(finding => finding.FindingKey, StringComparer.Ordinal); + var activeKeys = new HashSet(keys, StringComparer.Ordinal); + var scopeBatchIdSet = new HashSet(scopeBatchIds, StringComparer.Ordinal); foreach (ApplicationAuditReconciliationCandidate candidate in candidates) { if (existingByKey.TryGetValue(candidate.FindingKey, out ApplicationAuditReconciliationFinding? finding)) { - await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" + int updatedCount = await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" UPDATE [ApplicationAuditReconciliationFindings] SET [Severity] = {{candidate.Severity}}, [Guidance] = {{candidate.Guidance}}, @@ -462,44 +514,55 @@ UPDATE [ApplicationAuditReconciliationFindings] [ResolvedUtc] = {{(DateTime?)null}}, [ConcurrencyStamp] = {{Guid.NewGuid().ToString("N")}} WHERE [Id] = {{finding.Id}} + AND [ConcurrencyStamp] = {{finding.ConcurrencyStamp}} """, cancellationToken).ConfigureAwait(false); + + EnsureSingleRowWritten(updatedCount, candidate.FindingKey); } else { + // Insert only when no row holds the key, so a run that interleaved and inserted the same finding + // surfaces as a concurrency conflict (and a rollback) rather than a unique index violation. var id = Guid.NewGuid(); - await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" + int insertedCount = await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" INSERT INTO [ApplicationAuditReconciliationFindings] ([Id], [SchemaVersion], [FindingKey], [ReasonCode], [Severity], [MutationBatchId], [Destination], [Guidance], [RemediationStatus], [FirstObservedUtc], [LastObservedUtc], [ResolvedUtc], [ConcurrencyStamp]) - VALUES - ({{id}}, {{ApplicationAuditReconciliationFinding.CurrentSchemaVersion}}, {{candidate.FindingKey}}, {{candidate.ReasonCode}}, {{candidate.Severity}}, {{candidate.MutationBatchId}}, {{candidate.Destination}}, {{candidate.Guidance}}, {{ApplicationAuditReconciliationRemediationStatuses.Open}}, {{now}}, {{now}}, {{(DateTime?)null}}, {{Guid.NewGuid().ToString("N")}}) + SELECT + {{id}}, {{ApplicationAuditReconciliationFinding.CurrentSchemaVersion}}, {{candidate.FindingKey}}, {{candidate.ReasonCode}}, {{candidate.Severity}}, {{candidate.MutationBatchId}}, {{candidate.Destination}}, {{candidate.Guidance}}, {{ApplicationAuditReconciliationRemediationStatuses.Open}}, {{now}}, {{now}}, {{(DateTime?)null}}, {{Guid.NewGuid().ToString("N")}} + WHERE NOT EXISTS ( + SELECT 1 FROM [ApplicationAuditReconciliationFindings] + WHERE [FindingKey] = {{candidate.FindingKey}}) """, cancellationToken).ConfigureAwait(false); + + EnsureSingleRowWritten(insertedCount, candidate.FindingKey); } } - string[] scopeBatchIds = [.. auditBatchIds - .Concat(completionEntries.Select(entry => entry.MutationBatchId)) - .Where(batchId => !string.IsNullOrWhiteSpace(batchId)) - .Distinct(StringComparer.Ordinal)]; - string[] activeKeys = keys; - - List resolved = await _dbContext - .ApplicationAuditReconciliationFindings - .AsNoTracking() - .Where(finding => scopeBatchIds.Contains(finding.MutationBatchId) && - finding.RemediationStatus != ApplicationAuditReconciliationRemediationStatuses.Resolved && - !activeKeys.Contains(finding.FindingKey)) - .ToListAsync(cancellationToken) - .ConfigureAwait(false); - - foreach (ApplicationAuditReconciliationFinding finding in resolved) + foreach (ApplicationAuditReconciliationFinding finding in existing.Where(finding => + !activeKeys.Contains(finding.FindingKey) && + scopeBatchIdSet.Contains(finding.MutationBatchId) && + finding.RemediationStatus != ApplicationAuditReconciliationRemediationStatuses.Resolved)) { - await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" + int resolvedCount = await _dbContext.Database.ExecuteSqlInterpolatedAsync($$""" UPDATE [ApplicationAuditReconciliationFindings] SET [RemediationStatus] = {{ApplicationAuditReconciliationRemediationStatuses.Resolved}}, [ResolvedUtc] = {{now}}, [ConcurrencyStamp] = {{Guid.NewGuid().ToString("N")}} WHERE [Id] = {{finding.Id}} + AND [ConcurrencyStamp] = {{finding.ConcurrencyStamp}} """, cancellationToken).ConfigureAwait(false); + + EnsureSingleRowWritten(resolvedCount, finding.FindingKey); + } + } + + private static void EnsureSingleRowWritten(int affectedCount, string findingKey) + { + if (affectedCount != 1) + { + throw new DbUpdateConcurrencyException( + $"Audit reconciliation finding '{findingKey}' was modified by another writer after it was read. " + + "No reconciliation changes were committed; the next reconciliation run re-evaluates the finding."); } } diff --git a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueDisposition.cs b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueDisposition.cs index 67787a9..d6c4d35 100644 --- a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueDisposition.cs +++ b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueDisposition.cs @@ -16,10 +16,11 @@ public enum ApplicationAuditValueDisposition Mask = 1, /// - /// Records an unsalted SHA-256 hash of the value. - /// This does not protect low-entropy values such as email addresses because - /// an attacker holding the audit table can brute-force candidate values. - /// Use this only for high-entropy values where correlation matters more than secrecy. + /// Records an unsalted, unkeyed SHA-256 hash of the value for integrity and change detection only. + /// This is not a confidentiality control: low-entropy values such as email addresses, phone numbers, + /// national identifiers, booleans, enum names, and small numbers are trivially recovered by a + /// dictionary attack against the audit table. Use when the value must stay + /// confidential, and use this only for high-entropy values where correlation matters more than secrecy. /// Hash = 2, diff --git a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueProtector.cs b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueProtector.cs index b0df221..89130f6 100644 --- a/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueProtector.cs +++ b/src/ProjectTemplate.Infrastructure/Data/Auditing/ApplicationAuditValueProtector.cs @@ -1,6 +1,8 @@ +using System.Collections; using System.Globalization; using System.Security.Cryptography; using System.Text; +using System.Text.Json; namespace ProjectTemplate.Infrastructure.Data.Auditing; @@ -47,6 +49,9 @@ internal static bool TryProtect( } } + // Unkeyed SHA-256 is an integrity / change-detection digest only. It provides no confidentiality for + // low-entropy values (email addresses, phone numbers, identifiers, booleans, enum names, small numbers), + // which a holder of the audit table can recover by dictionary attack. HmacSha256 is the confidential option. private static string Hash(object? value) { byte[] canonicalValue = Encoding.UTF8.GetBytes(ToCanonicalString(value)); @@ -67,9 +72,24 @@ private static string HmacSha256(object? value, string? key) return Convert.ToHexString(hash); } - private static string ToCanonicalString(object? value) + // Produces a culture-invariant representation that is distinct for distinct values. Types whose + // Object.ToString() is only the type name (byte[], collections) are expanded, so change detection on + // binary and collection columns does not collapse to a single constant digest. + internal static string ToCanonicalString(object? value) { - return Convert.ToString(value, CultureInfo.InvariantCulture) ?? string.Empty; + return value switch + { + null => string.Empty, + string text => text, + byte[] bytes => Convert.ToHexString(bytes), + ReadOnlyMemory memory => Convert.ToHexString(memory.Span), + Memory memory => Convert.ToHexString(memory.Span), + DateTime dateTime => dateTime.ToString("O", CultureInfo.InvariantCulture), + DateTimeOffset dateTimeOffset => dateTimeOffset.ToString("O", CultureInfo.InvariantCulture), + IFormattable formattable => formattable.ToString(null, CultureInfo.InvariantCulture), + IEnumerable sequence => JsonSerializer.Serialize(sequence.Cast().Select(ToCanonicalString).ToArray()), + _ => Convert.ToString(value, CultureInfo.InvariantCulture) ?? string.Empty, + }; } private static string Truncate(object? value, int? maximumLength) diff --git a/tests/ProjectTemplate.Web.Tests/ApplicationAuditReconciliationTests.cs b/tests/ProjectTemplate.Web.Tests/ApplicationAuditReconciliationTests.cs index 0f56146..8d53830 100644 --- a/tests/ProjectTemplate.Web.Tests/ApplicationAuditReconciliationTests.cs +++ b/tests/ProjectTemplate.Web.Tests/ApplicationAuditReconciliationTests.cs @@ -134,6 +134,114 @@ await database.Context.ApplicationAuditReconciliationFindings finding => finding.ReasonCode == ApplicationAuditReconciliationReasonCodes.DuplicateCompletion); } + [Fact] + public async Task ReconcileAsync_FindingChangedAfterRead_ThrowsConcurrencyExceptionAndRollsBackRun() + { + var interceptor = new NonQueryInterceptor(); + await using TestDatabase database = await TestDatabase.CreateAsync(interceptor: interceptor); + ApplicationAuditReconciliationFinding finding = await CreateOpenFindingAsync(database, "reconcile-concurrency-batch"); + database.Context.AuditRecords.Add(CreateAuditRecord("reconcile-concurrency-new-batch")); + _ = await database.Context.SaveChangesAsync(TestContext.Current.CancellationToken); + + bool simulatedConcurrentWrite = false; + interceptor.OnNonQueryExecuting = async (command, cancellationToken) => + { + if (simulatedConcurrentWrite || + !command.CommandText.Contains("UPDATE [ApplicationAuditReconciliationFindings]", StringComparison.Ordinal)) + { + return; + } + + simulatedConcurrentWrite = true; + + // Simulate another reconciler run or remediation changing the finding after this run read it. + await using DbCommand concurrentWrite = command.Connection!.CreateCommand(); + concurrentWrite.Transaction = command.Transaction; + concurrentWrite.CommandText = + "UPDATE [ApplicationAuditReconciliationFindings] SET [ConcurrencyStamp] = 'concurrent-writer'"; + _ = await concurrentWrite.ExecuteNonQueryAsync(cancellationToken); + }; + + _ = await Assert.ThrowsAsync(() => database.Reconciler + .ReconcileAsync(TestContext.Current.CancellationToken)); + + interceptor.OnNonQueryExecuting = null; + + Assert.True(simulatedConcurrentWrite); + Assert.Null(database.Context.Database.CurrentTransaction); + ApplicationAuditReconciliationFinding current = Assert.Single( + await database.Context.ApplicationAuditReconciliationFindings + .AsNoTracking() + .ToListAsync(TestContext.Current.CancellationToken)); + Assert.Equal(finding.Id, current.Id); + Assert.Equal(finding.ConcurrencyStamp, current.ConcurrencyStamp); + } + + [Fact] + public async Task ReconcileAsync_ConcurrentRunInsertedSameFinding_ThrowsConcurrencyExceptionWithoutDuplicate() + { + var interceptor = new NonQueryInterceptor(); + await using TestDatabase database = await TestDatabase.CreateAsync(interceptor: interceptor); + database.Context.AuditRecords.Add(CreateAuditRecord("reconcile-insert-race-batch")); + _ = await database.Context.SaveChangesAsync(TestContext.Current.CancellationToken); + + bool simulatedConcurrentInsert = false; + interceptor.OnNonQueryExecuting = async (command, cancellationToken) => + { + if (simulatedConcurrentInsert || + !command.CommandText.Contains("INSERT INTO [ApplicationAuditReconciliationFindings]", StringComparison.Ordinal)) + { + return; + } + + simulatedConcurrentInsert = true; + + // Simulate an interleaved run inserting the same finding key between this run's read and insert. + await using DbCommand concurrentInsert = command.Connection!.CreateCommand(); + concurrentInsert.Transaction = command.Transaction; + concurrentInsert.CommandText = command.CommandText; + foreach (DbParameter parameter in command.Parameters) + { + _ = concurrentInsert.Parameters.Add(new SqliteParameter(parameter.ParameterName, parameter.Value)); + } + + concurrentInsert.Parameters[0].Value = Guid.NewGuid().ToString().ToUpperInvariant(); + _ = await concurrentInsert.ExecuteNonQueryAsync(cancellationToken); + }; + + _ = await Assert.ThrowsAsync(() => database.Reconciler + .ReconcileAsync(TestContext.Current.CancellationToken)); + + interceptor.OnNonQueryExecuting = null; + + Assert.True(simulatedConcurrentInsert); + Assert.Empty(await database.Context.ApplicationAuditReconciliationFindings + .AsNoTracking() + .ToListAsync(TestContext.Current.CancellationToken)); + } + + [Fact] + public async Task ReconcileAsync_CallerOwnedTransaction_JoinsTransactionWithoutCommitting() + { + await using TestDatabase database = await TestDatabase.CreateAsync(); + database.Context.AuditRecords.Add(CreateAuditRecord("reconcile-caller-transaction-batch")); + _ = await database.Context.SaveChangesAsync(TestContext.Current.CancellationToken); + + await using (IDbContextTransaction transaction = await database.Context.Database + .BeginTransactionAsync(TestContext.Current.CancellationToken)) + { + _ = await database.Reconciler.ReconcileAsync(TestContext.Current.CancellationToken); + + Assert.Same(transaction, database.Context.Database.CurrentTransaction); + + await transaction.RollbackAsync(TestContext.Current.CancellationToken); + } + + Assert.Empty(await database.Context.ApplicationAuditReconciliationFindings + .AsNoTracking() + .ToListAsync(TestContext.Current.CancellationToken)); + } + [Fact] public async Task RecordRemediationAsync_AppendsEvidenceAndResolvesFinding() { diff --git a/tests/ProjectTemplate.Web.Tests/ApplicationAuditValueProtectorTests.cs b/tests/ProjectTemplate.Web.Tests/ApplicationAuditValueProtectorTests.cs index 88994e1..b30a1be 100644 --- a/tests/ProjectTemplate.Web.Tests/ApplicationAuditValueProtectorTests.cs +++ b/tests/ProjectTemplate.Web.Tests/ApplicationAuditValueProtectorTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using ProjectTemplate.Infrastructure.Data.Auditing; namespace ProjectTemplate.Web.Tests; @@ -100,6 +101,72 @@ public void TryProtect_UnsupportedDisposition_ThrowsInvalidOperationException() Assert.Contains("Unsupported audit value disposition", exception.Message, StringComparison.Ordinal); } + [Theory] + [InlineData(ApplicationAuditValueDisposition.Hash)] + [InlineData(ApplicationAuditValueDisposition.HmacSha256)] + public void TryProtect_DigestDispositions_DistinctByteArrays_ProduceDistinctDigests( + ApplicationAuditValueDisposition disposition) + { + ApplicationAuditValueDecision decision = disposition == ApplicationAuditValueDisposition.HmacSha256 + ? ApplicationAuditValueDecision.HmacSha256("audit-key-1") + : new(disposition); + + object first = Protect(decision, new byte[] { 0x01, 0x02 }); + object second = Protect(decision, new byte[] { 0x01, 0x03 }); + + Assert.NotEqual(first, second); + Assert.NotEqual(Protect(decision, "System.Byte[]"), first); + } + + [Fact] + public void TryProtect_TruncateDisposition_ByteArray_UsesHexRatherThanTypeName() + { + object protectedValue = Protect( + new(ApplicationAuditValueDisposition.Truncate, MaximumLength: 64), + new byte[] { 0xDE, 0xAD, 0xBE, 0xEF }); + + Assert.Equal("DEADBEEF", protectedValue); + } + + [Fact] + public void ToCanonicalString_Collections_ExpandElementsDistinctly() + { + Assert.Equal("[\"a\",\"b\"]", ApplicationAuditValueProtector.ToCanonicalString(new List { "a", "b" })); + Assert.NotEqual( + ApplicationAuditValueProtector.ToCanonicalString(new List { "a,b" }), + ApplicationAuditValueProtector.ToCanonicalString(new List { "a", "b" })); + } + + [Fact] + public void ToCanonicalString_FormattableValues_AreCultureInvariantAndPrecise() + { + CultureInfo original = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = CultureInfo.GetCultureInfo("de-DE"); + + Assert.Equal("1.5", ApplicationAuditValueProtector.ToCanonicalString(1.5m)); + Assert.NotEqual( + ApplicationAuditValueProtector.ToCanonicalString(new DateTime(2026, 1, 1, 0, 0, 0, 1, DateTimeKind.Utc)), + ApplicationAuditValueProtector.ToCanonicalString(new DateTime(2026, 1, 1, 0, 0, 0, 2, DateTimeKind.Utc))); + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + private static object Protect(ApplicationAuditValueDecision decision, object? value) + { + Assert.True(ApplicationAuditValueProtector.TryProtect( + new FixedDecisionPolicy(decision), + typeof(string), + "Field", + value, + out object protectedValue)); + return protectedValue; + } + private sealed class FixedDecisionPolicy(ApplicationAuditValueDecision decision) : IApplicationAuditValuePolicy { public ApplicationAuditValueDecision Evaluate(Type entityType, string propertyName, object? value)