Added password text box to remote console window

[SlugFiller]

Proposed changes

This adds a password input box below the console when running remote commands such as push/pull. This allows entering the password in a native control if one is required by the connection.

It is also usable for other input, like accepting server fingerprints, although the input is hidden regardless of the query.

Screenshots

Before

If using OpenSSH with the embedded console, direct typing and drag-and-drop may work, but the embedded console can corrupt input from auto-typers like KeePass. It also allows paste via Ctrl+V but not through right click/context menu.

If using OpenSSH with the emulated console, input isn't requested at all, and password input automatically fails.

If using Plink, password input automatically fails even in normal terminal. The only workaround is to use TortoisePlink which offers its own built-in password dialog.

After

An additional password input box is available underneath the console. It is possible to type into it, auto-type into it, paste into it (with Ctrl+V or right click menu), or drag and drop text into it. Upon pressing Enter, the text is fed into the console (including line feed).

In OpenSSH with embedded console, SSH may not allow password input through a pipe, so extra measures are taken to ensure it uses SSH_ASKPASS instead.

Plink cannot be fixed. The only workaround is to use TortoisePlink.

Test methodology

• Made several pull requests to a password-protected SSH server, testing all possible combinations of SSH and console choice.

Test environment(s)

• GIT 2.41.0.windows.1
• Windows 10.0.19045

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

---

✒️ I contribute this code under The Developer Certificate of Origin.

[mstv]

Just curious: Most of this is directly supported by ConEmu, too. One just needs to focus the control. What are you missing?

For Plink, there is a hardcoded special handling by GE. The input is redirected and handled in FormRemoteProcess.cs.

Could add your sign off to contributors.txt in a separate PR, please?

[gerhardol]

For setups where ConEmu is not use, this is definitively useful.
For ConEmu, enter info works for me. It is unfortunately not easier to realize that input is needed by the input form.
I do not like that the input box hides some of the interesting text.

It would be great if the input box could be displayed and focused if needed, but I assume that is not possible.

[SlugFiller]

What are you missing?

ConEmu is not a real control. As such it doesn't "focus" in the usual sense. So, for example, if I switch to KeePass to trigger auto-type from it, it switches to GitExtensions and focuses on the "Abort" button instead of ConEmu.

And, as I've said, it kills auto-typing in general. Even if using the integrated keystroke inside ConEmu, with the simplest auto-type pattern ({Password}{ENTER}) it fails. While ssh through the command line succeeds. Mind you that password typing in the command line is also problematic, because if you use a slightly more complex auto-type pattern, e.g. one that makes use of Home or End, then OpenSSH (unlike Putty) interprets them as special character sequences.

A native input box solves all of that because it behaves just like any GUI element.

TL;DR: KeePass is one of, if not the, most popular password management software (which is the second most secure choice after PK), and it doesn't work at all with ConEmu.

For Plink, there is a hardcoded special handling by GE.

Which absolutely did not work in any of my tests. Vanilla, or with my patch. I get fatal: protocol error: bad line length character: Admi. TortoisePlink works, though.

Also, the special handling there only handles asking for a key file, or saving server fingerprint. Nothing for handling password authentication.

It would be great if the input box could be displayed and focused if needed, but I assume that is not possible.

It could be set to become visible if certain text is received from the server. But you can't rely on the SSH to consistently give the same text for password input. Even if you look for 's password:, you could end up with false positives, if such a text appears, for example, in a commit message.

I do not like that the input box hides some of the interesting text.

It doesn't hide any text. It is below the console, and the window is enlarged to add room for the input box. (Although you can scale it up manually to be even bigger). If a large amount of "interesting text" appears in the console, it will just scroll down. Or, if you missed anything, you can manually scroll up, since a scroll bar will appear. No text is hidden (other than the password itself).

[RussKie]

Is there a reason we can't open a new popup when the password is asked for?

gitextensions/GitUI/HelperDialogs/FormRemoteProcess.cs

Lines 74 to 139 in 0054eca

	protected override bool HandleOnExit(ref bool isError)
	{
	if (_restart)
	{
	Retry();
	return true;
	}
	// An error occurred!
	if (isError && Plink)
	{
	var output = GetOutputString();
	// there might be another error, this condition is too weak
	/*
	if (output.Contains("successfully authenticated"))
	{
	isError = false;
	return false;
	}
	*/
	// If the authentication failed because of a missing key, ask the user to supply one.
	if (output.Contains("FATAL ERROR") && output.Contains("authentication"))
	{
	if (FormPuttyError.AskForKey(this, out var loadedKey))
	{
	// To prevent future authentication errors, save this key for this remote.
	if (!string.IsNullOrEmpty(loadedKey) && !string.IsNullOrEmpty(Remote) &&
	string.IsNullOrEmpty(Commands.Module.GetSetting("remote.{0}.puttykeyfile")))
	{
	Commands.Module.SetPathSetting(string.Format("remote.{0}.puttykeyfile", Remote), loadedKey);
	}
	// Retry the command.
	Retry();
	return true;
	}
	}
	if (output.Contains("the server's host key is not cached in the registry", StringComparison.OrdinalIgnoreCase))
	{
	string remoteUrl;
	if (string.IsNullOrEmpty(_urlTryingToConnect))
	{
	remoteUrl = Commands.Module.GetSetting(string.Format(SettingKeyString.RemoteUrl, Remote));
	if (string.IsNullOrEmpty(remoteUrl))
	{
	remoteUrl = Remote;
	}
	}
	else
	{
	remoteUrl = _urlTryingToConnect;
	}
	if (AskForCacheHostkey(this, remoteUrl))
	{
	Retry();
	return true;
	}
	}
	}
	return base.HandleOnExit(ref isError);

[RussKie]

This means the new textbox is always shown, which isn't good. I use OpenSSH and I don't need or like to see this textbox.

[SlugFiller]

I explained it above, but there's no reliable way to detect if and when a password is being asked for, at least not without integrating with the SSH client a heck of a lot tighter than simple stdin/stdout piping. The issue stems from the fact that neither SSH nor Git report a successful login. A request for a password is indistinguishable from a successful public key login followed by a commit message that mentions "password" in its text.

This is why something as invasive as a popup is an absolute no-go. You can't do anything that would block, or even appear to block, the current operation.

The current placement is as uninvasive as possible. A good comparison would be the "Branches" and "Filters" boxes at the top of the main window. I don't need or like to see them, but I don't demand they be removed, because their functionality may be necessary to someone.

[SlugFiller]

Based on the feedback, I've added a checkbox for turning password input display on and off, so it can be hidden if not needed. I've also added a "Send" button that allows sending a password without using the keyboard, e.g. if using drag and drop.

[mstv]

The translation needs to be updated (for both your PRs).
Though please add mnemonics before.
Then update the screenshot in the PR description in order to ease reviews.

[mstv]

Suggested change

	public static bool ShowProcessDialogPasswordInput
	{
	get => GetBool("showprocessdialogpasswordinput", true);
	set => SetBool("showprocessdialogpasswordinput", value);
	}
	public static readonly ISetting<bool> ShowProcessDialogPasswordInput= Setting.Create(DetailedSettingsPath, nameof(ShowProcessDialogPasswordInput), true);

[mstv]

Is subclassing really necessary? OnKeyDown should suffice.

[SlugFiller]

It is. The Enter key is a special key, and is therefore consumed by the system rather than being sent to the event handler. I saw some older examples to use OnKeyDown or OnKeyPress, but testing revealed that code was broken in current .Net versions. Overriding the method intended for handling the Enter key is the only way that's guaranteed to not arbitrarily break on future .Net versions.

[SlugFiller]

@mstv Mnemonics do not apply to either PR. In this one, trying to get to a control in the process dialog by typing characters into whatever control has the current focus is a bad idea. Also, none of the other controls in this dialog have mnemonics. In the other PR, the only new line is for the settings dialog, in which there are no mnemonics for any of the options.

[mstv]

Suggested change

	this.PasswordSend.Text = "Send";
	this.PasswordSend.Text = "&Send";

or remove this Button if it is not used at all?

[SlugFiller]

As I explained when adding it, the button is used for mouse-only login. User flow:

• Click on "Show password" checkbox
• Drag password from another program and drop it into the input box
• Click "Send" button
  (It's technically possible to drag and drop a password directly to ConEmu, but not to the EditBox-based one)

Having a keyboard mnemonic for a mouse-only flow doesn't make a lot of sense.

Additionally, neither of the other buttons, "Abort", "Ok", have mnemonics. So it would stand out like a sore thumb.

Finally: It is a good thing none of the buttons have mnemonics. Imagine if the dialog opens, the keyboard focus is on the ConEmu, and a user, seeing the mnemonic, naively tries to press the matching key to reach the control. Rather than moving to the control, the key would be sent to the server. This is a pretty high-caliber footgun.

Unless this is an absolute must, I believe it would be better if it remained without a mnemonic.

[mstv]

As I explained when adding it,

The PR description shall describe the entire change. Comments are just complements for the reasoning and for the discussion of the implementation.

the button is used for mouse-only login.
Having a keyboard mnemonic for a mouse-only flow doesn't make a lot of sense.

One cannot anticipate all ways how users will interact with the dialog.
Mnemonics are missing at many places yet. New controls shall be accessible.
(New translations should not become necessary just because we forgot to add mnemonics when adding controls.)

Additionally, neither of the other buttons, "Abort", "Ok", have mnemonics. So it would stand out like a sore thumb.

Not really: OK is the Default button (AcceptButton) and Abort the Cancel button.
And that's the key for a clearer UX and a simpler implementation:
As long as the text box is focused, the Send button is the Default button (it must not close the dialog, of course). Then there is no need for class TextBoxSubmit.
Since the Default and Enabled states of OK and Send are toggled when the process has finished, the Send button does not need a mnemonic because it will correctly indicate that it can be triggered by Enter.

Finally: It is a good thing none of the buttons have mnemonics. Imagine if the dialog opens, the keyboard focus is on the ConEmu, and a user, seeing the mnemonic, naively tries to press the matching key to reach the control. Rather than moving to the control, the key would be sent to the server. This is a pretty high-caliber footgun.

I disagree.
Interception of mnemonics or not is / should be a question of suitable ConEmu configuration.
(git.exe runs locally.)

[SlugFiller]

Changed the Send button to be a default button when the password input is focused. I can see the difference now.

Is this fine now, or does it still require a mnemonic?

[mstv]

the Send button does not need a mnemonic

does it still require a mnemonic?

No, not the button but both checkboxes, please. (Yes, I am ignoring the sticky focus of ConEmu for now.)

And I think "Send" is too short and too technical. From user's perspective it is rather Enter input or just Input or maybe Send input. @RussKie?

[SlugFiller]

Both checkboxes? Should I change the text on KeepDialogOpen as well?

[mstv]

Anyway, I want to use #11182 for the process dialog without ConEmu in order to get rid of loading times and of additional console windows.

Are you not using the ConEmu?

As I do not really need it and it takes long before git is even started (#11044, depending on the PC and the annoying AV),
this PR will e.g. allow to confirm an SSH key (the only rare case I have to enter something). Then there will be no more need to open the console windows for fetch, push, etc. I guess.

[RussKie]

Anyway, I want to use #11182 for the process dialog without ConEmu in order to get rid of loading times and of additional console windows.

Are you not using the ConEmu?

As I do not really need it and it takes long before git is even started (#11044, depending on the PC and the annoying AV), this PR will e.g. allow to confirm an SSH key (the only rare case I have to enter something). Then there will be no more need to open the console windows for fetch, push, etc. I guess.

Is it possible to intercept a password request in the FormRemoteProcess.cs? #11182 (review)

[mstv]

Is it possible to intercept a password request

Such interceptions use to fail on unanticipated use cases. So rather provide a means for arbitrary input.

[mstv]

BTW: The "remote" in the title of this PR is wrong / misleading. It is about the Process dialog, used for (locally) running mostly git, which communicates with remote servers for some operations, of course.

[SlugFiller]

The "remote" in the title of this PR is wrong / misleading

It is designed so only FormRemoteProcess is affected (the controls are hidden in regular FormStatus). So if that's misleading, it's tech debt.

Kind of moot anyway. Since the other one was merged, feel free to close it. I won't close it myself (for now) because someone mentioned switching between the two if this one gets a sufficiently large public support.

[mstv]

Sorry, I was not aware of this old, misleading internal naming. The summary is a little clearer:

    /// <summary>
    /// Form that handles Plink exceptions.
    /// </summary>
    public partial class FormRemoteProcess : FormProcess