From 294f53b49aa83209f5c02774037b1b4ce5fd63d3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 15 Jul 2026 17:31:34 -0400 Subject: [PATCH] fix(secrets-ui): preserve metadata on rotate + audit unexpected mutation/reveal failures --- .../ISecretActorAccessor.cs | 10 ++- .../Components/ConfirmDeleteModal.razor | 52 ++++++++++-- .../Components/RevealButton.razor | 41 ++++++--- .../Components/SecretEditor.razor | 85 +++++++++++++++---- .../ConfirmDeleteModalTests.cs | 38 +++++++++ .../Fakes/RecordingSecretStore.cs | 16 ++++ .../RevealButtonTests.cs | 29 +++++++ .../SecretEditorTests.cs | 72 ++++++++++++++++ 8 files changed, 306 insertions(+), 37 deletions(-) diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/ISecretActorAccessor.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/ISecretActorAccessor.cs index 9f8f1dc..5c4dfed 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/ISecretActorAccessor.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Abstractions/ISecretActorAccessor.cs @@ -3,15 +3,17 @@ namespace ZB.MOM.WW.Secrets.Abstractions; /// /// Resolves the current principal (the ZB.MOM.WW.Auth identity) for audit attribution when a /// secret is resolved. A consumer wires this to whatever surfaces the ambient caller — an HTTP -/// context accessor, an actor context, a CLI identity, etc. A null -/// (or no accessor at all) is treated as the "system" actor by the resolver, so keyless / -/// background resolutions are still attributed rather than left blank. +/// context accessor, an actor context, a CLI identity, etc. When is +/// null (or no accessor is registered), the fallback actor string is consumer-defined so an +/// action is still attributed rather than left blank — e.g. "system" for background/keyless +/// resolution, or "unknown" for an unauthenticated UI action (the value the UI's +/// SecretActorResolver uses). /// public interface ISecretActorAccessor { /// /// The current principal to record as the audit Actor, or null when no - /// principal is available (treated as "system"). + /// principal is available (the resolver then substitutes its consumer-defined fallback actor). /// string? CurrentActor { get; } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/ConfirmDeleteModal.razor b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/ConfirmDeleteModal.razor index d5e2a12..2fe91de 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/ConfirmDeleteModal.razor +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/ConfirmDeleteModal.razor @@ -57,12 +57,53 @@ { _error = null; _busy = true; + bool deleted = false; try { string actor = await SecretActorResolver.ResolveAsync(Services, AuthState); - await Store.DeleteAsync(Name, actor, _cts.Token); + try + { + await Store.DeleteAsync(Name, actor, _cts.Token); + await Audit.WriteAsync( + new AuditEvent + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = actor, + Action = "secret.delete", + Outcome = AuditOutcome.Success, + Category = "Secrets", + Target = Name.Value, + }, + _cts.Token); + + deleted = true; + } + catch (Exception) + { + // An unexpected store fault must still be audited as a Failure and surfaced to the + // operator instead of tearing down the circuit. + _error = "Unable to delete the secret."; + await WriteFailureAuditAsync(actor); + } + } + finally + { + _busy = false; + } + + if (deleted) + { + await OnDeleted.InvokeAsync(); + } + } + + private async Task WriteFailureAuditAsync(string actor) + { + try + { await Audit.WriteAsync( new AuditEvent { @@ -70,17 +111,16 @@ OccurredAtUtc = DateTimeOffset.UtcNow, Actor = actor, Action = "secret.delete", - Outcome = AuditOutcome.Success, + Outcome = AuditOutcome.Failure, Category = "Secrets", Target = Name.Value, + DetailsJson = "{\"reason\":\"error\"}", }, _cts.Token); - - await OnDeleted.InvokeAsync(); } - finally + catch { - _busy = false; + // Best-effort audit of a failure; never let audit itself surface into the event pipeline. } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor index cdb5742..378a1c8 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor @@ -73,6 +73,15 @@ else _error = "Unable to decrypt secret."; outcome = AuditOutcome.Failure; } + catch (Exception) + { + // Any other unexpected store/cipher fault (e.g. master key unavailable, timeout) must + // still be audited as a Failure and surfaced to the user, not left to tear down the + // circuit. The value is never shown. + _value = null; + _error = "Unable to reveal secret."; + outcome = AuditOutcome.Failure; + } finally { _busy = false; @@ -80,18 +89,26 @@ else } string actor = await SecretActorResolver.ResolveAsync(Services, AuthState); - await Audit.WriteAsync( - new AuditEvent - { - EventId = Guid.NewGuid(), - OccurredAtUtc = DateTimeOffset.UtcNow, - Actor = actor, - Action = "secret.reveal", - Outcome = outcome, - Category = "Secrets", - Target = Name.Value, - }, - _cts.Token); + try + { + await Audit.WriteAsync( + new AuditEvent + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = actor, + Action = "secret.reveal", + Outcome = outcome, + Category = "Secrets", + Target = Name.Value, + DetailsJson = outcome == AuditOutcome.Failure ? "{\"reason\":\"error\"}" : null, + }, + _cts.Token); + } + catch + { + // Best-effort audit; never let an audit-write fault surface into the event pipeline. + } } private void Hide() diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor index 06a169d..62567cb 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor @@ -82,13 +82,26 @@ [CascadingParameter] private Task? AuthState { get; set; } private bool IsRotate => ExistingName is not null; + private bool _metadataLoaded; /// - protected override void OnParametersSet() + protected override async Task OnParametersSetAsync() { if (IsRotate) { _name = ExistingName!.Value.Value; + + // Rotate must preserve the existing row's metadata: the store's upsert overwrites the + // description and content type from the incoming row, so pre-fill both from the current + // row (metadata only — no plaintext is read or decrypted). Load once so a re-render does + // not clobber operator edits. The operator still supplies the NEW value. + if (!_metadataLoaded) + { + _metadataLoaded = true; + StoredSecret? row = await Store.GetAsync(ExistingName.Value, _cts.Token); + _description = row?.Description ?? string.Empty; + _contentType = row?.ContentType ?? SecretContentType.Text; + } } } @@ -108,37 +121,79 @@ } _busy = true; + bool saved = false; try { string actor = await SecretActorResolver.ResolveAsync(Services, AuthState); - StoredSecret row = Cipher.Encrypt(name, _value, _contentType); - row = row with + try { - Description = string.IsNullOrWhiteSpace(_description) ? null : _description, - CreatedBy = actor, - UpdatedBy = actor, - }; - await Store.UpsertAsync(row, _cts.Token); + StoredSecret row = Cipher.Encrypt(name, _value, _contentType); + row = row with + { + Description = string.IsNullOrWhiteSpace(_description) ? null : _description, + CreatedBy = actor, + UpdatedBy = actor, + }; + await Store.UpsertAsync(row, _cts.Token); + await Audit.WriteAsync( + new AuditEvent + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = actor, + Action = IsRotate ? "secret.rotate" : "secret.add", + Outcome = AuditOutcome.Success, + Category = "Secrets", + Target = name.Value, + }, + _cts.Token); + + saved = true; + } + catch (Exception) + { + // An unexpected store/cipher fault (e.g. master key unavailable, timeout) must still + // be audited as a Failure and surfaced to the operator — never the value — instead of + // tearing down the circuit. The detail carries no plaintext. + _error = IsRotate ? "Unable to rotate the secret." : "Unable to add the secret."; + await WriteFailureAuditAsync( + actor, IsRotate ? "secret.rotate" : "secret.add", name.Value); + } + } + finally + { + _busy = false; + } + + if (saved) + { + await OnSaved.InvokeAsync(); + } + } + + private async Task WriteFailureAuditAsync(string actor, string action, string target) + { + try + { await Audit.WriteAsync( new AuditEvent { EventId = Guid.NewGuid(), OccurredAtUtc = DateTimeOffset.UtcNow, Actor = actor, - Action = IsRotate ? "secret.rotate" : "secret.add", - Outcome = AuditOutcome.Success, + Action = action, + Outcome = AuditOutcome.Failure, Category = "Secrets", - Target = name.Value, + Target = target, + DetailsJson = "{\"reason\":\"error\"}", }, _cts.Token); - - await OnSaved.InvokeAsync(); } - finally + catch { - _busy = false; + // Best-effort audit of a failure; never let audit itself surface into the event pipeline. } } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/ConfirmDeleteModalTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/ConfirmDeleteModalTests.cs index de03688..775cf27 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/ConfirmDeleteModalTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/ConfirmDeleteModalTests.cs @@ -80,6 +80,44 @@ public class ConfirmDeleteModalTests : TestContext Assert.Empty(audit.Events); } + [Fact] + public void ConfirmDeleteModal_StoreFault_AuditsFailure_ShowsError_DoesNotThrow() + { + var store = new RecordingSecretStore + { + DeleteFault = new MasterKeyUnavailableException("KEK unavailable."), + }; + var audit = new CapturingAuditWriter(); + Services.AddSingleton(store); + Services.AddSingleton(audit); + + var name = new SecretName("db/primary"); + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("carol"); + auth.SetPolicies(SecretsAuthorization.ManagePolicy); + + bool deleted = false; + var cut = RenderComponent(p => p + .Add(c => c.Visible, true) + .Add(c => c.Name, name) + .Add(c => c.OnDeleted, () => deleted = true)); + + // The click must not throw into the Blazor event pipeline. + cut.Find("[data-testid=confirm-delete]").Click(); + + // OnDeleted not raised; a Failure audit recorded; an error surfaced. + Assert.False(deleted); + Assert.Single(audit.Events); + AuditEvent evt = audit.Events[0]; + Assert.Equal("secret.delete", evt.Action); + Assert.Equal(AuditOutcome.Failure, evt.Outcome); + Assert.Equal("Secrets", evt.Category); + Assert.Equal("db/primary", evt.Target); + Assert.Equal("carol", evt.Actor); + + Assert.NotEmpty(cut.FindAll("[data-testid=delete-error]")); + } + [Fact] public void ConfirmDeleteModal_NotVisible_RendersNothing() { diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingSecretStore.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingSecretStore.cs index 7b75f8d..3f274b1 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingSecretStore.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingSecretStore.cs @@ -20,6 +20,12 @@ public sealed class RecordingSecretStore : ISecretStore /// Delete calls (name + recorded actor), in call order. public IReadOnlyList<(SecretName Name, string? Actor)> Deletes => _deletes.ToArray(); + /// When set, throws this to drive the mutation-failure path. + public Exception? UpsertFault { get; set; } + + /// When set, throws this to drive the mutation-failure path. + public Exception? DeleteFault { get; set; } + /// Seeds a row so and can return it. /// The row to seed. public void Seed(StoredSecret row) => _rows[row.Name.Value] = row; @@ -31,6 +37,11 @@ public sealed class RecordingSecretStore : ISecretStore /// public Task UpsertAsync(StoredSecret row, CancellationToken ct) { + if (UpsertFault is not null) + { + throw UpsertFault; + } + _rows[row.Name.Value] = row; _upserts.Enqueue(row); return Task.CompletedTask; @@ -39,6 +50,11 @@ public sealed class RecordingSecretStore : ISecretStore /// public Task DeleteAsync(SecretName name, string? actor, CancellationToken ct) { + if (DeleteFault is not null) + { + throw DeleteFault; + } + _deletes.Enqueue((name, actor)); return Task.FromResult(_rows.TryRemove(name.Value, out _)); } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealButtonTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealButtonTests.cs index bd52d09..3f5ff46 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealButtonTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealButtonTests.cs @@ -85,6 +85,35 @@ public class RevealButtonTests : TestContext Assert.Equal("api/missing", evt.Target); } + [Fact] + public void RevealButton_DecryptFault_AuditsFailure_ShowsError_NoValue() + { + var (store, audit, cipher) = RegisterServices(); + var name = new SecretName("api/token"); + store.Seed(Row(name, PlainValue)); + cipher.FailDecrypt = true; + + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("carol"); + auth.SetPolicies(SecretsAuthorization.RevealPolicy); + + var cut = RenderComponent(p => p.Add(c => c.Name, name)); + + // The click must not throw into the Blazor event pipeline. + cut.Find("button.reveal-secret").Click(); + + // No value shown, an error surfaced, and a Failure reveal audit recorded (no value leak). + Assert.DoesNotContain(PlainValue, cut.Markup); + Assert.NotEmpty(cut.FindAll("[data-testid=reveal-error]")); + Assert.Single(audit.Events); + AuditEvent evt = audit.Events[0]; + Assert.Equal("secret.reveal", evt.Action); + Assert.Equal(AuditOutcome.Failure, evt.Outcome); + Assert.Equal("api/token", evt.Target); + Assert.Equal("carol", evt.Actor); + AssertNoValueLeak(evt); + } + private static void AssertNoValueLeak(AuditEvent evt) { foreach (string? field in new[] { evt.Action, evt.Actor, evt.Category, evt.Target, evt.DetailsJson, evt.ToString() }) diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorTests.cs index a51bc7b..f807ae6 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorTests.cs @@ -99,6 +99,78 @@ public class SecretEditorTests : TestContext AssertNoValueLeak(audit.Events[0], "rotated-value"); } + [Fact] + public void SecretEditor_Rotate_PreservesDescriptionAndContentType() + { + var (store, _, cipher) = RegisterServices(); + var name = new SecretName("db/primary"); + + // Seed an existing Json secret with a description; rotating must NOT blank either. + StoredSecret existing = cipher.Encrypt(name, "old-value", SecretContentType.Json) with + { + Description = "db creds", + Revision = 1, + CreatedUtc = DateTimeOffset.UnixEpoch, + UpdatedUtc = DateTimeOffset.UnixEpoch, + }; + store.Seed(existing); + + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("dave"); + auth.SetPolicies(SecretsAuthorization.ManagePolicy); + + var cut = RenderComponent(p => p + .Add(c => c.ExistingName, name) + .Add(c => c.OnSaved, () => { })); + + // The editor pre-filled the existing metadata (no plaintext read). + Assert.Equal("db creds", cut.Find("[data-testid=secret-description]").GetAttribute("value")); + + // The operator supplies ONLY a new value. + cut.Find("[data-testid=secret-value]").Change("rotated-value"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Single(store.Upserts); + StoredSecret row = store.Upserts[0]; + Assert.Equal("db/primary", row.Name.Value); + Assert.Equal("rotated-value", cipher.Decrypt(row)); + // Metadata preserved — content type not reset to Text, description not blanked. + Assert.Equal(SecretContentType.Json, row.ContentType); + Assert.Equal("db creds", row.Description); + } + + [Fact] + public void SecretEditor_Add_StoreFault_AuditsFailure_ShowsError_DoesNotThrow() + { + var (store, audit, _) = RegisterServices(); + store.UpsertFault = new MasterKeyUnavailableException("KEK unavailable."); + + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("carol"); + auth.SetPolicies(SecretsAuthorization.ManagePolicy); + + var cut = RenderComponent(p => p + .Add(c => c.OnSaved, () => { })); + + cut.Find("[data-testid=secret-name]").Change("app/thing"); + cut.Find("[data-testid=secret-value]").Change(EnteredValue); + + // The click must not throw into the Blazor event pipeline. + cut.Find("[data-testid=editor-submit]").Click(); + + // Nothing persisted, a Failure audit recorded, an error surfaced, and no value leak. + Assert.Empty(store.Upserts); + Assert.Single(audit.Events); + AuditEvent evt = audit.Events[0]; + Assert.Equal("secret.add", evt.Action); + Assert.Equal(AuditOutcome.Failure, evt.Outcome); + Assert.Equal("app/thing", evt.Target); + Assert.Equal("carol", evt.Actor); + AssertNoValueLeak(evt); + + Assert.NotEmpty(cut.FindAll("[data-testid=editor-error]")); + } + private static void AssertNoValueLeak(AuditEvent evt, string value = EnteredValue) { foreach (string? field in new[] { evt.Action, evt.Actor, evt.Category, evt.Target, evt.DetailsJson, evt.ToString() })