Keep passwords out of local echo and transcripts (#125, finished)

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 <name> <password>`, 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

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/`:
 

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.</item>
 /// <item><b>Substitution at send time keeps the resolved line off every local surface.</b> Typed input is
-/// echoed and written to the session transcript (<c>WorldSession.SendUserInputAsync</c> →
-/// <c>Print</c>) and is offered to <c>InputHistory</c>; the login line goes straight out through
+/// offered to <c>InputHistory</c> and to local echo, which hides recognized credential commands
+/// from the output and transcript. The configured login line goes straight out through
 /// <c>SendRawAsync</c> with no echo, no transcript write and no history entry. That is unchanged by
 /// where the password is stored.</item>
 /// </list>

src/SharpMUTerm.Core/Input/HistorySecrets.cs

@@ -1,8 +1,7 @@
 namespace SharpMUTerm.Core.Input;
 
 /// <summary>
-/// The rule that keeps typed passwords out of <see cref="InputHistory"/>: 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.
 /// <para>
 /// A configured character never comes through here — <c>WorldSession.SendLoginAsync</c> 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<string> Verbs { get; } =
         LoginVerbs.Concat(PasswordVerbs).ToArray();
 
+    /// <summary>
+    /// 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 <paramref name="separator"/> and stripped of a <c>#N</c> repeat
+    /// (<c>look;#1 connect Corvid hunter2</c>). Split without honouring <c>\</c> 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.
+    /// </summary>
+    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;
+    }
+
+    /// <summary><c>#3 connect Corvid hunter2</c> → <c>connect Corvid hunter2</c>; anything else unchanged.</summary>
+    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;
+    }
+
     /// <summary>
     /// 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.

src/SharpMUTerm.Core/Session/WorldSession.cs

@@ -557,6 +557,9 @@ private void DeclareMeter(MxpGauge gauge)
 
     public bool IsConnected => _telnet?.IsConnected == true;
 
+    /// <summary>Whether the server has taken over echo, as it can do at a password prompt.</summary>
+    public bool ServerEchoesInput => _telnet?.ServerEchoesInput == true;
+
     /// <summary>
     /// 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
     /// <summary>
     /// Handles a line the player <em>typed</em> on the command line: the world's command stacking first
     /// (<see cref="Input.TypedCommands"/> — <c>n;e;e</c>, <c>#3 kill rat</c>), then each command through
-    /// <see cref="SendUserInputAsync"/>'s alias expansion. The line is echoed once, as typed. A composed post, a
-    /// grab and a script go through <see cref="SendUserInputAsync"/> or <see cref="SendRawAsync"/> instead, so a
+    /// <see cref="SendUserInputAsync"/>'s alias expansion. An accepted line is echoed once, with credential
+    /// lines hidden. A composed post, a grab and a script go through <see cref="SendUserInputAsync"/> or
+    /// <see cref="SendRawAsync"/> instead, so a
     /// <c>;</c> in a post is never a command boundary.
     /// </summary>
     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<string>? 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));
         }
     }
 
+    /// <summary>What local echo and the transcript show in place of a line carrying a password.</summary>
+    public const string CredentialHidden = "(credential command hidden)";
+
     private async Task ExpandAndSendAsync(string input, CancellationToken cancellationToken)
     {
         var expansion = Aliases.Expand(input);

src/SharpMUTerm.Core/Telnet/ITelnetSession.cs

@@ -9,6 +9,9 @@ public interface ITelnetSession : IAsyncDisposable
 {
     bool IsConnected { get; }
 
+    /// <summary>Whether the server has negotiated responsibility for echoing input.</summary>
+    bool ServerEchoesInput => false;
+
     /// <summary>
     /// 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.

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;
+
     /// <summary>
     /// 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<TelnetInterpreter> BuildInterpreterAsync()
             // Answers a server's DO NEW-ENVIRON; offers nothing on its own (the unsolicited-negotiation rule).
             .AddPlugin<NewEnvironProtocol>().WithClientEnvironmentVariables(TelnetSessionOptions.MnesIdentity(_options.TerminalTypes))
             .AddPlugin<CharsetProtocol>().WithCharsetOrder(_options.CharsetOrder)
+            .AddPlugin<EchoProtocol>().OnEchoStateChanged(enabled =>
+            {
+                _serverEchoesInput = enabled;
+                return ValueTask.CompletedTask;
+            })
             .AddPlugin<EORProtocol>().OnPrompt(OnPromptAsync)
             .AddPlugin<SuppressGoAheadProtocol>().OnPrompt(OnPromptAsync)
             .AddPlugin<PacketPatchProtocol>().OnPrompt(OnPromptAsync)

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() =>
     /// <c>WorldSession.SendLoginAsync</c> → <c>SendRawAsync</c>, 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.
     /// </para>
+    /// <para>
+    /// Judged as typed, stacking included: <c>look;#1 connect Corvid hunter2</c> is dropped on a world whose separator
+    /// is <c>;</c>, refused stack or not (<see cref="HistorySecrets.CarriesCredential"/>).
+    /// </para>
     /// </summary>
     private bool IgnoreForHistory(string command) =>
-        _config.Input.ExcludeCredentials && HistorySecrets.LooksLikeCredential(command);
+        _config.Input.ExcludeCredentials
+        && HistorySecrets.CarriesCredential(command, SendTarget()?.World.CommandSeparator);
 
     /// <summary>
     /// Which command line the ⌃R surface is showing the history of, in words — the second bar has its own

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");
     }
+    /// <summary>A typed line is judged as typed: any physical line, and any stacked piece once its repeat is stripped.</summary>
+    [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();
+    }
 }

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();
     }
 
-    /// <summary>
-    /// The asymmetry that justifies the whole feature, pinned so nobody closes the gap from the wrong end.
-    /// A user who types <c>connect Corvid …</c> 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 <em>for</em>.
-    /// Auto-login is the way out of that, not a reason to start filtering the command line.
-    /// </summary>
     [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();
     }
 
     /// <summary>