Show users a fixed phrase when a target connection fails, not the error - #2548
Open
janisdombr wants to merge 28 commits into
Open
janisdombr wants to merge 28 commits into
janisdombr wants to merge 28 commits into
Conversation
This was referenced Sep 5, 2026
RDP forwarded the full anyhow chain, VNC only the top-level cause, and web-ssh forwarded RCEvent::Error/ConnectionError verbatim (including a raw WarpgateError, e.g. a DB error) — three different, arbitrary answers to the same question. One conversion point per protocol now decides what the viewer is owed (the top-level cause for desktop protocols, a fixed safe message for SSH's typed errors), while the full chain still goes to the log. The terminal is the same sink. `RCEvent::ConnectionError` and `RCEvent::Error` in the SSH session wrote the error's own `Display` onto the user's PTY, so hardening only the browser leg would leave an identical path open beside it — and `ConnectionError::Warpgate` is `#[error(transparent)]`, so that path rendered `WarpgateError` itself. One hardened path and one unhardened path to the same sink is not a boundary. What crosses that boundary is now a named function rather than a call inside an event loop, because the thing worth guarding is the choice — `client_message()` and not `Display` — and a call in an async loop cannot be reached by a test without driving a whole browser session. The tests assert on `Warpgate` specifically, first that the fixture's own text carries the leak and then that the shown text does not, so they cannot pass by testing nothing. Also raises the SSH target-connect failure log from debug! to error!: it was the only server-side record of a user-visible connect failure, and at the default level there was none at all. The log lines take the error by `Debug`, not `Display`: `RCEvent::Error` carries an `anyhow::Error`, whose `Display` is only the outermost context, and the promise made here is that the whole chain reaches the log.
`sanitising_survives_a_context_wrapper` asserted that a wrapped `SshClientError::Warpgate` renders as "Internal error in the target connection". That is also what `client_error_message` returns when the downcast finds nothing, because `client_message()` maps the `Warpgate` variant to the same string as the fallback. The assertion therefore could not tell a downcast that walked the context chain from the exact degradation the test's own comment says it exists to catch: deleting the downcast entirely and returning the fallback unconditionally left all three tests in the module green. Assert on `MpscError` instead, whose message only a successful downcast can produce, and keep the leak direction as a second assertion on the original fixture. Under the same mutation the test now fails.
`spawn_client` routes a connect-time failure through `DesktopEvent::backend_error`, whose doc calls itself "one conversion point for both RDP and VNC". It is not: `map_event` carries a mid-session `VncEvent::Error` to the same sink and passed the payload through untouched, reaching a browser verbatim via `warpgate-web-desktop`'s `From<DesktopEvent>`. The payload is not ours to shape. The vendored decoder emits it from its decode loop for anything that is not a clean EOF, and `vnc::VncError::IoError` is `#[error(transparent)]` over `std::io::Error`, so the string is whatever the OS said. There is no structured error here to take a top-level cause from, so the viewer gets a fixed phrase and the log gets the text. The native VNC proxy was never exposed to this: its consumer discards `DesktopEvent::Error` text rather than displaying it. That is a property of that one consumer, not a second guard, and it is not what the browser path does.
A target whose port nothing is listening on, reached over a real SSH session
through a running Warpgate. `-tt` is not decoration: `emit_pty_output` writes
only to channels that carry a PTY, so a plain `ssh host command` never sees
any of this.
Against the parent commit the terminal reads
● Warpgate: Target connection failed: Connection refused (os error 61)
and the test fails on it. The reason still has to reach the log, which is
asserted too -- otherwise the same test would pass on a change that discarded
the error rather than relocating it.
The browser leg is not here, and the module doc says why rather than leaving
it looking like an oversight: a connect-time failure tears the web-SSH session
down before a websocket can attach (a GET on the session id 404s immediately
after the POST that created it returned 201), and a mid-session death does not
produce `ConnectionError` at all -- the connection was established, so it
arrives as `Drop for ClientHandler` -> `State(Disconnected)`. That arm keeps
its unit test.
The added comments ran at about eight times the density of the code around them: 21% of added lines against 2.5% across the crate. Most of it narrated what the code already says, or repeated the same non-vacuity note in three slightly different wordings. What stays is the part a reader needs in order not to undo the change: why the transparent variants cannot be rendered to a user, why the boundary is a named function rather than a call in the loop, why the context-wrapper test uses a fixture whose message differs from the fallback, and why the VNC live loop needed the same treatment as the connect path.
janisdombr
force-pushed
the
upstream-fix/desktop-error-forwarding
branch
from
September 6, 2026 12:49
4935bb7 to
72019dc
Compare
`disconnect_server` queued the connection-level `Disconnect` behind the channel closes, so it left for the client in the same breath as whatever the session had just said. A client handed both in one read acts on the disconnect and exits without printing what it already holds, and the session's last words are lost — which is the one thing those messages exist to prevent. A session closed for inactivity says so and disconnects in the next statement, so that path lost its notice every time: the client saw the target's output and then nothing. The disconnect now goes out from the task that already waits out the flush grace before closing the socket, and only to a client that is reading; one that is not still gets cut immediately. The channel closes stay in the queue, so per-channel ordering is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TargetDataRequest gained three required booleans and the SSH options one more while this branch was open; the new test constructed the target without them and would fail validation before reaching the behaviour it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gy1bTv7ou9aRZG8UrNEJHi
The connection-level disconnect now leaves from the shutdown task, so the queue variant and its enqueue method had no callers left.
4 tasks
janisdombr
added a commit
to janisdombr/warpgate
that referenced
this pull request
Sep 22, 2026
A Vault error body arrives verbatim — bounded and UTF-8-repaired, but with its
control characters intact — and a newline in it forges a whole record in the
default text format that a reader cannot tell from one Warpgate wrote. The
connect path already rendered this value with `{:?}` and said why; the session's
sink for the same value — it arrives there as `RCEvent::ConnectionError`, sent
from that very line — used `{}`, so every forgery the first sink refused went
through the second untouched.
Escaped at the type and at the sink, because one sink is not the property. The
`Api` variant renders its body with `{body:?}`, which is what makes web-ssh's
`%e` and every sink added after it safe without editing any of them; the field
stays raw, so `client_message` still classifies on its text and nothing else
reads the rendered form. The session's sink keeps `{:?}` for the rest of the
enum, whose transparent variants hand their inner text to the log as written.
The call moves into `log_target_connection_failure`, named the way web-ssh's
`shown_to_the_browser` is, so a test has somewhere to stand that a call inside
the event loop does not. `#[deny(dead_code)]` ties the two back together: `mod
tests` is `#[cfg(test)]`, so a call site that goes back to logging inline fails
an ordinary build instead of orphaning the helper while its test stays green.
The regression test's fixture is a transparent variant rather than the Vault one
it was written against — Vault's body is escaped by its own Display now, so that
fixture would pass with the sink reverted and prove nothing about it. The Vault
case stays as a second test of the path end to end: it holds while either layer
does, which is what having both is for.
web-ssh's two `error=%e` sinks and `RCEvent::Error` are deliberately left alone.
warp-tech#2548 changes all three, and after this the Vault body — the only hostile text
this branch introduces — is escaped before it reaches them anyway; editing them
here would only manufacture a conflict.
Guards 57 and 58 in the mutation matrix anchor on the sink and on the rendering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a connection to a target failed, the user was shown the error's own words. A real
sshclient sawTarget connection failed: Connection refused (os error 61); a browser watching a VNC session saw whatever the decoder said, which for itsIoErrorvariant is#[error(transparent)]overstd::io::Error.Every leg now goes through one conversion. Both
client_message()methods return a fixed phrase per error kind.SshClientError::client_message()is aconst fnreturning&'static str, so the type rules out runtime text;ConnectionError::client_message()returnsString, and its fixed phrases are guaranteed by the match arms rather than by the signature. The PTY leg, the web terminal and the desktop legs all use it. Two paths were hardened only on one side and are now finished: the SSH PTY leg, whose doc comment already claimed to cover it, and the VNC mid-session path, wheremap_eventpassed the decoder's string straight through while the connect-time path beside it was already sanitised.Measured with a real
sshclient against an unbound port. Before:Target connection failed: Connection refused (os error 61). After:Target connection failed: SSH protocol error, with the OS reason still in the log.Tests: 91 unit, plus
tests/test_ssh_terminal_error_disclosure.py, which runs a real Warpgate and a real OpenSSH client. Every seam was checked by removing it and confirming the test goes red. One test could not fail and was repaired: it asserted a value the fallback also returns, so deleting the downcast entirely left it green. The browser leg has no end-to-end test and the module doc says why: a connect-time failure tears the web session down before a websocket can attach, and a mid-session death never produces that error at all.Carries #2584:
disconnect_serverqueued the connection-levelDisconnectbehind the channel closes, so a client handed both in one read exited without printing the phrase this PR installs. On main the test here fails 2 of 10 local runs and has failed twice on CI (Received disconnect from ::1 port N:11:and nothing before it); with #2584's two commits, 0 of 10. #2584 is closed in favour of this one.Fork CI and CodeRabbit: janisdombr#9
Description
...
AI Usage
Choose the level of AI involvement for this PR.
This is not to block AI contributions but rather to speed up PR review (saves time on trying to deduce the logic behind AI hallucinations).