fix(secrets-cli): shell — contain UnauthorizedAccess, flow-title guard, cancel-path test

2026-07-19 09:26:04 -04:00
parent e49496c856
commit 21f6a0df92
2 changed files with 120 additions and 17 deletions
@@ -48,6 +48,24 @@ public sealed class InteractiveShell
        ArgumentNullException.ThrowIfNull(sessions);
        ArgumentNullException.ThrowIfNull(flows);

+        // A flow's Title is the menu routing key (BuildMenu emits it; the loop dispatches with a
+        // .First() title match), so a duplicate — or a collision with a reserved built-in label —
+        // would silently misroute selections. Reject both at construction rather than at click time.
+        HashSet<string> seenTitles = new(StringComparer.Ordinal);
+        foreach (IInteractiveFlow flow in flows)
+        {
+            if (flow.Title is SwitchLabel or QuitLabel)
+            {
+                throw new ArgumentException(
+                    $"Flow title '{flow.Title}' collides with a reserved menu action.", nameof(flows));
+            }
+
+            if (!seenTitles.Add(flow.Title))
+            {
+                throw new ArgumentException($"Duplicate flow title '{flow.Title}'.", nameof(flows));
+            }
+        }
+
        _console = console;
        _recents = recents;
        _reader = reader;
@@ -60,23 +78,37 @@ public sealed class InteractiveShell
    /// <returns><c>0</c> on a clean quit.</returns>
    public async Task<int> RunAsync(CancellationToken ct)
    {
-        while (true)
+        try
        {
-            StoreTarget? target = PickTarget();
-            if (target is null)
+            while (true)
            {
-                return 0; // operator cancelled the picker (Ctrl-C) → exit
+                if (ct.IsCancellationRequested)
+                {
+                    return 0;
+                }
+
+                StoreTarget? target = PickTarget();
+                if (target is null)
+                {
+                    return 0; // operator cancelled the picker (Ctrl-C) → exit
+                }
+
+                SecretsSession session = await _sessions.OpenAsync(target, overrideKek: null, ct).ConfigureAwait(false);
+                RenderWarnings(session);
+
+                if (await RunMenuLoopAsync(session, ct).ConfigureAwait(false) == MenuOutcome.Quit)
+                {
+                    return 0;
+                }
+
+                // MenuOutcome.SwitchTarget → fall through and re-open the picker.
            }
-
-            SecretsSession session = await _sessions.OpenAsync(target, overrideKek: null, ct).ConfigureAwait(false);
-            RenderWarnings(session);
-
-            if (await RunMenuLoopAsync(session, ct).ConfigureAwait(false) == MenuOutcome.Quit)
-            {
-                return 0;
-            }
-
-            // MenuOutcome.SwitchTarget → fall through and re-open the picker.
+        }
+        catch (OperationCanceledException)
+        {
+            // Cooperative cancellation of the shell's token (Ctrl-C, or a flow/host cancelling it) is a
+            // clean shutdown, not an error — surfaced e.g. from a pre-cancelled session open.
+            return 0;
        }
    }

@@ -91,6 +123,14 @@ public sealed class InteractiveShell
    {
        while (true)
        {
+            // Honor cooperative cancellation between actions. Spectre's blocking prompt cannot observe
+            // the token itself (TestConsole likewise cannot inject a Ctrl-C into a prompt), so this
+            // top-of-loop check is what turns a token cancelled during a flow into a clean exit.
+            if (ct.IsCancellationRequested)
+            {
+                return MenuOutcome.Quit;
+            }
+
            RenderHeader(session);

            string choice;
@@ -140,10 +180,13 @@ public sealed class InteractiveShell

    // The store/key faults a flow may realistically surface. SecretDecryptionException and
    // MasterKeyUnavailableException both derive from InvalidOperationException; DbException (base of both
-    // Microsoft.Data.Sqlite and Microsoft.Data.SqlClient exceptions) covers store-provider faults. A
-    // blanket catch is deliberately avoided so genuinely unexpected faults still surface.
+    // Microsoft.Data.Sqlite and Microsoft.Data.SqlClient exceptions) covers store-provider faults;
+    // UnauthorizedAccessException (a permission-denied key file or appsettings — NOT an IOException
+    // subtype) is exactly the lockout path this console exists to recover, so it must be contained too.
+    // A blanket catch is deliberately avoided so genuinely unexpected faults still surface.
    private static bool IsContainable(Exception ex) =>
-        ex is InvalidOperationException or IOException or ArgumentException or DbException;
+        ex is InvalidOperationException or IOException or ArgumentException or DbException
+            or UnauthorizedAccessException;

    // Presents the target picker: recents newest-first, then the two entry options. Returns the resolved
    // target, or null if the operator cancelled the selection (Ctrl-C). Re-prompts on a recoverable
@@ -256,6 +256,66 @@ public sealed class InteractiveShellTests : IDisposable
        Assert.Contains("KEK doctor", console.Output, StringComparison.Ordinal);
    }

+    [Fact]
+    public async Task Flow_throwing_UnauthorizedAccess_is_contained_and_returns_to_menu()
+    {
+        var flow = new FakeFlow(
+            "Read key file",
+            requiresKek: false,
+            onRun: _ => throw new UnauthorizedAccessException("permission denied"));
+
+        TestConsole console = NewConsole();
+        InteractiveShell shell = NewShell(console, NewRecents(), flow);
+
+        ScriptManualEntry(console, UnsetEnvVarName());
+        SelectIndex(console, 0); // run the throwing flow
+        SelectIndex(console, 2); // loop returned to menu → Quit
+
+        int result = await shell.RunAsync(CancellationToken.None);
+
+        Assert.Equal(0, result);
+        Assert.Equal(1, flow.InvocationCount);
+        Assert.Contains("KEK doctor", console.Output, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void Constructor_rejects_duplicate_flow_titles()
+    {
+        var first = new FakeFlow("Same title");
+        var second = new FakeFlow("Same title");
+
+        Assert.Throws<ArgumentException>(() => NewShell(NewConsole(), NewRecents(), first, second));
+    }
+
+    [Fact]
+    public void Constructor_rejects_flow_title_colliding_with_a_builtin()
+    {
+        var reserved = new FakeFlow("Quit");
+
+        Assert.Throws<ArgumentException>(() => NewShell(NewConsole(), NewRecents(), reserved));
+    }
+
+    [Fact]
+    public async Task Cancelled_token_exits_menu_loop_cleanly_with_zero()
+    {
+        using var cts = new CancellationTokenSource();
+        // The flow cancels the shell's own token mid-action; the menu loop's top-of-loop cancellation
+        // check must then exit 0 without prompting for (unscripted) further input or throwing. This is
+        // the testable stand-in for a menu-level Ctrl-C, which TestConsole cannot inject into a prompt.
+        var flow = new FakeFlow("Cancel the shell", requiresKek: false, onRun: _ => cts.Cancel());
+
+        TestConsole console = NewConsole();
+        InteractiveShell shell = NewShell(console, NewRecents(), flow);
+
+        ScriptManualEntry(console, UnsetEnvVarName());
+        SelectIndex(console, 0); // run the flow, which cancels the token — no more input is scripted
+
+        int result = await shell.RunAsync(cts.Token);
+
+        Assert.Equal(0, result);
+        Assert.Equal(1, flow.InvocationCount);
+    }
+
    [Fact]
    public async Task Switch_target_reopens_picker()
    {