From fe94b32d429022bf5e3f665a0eaea3ead14d4022 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 15:17:59 -0500 Subject: [PATCH] fix(secrets-ui): Add refuses an existing name; restoring a tombstone needs a second submit (scadaproj#45) Secrets-014: SecretEditor in add mode reads the name first. A live row is refused ("already exists. Use Rotate") with no write and no audit, so a pasted name can no longer replace a live credential and reset its description/content type under a "secret.add" audit. A tombstoned name is restored only when the operator submits the same name a second time after the warning. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- .../Components/SecretEditor.razor | 41 ++++++++ .../SecretEditorAddExistingTests.cs | 97 +++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorAddExistingTests.cs 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 5f9105c..9a9e497 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 @@ -129,6 +129,22 @@ try { + if (!IsRotate) + { + // Add must never silently overwrite (Secrets-014): the store's upsert replaces the + // value, clears a tombstone and resets description/content type, and it would be + // audited as "add". A live row is refused; a tombstone needs a second, deliberate + // submit of the same name. (Check-then-write is not atomic; a concurrent add of the + // same name in another session can still race — the window is the round trip.) + StoredSecret? existing = await Store.GetAsync(name, _cts.Token); + string? refusal = AddRefusal(name, existing); + if (refusal is not null) + { + _error = refusal; + return; + } + } + StoredSecret row = Cipher.Encrypt(name, _value, _contentType); row = row with { @@ -177,6 +193,31 @@ } } + // The name whose tombstone the operator has been warned about; a second submit of the SAME + // name restores it. Any other name starts over. + private string? _restoreConfirmedFor; + + private string? AddRefusal(SecretName name, StoredSecret? existing) + { + if (existing is null) + { + return null; + } + + if (!existing.IsDeleted) + { + return $"A secret named '{name.Value}' already exists. Use Rotate to change its value."; + } + + if (string.Equals(_restoreConfirmedFor, name.Value, StringComparison.Ordinal)) + { + return null; + } + + _restoreConfirmedFor = name.Value; + return $"A deleted secret named '{name.Value}' exists. Select Add again to restore it with this value."; + } + private async Task WriteSuccessAuditAsync(string actor, string action, string target) { try diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorAddExistingTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorAddExistingTests.cs new file mode 100644 index 0000000..a0f3623 --- /dev/null +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Ui.Tests/SecretEditorAddExistingTests.cs @@ -0,0 +1,97 @@ +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; + +/// +/// Secrets-014: "Add secret" on a name that already exists must not silently overwrite it (value, +/// description, content type) and audit the overwrite as secret.add. A live row is refused; +/// a tombstoned one is restored only on a second, deliberate submit of the same name. +/// +public class SecretEditorAddExistingTests : TestContext +{ + private (RecordingSecretStore Store, CapturingAuditWriter Audit, FakeCipher Cipher) Setup() + { + var store = new RecordingSecretStore(); + var audit = new CapturingAuditWriter(); + var cipher = new FakeCipher(); + Services.AddSingleton(store); + Services.AddSingleton(audit); + Services.AddSingleton(cipher); + var auth = this.AddTestAuthorization(); + auth.SetAuthorized("carol"); + auth.SetPolicies(SecretsAuthorization.ManagePolicy); + return (store, audit, cipher); + } + + private static StoredSecret Existing(FakeCipher cipher, string name, bool deleted) => + cipher.Encrypt(new SecretName(name), "live-credential", SecretContentType.ConnectionString) with + { + Description = "central db", + Revision = 4, + IsDeleted = deleted, + CreatedUtc = DateTimeOffset.UnixEpoch, + UpdatedUtc = DateTimeOffset.UnixEpoch, + }; + + private IRenderedComponent FillAdd(string name, string value) + { + var cut = RenderComponent(p => p.Add(c => c.OnSaved, () => { })); + cut.Find("[data-testid=secret-name]").Change(name); + cut.Find("[data-testid=secret-value]").Change(value); + return cut; + } + + [Fact] + public void Adding_an_existing_live_name_is_refused_and_writes_nothing() + { + var (store, audit, cipher) = Setup(); + store.Seed(Existing(cipher, "db/central", deleted: false)); + + var cut = FillAdd("db/central", "pasted-from-other-env"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Empty(store.Upserts); + Assert.Empty(audit.Events); + Assert.Contains("already exists", cut.Find("[data-testid=editor-error]").TextContent); + + // A second click does not get round the refusal. + cut.Find("[data-testid=editor-submit]").Click(); + Assert.Empty(store.Upserts); + } + + [Fact] + public void Adding_a_tombstoned_name_needs_a_second_submit_to_restore() + { + var (store, audit, cipher) = Setup(); + store.Seed(Existing(cipher, "db/retired", deleted: true)); + + var cut = FillAdd("db/retired", "fresh-value"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Empty(store.Upserts); + Assert.Contains("deleted secret", cut.Find("[data-testid=editor-error]").TextContent); + + cut.Find("[data-testid=editor-submit]").Click(); + + StoredSecret row = Assert.Single(store.Upserts); + Assert.Equal("fresh-value", cipher.Decrypt(row)); + Assert.Equal("secret.add", Assert.Single(audit.Events).Action); + } + + [Fact] + public void Adding_a_new_name_still_works_in_one_submit() + { + var (store, _, _) = Setup(); + + var cut = FillAdd("db/new", "v"); + cut.Find("[data-testid=editor-submit]").Click(); + + Assert.Single(store.Upserts); + } +}