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
2026-11-09 14:56:14 -05:00
parent 6112a9f447
commit e1487fbf10
8 changed files with 215 additions and 35 deletions
@@ -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<SecretEditor>` / `ILogger<ConfirmDeleteModal>`, when the host
+  registers logging).

 ## 0.6.5 — unreleased (NOT published to the feed)

@@ -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<ILogger<ConfirmDeleteModal>>()?.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
@@ -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<ILogger<SecretEditor>>()?.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
@@ -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;
+
+/// <summary>
+/// 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 <c>Failure</c> audit row, and a resolver still serving the old plaintext.
+/// </summary>
+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<ISecretStore>(store);
+        Services.AddSingleton<IAuditWriter>(audit);
+        Services.AddSingleton<ISecretCipher>(new FakeCipher());
+        Services.AddSingleton<ISecretCacheInvalidator>(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<SecretEditor>(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<SecretEditor>(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<ConfirmDeleteModal>(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);
+    }
+}
@@ -15,9 +15,21 @@ public sealed class CapturingAuditWriter : IAuditWriter
    /// <summary>The events captured so far, in write order.</summary>
    public IReadOnlyList<AuditEvent> Events => _events.ToArray();

+    /// <summary>
+    /// When set, every <see cref="AuditOutcome.Success"/> 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.
+    /// </summary>
+    public Exception? SuccessFault { get; set; }
+
    /// <inheritdoc />
    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;
    }
@@ -0,0 +1,16 @@
+using System.Collections.Concurrent;
+using ZB.MOM.WW.Secrets.Abstractions;
+
+namespace ZB.MOM.WW.Secrets.Ui.Tests.Fakes;
+
+/// <summary>Records every <see cref="Invalidate"/> call so a test can assert the cache was evicted.</summary>
+public sealed class RecordingCacheInvalidator : ISecretCacheInvalidator
+{
+    private readonly ConcurrentQueue<SecretName> _invalidated = new();
+
+    /// <summary>The names evicted so far, in call order.</summary>
+    public IReadOnlyList<SecretName> Invalidated => _invalidated.ToArray();
+
+    /// <inheritdoc />
+    public void Invalidate(SecretName name) => _invalidated.Enqueue(name);
+}
@@ -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)
@@ -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<T>`. 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`