Keep passwords out of local echo and session transcripts

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

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
@@ -1157,36 +1160,40 @@ 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 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)

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.
@@ -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<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

@@ -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());

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>

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
     /// <c>connect guest</c> would be teaching the user that history is unreliable.
     /// </summary>
+    [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()
     {

tests/SharpMUTerm.Tui.Tests/RecordingTelnetSession.cs

@@ -10,6 +10,8 @@ namespace SharpMUTerm.Tui.Tests;
 /// </summary>
 internal sealed class RecordingTelnetSession : ITelnetSession
 {
+    public bool ServerEchoesInput { get; set; }
+
     private readonly List<(int Width, int Height)> _sizes = new();
     private readonly List<string> _lines = new();