diff --git a/CHANGELOG.md b/CHANGELOG.md index 77bb268f..5702560d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,12 @@ and a release's notes are its section. The procedure is in [`docs/RELEASING.md`] ### Fixed +- A password is no longer echoed into the output or written to the session log. When a game takes over echo at its + password prompt (telnet `WILL ECHO`), the line you send is not echoed locally or kept in command history, and echo + comes back when the game hands it back. A typed `connect `, including one inside a stacked line + such as `look;connect …`, is shown as `(credential command hidden)` and is kept out of history; the game still gets + the real line. + - Desktop overlays follow modal visibility changes on the client UI thread, avoiding background-thread reads of changing pane layouts. - The desktop window answers terminal cell-pixel queries with its actual measured pixels, allowing 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..faed04dd 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 @@ -46,6 +45,61 @@ public static class HistorySecrets public static IReadOnlyList Verbs { get; } = LoginVerbs.Concat(PasswordVerbs).ToArray(); + /// + /// Whether a typed line carries a password anywhere in it: on any of its physical lines, or, where the world + /// stacks commands, in any piece once split on and stripped of a #N repeat + /// (look;#1 connect Corvid hunter2). Split without honouring \ escapes and whether or not the stack + /// would be refused, because this is over-inclusive on purpose (see the type's remarks): the line is judged as + /// typed, not as sent. + /// + public static bool CarriesCredential(string? line, string? separator) + { + if (string.IsNullOrWhiteSpace(line)) + { + return false; + } + + foreach (var physical in line.Split(['\r', '\n'])) + { + if (LooksLikeCredential(physical)) + { + return true; + } + + if (string.IsNullOrWhiteSpace(separator)) + { + continue; + } + + foreach (var piece in physical.Split(separator)) + { + if (LooksLikeCredential(WithoutRepeat(piece.Trim()))) + { + return true; + } + } + } + + return false; + } + + /// #3 connect Corvid hunter2 → connect Corvid hunter2; anything else unchanged. + private static string WithoutRepeat(string piece) + { + if (piece.Length < 3 || piece[0] != '#') + { + return piece; + } + + var end = 1; + while (end < piece.Length && char.IsAsciiDigit(piece[end])) + { + end++; + } + + return end > 1 && end < piece.Length && char.IsWhiteSpace(piece[end]) ? piece[end..] : piece; + } + /// /// Whether a command line looks like it carries a password and so must not be recorded. Verb /// matching is case-insensitive, because MU* command dispatch is. diff --git a/src/SharpMUTerm.Core/Session/WorldSession.cs b/src/SharpMUTerm.Core/Session/WorldSession.cs index e8e92fef..79fca14f 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 @@ -1158,38 +1161,43 @@ 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 hidden = Input.HistorySecrets.CarriesCredential(input, World.CommandSeparator) + || commands?.Any(command => Input.HistorySecrets.CarriesCredential(command, null)) == true; + Print(StyledLine.FromText(hidden ? CredentialHidden : input, EchoStyle)); } } + /// What local echo and the transcript show in place of a line carrying a password. + public const string CredentialHidden = "(credential command hidden)"; + private async Task ExpandAndSendAsync(string input, CancellationToken cancellationToken) { var expansion = Aliases.Expand(input); diff --git a/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs b/src/SharpMUTerm.Core/Telnet/ITelnetSession.cs index 0dc77e71..d73d823c 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 bcee3336..89f42431 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. @@ -434,6 +437,7 @@ public async Task ConnectAsync(CancellationToken cancellationToken = default) await _transport.ConnectAsync(cancellationToken).ConfigureAwait(false); _msdpAgreed = false; _mxpOn = false; + _serverEchoesInput = false; try { @@ -488,6 +492,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 e54743f5..3180c35e 100644 --- a/src/SharpMUTerm.Tui/SharpMUTermApp.cs +++ b/src/SharpMUTerm.Tui/SharpMUTermApp.cs @@ -5548,8 +5548,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()); @@ -6014,9 +6018,14 @@ private string HistoryOwner() => /// WorldSession.SendLoginAsync → SendRawAsync, which never touches the UI's history at /// all; the line this is for is the one someone types on a world they have not configured yet. /// + /// + /// Judged as typed, stacking included: look;#1 connect Corvid hunter2 is dropped on a world whose separator + /// is ;, refused stack or not (). + /// /// private bool IgnoreForHistory(string command) => - _config.Input.ExcludeCredentials && HistorySecrets.LooksLikeCredential(command); + _config.Input.ExcludeCredentials + && HistorySecrets.CarriesCredential(command, SendTarget()?.World.CommandSeparator); /// /// Which command line the ⌃R surface is showing the history of, in words — the second bar has its own diff --git a/tests/SharpMUTerm.Core.Tests/Input/HistorySecretsTests.cs b/tests/SharpMUTerm.Core.Tests/Input/HistorySecretsTests.cs index 9d437ef2..7da5c6c3 100644 --- a/tests/SharpMUTerm.Core.Tests/Input/HistorySecretsTests.cs +++ b/tests/SharpMUTerm.Core.Tests/Input/HistorySecretsTests.cs @@ -60,4 +60,29 @@ public async Task TheVerbsAreNamed() await Assert.That(HistorySecrets.Verbs).Contains("connect"); await Assert.That(HistorySecrets.Verbs).Contains("@password"); } + /// A typed line is judged as typed: any physical line, and any stacked piece once its repeat is stripped. + [Test] + [Arguments("connect Corvid hunter2", null)] + [Arguments("look\nconnect Corvid hunter2", null)] + [Arguments("look;connect Corvid hunter2", ";")] + [Arguments("look;#1 connect Corvid hunter2", ";")] + [Arguments("look;#101 connect Corvid hunter2", ";")] // a refused stack is still kept out + [Arguments("look | @password old=new", "|")] + [Arguments("look;;connect Corvid hunter2", ";;")] + public async Task AStackedOrMultiLineLoginCarriesACredential(string line, string? separator) + { + await Assert.That(HistorySecrets.CarriesCredential(line, separator)).IsTrue(); + } + + [Test] + [Arguments("look;connect Corvid hunter2", null)] // no stacking: `look;connect` is the verb + [Arguments("look;#1 connect guest", ";")] + [Arguments("look;say connect Corvid hunter2", ";")] + [Arguments("#3 north", ";")] + [Arguments("", ";")] + [Arguments(null, ";")] + public async Task OtherLinesDoNot(string? line, string? separator) + { + await Assert.That(HistorySecrets.CarriesCredential(line, separator)).IsFalse(); + } } 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..9d73634c 100644 --- a/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs +++ b/tests/SharpMUTerm.Tui.Tests/HistorySearchEndToEndTests.cs @@ -58,12 +58,13 @@ public ITelnetSession Open(ConnectionOptions options) /// absence of a fifth line mean something. /// /// - private static async Task<(SharpMUTermApp App, Transports Telnet)> Demo(string? password = null) + private static async Task<(SharpMUTermApp App, Transports Telnet)> Demo(string? password = null, string? separator = null) { Console.SetIn(TextReader.Null); var config = DemoScene.Build(); config.Worlds[0].Characters[0].Password = password; + config.Worlds[0].CommandSeparator = separator; var app = new SharpMUTermApp(config, Headless, new HeadlessConsoleDriver(Width, Height)); var telnet = new Transports(); @@ -448,6 +449,43 @@ public async Task NorByRecall() await Assert.That(app.ArmedInputText).IsEqualTo("look"); // there is nothing older } + [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"); + } + + /// + /// A stacked line is judged as typed: a login inside it keeps the whole line out of history, whether the stack is + /// sent (#1) or refused (#101, past the repeat cap), and the echo and the wire behave as they would. + /// + [Test] + [Arguments("#1")] + [Arguments("#101")] + public async Task AStackedLoginIsKeptOutOfHistoryAndSearch(string repeat) + { + var (app, _) = await Demo(separator: ";"); + const string secret = "stacked-secret"; + + Send(app, $"look;{repeat} connect Corvid {secret}"); + Send(app, "inventory"); + + await Assert.That(app.HistoryEntries(InputBar.Primary).Any(e => e.Contains(secret, StringComparison.Ordinal))) + .IsFalse(); + await Assert.That(app.HistoryEntries(InputBar.Primary)).Contains("inventory"); + } + /// /// 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. diff --git a/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs b/tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs index c9be6168..4815a030 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();