From a84c64e26f4b62c0da1b4669ff9b39c8a7c3a21f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:18:49 -0500 Subject: [PATCH] fix(secrets-ui): Reveal fails closed when its audit cannot be written (scadaproj#45) Secrets-015: RevealButton now writes the secret.reveal Success audit before showing the value and shows nothing ("audit record could not be written") if that write throws; the failure is logged as a warning with actor and name, never the value. The cancellation token is captured up front, so a component disposed mid-reveal can no longer turn the audit into a swallowed ObjectDisposedException. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- .../Components/RevealButton.razor | 57 +++++++++++----- .../RevealAuditFailureTests.cs | 67 +++++++++++++++++++ 2 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealAuditFailureTests.cs 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 378a1c8..e28541d 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 @@ -4,6 +4,7 @@ @inject ISecretCipher Cipher @inject IAuditWriter Audit @inject IServiceProvider Services +@using Microsoft.Extensions.Logging @* Components/RevealButton.razor — a policy-gated, audited reveal. The plaintext is loaded and shown only AFTER the click, straight from the store+cipher (a human reveal shows the CURRENT value, distinct from any cached machine resolve). The value never enters an audit field. *@ @@ -47,30 +48,39 @@ else /// The cascading authentication state, used as the fallback source of the audit actor. [CascadingParameter] private Task? AuthState { get; set; } + // Fail CLOSED on audit (Secrets-015): the plaintext is shown only after its secret.reveal + // Success audit has been written. Reveal is the most privileged operation here and the audit is + // the only record that a human saw the value; if the sink is down (or made to fail), nothing is + // shown and the failure is logged (actor + name, never the value). Unlike add/rotate/delete + // (scadaproj#34), nothing has happened yet that a failed audit would misreport — withholding the + // value costs only a retry. private async Task RevealAsync() { _busy = true; + // Captured now: if the component is disposed mid-reveal, reading _cts.Token later would throw + // ObjectDisposedException; a captured token just reports cancellation, which fails closed. + CancellationToken ct = _cts.Token; + string? plaintext = null; AuditOutcome outcome; + string? error; try { - StoredSecret? row = await Store.GetAsync(Name, _cts.Token); + StoredSecret? row = await Store.GetAsync(Name, ct); if (row is null) { - _value = null; - _error = "Secret not found."; + error = "Secret not found."; outcome = AuditOutcome.Failure; } else { - _value = Cipher.Decrypt(row); - _error = null; + plaintext = Cipher.Decrypt(row); + error = null; outcome = AuditOutcome.Success; } } catch (SecretDecryptionException) { - _value = null; - _error = "Unable to decrypt secret."; + error = "Unable to decrypt secret."; outcome = AuditOutcome.Failure; } catch (Exception) @@ -78,17 +88,12 @@ else // 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."; + error = "Unable to reveal secret."; outcome = AuditOutcome.Failure; } - finally - { - _busy = false; - _revealed = true; - } string actor = await SecretActorResolver.ResolveAsync(Services, AuthState); + bool audited; try { await Audit.WriteAsync( @@ -103,12 +108,30 @@ else Target = Name.Value, DetailsJson = outcome == AuditOutcome.Failure ? "{\"reason\":\"error\"}" : null, }, - _cts.Token); + ct); + audited = true; } - catch + catch (Exception ex) { - // Best-effort audit; never let an audit-write fault surface into the event pipeline. + audited = false; + Services.GetService>()?.LogWarning( + ex, + "The secret.reveal audit of {SecretName} by {Actor} could not be written; {Disposition}.", + Name.Value, + actor, + outcome == AuditOutcome.Success ? "the value was NOT shown" : "the reveal had already failed"); } + + if (outcome == AuditOutcome.Success && !audited) + { + plaintext = null; + error = "Reveal is unavailable: the audit record could not be written."; + } + + _value = plaintext; + _error = error; + _busy = false; + _revealed = true; } private void Hide() diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealAuditFailureTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealAuditFailureTests.cs new file mode 100644 index 0000000..88f0d43 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/RevealAuditFailureTests.cs @@ -0,0 +1,67 @@ +using Bunit; +using Bunit.TestDoubles; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +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; + +/// +/// Secrets-015: a reveal whose audit write fails must not show the plaintext (fail closed), and the +/// failure must be logged — actor and name, never the value. +/// +public class RevealAuditFailureTests : TestContext +{ + private const string PlainValue = "the-plaintext-value-42"; + + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = new(); + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => Entries.Add((logLevel, formatter(state, exception))); + } + + [Fact] + public void A_reveal_whose_audit_fails_shows_nothing_and_logs_it() + { + var store = new RecordingSecretStore(); + var audit = new CapturingAuditWriter { SuccessFault = new InvalidOperationException("audit sink down") }; + var logger = new CapturingLogger(); + Services.AddSingleton(store); + Services.AddSingleton(audit); + Services.AddSingleton(new FakeCipher()); + Services.AddSingleton>(logger); + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("mallory"); + auth.SetPolicies(SecretsAuthorization.RevealPolicy); + + var name = new SecretName("prod/db"); + store.Seed(new FakeCipher().Encrypt(name, PlainValue, SecretContentType.Text) with + { + Revision = 1, + CreatedUtc = DateTimeOffset.UnixEpoch, + UpdatedUtc = DateTimeOffset.UnixEpoch, + }); + + var cut = RenderComponent(p => p.Add(c => c.Name, name)); + cut.Find("button.reveal-secret").Click(); + + Assert.DoesNotContain(PlainValue, cut.Markup); + Assert.Empty(cut.FindAll("[data-testid=revealed-value]")); + Assert.Contains("audit", cut.Find("[data-testid=reveal-error]").TextContent); + + (LogLevel level, string message) = Assert.Single(logger.Entries); + Assert.Equal(LogLevel.Warning, level); + Assert.Contains("prod/db", message); + Assert.Contains("mallory", message); + Assert.DoesNotContain(PlainValue, message); + } +}