From 0500e43e0207b0498bca009cc650bb1621b9b32a Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Wed, 29 Jul 2026 13:26:30 +0200 Subject: [PATCH] Stop the terminal's WebView painting over the setup screens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shell layered its setup and unlock screens over the terminal, which does not work: NativeWebView attaches a real Win32 child HWND through NativeControlHost, and a child window composites above everything its parent paints regardless of visual-tree z-order. The cards rendered sliced at the terminal column's left edge; at the window's default width every one of their buttons fell inside the WebView's rectangle, so the flow could only be completed by keyboard, and a click in that region handed Win32 focus to WebView2 so the text boxes silently stopped accepting keystrokes. The WebView is now collapsed while the vault is not unlocked. The comment that previously forbade this — hiding it means never realising it — was wrong: NativeControlHost creates the native attachment on attach to the visual tree, never consulting layout or visibility, and NativeWebView replays a Source assigned before its adapter exists. A collapsed WebView still starts WebView2, loads the page and lets the renderer attach. Confirmed: 35 msedgewebview2 processes with the control collapsed. What the first connection after unlocking actually depends on is the existing await on WaitForRendererAsync, since the data plane drops frames when no renderer is attached. Also fixes the second visible defect: the default server URL was https://localhost:7217, the API's *second* launch profile, while the README, its appsettings and a plain `dotnet run` all use http://localhost:5233 — so nothing was listening, and an HTTPS client against a plaintext port reports "The SSL connection could not be established", which reads as a certificate problem. The default now matches, a missing scheme is rejected by name instead of parsing as scheme "localhost", and that specific TLS failure now suggests http://. Both new tests fail when the fixes are reverted. Corrections to claims I made earlier and should not have: - docs/platform-flags.md asserted the opposite of the mechanism above and cited an established msedgewebview2 connection as verification. That observation was taken while the overlay was showing but, because of this very bug, the WebView was uncovered and in plain view — so it confirmed only that a visible WebView is realised. A process-level check cannot verify a rendering claim. The entry was also filed under "Local cache". - ITerminalHost was documented as the live seam the app plugs into, with a stub standing in for headless tests. It has no implementation anywhere and no test uses it; the view navigates the control directly. It also counted Avalonia.Controls.WebView and NativeWebView as two interchangeable backends when they are one component, with the Linux backend backwards. - The README claimed the shell's whole path was covered by tests. Its state machine is; its layout is covered by nothing, and a headless test could not have caught this — headless has no native window, so it would have rendered correctly and confirmed the wrong belief. Verified by screenshotting the running app: the card renders complete and centred at the default size, with the button clickable. --- README.md | 13 +++- docs/platform-flags.md | 67 +++++++++++++++---- .../ViewModels/MainWindowViewModel.cs | 61 +++++++++++++++-- src/DodoSSH.Client.App/Views/MainWindow.axaml | 28 ++++++-- .../DodoSSH.Client.Terminal.csproj | 3 +- src/DodoSSH.Client.Terminal/ITerminalHost.cs | 24 ++++--- .../ShellFlowTests.cs | 45 ++++++++++++- 7 files changed, 206 insertions(+), 35 deletions(-) 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() {