From 98b07ec67d32d4c9ffcc1c0363c0cd483d0a4e59 Mon Sep 17 00:00:00 2001 From: nickrobnett <78187895+nickrobnett@users.noreply.github.com> Date: Sun, 8 Nov 2026 20:55:40 -0500 Subject: [PATCH] Keep login credentials out of local echo Honor Telnet ECHO negotiation so standalone password responses stay out of local scrollback, transcripts, and command history. Resume normal local echo when the server sends WONT ECHO. Reuse the existing credential detector to redact local echoes of commands such as connect . --- docs/design/README.md | 6 +- .../Configuration/ConnectStringTemplate.cs | 4 +- src/SharpMUTerm.Core/Input/HistorySecrets.cs | 3 +- src/SharpMUTerm.Core/Session/WorldSession.cs | 25 ++++--- src/SharpMUTerm.Core/Telnet/ITelnetSession.cs | 3 + src/SharpMUTerm.Core/Telnet/TelnetSession.cs | 9 +++ src/SharpMUTerm.Tui/SharpMUTermApp.cs | 8 +- .../Session/PasswordLeakTests.cs | 74 ++++++++++++++++--- .../HistorySearchEndToEndTests.cs | 17 +++++ .../RecordingTelnetSession.cs | 2 + 10 files changed, 121 insertions(+), 30 deletions(-) diff --git a/docs/design/README.md b/docs/design/README.md index d95cac0c..f4fac6fd 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -111,10 +111,8 @@ would re-impose it on the users who never chose it. #### Passwords live in `secrets.json`, not in `config.json` -Character passwords are saved — a client that forgets them is a client whose users type credentials into -the command line, where they are echoed, written to the session transcript and offered to history. But they -are **not saved in `config.json`**, because that is the file people share: it goes into bug reports, help -channels, screenshots and dotfiles repositories. +Character passwords can be saved so players can log in automatically. They are stored outside +`config.json` because people share that file in bug reports, help channels and dotfiles repositories. So there are two files, side by side in `~/.config/SharpMUTerm/`: diff --git a/src/SharpMUTerm.Core/Configuration/ConnectStringTemplate.cs b/src/SharpMUTerm.Core/Configuration/ConnectStringTemplate.cs index 5c0943dd..36bd7140 100644 --- a/src/SharpMUTerm.Core/Configuration/ConnectStringTemplate.cs +++ b/src/SharpMUTerm.Core/Configuration/ConnectStringTemplate.cs @@ -26,8 +26,8 @@ namespace SharpMUTerm.Core.Configuration; /// screenshots, because it is an ordinary text field and has to be readable to be editable. A secret in it /// is a secret on screen. /// Substitution at send time keeps the resolved line off every local surface. Typed input is -/// echoed and written to the session transcript (WorldSession.SendUserInputAsync → -/// Print) and is offered to InputHistory; the login line goes straight out through +/// offered to InputHistory and to local echo, which hides recognized credential commands +/// from the output and transcript. The configured login line goes straight out through /// SendRawAsync with no echo, no transcript write and no history entry. That is unchanged by /// where the password is stored. /// diff --git a/src/SharpMUTerm.Core/Input/HistorySecrets.cs b/src/SharpMUTerm.Core/Input/HistorySecrets.cs index eb46105c..c2902351 100644 --- a/src/SharpMUTerm.Core/Input/HistorySecrets.cs +++ b/src/SharpMUTerm.Core/Input/HistorySecrets.cs @@ -1,8 +1,7 @@ namespace SharpMUTerm.Core.Input; /// -/// The rule that keeps typed passwords out of : a login line carries a -/// plaintext password, and history is the one place in this client that would keep it. +/// Recognizes commands carrying typed passwords so command history and local echo can protect them. /// /// A configured character never comes through here — WorldSession.SendLoginAsync writes its /// connect string straight to the transport and never touches the UI's history. The line this exists for diff --git a/src/SharpMUTerm.Core/Session/WorldSession.cs b/src/SharpMUTerm.Core/Session/WorldSession.cs index 5cb6839b..a27b1332 100644 --- a/src/SharpMUTerm.Core/Session/WorldSession.cs +++ b/src/SharpMUTerm.Core/Session/WorldSession.cs @@ -557,6 +557,9 @@ private void DeclareMeter(MxpGauge gauge) public bool IsConnected => _telnet?.IsConnected == true; + /// Whether the server has taken over echo, as it can do at a password prompt. + public bool ServerEchoesInput => _telnet?.ServerEchoesInput == true; + /// /// The encoding this session's text is actually going over the wire in, and why — negotiated by /// CHARSET, pinned by the world, or assumed because nothing negotiated. Null when there is no @@ -1157,36 +1160,40 @@ public async Task SendUserInputAsync(string input, CancellationToken cancellatio /// /// Handles a line the player typed on the command line: the world's command stacking first /// ( — n;e;e, #3 kill rat), then each command through - /// 's alias expansion. The line is echoed once, as typed. A composed post, a - /// grab and a script go through or instead, so a + /// 's alias expansion. An accepted line is echoed once, with credential + /// lines hidden. A composed post, a grab and a script go through or + /// instead, so a /// ; in a post is never a command boundary. /// public async Task SendTypedAsync(string input, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(input); var stacked = Input.TypedCommands.Expand(input, World.CommandSeparator); - Echo(input); if (stacked.Refusal is { } refusal) { PrintSystem($"*** {refusal}"); return; } + Echo(input, stacked.Commands); foreach (var command in stacked.Commands) { await ExpandAndSendAsync(command, cancellationToken).ConfigureAwait(false); } } - private void Echo(string input) + private void Echo(string input, IReadOnlyList? commands = null) { - // Two switches, and both have to be on: the world's (some servers echo for you, some don't) - // and F8's app-wide one. The app-wide one is read per line, not captured, so unticking it - // stops the echo on the next command rather than the next session. - if (World.LocalEcho && _input?.LocalEcho != false) + // A server can take over echo during a password prompt. Otherwise both the world's and + // the app-wide local echo settings must be on. + if (!ServerEchoesInput && World.LocalEcho && _input?.LocalEcho != false) { - Print(StyledLine.FromText(input, EchoStyle)); + var containsCredential = ContainsCredential(input) || commands?.Any(ContainsCredential) == true; + Print(StyledLine.FromText(containsCredential ? "(credential command hidden)" : input, EchoStyle)); } + + static bool ContainsCredential(string text) => + text.Split(['\r', '\n']).Any(Input.HistorySecrets.LooksLikeCredential); } private async Task ExpandAndSendAsync(string input, CancellationToken cancellationToken) diff --git a/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs b/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs index 7a55186f..fba8c5ae 100644 --- a/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs +++ b/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs @@ -9,6 +9,9 @@ public interface ITelnetSession : IAsyncDisposable { bool IsConnected { get; } + /// Whether the server has negotiated responsibility for echoing input. + bool ServerEchoesInput => false; + /// /// The encoding this session is decoding and encoding with, and whether CHARSET negotiated it, /// the world pinned it, or it is merely what was assumed because nothing negotiated. diff --git a/src/SharpMUTerm.Core/Telnet/TelnetSession.cs b/src/SharpMUTerm.Core/Telnet/TelnetSession.cs index e94e153a..ac099a22 100644 --- a/src/SharpMUTerm.Core/Telnet/TelnetSession.cs +++ b/src/SharpMUTerm.Core/Telnet/TelnetSession.cs @@ -329,6 +329,7 @@ public sealed class TelnetSession : ITelnetSession private CancellationTokenSource? _loopCts; private Task? _readLoop; private int _disconnected; + private volatile bool _serverEchoesInput; public TelnetSession(ITransport transport, ILogger? logger = null, TelnetSessionOptions? options = null) { @@ -339,6 +340,8 @@ public TelnetSession(ITransport transport, ILogger? logger = null, TelnetSession public bool IsConnected => _transport.IsConnected && _interpreter is not null; + public bool ServerEchoesInput => IsConnected && _serverEchoesInput; + /// /// The encoding this session is decoding and encoding with now, and why — derived, never cached, /// so it is exact at the moment of every decode rather than as of the last event. @@ -428,6 +431,7 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default) await _transport.ConnectAsync(cancellationToken).ConfigureAwait(false); _msdpAgreed = false; _mxpOn = false; + _serverEchoesInput = false; try { @@ -474,6 +478,11 @@ private Task BuildInterpreterAsync() // Answers a server's DO NEW-ENVIRON; offers nothing on its own (the unsolicited-negotiation rule). .AddPlugin().WithClientEnvironmentVariables(TelnetSessionOptions.MnesIdentity(_options.TerminalTypes)) .AddPlugin().WithCharsetOrder(_options.CharsetOrder) + .AddPlugin().OnEchoStateChanged(enabled => + { + _serverEchoesInput = enabled; + return ValueTask.CompletedTask; + }) .AddPlugin().OnPrompt(OnPromptAsync) .AddPlugin().OnPrompt(OnPromptAsync) .AddPlugin().OnPrompt(OnPromptAsync) diff --git a/src/SharpMUTerm.Tui/SharpMUTermApp.cs b/src/SharpMUTerm.Tui/SharpMUTermApp.cs index d9bc9865..dd770b7a 100644 --- a/src/SharpMUTerm.Tui/SharpMUTermApp.cs +++ b/src/SharpMUTerm.Tui/SharpMUTermApp.cs @@ -5533,8 +5533,12 @@ private void OnCommandEntered(InputBar bar, string command) { // The entered command clears this window's draft for the bar it came from and its unsent-input // marker, and joins that bar's draft-safe history so ↑/↓ can recall it without clobbering a - // future draft. The bar has already emptied itself — ⏎ never moves the caret off it. - HistoryFor(bar).Add(command); + // future draft. A server echo window can carry a standalone password, so skip history in + // that case. The bar has already emptied itself — ⏎ never moves the caret off it. + if (SendTarget()?.ServerEchoesInput != true) + { + HistoryFor(bar).Add(command); + } var windowId = ActiveWindowId(); _drafts.Clear(windowId, bar); _workspace.SetUnsentInput(windowId, AnyBarHasText()); diff --git a/tests/SharpMUTerm.Core.Tests/Session/PasswordLeakTests.cs b/tests/SharpMUTerm.Core.Tests/Session/PasswordLeakTests.cs index 1077b0d9..10e917e4 100644 --- a/tests/SharpMUTerm.Core.Tests/Session/PasswordLeakTests.cs +++ b/tests/SharpMUTerm.Core.Tests/Session/PasswordLeakTests.cs @@ -146,26 +146,78 @@ await Assert.That(session.Scrollback.Snapshot().Any(l => l.Text.Contains(Secret, await Assert.That(log.Lines).IsNotEmpty(); } - /// - /// The asymmetry that justifies the whole feature, pinned so nobody closes the gap from the wrong end. - /// A user who types connect Corvid … by hand has it echoed into the output window and written - /// into the transcript on disk, because that is what local echo and session logging are for. - /// Auto-login is the way out of that, not a reason to start filtering the command line. - /// [Test] - public async Task TypingTheLoginLineByHandIsEchoedAndTranscribed_WhichIsWhyTheFieldExists() + [Arguments(true)] + [Arguments(false)] + public async Task ALoginCommandReachesTelnetButIsHiddenInTheEcho(bool typed) { var log = new RecordingSink(); var telnet = new FakeTelnetSession(); var session = new WorldSession(World(), sessionFactory: _ => telnet, log: log); await session.ConnectAsync(); - await session.SendUserInputAsync($"connect Corvid {Secret}"); + var command = $"connect Corvid {Secret}"; + if (typed) + { + await session.SendTypedAsync(command); + } + else + { + await session.SendUserInputAsync(command); + } - await Assert.That(telnet.SentLines).Contains($"connect Corvid {Secret}"); + await Assert.That(telnet.SentLines).Contains(command); await Assert.That(session.Scrollback.Snapshot().Any(l => l.Text.Contains(Secret, StringComparison.Ordinal))) - .IsTrue(); - await Assert.That(log.Lines.Any(l => l.Contains(Secret, StringComparison.Ordinal))).IsTrue(); + .IsFalse(); + await Assert.That(log.Lines.Any(l => l.Contains(Secret, StringComparison.Ordinal))).IsFalse(); + await Assert.That(log.Lines).Contains("(credential command hidden)"); + } + + [Test] + [Arguments("#1", true)] + [Arguments("#101", false)] + public async Task StackedInputKeepsCredentialsOutOfTheEcho(string repeat, bool accepted) + { + var log = new RecordingSink(); + var telnet = new FakeTelnetSession(); + var world = World(); + world.CommandSeparator = ";"; + var session = new WorldSession(world, sessionFactory: _ => telnet, log: log); + await session.ConnectAsync(); + + var command = $"connect Corvid {Secret}"; + await session.SendTypedAsync($"look;{repeat} {command}"); + + await Assert.That(telnet.SentLines.Contains(command)).IsEqualTo(accepted); + await Assert.That(session.Scrollback.Snapshot().Any(l => l.Text.Contains(Secret, StringComparison.Ordinal))) + .IsFalse(); + await Assert.That(log.Lines.Any(l => l.Contains(Secret, StringComparison.Ordinal))).IsFalse(); + } + + [Test] + public async Task ServerEchoNegotiationHidesStandalonePasswordAndRestoresLocalEcho() + { + var log = new RecordingSink(); + var transport = new FakeTransport(); + var telnet = new TelnetSession(transport); + await using var session = new WorldSession(World(), sessionFactory: _ => telnet, log: log); + await session.ConnectAsync(); + + transport.FeedInbound([255, 251, 1]); // IAC WILL ECHO + await Assert.That(await WaitUntilAsync(() => session.ServerEchoesInput)).IsTrue(); + await session.SendTypedAsync(Secret); + await Assert.That(await WaitUntilAsync(() => + Encoding.ASCII.GetString(transport.SentBytes).Contains(Secret, StringComparison.Ordinal))).IsTrue(); + + transport.FeedInbound([255, 252, 1]); // IAC WONT ECHO + await Assert.That(await WaitUntilAsync(() => !session.ServerEchoesInput)).IsTrue(); + await session.SendTypedAsync("look"); + + var scrollback = session.Scrollback.Snapshot().Select(line => line.Text).ToArray(); + await Assert.That(scrollback).Contains("look"); + await Assert.That(log.Lines).Contains("look"); + await Assert.That(scrollback.Any(line => line.Contains(Secret, StringComparison.Ordinal))).IsFalse(); + await Assert.That(log.Lines.Any(line => line.Contains(Secret, StringComparison.Ordinal))).IsFalse(); } /// diff --git a/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs b/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs index fb511653..77fae52d 100644 --- a/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs +++ b/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs @@ -452,6 +452,23 @@ public async Task NorByRecall() /// The rule is narrow: the same verb without a password is ordinary history. A rule that swallowed /// connect guest would be teaching the user that history is unreliable. /// + [Test] + public async Task ServerEchoKeepsStandalonePasswordOutOfHistory() + { + var (app, telnet) = await Demo(); + const string secret = "standalone-password"; + var connection = telnet.Opened.Single(); + connection.ServerEchoesInput = true; + Send(app, secret); + + connection.ServerEchoesInput = false; + Send(app, "look"); + + await Assert.That(telnet.Sent).Contains(secret); + await Assert.That(app.HistoryEntries(InputBar.Primary)).DoesNotContain(secret); + await Assert.That(app.HistoryEntries(InputBar.Primary)).Contains("look"); + } + [Test] public async Task ALoginLineWithNoPasswordIsKept() { diff --git a/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs b/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs index ec2bf9ff..8e034221 100644 --- a/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs +++ b/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs @@ -10,6 +10,8 @@ namespace SharpMUTerm.Tui.Tests; /// internal sealed class RecordingTelnetSession : ITelnetSession { + public bool ServerEchoesInput { get; set; } + private readonly List<(int Width, int Height)> _sizes = new(); private readonly List _lines = new();