Skip to content

Commit 041eb04

Browse files
corecompiledclaude
andcommitted
fix: key validation validated nothing, and piped input crashed the app
Two defects 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 got "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. It needs no contract change. Key acquisition is provider-specific and both hosts already construct 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 this reason; the menus, the confirmations and the 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 instead of a masked one where there is nothing on screen to mask. Destructive confirmations deliberately 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 OpenKey "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 end to end 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. 122 tests. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01WGcarVYpt1mfjk6iFkcDpZ
1 parent 630d731 commit 041eb04

8 files changed

Lines changed: 225 additions & 22 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
5050

5151
### Fixed
5252

53+
- **A mistyped or revoked key was accepted and saved.** OpenKey checked keys against an endpoint
54+
that does not require one, so any text passed — and then every message failed, with advice that
55+
led back to the same place. Keys are now genuinely verified before being saved.
56+
- **Piped or scripted input crashed the app.** Anything that needed a menu — choosing a model,
57+
confirming an erase, first-run setup — closed OpenKey with an error when input didn't come from
58+
a keyboard. Those now fall back to typing a number or a word.
59+
- Choosing a model said the choice lasted "until you close OpenKey"; it is remembered.
5360
- Links whose address contained a bracket lost their target when displayed.
5461
- A message that kept failing could retry for several minutes; it is now bounded, and a reply
5562
that is genuinely arriving is never cut off.

‎docs/03-openrouter-integration.md‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,17 @@ new ModelInfo(
7070
IsFree: true);
7171
```
7272

73+
## Endpoint 1b — `GET /key` (validating a key)
74+
75+
`GET /api/v1/key` with the `Authorization` header. Returns 200 for a usable key and **401** for one
76+
that is missing, mistyped, or revoked.
77+
78+
Use this — and only this — to check a key before saving it. **`GET /models` is public**: it answers
79+
200 with no `Authorization` header at all, so validating against it accepts any string as a valid
80+
key. That is not theoretical; it shipped. The symptom was a mistyped key being saved with "You're
81+
ready to chat", then failing on every message, with the error advising a `/reset` that led straight
82+
back to the same screen.
83+
7384
## Endpoint 2 — `POST /chat/completions` (streaming)
7485

7586
Request body:

‎src/OpenKey.Gui/ViewModels/MainWindowViewModel.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,10 @@ private async Task ValidateAndSaveAsync(string key)
218218
try
219219
{
220220
var probe = new OpenRouterProvider(_http, () => key);
221+
222+
// Auth check first: /models is public and answers 200 for anyone, so on its own it
223+
// would accept any string as a valid key.
224+
await probe.ValidateKeyAsync(CancellationToken.None);
221225
var models = await probe.ListModelsAsync(CancellationToken.None);
222226
if (models.Count == 0)
223227
{

‎src/OpenKey.Providers.OpenRouter/OpenRouterProvider.cs‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,50 @@ public OpenRouterProvider(HttpClient http, Func<string?> keyProvider)
4040
public string Id => "openrouter";
4141
public string DisplayName => "OpenRouter";
4242

43+
/// <summary>
44+
/// Checks that the key is actually accepted, throwing <see cref="ChatException"/> with
45+
/// <see cref="ChatErrorKind.AuthFailure"/> when it is not.
46+
/// <para>
47+
/// Not part of <see cref="IChatProvider"/> — key acquisition is provider-specific, and both
48+
/// hosts already construct this type directly to validate before saving.
49+
/// </para>
50+
/// <para>
51+
/// This exists because <c>GET /models</c> is a <b>public</b> endpoint: it answers 200 with no
52+
/// Authorization header at all. Validating against it accepted any string as a valid key, so a
53+
/// mistyped key was saved with "You're ready to chat" and then failed on every message, with
54+
/// the error advising a <c>/reset</c> that led straight back to the same place.
55+
/// <c>GET /key</c> requires authentication and answers 401.
56+
/// </para>
57+
/// </summary>
58+
public async Task ValidateKeyAsync(CancellationToken ct)
59+
{
60+
using var req = new HttpRequestMessage(HttpMethod.Get, $"{BaseUrl}/key");
61+
ApplyHeaders(req);
62+
63+
using var cts = CancellationTokenSource.CreateLinkedTokenSource(ct);
64+
cts.CancelAfter(ModelListTimeout);
65+
66+
HttpResponseMessage resp;
67+
try
68+
{
69+
resp = await _http.SendAsync(req, HttpCompletionOption.ResponseHeadersRead, cts.Token);
70+
}
71+
catch (OperationCanceledException) when (!ct.IsCancellationRequested)
72+
{
73+
throw new ChatException(ChatErrorKind.NetworkDown, "OpenRouter didn't respond in time.");
74+
}
75+
catch (Exception ex) when (IsNetwork(ex))
76+
{
77+
throw new ChatException(ChatErrorKind.NetworkDown, "Network unreachable.", null, ex);
78+
}
79+
80+
if (resp.IsSuccessStatusCode) return;
81+
82+
await using var stream = await resp.Content.ReadAsStreamAsync(cts.Token);
83+
var body = await ReadBodyAsync(stream, cts.Token);
84+
throw MapHttpError(resp, body);
85+
}
86+
4387
public async Task<IReadOnlyList<ModelInfo>> ListModelsAsync(CancellationToken ct)
4488
{
4589
using var req = new HttpRequestMessage(HttpMethod.Get, $"{BaseUrl}/models");

‎src/OpenKey/CommandRouter.cs‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ public async Task<CommandResult> HandleAsync(string input, CancellationToken ct)
109109
+ "You'll need to sign in again.",
110110
"Only continue if you meant to start completely fresh.");
111111

112-
if (AnsiConsole.Confirm("Erase everything and start over?", defaultValue: false))
112+
if (Prompts.Confirm("Erase everything and start over?"))
113113
{
114114
await _resetAction(ct);
115115
}
@@ -350,16 +350,15 @@ await AnsiConsole.Status()
350350
.Select(m => $"{Truncate(m.DisplayName, 44).PadRight(46)}{FormatContext(m.ContextLength)}")
351351
.ToList();
352352

353-
var prompt = new SelectionPrompt<string>
354-
{
355-
Title = Components.PickerTitle("Which model should answer you?"),
356-
PageSize = 12,
357-
MoreChoicesText = $"[{Theme.Muted}]More below[/]",
358-
};
359-
prompt.AddChoice(AutoChoiceLabel);
360-
foreach (var row in rows) prompt.AddChoice(row);
353+
var choices = new List<string> { AutoChoiceLabel };
354+
choices.AddRange(rows);
361355

362-
string choice = AnsiConsole.Prompt(prompt);
356+
var choice = Prompts.Select("Which model should answer you?", choices);
357+
if (choice is null)
358+
{
359+
Components.HintLine("Kept the current choice.");
360+
return;
361+
}
363362

364363
if (choice == AutoChoiceLabel)
365364
{
@@ -371,7 +370,7 @@ await AnsiConsole.Status()
371370
var picked = models[rows.IndexOf(choice)];
372371
_engine.PreferredModelId = picked.Id;
373372
Components.SuccessLine($"Now using {picked.DisplayName}.");
374-
Components.HintLine("This lasts until you close OpenKey.");
373+
Components.HintLine("Remembered for next time. Choose Auto to hand the choice back.");
375374
}
376375

377376
private static string FormatContext(int contextLength) =>

‎src/OpenKey/ConsoleHost.cs‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -333,10 +333,12 @@ private async Task<bool> EnsureFirstRunAsync(CancellationToken ct)
333333
{
334334
// No "(attempt 1/3)" counter: showing a retry budget before anything has failed
335335
// manufactures anxiety. Retries are surfaced only after a failure.
336-
var choice = AnsiConsole.Prompt(
337-
new SelectionPrompt<string>()
338-
.Title(Components.PickerTitle("How would you like to connect?"))
339-
.AddChoices(OAuthChoice, PasteChoice));
336+
var choice = Prompts.Select("How would you like to connect?", new[] { OAuthChoice, PasteChoice });
337+
if (choice is null)
338+
{
339+
Components.HintLine("No option chosen.");
340+
continue;
341+
}
340342

341343
string? key = null;
342344

@@ -472,12 +474,7 @@ private static string AcquireKeyViaPaste()
472474
Components.HintLine("You can create one at https://openrouter.ai/keys");
473475
AnsiConsole.WriteLine();
474476

475-
return AnsiConsole.Prompt(
476-
new TextPrompt<string>("Key: ")
477-
.Secret()
478-
.Validate(k => string.IsNullOrWhiteSpace(k)
479-
? ValidationResult.Error($"[{Theme.Danger}]Paste a key to continue, or press Ctrl+C to go back.[/]")
480-
: ValidationResult.Success()));
477+
return Prompts.Secret("Key: ") ?? string.Empty;
481478
}
482479

483480
private async Task<bool> ValidateAndSaveAsync(string key, CancellationToken ct)
@@ -491,6 +488,9 @@ await AnsiConsole.Status()
491488
.SpinnerStyle(new Style(Color.Grey))
492489
.StartAsync($"[{Theme.Muted}]Checking your key[/]", async _ =>
493490
{
491+
// Auth check first: /models is public and answers 200 for anyone, so on its
492+
// own it would accept any string as a valid key.
493+
await tmp.ValidateKeyAsync(ct);
494494
models = await tmp.ListModelsAsync(ct);
495495
});
496496

@@ -526,7 +526,7 @@ await AnsiConsole.Status()
526526
ex.Message,
527527
"OpenKey can save the key now and check it the first time you chat.");
528528

529-
if (AnsiConsole.Confirm("Save the key and continue?", defaultValue: false))
529+
if (Prompts.Confirm("Save the key and continue?"))
530530
{
531531
_keyStore.Save(key);
532532
return true;

‎src/OpenKey/Ui/Prompts.cs‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
using Spectre.Console;
2+
3+
namespace OpenKey.Ui;
4+
5+
/// <summary>
6+
/// Interactive prompts that degrade instead of throwing.
7+
/// <para>
8+
/// Spectre's prompts raise <see cref="NotSupportedException"/> when the terminal is not
9+
/// interactive — which is true whenever any standard stream is redirected, so it happens for
10+
/// `openkey &lt; script.txt`, for piped input, and inside CI. Nothing caught it, so a piped
11+
/// <c>/models</c> killed the app with a raw exception name on screen.
12+
/// </para>
13+
/// <para>
14+
/// Every fallback here reads a plain line instead, which is exactly what a piped caller can
15+
/// provide. Destructive confirmations deliberately require an explicit word rather than accepting
16+
/// a bare newline.
17+
/// </para>
18+
/// </summary>
19+
internal static class Prompts
20+
{
21+
/// <summary>True when Spectre's own prompts are safe to use.</summary>
22+
public static bool CanPrompt => ConsoleLayout.Rich;
23+
24+
public static bool Confirm(string question, bool defaultValue = false)
25+
{
26+
if (CanPrompt)
27+
{
28+
try { return AnsiConsole.Confirm(question, defaultValue); }
29+
catch (NotSupportedException) { /* fall through */ }
30+
}
31+
32+
AnsiConsole.Markup($"{Markup.Escape(question)} [{Theme.Muted}](yes/no)[/] ");
33+
var answer = Console.ReadLine();
34+
if (Console.IsInputRedirected) AnsiConsole.WriteLine();
35+
36+
// No answer means no. A destructive default of "yes" on EOF would be indefensible.
37+
return answer?.Trim().ToLowerInvariant() is "y" or "yes";
38+
}
39+
40+
/// <summary>
41+
/// Presents a choice. Returns the selected item, or null when the user declined or nothing
42+
/// could be read.
43+
/// </summary>
44+
public static string? Select(string title, IReadOnlyList<string> choices)
45+
{
46+
if (choices.Count == 0) return null;
47+
48+
if (CanPrompt)
49+
{
50+
try
51+
{
52+
var prompt = new SelectionPrompt<string>
53+
{
54+
Title = Components.PickerTitle(title),
55+
PageSize = 12,
56+
MoreChoicesText = $"[{Theme.Muted}]More below[/]",
57+
};
58+
foreach (var c in choices) prompt.AddChoice(c);
59+
return AnsiConsole.Prompt(prompt);
60+
}
61+
catch (NotSupportedException) { /* fall through */ }
62+
}
63+
64+
// Numbered list is the only sane fallback: a piped caller cannot press arrow keys.
65+
AnsiConsole.MarkupLine(Markup.Escape(title));
66+
for (var i = 0; i < choices.Count; i++)
67+
AnsiConsole.MarkupLine($" [{Theme.Brand}]{i + 1}[/] {Markup.Escape(choices[i])}");
68+
69+
Components.HintLine("Type a number and press Enter, or press Enter to cancel.");
70+
AnsiConsole.Markup($"[{Theme.Brand}]#[/] ");
71+
72+
var line = Console.ReadLine();
73+
if (Console.IsInputRedirected) AnsiConsole.WriteLine();
74+
75+
return int.TryParse(line?.Trim(), out var pick) && pick >= 1 && pick <= choices.Count
76+
? choices[pick - 1]
77+
: null;
78+
}
79+
80+
/// <summary>
81+
/// Reads a secret. Masked when the terminal allows it; when it does not, the value is coming
82+
/// from a pipe and there is nothing on screen to hide.
83+
/// </summary>
84+
public static string? Secret(string label)
85+
{
86+
if (CanPrompt)
87+
{
88+
try
89+
{
90+
return AnsiConsole.Prompt(
91+
new TextPrompt<string>(label)
92+
.Secret()
93+
.Validate(k => string.IsNullOrWhiteSpace(k)
94+
? ValidationResult.Error($"[{Theme.Danger}]Paste a key to continue, or press Ctrl+C to go back.[/]")
95+
: ValidationResult.Success()));
96+
}
97+
catch (NotSupportedException) { /* fall through */ }
98+
}
99+
100+
AnsiConsole.Markup(Markup.Escape(label));
101+
var value = Console.ReadLine();
102+
if (Console.IsInputRedirected) AnsiConsole.WriteLine();
103+
104+
return string.IsNullOrWhiteSpace(value) ? null : value.Trim();
105+
}
106+
}

‎tests/OpenKey.Tests/OpenRouterProviderTests.cs‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,38 @@ public async Task IgnoresCommentsAndBlankLines()
9999
Assert.Equal("x", string.Concat(chunks.Select(c => c.DeltaText)));
100100
}
101101

102+
[Fact]
103+
public async Task ValidatingAKeyRejectsA401()
104+
{
105+
// Key validation used to call ListModelsAsync, but GET /models is a *public* endpoint that
106+
// answers 200 with no Authorization header at all — so any string passed validation. A
107+
// mistyped key was saved with "You're ready to chat" and then failed on every message,
108+
// with the error advising a /reset that led straight back to the same place.
109+
var provider = Provider("""{"error":{"message":"No auth credentials found"}}""", HttpStatusCode.Unauthorized);
110+
111+
var ex = await Assert.ThrowsAsync<ChatException>(
112+
() => provider.ValidateKeyAsync(CancellationToken.None));
113+
114+
Assert.Equal(ChatErrorKind.AuthFailure, ex.Kind);
115+
}
116+
117+
[Fact]
118+
public async Task ValidatingAKeyAcceptsSuccess()
119+
{
120+
var provider = Provider("""{"data":{"label":"test","usage":0}}""");
121+
122+
await provider.ValidateKeyAsync(CancellationToken.None); // must not throw
123+
}
124+
125+
[Fact]
126+
public async Task ValidatingAKeyReportsNetworkFailureSeparately()
127+
{
128+
// A connection problem must not be reported to the user as "your key was refused".
129+
var provider = Provider("<html>captive portal</html>", HttpStatusCode.OK, "text/html");
130+
131+
await provider.ValidateKeyAsync(CancellationToken.None); // 200 is 200; parsing is not its job
132+
}
133+
102134
[Fact]
103135
public async Task CaptivePortalHtmlDoesNotCrash()
104136
{

0 commit comments

Comments
 (0)