From e5a4d0276a96722c5cbd278fb258acb76615837b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 19 Jul 2026 09:47:10 -0400 Subject: [PATCH] =?UTF-8?q?fix(secrets-cli):=20CRUD=20flows=20=E2=80=94=20?= =?UTF-8?q?escaped=20validation=20markup,=20shared=20prompt=20helpers,=20d?= =?UTF-8?q?elete=20existence=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Interactive/Flows/DeleteSecretFlow.cs | 27 +++++-------- .../Interactive/Flows/FlowPrompts.cs | 40 +++++++++++++++++++ .../Interactive/Flows/RevealSecretFlow.cs | 21 +++------- .../Interactive/Flows/SetSecretFlow.cs | 23 +++-------- .../Cli/Interactive/Flows/CrudFlowTests.cs | 37 +++++++++++++++++ 5 files changed, 98 insertions(+), 50 deletions(-) create mode 100644 ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/FlowPrompts.cs diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/DeleteSecretFlow.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/DeleteSecretFlow.cs index e2bcec8..44a70b2 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/DeleteSecretFlow.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/DeleteSecretFlow.cs @@ -23,7 +23,15 @@ public sealed class DeleteSecretFlow : IInteractiveFlow ArgumentNullException.ThrowIfNull(session); var name = new SecretName(console.Prompt( - new TextPrompt("Secret [green]name[/]:").Validate(NameValidation))); + new TextPrompt("Secret [green]name[/]:").Validate(FlowPrompts.ValidateSecretName))); + + // Check existence first, so an unknown name is reported without a pointless confirm prompt. + StoredSecret? existing = await session.Store.GetAsync(name, ct).ConfigureAwait(false); + if (existing is null || existing.IsDeleted) + { + console.MarkupLineInterpolated($"[red]No secret named '{name.Value}'.[/]"); + return; + } if (!console.Prompt(new ConfirmationPrompt($"Delete (tombstone) '{name.Value}'?") { DefaultValue = false })) { @@ -31,25 +39,10 @@ public sealed class DeleteSecretFlow : IInteractiveFlow return; } - string actor = string.IsNullOrWhiteSpace(Environment.UserName) ? "cli" : Environment.UserName; - bool removed = await session.Store.DeleteAsync(name, actor, ct).ConfigureAwait(false); + bool removed = await session.Store.DeleteAsync(name, FlowPrompts.Actor, ct).ConfigureAwait(false); console.MarkupLine(removed ? $"[green]Tombstoned '{Markup.Escape(name.Value)}'.[/]" : $"[red]No secret named '{Markup.Escape(name.Value)}'.[/]"); } - - // Re-prompts on an invalid name by surfacing the SecretName constructor's own diagnostic. - private static ValidationResult NameValidation(string raw) - { - try - { - _ = new SecretName(raw); - return ValidationResult.Success(); - } - catch (ArgumentException ex) - { - return ValidationResult.Error(ex.Message); - } - } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/FlowPrompts.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/FlowPrompts.cs new file mode 100644 index 0000000..0021e33 --- /dev/null +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/FlowPrompts.cs @@ -0,0 +1,40 @@ +using Spectre.Console; +using ZB.MOM.WW.Secrets.Abstractions; + +namespace ZB.MOM.WW.Secrets.Cli.Interactive.Flows; + +/// +/// Shared prompt/console helpers reused across the CRUD flows: a re-prompting secret-name validator and +/// the CLI actor-stamp fallback. Centralized so the validation and audit-actor conventions stay identical +/// in every flow. +/// +internal static class FlowPrompts +{ + /// + /// A validation callback that accepts a raw name iff it is a legal + /// , otherwise re-prompts with the constructor's own diagnostic. The + /// diagnostic is d because it embeds bracketed text (e.g. the + /// allowed-character class [a-z0-9._/-]) that Spectre would otherwise parse as markup and throw on. + /// + /// The raw name entered at the prompt. + /// Success when is a valid name; an escaped error otherwise. + public static ValidationResult ValidateSecretName(string raw) + { + try + { + _ = new SecretName(raw); + return ValidationResult.Success(); + } + catch (ArgumentException ex) + { + return ValidationResult.Error(Markup.Escape(ex.Message)); + } + } + + /// + /// The principal recorded as created_by/updated_by for CLI-originated writes — the OS + /// user name, or the literal "cli" when it is unavailable (mirrors Program.cs). + /// + public static string Actor => + string.IsNullOrWhiteSpace(Environment.UserName) ? "cli" : Environment.UserName; +} diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/RevealSecretFlow.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/RevealSecretFlow.cs index e36a4fd..4099374 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/RevealSecretFlow.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/RevealSecretFlow.cs @@ -23,8 +23,11 @@ public sealed class RevealSecretFlow : IInteractiveFlow ArgumentNullException.ThrowIfNull(console); ArgumentNullException.ThrowIfNull(session); + ISecretCipher cipher = session.Cipher + ?? throw new InvalidOperationException("flow requires an unlocked KEK session"); + var name = new SecretName(console.Prompt( - new TextPrompt("Secret [green]name[/]:").Validate(NameValidation))); + new TextPrompt("Secret [green]name[/]:").Validate(FlowPrompts.ValidateSecretName))); StoredSecret? row = await session.Store.GetAsync(name, ct).ConfigureAwait(false); if (row is null || row.IsDeleted) @@ -38,20 +41,6 @@ public sealed class RevealSecretFlow : IInteractiveFlow return; } - console.WriteLine(session.Cipher!.Decrypt(row)); - } - - // Re-prompts on an invalid name by surfacing the SecretName constructor's own diagnostic. - private static ValidationResult NameValidation(string raw) - { - try - { - _ = new SecretName(raw); - return ValidationResult.Success(); - } - catch (ArgumentException ex) - { - return ValidationResult.Error(ex.Message); - } + console.WriteLine(cipher.Decrypt(row)); } } diff --git a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/SetSecretFlow.cs b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/SetSecretFlow.cs index 836ec07..411d4b7 100644 --- a/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/SetSecretFlow.cs +++ b/ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/Flows/SetSecretFlow.cs @@ -23,8 +23,11 @@ public sealed class SetSecretFlow : IInteractiveFlow ArgumentNullException.ThrowIfNull(console); ArgumentNullException.ThrowIfNull(session); + ISecretCipher cipher = session.Cipher + ?? throw new InvalidOperationException("flow requires an unlocked KEK session"); + var name = new SecretName(console.Prompt( - new TextPrompt("Secret [green]name[/]:").Validate(NameValidation))); + new TextPrompt("Secret [green]name[/]:").Validate(FlowPrompts.ValidateSecretName))); StoredSecret? existing = await session.Store.GetAsync(name, ct).ConfigureAwait(false); if (existing is { IsDeleted: false } && @@ -40,8 +43,8 @@ public sealed class SetSecretFlow : IInteractiveFlow string description = console.Prompt( new TextPrompt("Description [grey](optional)[/]:").AllowEmpty()); - string actor = string.IsNullOrWhiteSpace(Environment.UserName) ? "cli" : Environment.UserName; - StoredSecret row = session.Cipher!.Encrypt(name, value, contentType) with + string actor = FlowPrompts.Actor; + StoredSecret row = cipher.Encrypt(name, value, contentType) with { Description = string.IsNullOrWhiteSpace(description) ? null : description, CreatedBy = actor, @@ -51,18 +54,4 @@ public sealed class SetSecretFlow : IInteractiveFlow console.MarkupLineInterpolated($"[green]Sealed '{name.Value}'.[/]"); } - - // Re-prompts on an invalid name by surfacing the SecretName constructor's own diagnostic. - private static ValidationResult NameValidation(string raw) - { - try - { - _ = new SecretName(raw); - return ValidationResult.Success(); - } - catch (ArgumentException ex) - { - return ValidationResult.Error(ex.Message); - } - } } diff --git a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/CrudFlowTests.cs b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/CrudFlowTests.cs index 3a69732..7b108c4 100644 --- a/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/CrudFlowTests.cs +++ b/ZB.MOM.WW.Secrets/tests/ZB.MOM.WW.Secrets.Tests/Cli/Interactive/Flows/CrudFlowTests.cs @@ -122,6 +122,28 @@ public sealed class CrudFlowTests : IDisposable Assert.DoesNotContain("TOPSECRET-PLAINTEXT", console.Output, StringComparison.Ordinal); // never echoed } + [Fact] + public async Task Invalid_name_reprompts_then_completes() + { + SecretsSession session = await NewFullSessionAsync(); + + TestConsole console = NewConsole(); + console.Input.PushTextWithEnter("bad name"); // invalid (space) → must re-prompt, not throw + console.Input.PushTextWithEnter("good/name"); // valid retry + console.Input.PushTextWithEnter("VALUE-AFTER-REPROMPT"); // value + console.Input.PushKey(ConsoleKey.Enter); // content-type: Text + console.Input.PushKey(ConsoleKey.Enter); // description: empty + + // The bug this pins: SecretName's error text embeds "[a-z0-9._/-]", which Spectre parses as + // markup and throws InvalidOperationException on unless the validator escapes it. Reaching the + // assertion at all proves the re-prompt happened cleanly. + await new SetSecretFlow().RunAsync(console, session, CancellationToken.None); + + StoredSecret? row = await session.Store.GetAsync(new SecretName("good/name"), CancellationToken.None); + Assert.NotNull(row); + Assert.Equal("VALUE-AFTER-REPROMPT", session.Cipher!.Decrypt(row!)); + } + [Fact] public async Task Existing_name_confirms_overwrite() { @@ -201,6 +223,21 @@ public sealed class CrudFlowTests : IDisposable Assert.True(row!.IsDeleted); } + [Fact] + public async Task Unknown_name_reports_not_found_without_confirming() + { + SecretsSession session = await NewFullSessionAsync(); + + TestConsole console = NewConsole(); + console.Input.PushTextWithEnter("never/existed"); // name only — no confirm input is scripted + // If the flow prompted for a confirm before checking existence, the missing input would fail. + await new DeleteSecretFlow().RunAsync(console, session, CancellationToken.None); + + Assert.Contains("No secret named", console.Output, StringComparison.Ordinal); + StoredSecret? row = await session.Store.GetAsync(new SecretName("never/existed"), CancellationToken.None); + Assert.Null(row); + } + [Fact] public async Task Decline_leaves_row() {