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