From e1487fbf10482f111c85431a9087808e8c4ea8d8 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:56:14 -0500 Subject: [PATCH] fix(secrets): UI reports a committed change as saved when its audit throws (scadaproj#34) SecretEditor and ConfirmDeleteModal wrote the success audit inside the same try as the store write, so a throwing IAuditWriter after a committed add/rotate/delete showed an error, wrote a Failure audit and skipped the resolver-cache eviction - the old plaintext kept resolving for the TTL. The mutation try now covers only the cipher + store call; the success audit is best-effort afterwards and logs a warning (no value) on failure, so the change is reported saved, the callback fires and the cache is evicted. - tests: AuditFailureAfterCommitTests (add, rotate, delete) with a throwing success audit; all fail without the fix - code-reviews: Secrets-013 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- ZB.MOM.WW.Secrets/CHANGELOG.md | 7 ++ .../Components/ConfirmDeleteModal.razor | 50 +++++++--- .../Components/SecretEditor.razor | 55 ++++++++--- .../AuditFailureAfterCommitTests.cs | 93 +++++++++++++++++++ .../Fakes/CapturingAuditWriter.cs | 12 +++ .../Fakes/RecordingCacheInvalidator.cs | 16 ++++ code-reviews/README.md | 11 +-- code-reviews/Secrets/findings.md | 6 +- 8 files changed, 215 insertions(+), 35 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/AuditFailureAfterCommitTests.cs create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingCacheInvalidator.cs diff --git a/ZB.MOM.WW.Secrets/CHANGELOG.md b/ZB.MOM.WW.Secrets/CHANGELOG.md index bc46024..fc3c9fd 100644 --- a/ZB.MOM.WW.Secrets/CHANGELOG.md +++ b/ZB.MOM.WW.Secrets/CHANGELOG.md @@ -43,6 +43,13 @@ Fixes six scadaproj#15 code-review findings (`code-reviews/Secrets/findings.md`) `StartAsync` when no lifetime is registered. Behaviour change: the application's `ActorSystem` must be resolvable once the host has started even if no secret is ever touched; a failure there is logged. OtOpcUa's own `SecretReplicationStarter` becomes redundant (harmless — same actor). +- **UI: a committed add/rotate/delete is no longer reported as failed when its audit write throws** + (scadaproj#34, Secrets-013). `SecretEditor` and `ConfirmDeleteModal` wrote the success audit inside the + store write's `try`, so a throwing `IAuditWriter` produced an error message, a `Failure` audit row and no + cache eviction for a change that had committed. The success audit is now best-effort after the commit: + the change is reported saved, `OnSaved`/`OnDeleted` fire, the resolver cache is evicted, and the audit + failure is logged as a warning (`ILogger` / `ILogger`, when the host + registers logging). ## 0.6.5 — unreleased (NOT published to the feed) 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 edefffc..ebb507f 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 @@ -3,6 +3,7 @@ @inject ISecretStore Store @inject IAuditWriter Audit @inject IServiceProvider Services +@using Microsoft.Extensions.Logging @* Components/ConfirmDeleteModal.razor — an in-page Blazor overlay (NOT a JS confirm/alert dialog). Shown while Visible is true; Confirm tombstones + audits the delete, Cancel dismisses. *@ @@ -90,19 +91,7 @@ { 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); - + // The tombstone is committed from here on. deleted = true; } catch (Exception) @@ -112,6 +101,13 @@ _error = "Unable to delete the secret."; await WriteFailureAuditAsync(actor); } + + if (deleted) + { + // Outside the mutation try (scadaproj#34): a throwing audit sink must not turn a + // committed delete into a reported failure, nor skip the cache eviction below. + await WriteSuccessAuditAsync(actor); + } } finally { @@ -129,6 +125,34 @@ } } + private async Task WriteSuccessAuditAsync(string actor) + { + try + { + 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); + } + catch (Exception ex) + { + // Best-effort: the delete itself succeeded. Logged so the audit gap is visible. + Services.GetService>()?.LogWarning( + ex, + "Secret delete of {SecretName} by {Actor} succeeded, but its audit event could not be written.", + Name.Value, + actor); + } + } + private async Task WriteFailureAuditAsync(string actor) { try 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 0a51c0a..5f9105c 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 @@ -4,6 +4,7 @@ @inject ISecretStore Store @inject IAuditWriter Audit @inject IServiceProvider Services +@using Microsoft.Extensions.Logging @* Components/SecretEditor.razor — add (ExistingName == null) or rotate (ExistingName set) a secret. The value is entered through a masked input and never rendered back or placed in any audit field. *@ @@ -137,19 +138,7 @@ }; 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); - + // The write is committed (and, on a replicated host, already published) from here on. saved = true; } catch (Exception) @@ -161,6 +150,16 @@ await WriteFailureAuditAsync( actor, IsRotate ? "secret.rotate" : "secret.add", name.Value); } + + if (saved) + { + // Outside the mutation try (scadaproj#34): an audit sink that throws AFTER the store + // committed must not report the change as failed, audit it as a Failure, or skip the + // cache eviction below — the operator would retry, or believe the leaked credential + // was still live while the resolver kept serving the old plaintext. + await WriteSuccessAuditAsync( + actor, IsRotate ? "secret.rotate" : "secret.add", name.Value); + } } finally { @@ -178,6 +177,36 @@ } } + private async Task WriteSuccessAuditAsync(string actor, string action, string target) + { + try + { + await Audit.WriteAsync( + new AuditEvent + { + EventId = Guid.NewGuid(), + OccurredAtUtc = DateTimeOffset.UtcNow, + Actor = actor, + Action = action, + Outcome = AuditOutcome.Success, + Category = "Secrets", + Target = target, + }, + _cts.Token); + } + catch (Exception ex) + { + // Best-effort: the change itself succeeded. Logged (name + action only, never the value) + // so the gap in the audit trail is visible to whoever runs the host. + Services.GetService>()?.LogWarning( + ex, + "Secret {Action} of {SecretName} by {Actor} succeeded, but its audit event could not be written.", + action, + target, + actor); + } + } + private async Task WriteFailureAuditAsync(string actor, string action, string target) { try diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/AuditFailureAfterCommitTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/AuditFailureAfterCommitTests.cs new file mode 100644 index 0000000..28fcc11 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/AuditFailureAfterCommitTests.cs @@ -0,0 +1,93 @@ +using Bunit; +using Bunit.TestDoubles; +using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.Audit; +using ZB.MOM.WW.Secrets.Abstractions; +using ZB.MOM.WW.Secrets.Ui.Components; +using ZB.MOM.WW.Secrets.Ui.Tests.Fakes; + +namespace ZB.MOM.WW.Secrets.Ui.Tests; + +/// +/// scadaproj#34 (Secrets-013): when the success audit throws AFTER the store committed, the UI must +/// still report the change as saved, raise its saved/deleted callback, evict the resolver cache, and +/// never audit the committed change as a Failure. Before the fix the store write and the success +/// audit shared one try, so a throwing audit sink turned a real rotation into "Unable to rotate the +/// secret.", a Failure audit row, and a resolver still serving the old plaintext. +/// +public sealed class AuditFailureAfterCommitTests : TestContext +{ + private (RecordingSecretStore Store, CapturingAuditWriter Audit, RecordingCacheInvalidator Cache) Register() + { + var store = new RecordingSecretStore(); + var audit = new CapturingAuditWriter { SuccessFault = new InvalidOperationException("audit sink down") }; + var cache = new RecordingCacheInvalidator(); + Services.AddSingleton(store); + Services.AddSingleton(audit); + Services.AddSingleton(new FakeCipher()); + Services.AddSingleton(cache); + + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("carol"); + auth.SetPolicies(SecretsAuthorization.ManagePolicy); + return (store, audit, cache); + } + + [Fact] + public void Rotate_is_reported_saved_and_evicted_when_the_success_audit_throws() + { + var (store, audit, cache) = Register(); + var name = new SecretName("db/central"); + bool saved = false; + + var cut = RenderComponent(p => p + .Add(c => c.ExistingName, name) + .Add(c => c.OnSaved, () => saved = true)); + cut.Find("[data-testid=secret-value]").Change("new-value"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Single(store.Upserts); // committed + Assert.True(saved, "OnSaved was not raised for a committed rotation"); + Assert.Equal("db/central", Assert.Single(cache.Invalidated).Value); + Assert.Empty(cut.FindAll("[data-testid=editor-error]")); + Assert.DoesNotContain(audit.Events, e => e.Outcome == AuditOutcome.Failure); + } + + [Fact] + public void Add_is_reported_saved_and_evicted_when_the_success_audit_throws() + { + var (store, audit, cache) = Register(); + bool saved = false; + + var cut = RenderComponent(p => p.Add(c => c.OnSaved, () => saved = true)); + cut.Find("[data-testid=secret-name]").Change("app/thing"); + cut.Find("[data-testid=secret-value]").Change("v"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Single(store.Upserts); + Assert.True(saved); + Assert.Equal("app/thing", Assert.Single(cache.Invalidated).Value); + Assert.Empty(cut.FindAll("[data-testid=editor-error]")); + Assert.DoesNotContain(audit.Events, e => e.Outcome == AuditOutcome.Failure); + } + + [Fact] + public void Delete_is_reported_deleted_and_evicted_when_the_success_audit_throws() + { + var (store, audit, cache) = Register(); + var name = new SecretName("db/central"); + bool deleted = false; + + var cut = RenderComponent(p => p + .Add(c => c.Visible, true) + .Add(c => c.Name, name) + .Add(c => c.OnDeleted, () => deleted = true)); + cut.Find("[data-testid=confirm-delete]").Click(); + + Assert.Single(store.Deletes); // tombstoned + Assert.True(deleted, "OnDeleted was not raised for a committed delete"); + Assert.Equal("db/central", Assert.Single(cache.Invalidated).Value); + Assert.DoesNotContain("Unable to delete", cut.Markup, StringComparison.Ordinal); + Assert.DoesNotContain(audit.Events, e => e.Outcome == AuditOutcome.Failure); + } +} diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/CapturingAuditWriter.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/CapturingAuditWriter.cs index e94d4ce..421530d 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/CapturingAuditWriter.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/CapturingAuditWriter.cs @@ -15,9 +15,21 @@ public sealed class CapturingAuditWriter : IAuditWriter /// The events captured so far, in write order. public IReadOnlyList Events => _events.ToArray(); + /// + /// When set, every write throws this instead of being captured + /// — the "audit sink briefly unavailable after the store committed" case (scadaproj#34). + /// Failure-outcome writes are still captured, so a test can prove none was written. + /// + public Exception? SuccessFault { get; set; } + /// public Task WriteAsync(AuditEvent evt, CancellationToken ct = default) { + if (SuccessFault is not null && evt.Outcome == AuditOutcome.Success) + { + throw SuccessFault; + } + _events.Enqueue(evt); return Task.CompletedTask; } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingCacheInvalidator.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingCacheInvalidator.cs new file mode 100644 index 0000000..91ee062 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/Fakes/RecordingCacheInvalidator.cs @@ -0,0 +1,16 @@ +using System.Collections.Concurrent; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.Secrets.Ui.Tests.Fakes; + +/// Records every call so a test can assert the cache was evicted. +public sealed class RecordingCacheInvalidator : ISecretCacheInvalidator +{ + private readonly ConcurrentQueue _invalidated = new(); + + /// The names evicted so far, in call order. + public IReadOnlyList Invalidated => _invalidated.ToArray(); + + /// + public void Invalidate(SecretName name) => _invalidated.Enqueue(name); +} diff --git a/code-reviews/README.md b/code-reviews/README.md index 14fc7a1..d854509 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 26 pending findings across all libraries. +10 of 10 libraries reviewed. 25 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -50,9 +50,9 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). |----------|---------------| | Critical | 0 | | High | 0 | -| Medium | 7 | +| Medium | 6 | | Low | 19 | -| **Total** | **26** | +| **Total** | **25** | ## Library Status @@ -65,7 +65,7 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | | [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | | [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | -| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/13 | 15 | 19 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/13 | 14 | 19 | | [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 | | [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | @@ -84,7 +84,7 @@ _None open._ _None open._ -### Medium (7) +### Medium (6) | ID | Library | Title | |----|---------|-------| @@ -93,7 +93,6 @@ _None open._ | LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | | LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | | LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | -| Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | | Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | ### Low (19) diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md index 03a9e1f..45e5787 100644 --- a/code-reviews/Secrets/findings.md +++ b/code-reviews/Secrets/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 | | Reviewer | Claude (scadaproj#15) | | Commit reviewed | `91fb2b4` | -| Open findings | 15 | +| Open findings | 14 | ## Summary @@ -438,7 +438,7 @@ _Unresolved._ | Severity | Medium | | Priority | P2 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:138` (also `ConfirmDeleteModal.razor:91`) | **Description** @@ -465,7 +465,7 @@ Move the success audit out of the mutation `try`. Once the store write returns, **Resolution** -_Unresolved._ +Resolved 2026-11-09 in the commit tagged `(scadaproj#34)` (Secrets `0.6.6`, unpublished). In both `SecretEditor.SubmitAsync` and `ConfirmDeleteModal.ConfirmAsync` the mutation `try` now covers only the cipher + store call and sets `saved`/`deleted` as soon as the store returns; the success audit moved to a separate best-effort `WriteSuccessAuditAsync` that catches and logs a warning (name, action and actor only — never the value) through an optional `ILogger`. The existing post-commit block (cache `Invalidate` + `OnSaved`/`OnDeleted`) therefore always runs for a committed change. Regression tests `AuditFailureAfterCommitTests` (add, rotate, delete) use a `CapturingAuditWriter.SuccessFault` that throws on every Success write and assert: committed, callback raised, cache evicted for the name, no error shown, no Failure audit. All three fail without the fix. ### Secrets-014 — UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as `secret.add`