diff --git a/README.md b/README.md index f06f112..84f8108 100644 --- a/README.md +++ b/README.md @@ -173,9 +173,16 @@ off-Windows. *Client done:* the key hierarchy, the OIDC flow with the key binding, SSH connections with host key trust, the terminal data plane, the encrypted local cache with the sync client — offline unlock, an outbox and a field-level three-way merge, conflict matrix green — and an Avalonia shell that is - vault-backed: server URL → browser sign-in → enroll → unlock → host list → terminal. The shell's whole - path is covered by tests against an in-memory server, so the states that matter most (the recovery code - that cannot be skipped, the unlock that needs no network) are checked rather than remembered. + vault-backed: server URL → browser sign-in → enroll → unlock → host list → terminal. The shell's *state + machine* is covered by tests against an in-memory server, so the states that matter most (the recovery + code that cannot be skipped, the unlock that needs no network) are checked rather than remembered. + + Its *layout* is not covered by anything, and that gap has already cost a shipped defect: the setup and + unlock screens were layered over the terminal's WebView, which on Windows is a native child window that + cannot be covered, so they rendered sliced with their buttons unclickable. No test in this repository + loads a `.axaml` file, and a headless one could not have caught this — there is no native window in + headless, so it would have rendered perfectly and confirmed the wrong belief. Screens get looked at, or + they are unverified. *Verified end to end:* `tests/DodoSSH.SystemTests` drives the whole slice against a real Keycloak, a real API, a real PostgreSQL and a real `sshd` — sign-in, the identity-provider key binding, enrollment, offline unlock, a host through the vault to a second machine, and an interactive shell. See diff --git a/docs/platform-flags.md b/docs/platform-flags.md index e2e6fd2..a93038e 100644 --- a/docs/platform-flags.md +++ b/docs/platform-flags.md @@ -28,10 +28,50 @@ or notarization fails with an error that does not name the offending file. ## Desktop client -**The WebView works on Windows.** `Avalonia.Controls.WebView` 12.0.1 (MIT, no licence key) hosts the -terminal page successfully: WebView2 launches, navigates to the loopback page, runs its JavaScript and -completes the WebSocket handshake. Verified by observing an established TCP connection from -`msedgewebview2` to the data plane port. +**The WebView runs on Windows.** `Avalonia.Controls.WebView` 12.0.1 (MIT, no licence key) hosts the +terminal page: WebView2 launches, navigates to the loopback page, runs its JavaScript and completes the +WebSocket handshake. Verified by observing an established TCP connection from `msedgewebview2` to the data +plane port. + +Note precisely what that evidence covers, because it was once stretched to cover more: every clause above +is about the process and the socket. It says nothing about how the control **composites** with +Avalonia-drawn content, which is the axis on which it does not behave like an ordinary control — see the +next entry. + +**A native child window cannot be covered by Avalonia content, on any platform that hosts it windowed.** +`NativeWebView` attaches a real Win32 child HWND through `NativeControlHost`, and a child window paints +above everything its parent draws, whatever the visual tree's z-order says. Layering a screen over the +terminal therefore does nothing: the WebView's rectangle stays on top. In this shell that sliced the setup +and unlock cards at the terminal column's left edge, put every one of their buttons inside the WebView's +rectangle at the window's default width — so the flow could only be completed by keyboard — and handed +Win32 focus to WebView2 on any click in that region, which makes a text box stop accepting keystrokes with +no visible cause. + +The fix is to collapse the control, not to cover it: `IsVisible="{Binding IsUnlocked}"` on the +`NativeWebView`. That is safe, and this is the part worth recording, because the opposite was asserted here +for a while: + +- `NativeControlHost` creates the native attachment from **attach to the visual tree**, not from layout and + not from visibility. Its `UpdateHost` never reads `IsEffectivelyVisible`; only + `TryUpdateNativeControlPosition` does, choosing `HideWithSize` over `ShowInBounds`. +- `NativeWebView` stashes a `Source` assigned before its adapter exists and replays it once created, so + navigation is never lost to ordering. The shell already depends on that replay. +- So a collapsed WebView still starts WebView2, still loads the page and still lets the renderer attach its + socket. Confirmed on Windows: 35 `msedgewebview2` processes with the control collapsed behind the setup + screen. + +The previous version of this entry claimed the reverse — that hiding it would mean never realising it — and +cited the `msedgewebview2` connection as verification. That observation was made while the overlay was +showing but, because of the airspace behaviour above, the WebView was in fact uncovered and in plain view. +It confirmed only that a *visible* WebView is realised, which nobody disputed, and could not discriminate +the case it was attached to. A process-level check cannot verify a rendering claim; that needs a +screenshot, and this defect shipped because one was never taken. + +**What the first connection after unlocking actually depends on** is the `await +workspace.WaitForRendererAsync()` in `VaultViewModel.ConnectAsync`, because `TerminalDataPlane.SendAsync` +drops frames when no renderer is attached rather than queueing them. That await is the invariant; the +control's visibility is not. It currently has no timeout, so a WebView2 that fails to initialise hangs +Connect with the busy flag stuck — worth fixing on its own merits. **The Windows app manifest must declare a `supportedOS` list.** Without it the process reports a downlevel Windows version and Avalonia's native control host fails outright — *"Unable to create child @@ -48,8 +88,17 @@ package's own release notes say `NativeWebView` gained Linux support via a **WPE (`libwpewebkit-2.0`), which is much less widely installed than WebKitGTK — and it ships a separate `NativeWebDialog` described as *"particularly useful for platforms like Linux where embedded WebView controls might not be available"*, which is the vendor confirming the concern. *Unverified:* a spike -must cover Ubuntu on both Wayland and X11, Fedora KDE, and macOS 15. This is why the terminal sits -behind `ITerminalHost`; that seam should not be collapsed away for convenience. +must cover Ubuntu on both Wayland and X11, Fedora KDE, and macOS 15. + +`ITerminalHost` was supposed to be the seam that keeps a backend swap cheap, and it is **declared but not +implemented** — nothing in the application uses it, and the view navigates `NativeWebView.Source` directly. +Swapping backends today means editing `MainWindow.axaml` and its code-behind. That is a small job, but do +not plan around a seam that is currently only a file. + +One more reason the Linux picture may be better than this entry assumes: the package also ships +`NativeWebViewCompositorHost`, a non-windowed host drawn through Avalonia's compositor. A compositor host +would not have the airspace problem described below at all. Whether it can be selected deliberately is +unknown and worth establishing during the spike, because it would change how overlays can be built. **`Avalonia.Diagnostics` has no 12.x release** (latest is 11.3.18), so the developer tools overlay is unavailable on Avalonia 12. Development-only, so nothing ships differently — but debugging a layout @@ -180,12 +229,6 @@ licence obligation — and `bundle_e_sqlcipher` was deprecated in SQLitePCLRaw 3 be honest about: the cache offers no protection against another process running as the same user. See `LocalCacheProtector` for what it does and does not defend against. -**A `NativeWebView` that is never laid out is never realised.** The shell covers the terminal with its -setup and unlock screens rather than collapsing it with `IsVisible`, because the control hosts a real -child window and hiding it would leave the terminal blank on the first connection after unlocking. -Verified on Windows: with the unlock overlay showing, `msedgewebview2` still had an established -connection to the data plane port, so the page had loaded and completed its WebSocket handshake. - ## Build and CI **Integration tests need a Docker daemon** (Testcontainers). They run on `ubuntu-latest` in CI. diff --git a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs index be3d085..3cebb3f 100644 --- a/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.App/ViewModels/MainWindowViewModel.cs @@ -1,3 +1,4 @@ +using System.Security.Authentication; using CommunityToolkit.Mvvm.ComponentModel; using CommunityToolkit.Mvvm.Input; using DodoSSH.Client.Auth; @@ -105,8 +106,18 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp [ObservableProperty] private bool isBusy; + /// + /// The address dotnet run --project src/DodoSSH.Api actually serves, so the first launch after + /// a clone works without the user having to know a port. This was https://localhost:7217, which + /// is the API's second launch profile: the first is HTTP on 5233 and is the one both the + /// README and a plain dotnet run select, so nothing was listening on 7217. Pointing an HTTPS + /// client at a plaintext port fails as "The SSL connection could not be established", which sends + /// people looking for a certificate problem — see . A real + /// deployment is HTTPS behind a proxy and its address is typed over this one; the placeholder in the + /// setup card shows that shape. + /// [ObservableProperty] - private string serverUrl = "https://localhost:7217"; + private string serverUrl = "http://localhost:5233"; [ObservableProperty] private string passphrase = string.Empty; @@ -192,9 +203,19 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp return; } + // Checked separately from parsing, because "localhost:5233" parses perfectly well as an absolute + // URI whose scheme is "localhost" — and then fails much later with something unrelated to the + // actual mistake. + if (url.Scheme is not ("http" or "https")) + { + StatusMessage = $"A server URL has to start with http:// or https://, not {url.Scheme}:."; + return; + } + await RunAsync( "Opening your browser to sign in…", - async () => + explain: exception => ExplainSignInFailure(exception, url), + work: async () => { connection?.Dispose(); connection = null; @@ -383,7 +404,17 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp /// Every command funnels through here so the busy flag and the failure message are handled once. A /// command that forgot either would leave the window permanently disabled or silently doing nothing. /// - private async Task RunAsync(string busyMessage, Func work) + /// Shown while the work runs. + /// The work. + /// + /// Turns a failure into something a user can act on. Optional, because most failures here already + /// carry their own explanation; the ones that do not are the ones crossing into another process's + /// vocabulary, where the exception describes a symptom and not the mistake. + /// + private async Task RunAsync( + string busyMessage, + Func work, + Func? explain = null) { if (IsBusy) { @@ -403,7 +434,7 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp } catch (Exception exception) { - StatusMessage = exception.Message; + StatusMessage = explain?.Invoke(exception) ?? exception.Message; } finally { @@ -411,6 +442,28 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp } } + /// + /// One case earns a translation rather than the exception's own words: pointing an HTTPS client at a + /// plaintext port reports "The SSL connection could not be established", which sends people looking + /// for a certificate problem. The scheme is the mistake, and the development stack serves HTTP, so + /// this is the first thing a new user will hit. + /// + private static string ExplainSignInFailure(Exception exception, Uri server) + { + var secureChannelFailed = exception is HttpRequestException + && exception.GetBaseException() is AuthenticationException; + + if (secureChannelFailed && server.Scheme is "https") + { + var plain = new UriBuilder(server) { Scheme = "http" }.Uri; + + return $"{exception.Message} {server.Host} answered, but not with TLS. If this is a " + + $"development server it probably serves plain HTTP — try {plain.GetLeftPart(UriPartial.Authority)}."; + } + + return exception.Message; + } + partial void OnStateChanged(ShellState value) { OnPropertyChanged(nameof(IsStarting)); diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml b/src/DodoSSH.Client.App/Views/MainWindow.axaml index c5876c6..ebb6ec0 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml @@ -31,9 +31,21 @@ @@ -205,14 +217,20 @@ - + - + diff --git a/src/DodoSSH.Client.Terminal/DodoSSH.Client.Terminal.csproj b/src/DodoSSH.Client.Terminal/DodoSSH.Client.Terminal.csproj index 5c6cd27..a89ce04 100644 --- a/src/DodoSSH.Client.Terminal/DodoSSH.Client.Terminal.csproj +++ b/src/DodoSSH.Client.Terminal/DodoSSH.Client.Terminal.csproj @@ -3,7 +3,8 @@ diff --git a/src/DodoSSH.Client.Terminal/ITerminalHost.cs b/src/DodoSSH.Client.Terminal/ITerminalHost.cs index c2c485e..570747d 100644 --- a/src/DodoSSH.Client.Terminal/ITerminalHost.cs +++ b/src/DodoSSH.Client.Terminal/ITerminalHost.cs @@ -5,16 +5,24 @@ namespace DodoSSH.Client.Terminal; /// /// /// -/// Deliberately tiny. Everything the renderer needs — its files, its connection token, its socket -/// URL — arrives over the loopback HTTP server, so the only thing the host has to do is navigate. -/// That is what keeps three WebView backends interchangeable: the official -/// Avalonia.Controls.WebView, the community NativeWebView whose Linux backend is the -/// more widely installed WebKitGTK, and CEF as the heavyweight escape hatch. +/// Declared, not yet wired. Nothing in the application implements this today: the view assigns +/// NativeWebView.Source directly in MainWindow.axaml.cs, and the headless shell tests +/// substitute instead — they never need a browser, because the view +/// models do not own one. So swapping WebView backends currently means editing the XAML and its code-behind. +/// This interface records the shape that swap should take; it is not a seam that exists yet, and it should +/// not be cited as one. /// /// -/// It is also what makes the terminal testable headlessly. Avalonia's headless platform has no -/// WebView at all, so a stub implementing this interface stands in — and because the interface is one -/// method, the stub cannot drift from the real thing. +/// Deliberately tiny, and that part is worth keeping. Everything the renderer needs — its files, its +/// connection token, its socket URL — arrives over the loopback HTTP server, so the only thing a host has to +/// do is navigate. A backend swap is therefore one method wide however it is eventually wired. +/// +/// +/// The candidates are two, not three: Avalonia.Controls.WebView is the package and +/// NativeWebView is the control it ships, so they are one option — whose Linux backend is WPE WebKit, +/// with WebKitGTK the more widely installed library it is not using — and CEF is the heavyweight escape +/// hatch. An earlier version of this remark counted the package and the control separately and had the +/// Linux backend the wrong way round, which made the interchangeability argument rest on a miscount. /// /// public interface ITerminalHost diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index 4758428..5324310 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -29,6 +29,8 @@ public sealed class ShellFlowTests : IAsyncLifetime private readonly FakeVaultServer server = new(); + private int signInAttempts; + private string directory = null!; private ClientPaths paths = null!; private ClientCacheFactory caches = null!; @@ -132,6 +134,39 @@ public sealed class ShellFlowTests : IAsyncLifetime shell.IsOnline.ShouldBeFalse(); } + /// + /// Separate from the case above because it is not caught by the same check. Uri.TryCreate + /// accepts this happily as an absolute URI whose scheme is "localhost" and whose host is + /// empty, so without an explicit scheme check the mistake surfaces much later as something that reads + /// like a network fault. + /// + [Fact] + public async Task AServerUrlWithNoScheme_SaysSoRatherThanFailingLater() + { + await shell.StartAsync(Token); + + shell.ServerUrl = "localhost:5233"; + await shell.SignInCommand.ExecuteAsync(null); + + shell.State.ShouldBe(ShellState.NeedsServer); + shell.IsOnline.ShouldBeFalse(); + shell.StatusMessage.ShouldContain("http://"); + signInAttempts.ShouldBe(0, "a malformed URL must not open a browser"); + } + + /// + /// The shipped default is a value a user is invited to accept unread, so it is worth one assertion. + /// It was https://localhost:7217 — the API's second launch profile — while the README, the + /// API's appsettings and a plain dotnet run all use HTTP on 5233, and pointing an HTTPS client + /// at a plaintext port reports a TLS failure that reads like a certificate problem. Nothing failed + /// except the first thing a new user does. + /// + [Fact] + public void TheDefaultServerUrl_IsTheAddressTheApiActuallyServes() + { + shell.ServerUrl.ShouldBe("http://localhost:5233"); + } + [Theory] [InlineData("short", "short")] [InlineData("a sufficiently long passphrase", "a different one")] @@ -384,10 +419,16 @@ public sealed class ShellFlowTests : IAsyncLifetime private static CancellationToken Token => TestContext.Current.CancellationToken; - private Task SignInAsync(Uri serverUrl, CancellationToken cancellationToken) => - server.SignInFailure is { } failure + private Task SignInAsync(Uri serverUrl, CancellationToken cancellationToken) + { + // Counted so a test can assert that a rejected URL never got this far. Reaching here means a + // browser would have opened in the real application. + signInAttempts++; + + return server.SignInFailure is { } failure ? Task.FromException(failure) : Task.FromResult(server); + } private async Task SignedInAsync() {