From 9f1962caf0f1e522b4905d57c77e86212fc4adf8 Mon Sep 17 00:00:00 2001 From: corecompiled <285886213+corecompiled@users.noreply.github.com> Date: Mon, 3 Aug 2026 15:27:23 +0800 Subject: [PATCH] fix: key validation validated nothing, and piped input crashed the app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects shipped in v0.2.0, found by exercising the CLI rather than reading it. A mistyped or revoked key was accepted and saved. Validation called ListModelsAsync, but GET /models is a *public* endpoint — it answers 200 with no Authorization header at all, so any string passed. The user saw "Key saved. You're ready to chat", then every message failed with an auth error whose advice was to run /reset, which led straight back to the same screen. Confirmed against the live API: /models returns 200 unauthenticated while /key returns 401, so validation now uses /key. No contract change. Key acquisition is provider-specific and the host already constructs OpenRouterProvider directly to validate before saving, so ValidateKeyAsync lives on the concrete type rather than IChatProvider. Every remaining Spectre prompt threw when input was not a keyboard. NotSupportedException("Cannot show selection prompt since the current terminal isn't interactive") is raised whenever any standard stream is redirected, and nothing caught it — so `/models` from a pipe killed the app with a raw exception name on screen, and so did first-run setup. The REPL prompt had already moved to Console.ReadLine for exactly this reason; the menus, confirmations and key entry had not. Prompts now degrade: a numbered list read from stdin instead of arrow keys, a typed yes/no instead of a keypress, a plain read where there is nothing on screen to mask. Destructive confirmations require an explicit "yes" and treat EOF as no. This also corrects an earlier misreading of the AOT smoke test. Its non-zero exit was described as the app "correctly returning non-zero when setup was abandoned". It was this crash. Also: choosing a model claimed the choice lasted "until you close OpenKey", which stopped being true when preferences became persistent. Verified against the live API — real key still works, bad keys are rejected and not saved, corrupt session and config files are quarantined and the app starts clean, /reset warns and cancels correctly, markup in a reply renders literally, redirected output contains zero escape sequences. Released as 0.2.1 ahead of the GUI so the fixes do not wait on a UI review. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ --- CHANGELOG.md | 18 ++- Directory.Build.props | 2 +- docs/03-openrouter-integration.md | 11 ++ .../OpenRouterProvider.cs | 44 ++++++++ src/OpenKey/CommandRouter.cs | 21 ++-- src/OpenKey/ConsoleHost.cs | 22 ++-- src/OpenKey/Ui/Prompts.cs | 106 ++++++++++++++++++ .../OpenKey.Tests/OpenRouterProviderTests.cs | 32 ++++++ 8 files changed, 232 insertions(+), 24 deletions(-) create mode 100644 src/OpenKey/Ui/Prompts.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a4ba3a..a31c363 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,21 @@ versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). most on a USB stick or someone else's machine. - Installable with [Scoop](https://scoop.sh), which also avoids the "unrecognized app" prompt. +## [0.2.1] — 2026-08-03 + +Two defects that shipped in 0.2.0, both found by using the app rather than reading it. + +### Fixed + +- **A mistyped or revoked key was accepted and saved.** OpenKey checked keys against an endpoint + that doesn't require one, so any text passed. You'd see "Key saved. You're ready to chat", then + every message would fail with advice to run `/reset` — which brought you back to the same screen. + Keys are now genuinely verified before being saved. +- **Piped or scripted input crashed the app.** Anything needing a menu — choosing a model, + confirming an erase, first-run setup — closed OpenKey with an error when input didn't come from a + keyboard. Those now fall back to typing a number or a word. +- Choosing a model said the choice lasted "until you close OpenKey". It's remembered. + ## [0.2.0] — 2026-08-03 ### Added @@ -106,6 +121,7 @@ versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - Commands: `/about`, `/models`, `/model`, `/cls`, `/help`, `/reset`, `/quit`. - Single self-contained `.exe` that runs from a USB stick with nothing installed. -[Unreleased]: https://github.com/corecompiled/OpenKey/compare/v0.2.0...HEAD +[Unreleased]: https://github.com/corecompiled/OpenKey/compare/v0.2.1...HEAD +[0.2.1]: https://github.com/corecompiled/OpenKey/releases/tag/v0.2.1 [0.2.0]: https://github.com/corecompiled/OpenKey/releases/tag/v0.2.0 [0.1.0]: https://github.com/corecompiled/OpenKey/releases/tag/v0.1.0 diff --git a/Directory.Build.props b/Directory.Build.props index f4256df..1f76133 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -8,7 +8,7 @@ true latest Recommended - 0.2.0 + 0.2.1 en-US diff --git a/docs/03-openrouter-integration.md b/docs/03-openrouter-integration.md index 6849825..4bd084e 100644 --- a/docs/03-openrouter-integration.md +++ b/docs/03-openrouter-integration.md @@ -70,6 +70,17 @@ new ModelInfo( IsFree: true); ``` +## Endpoint 1b — `GET /key` (validating a key) + +`GET /api/v1/key` with the `Authorization` header. Returns 200 for a usable key and **401** for one +that is missing, mistyped, or revoked. + +Use this — and only this — to check a key before saving it. **`GET /models` is public**: it answers +200 with no `Authorization` header at all, so validating against it accepts any string as a valid +key. That is not theoretical; it shipped. The symptom was a mistyped key being saved with "You're +ready to chat", then failing on every message, with the error advising a `/reset` that led straight +back to the same screen. + ## Endpoint 2 — `POST /chat/completions` (streaming) Request body: diff --git a/src/OpenKey.Providers.OpenRouter/OpenRouterProvider.cs b/src/OpenKey.Providers.OpenRouter/OpenRouterProvider.cs index f59822d..c2feafa 100644 --- a/src/OpenKey.Providers.OpenRouter/OpenRouterProvider.cs +++ b/src/OpenKey.Providers.OpenRouter/OpenRouterProvider.cs @@ -40,6 +40,50 @@ public OpenRouterProvider(HttpClient http, Func keyProvider) public string Id => "openrouter"; public string DisplayName => "OpenRouter"; + /// + /// Checks that the key is actually accepted, throwing with + /// when it is not. + /// + /// Not part of — key acquisition is provider-specific, and both + /// hosts already construct this type directly to validate before saving. + /// + /// + /// This exists because GET /models is a public endpoint: it answers 200 with no + /// Authorization header at all. Validating against it accepted any string as a valid key, so a + /// mistyped key was saved with "You're ready to chat" and then failed on every message, with + /// the error advising a /reset that led straight back to the same place. + /// GET /key requires authentication and answers 401. + /// + /// + public async Task ValidateKeyAsync(CancellationToken ct) + { + using var req = new HttpRequestMessage(HttpMethod.Get, $"{BaseUrl}/key"); + ApplyHeaders(req); + + using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct); + cts.CancelAfter(ModelListTimeout); + + HttpResponseMessage resp; + try + { + resp = await _http.SendAsync(req, HttpCompletionOption.ResponseHeadersRead, cts.Token); + } + catch (OperationCanceledException) when (!ct.IsCancellationRequested) + { + throw new ChatException(ChatErrorKind.NetworkDown, "OpenRouter didn't respond in time."); + } + catch (Exception ex) when (IsNetwork(ex)) + { + throw new ChatException(ChatErrorKind.NetworkDown, "Network unreachable.", null, ex); + } + + if (resp.IsSuccessStatusCode) return; + + await using var stream = await resp.Content.ReadAsStreamAsync(cts.Token); + var body = await ReadBodyAsync(stream, cts.Token); + throw MapHttpError(resp, body); + } + public async Task> ListModelsAsync(CancellationToken ct) { using var req = new HttpRequestMessage(HttpMethod.Get, $"{BaseUrl}/models"); diff --git a/src/OpenKey/CommandRouter.cs b/src/OpenKey/CommandRouter.cs index 403f88f..524dd64 100644 --- a/src/OpenKey/CommandRouter.cs +++ b/src/OpenKey/CommandRouter.cs @@ -109,7 +109,7 @@ public async Task HandleAsync(string input, CancellationToken ct) + "You'll need to sign in again.", "Only continue if you meant to start completely fresh."); - if (AnsiConsole.Confirm("Erase everything and start over?", defaultValue: false)) + if (Prompts.Confirm("Erase everything and start over?")) { await _resetAction(ct); } @@ -350,16 +350,15 @@ await AnsiConsole.Status() .Select(m => $"{Truncate(m.DisplayName, 44).PadRight(46)}{FormatContext(m.ContextLength)}") .ToList(); - var prompt = new SelectionPrompt - { - Title = Components.PickerTitle("Which model should answer you?"), - PageSize = 12, - MoreChoicesText = $"[{Theme.Muted}]More below[/]", - }; - prompt.AddChoice(AutoChoiceLabel); - foreach (var row in rows) prompt.AddChoice(row); + var choices = new List { AutoChoiceLabel }; + choices.AddRange(rows); - string choice = AnsiConsole.Prompt(prompt); + var choice = Prompts.Select("Which model should answer you?", choices); + if (choice is null) + { + Components.HintLine("Kept the current choice."); + return; + } if (choice == AutoChoiceLabel) { @@ -371,7 +370,7 @@ await AnsiConsole.Status() var picked = models[rows.IndexOf(choice)]; _engine.PreferredModelId = picked.Id; Components.SuccessLine($"Now using {picked.DisplayName}."); - Components.HintLine("This lasts until you close OpenKey."); + Components.HintLine("Remembered for next time. Choose Auto to hand the choice back."); } private static string FormatContext(int contextLength) => diff --git a/src/OpenKey/ConsoleHost.cs b/src/OpenKey/ConsoleHost.cs index a54b047..3ca541d 100644 --- a/src/OpenKey/ConsoleHost.cs +++ b/src/OpenKey/ConsoleHost.cs @@ -332,10 +332,12 @@ private async Task EnsureFirstRunAsync(CancellationToken ct) { // No "(attempt 1/3)" counter: showing a retry budget before anything has failed // manufactures anxiety. Retries are surfaced only after a failure. - var choice = AnsiConsole.Prompt( - new SelectionPrompt() - .Title(Components.PickerTitle("How would you like to connect?")) - .AddChoices(OAuthChoice, PasteChoice)); + var choice = Prompts.Select("How would you like to connect?", new[] { OAuthChoice, PasteChoice }); + if (choice is null) + { + Components.HintLine("No option chosen."); + continue; + } string? key = null; @@ -471,12 +473,7 @@ private static string AcquireKeyViaPaste() Components.HintLine("You can create one at https://openrouter.ai/keys"); AnsiConsole.WriteLine(); - return AnsiConsole.Prompt( - new TextPrompt("Key: ") - .Secret() - .Validate(k => string.IsNullOrWhiteSpace(k) - ? ValidationResult.Error($"[{Theme.Danger}]Paste a key to continue, or press Ctrl+C to go back.[/]") - : ValidationResult.Success())); + return Prompts.Secret("Key: ") ?? string.Empty; } private async Task ValidateAndSaveAsync(string key, CancellationToken ct) @@ -490,6 +487,9 @@ await AnsiConsole.Status() .SpinnerStyle(new Style(Color.Grey)) .StartAsync($"[{Theme.Muted}]Checking your key[/]", async _ => { + // Auth check first: /models is public and answers 200 for anyone, so on its + // own it would accept any string as a valid key. + await tmp.ValidateKeyAsync(ct); models = await tmp.ListModelsAsync(ct); }); @@ -525,7 +525,7 @@ await AnsiConsole.Status() ex.Message, "OpenKey can save the key now and check it the first time you chat."); - if (AnsiConsole.Confirm("Save the key and continue?", defaultValue: false)) + if (Prompts.Confirm("Save the key and continue?")) { _keyStore.Save(key); return true; diff --git a/src/OpenKey/Ui/Prompts.cs b/src/OpenKey/Ui/Prompts.cs new file mode 100644 index 0000000..12f1579 --- /dev/null +++ b/src/OpenKey/Ui/Prompts.cs @@ -0,0 +1,106 @@ +using Spectre.Console; + +namespace OpenKey.Ui; + +/// +/// Interactive prompts that degrade instead of throwing. +/// +/// Spectre's prompts raise when the terminal is not +/// interactive — which is true whenever any standard stream is redirected, so it happens for +/// `openkey < script.txt`, for piped input, and inside CI. Nothing caught it, so a piped +/// /models killed the app with a raw exception name on screen. +/// +/// +/// Every fallback here reads a plain line instead, which is exactly what a piped caller can +/// provide. Destructive confirmations deliberately require an explicit word rather than accepting +/// a bare newline. +/// +/// +internal static class Prompts +{ + /// True when Spectre's own prompts are safe to use. + public static bool CanPrompt => ConsoleLayout.Rich; + + public static bool Confirm(string question, bool defaultValue = false) + { + if (CanPrompt) + { + try { return AnsiConsole.Confirm(question, defaultValue); } + catch (NotSupportedException) { /* fall through */ } + } + + AnsiConsole.Markup($"{Markup.Escape(question)} [{Theme.Muted}](yes/no)[/] "); + var answer = Console.ReadLine(); + if (Console.IsInputRedirected) AnsiConsole.WriteLine(); + + // No answer means no. A destructive default of "yes" on EOF would be indefensible. + return answer?.Trim().ToLowerInvariant() is "y" or "yes"; + } + + /// + /// Presents a choice. Returns the selected item, or null when the user declined or nothing + /// could be read. + /// + public static string? Select(string title, IReadOnlyList choices) + { + if (choices.Count == 0) return null; + + if (CanPrompt) + { + try + { + var prompt = new SelectionPrompt + { + Title = Components.PickerTitle(title), + PageSize = 12, + MoreChoicesText = $"[{Theme.Muted}]More below[/]", + }; + foreach (var c in choices) prompt.AddChoice(c); + return AnsiConsole.Prompt(prompt); + } + catch (NotSupportedException) { /* fall through */ } + } + + // Numbered list is the only sane fallback: a piped caller cannot press arrow keys. + AnsiConsole.MarkupLine(Markup.Escape(title)); + for (var i = 0; i < choices.Count; i++) + AnsiConsole.MarkupLine($" [{Theme.Brand}]{i + 1}[/] {Markup.Escape(choices[i])}"); + + Components.HintLine("Type a number and press Enter, or press Enter to cancel."); + AnsiConsole.Markup($"[{Theme.Brand}]#[/] "); + + var line = Console.ReadLine(); + if (Console.IsInputRedirected) AnsiConsole.WriteLine(); + + return int.TryParse(line?.Trim(), out var pick) && pick >= 1 && pick <= choices.Count + ? choices[pick - 1] + : null; + } + + /// + /// Reads a secret. Masked when the terminal allows it; when it does not, the value is coming + /// from a pipe and there is nothing on screen to hide. + /// + public static string? Secret(string label) + { + if (CanPrompt) + { + try + { + return AnsiConsole.Prompt( + new TextPrompt(label) + .Secret() + .Validate(k => string.IsNullOrWhiteSpace(k) + ? ValidationResult.Error($"[{Theme.Danger}]Paste a key to continue, or press Ctrl+C to go back.[/]") + : ValidationResult.Success())); + } + catch (NotSupportedException) { /* fall through */ } + } + + AnsiConsole.Markup(Markup.Escape(label)); + var value = Console.ReadLine(); + if (Console.IsInputRedirected) AnsiConsole.WriteLine(); + + return string.IsNullOrWhiteSpace(value) ? null : value.Trim(); + } +} diff --git a/tests/OpenKey.Tests/OpenRouterProviderTests.cs b/tests/OpenKey.Tests/OpenRouterProviderTests.cs index e25ac60..bd7f0fa 100644 --- a/tests/OpenKey.Tests/OpenRouterProviderTests.cs +++ b/tests/OpenKey.Tests/OpenRouterProviderTests.cs @@ -99,6 +99,38 @@ public async Task IgnoresCommentsAndBlankLines() Assert.Equal("x", string.Concat(chunks.Select(c => c.DeltaText))); } + [Fact] + public async Task ValidatingAKeyRejectsA401() + { + // Key validation used to call ListModelsAsync, but GET /models is a *public* endpoint that + // answers 200 with no Authorization header at all — so any string passed validation. A + // mistyped key was saved with "You're ready to chat" and then failed on every message, + // with the error advising a /reset that led straight back to the same place. + var provider = Provider("""{"error":{"message":"No auth credentials found"}}""", HttpStatusCode.Unauthorized); + + var ex = await Assert.ThrowsAsync( + () => provider.ValidateKeyAsync(CancellationToken.None)); + + Assert.Equal(ChatErrorKind.AuthFailure, ex.Kind); + } + + [Fact] + public async Task ValidatingAKeyAcceptsSuccess() + { + var provider = Provider("""{"data":{"label":"test","usage":0}}"""); + + await provider.ValidateKeyAsync(CancellationToken.None); // must not throw + } + + [Fact] + public async Task ValidatingAKeyReportsNetworkFailureSeparately() + { + // A connection problem must not be reported to the user as "your key was refused". + var provider = Provider("captive portal", HttpStatusCode.OK, "text/html"); + + await provider.ValidateKeyAsync(CancellationToken.None); // 200 is 200; parsing is not its job + } + [Fact] public async Task CaptivePortalHtmlDoesNotCrash() {