diff --git a/.agents/rules/modules/auditing.md b/.agents/rules/modules/auditing.md index 75e581b857..e98b9fc912 100644 --- a/.agents/rules/modules/auditing.md +++ b/.agents/rules/modules/auditing.md @@ -13,3 +13,4 @@ Append-only audit trail (entity changes, security events, exceptions, HTTP activ - `SqlAuditSink` groups a batch by `TenantId` and sets tenant context per group in a fresh scope (null → Root) — background writer has no ambient tenant. - **JSON masking** redacts fields by keyword (password/secret/token/apiKey/connectionString…) → `****`. Add sensitive keys there. - Exclude an endpoint from activity auditing with `[NoAudit]` / the `NoAudit` endpoint extension. +- **Entity diffs mask sensitive values** in `EntityDiffBuilder` (property name contains password/secret/token/apikey/connectionstring/securitystamp → `****`, null stays null). `[NoAudit]` does **not** affect entity diffs — to keep an entity's values out of `AuditRecords` entirely, implement `IAuditExempt` (Contracts) on the entity. diff --git a/src/Modules/Auditing/Modules.Auditing.Contracts/IAuditExempt.cs b/src/Modules/Auditing/Modules.Auditing.Contracts/IAuditExempt.cs new file mode 100644 index 0000000000..29705c61dd --- /dev/null +++ b/src/Modules/Auditing/Modules.Auditing.Contracts/IAuditExempt.cs @@ -0,0 +1,11 @@ +namespace FSH.Modules.Auditing.Contracts; + +/// +/// Opt-out marker for the entity-change audit trail. An entity implementing this is skipped by the +/// auditing SaveChanges interceptor, so no property-level diff of it is ever captured or stored. +/// Use it for entities whose values must live in one table only (e.g. confidential reports, health +/// records). is unrelated: it only governs HTTP activity auditing. +/// +#pragma warning disable CA1040 // Marker interface is the intended shape (checked with `is`, not reflection). +public interface IAuditExempt; +#pragma warning restore CA1040 diff --git a/src/Modules/Auditing/Modules.Auditing.Contracts/PropertyChange.cs b/src/Modules/Auditing/Modules.Auditing.Contracts/PropertyChange.cs index 6e42ec8346..1d6a3f4818 100644 --- a/src/Modules/Auditing/Modules.Auditing.Contracts/PropertyChange.cs +++ b/src/Modules/Auditing/Modules.Auditing.Contracts/PropertyChange.cs @@ -8,5 +8,5 @@ public sealed record PropertyChange( string? DataType, // e.g., "string", "int", "datetime" object? OldValue, object? NewValue, - bool IsSensitive // true => value already masked/hashed + bool IsSensitive // true => OldValue/NewValue are masked ("****"), null kept as null ); \ No newline at end of file diff --git a/src/Modules/Auditing/Modules.Auditing/Persistence/AuditingSaveChangesInterceptor.cs b/src/Modules/Auditing/Modules.Auditing/Persistence/AuditingSaveChangesInterceptor.cs index 3577bce7c7..8476a3b4c0 100644 --- a/src/Modules/Auditing/Modules.Auditing/Persistence/AuditingSaveChangesInterceptor.cs +++ b/src/Modules/Auditing/Modules.Auditing/Persistence/AuditingSaveChangesInterceptor.cs @@ -31,7 +31,9 @@ public override async ValueTask> SavingChangesAsync( // inserts whose PayloadJson embeds the prior), growing until System.Text.Json rejects it. if (ctx is AuditDbContext) return result; + // IAuditExempt entities opt out entirely: their values must never be copied into AuditRecords. var entries = ctx.ChangeTracker.Entries() + .Where(e => e.Entity is not IAuditExempt) .Where(e => e.State is EntityState.Added or EntityState.Modified or EntityState.Deleted) .ToArray(); diff --git a/src/Modules/Auditing/Modules.Auditing/Persistence/EntityDiffBuilder.cs b/src/Modules/Auditing/Modules.Auditing/Persistence/EntityDiffBuilder.cs index da5c30b54f..2635b3e903 100644 --- a/src/Modules/Auditing/Modules.Auditing/Persistence/EntityDiffBuilder.cs +++ b/src/Modules/Auditing/Modules.Auditing/Persistence/EntityDiffBuilder.cs @@ -116,12 +116,16 @@ private static bool ShouldSkipProperty(PropertyEntry property) return null; } + // Sensitive values are masked here, before the payload leaves the interceptor: the audit row must + // record that the value changed, never the value itself (PasswordHash, refresh tokens, secrets…). + bool isSensitive = IsSensitive(property.Metadata.Name); + return new PropertyChange( Name: property.Metadata.Name, DataType: ToSimpleTypeName(property.Metadata.ClrType), - OldValue: oldVal, - NewValue: newVal, - IsSensitive: IsSensitive(property.Metadata.Name)); + OldValue: isSensitive ? Mask(oldVal) : oldVal, + NewValue: isSensitive ? Mask(newVal) : newVal, + IsSensitive: isSensitive); } private static (object? OldValue, object? NewValue, bool IsModified) GetPropertyValues( @@ -173,10 +177,17 @@ private static bool DetectSoftDelete(EntityEntry entry) return !orig && curr; } + private const string MaskValue = "****"; + + // Substring match on the property name. Keep in step with JsonMaskingService's keywords. + private static readonly string[] SensitiveKeywords = + ["password", "secret", "token", "apikey", "connectionstring", "securitystamp"]; + private static bool IsSensitive(string propertyName) => - propertyName.Contains("password", StringComparison.OrdinalIgnoreCase) || - propertyName.Contains("secret", StringComparison.OrdinalIgnoreCase) || - propertyName.Contains("token", StringComparison.OrdinalIgnoreCase); + SensitiveKeywords.Any(k => propertyName.Contains(k, StringComparison.OrdinalIgnoreCase)); + + // Null stays null so the trail still shows a value being set or cleared. + private static string? Mask(object? value) => value is null ? null : MaskValue; private static bool IsScalar(Type t) { diff --git a/src/Tests/Auditing.Tests/Auditing.Tests.csproj b/src/Tests/Auditing.Tests/Auditing.Tests.csproj index 4bbcc25c4f..f84f37fa4e 100644 --- a/src/Tests/Auditing.Tests/Auditing.Tests.csproj +++ b/src/Tests/Auditing.Tests/Auditing.Tests.csproj @@ -20,6 +20,7 @@ + diff --git a/src/Tests/Auditing.Tests/Persistence/AuditingSaveChangesInterceptorTests.cs b/src/Tests/Auditing.Tests/Persistence/AuditingSaveChangesInterceptorTests.cs new file mode 100644 index 0000000000..a90c5f3119 --- /dev/null +++ b/src/Tests/Auditing.Tests/Persistence/AuditingSaveChangesInterceptorTests.cs @@ -0,0 +1,121 @@ +using FSH.Modules.Auditing.Contracts; +using FSH.Modules.Auditing.Persistence; +using Microsoft.EntityFrameworkCore; + +namespace Auditing.Tests.Persistence; + +public sealed class AuditingSaveChangesInterceptorTests +{ + [Fact] + public async Task SavingChanges_Should_MaskSensitiveValues_When_EntityIsInserted() + { + // Arrange + var publisher = new CapturingPublisher(); + await using var db = CreateContext(publisher); + db.Accounts.Add(new Account { Id = 1, Email = "a@b.com", PasswordHash = "hash-value", RefreshToken = "refresh-value" }); + + // Act + await db.SaveChangesAsync(); + + // Assert + var changes = publisher.ChangesFor(nameof(Account)); + changes.Single(c => c.Name == nameof(Account.Email)).NewValue.ShouldBe("a@b.com"); + AssertMasked(changes.Single(c => c.Name == nameof(Account.PasswordHash))); + AssertMasked(changes.Single(c => c.Name == nameof(Account.RefreshToken))); + } + + [Fact] + public async Task SavingChanges_Should_MaskOldAndNewValues_When_SensitivePropertyIsUpdated() + { + // Arrange + var publisher = new CapturingPublisher(); + await using var db = CreateContext(publisher); + var account = new Account { Id = 1, Email = "a@b.com", PasswordHash = "old-hash", RefreshToken = null }; + db.Accounts.Add(account); + await db.SaveChangesAsync(); + publisher.Events.Clear(); + + // Act + account.PasswordHash = "new-hash"; + await db.SaveChangesAsync(); + + // Assert + var change = publisher.ChangesFor(nameof(Account)).Single(c => c.Name == nameof(Account.PasswordHash)); + change.IsSensitive.ShouldBeTrue(); + change.OldValue.ShouldBe("****"); + change.NewValue.ShouldBe("****"); + } + + [Fact] + public async Task SavingChanges_Should_SkipEntity_When_EntityIsAuditExempt() + { + // Arrange + var publisher = new CapturingPublisher(); + await using var db = CreateContext(publisher); + db.Reports.Add(new WhistleblowerReport { Id = 1, Body = "confidential report" }); + db.Accounts.Add(new Account { Id = 1, Email = "a@b.com", PasswordHash = "x" }); + + // Act + await db.SaveChangesAsync(); + + // Assert — the non-exempt entity is audited (positive control), the exempt one never is. + publisher.ChangesFor(nameof(Account)).ShouldNotBeEmpty(); + publisher.ChangesFor(nameof(WhistleblowerReport)).ShouldBeEmpty(); + } + + private static void AssertMasked(PropertyChange change) + { + change.IsSensitive.ShouldBeTrue(); + change.OldValue.ShouldBeNull(); + change.NewValue.ShouldBe("****"); + } + + private static TestDbContext CreateContext(IAuditPublisher publisher) + { + var options = new DbContextOptionsBuilder() + .UseInMemoryDatabase(Guid.NewGuid().ToString()) + .AddInterceptors(new AuditingSaveChangesInterceptor(publisher, TimeProvider.System)) + .Options; + return new TestDbContext(options); + } + + private sealed class Account + { + public int Id { get; set; } + public string Email { get; set; } = default!; + public string? PasswordHash { get; set; } + public string? RefreshToken { get; set; } + } + + private sealed class WhistleblowerReport : IAuditExempt + { + public int Id { get; set; } + public string Body { get; set; } = default!; + } + + private sealed class TestDbContext(DbContextOptions options) : DbContext(options) + { + public DbSet Accounts => Set(); + public DbSet Reports => Set(); + } + + private sealed class CapturingPublisher : IAuditPublisher + { + public List Events { get; } = []; + + public IAuditScope CurrentScope => throw new NotSupportedException(); + + public ValueTask PublishAsync(IAuditEvent auditEvent, CancellationToken ct = default) + { + Events.Add(auditEvent); + return ValueTask.CompletedTask; + } + + public List ChangesFor(string entityName) => + Events.Select(e => e.Payload) + .OfType() + .Where(p => p.EntityName == entityName) + .SelectMany(p => p.Changes) + .ToList(); + } +}