Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace backspace code on client side from 0x7F by 0x08 #715

Open
wants to merge 2 commits into
base: latestw_all
Choose a base branch
from

Conversation

kor44
Copy link

@kor44 kor44 commented Jan 24, 2024

PR Summary

Replace backspace code from Control-? to Control-H

PR Context

Some proprietary SSH servers use Control-H (ASCII code 8) as default code for backpace if ssh client does not provide ERASE option. OpenSSH does not support to set this option as Putty. I have modified input processing on client side to replace Control-? (ASCII code 127) by Control-H (ASCII code 8).
There is pull request Handle backspace (Control+?) in no-pty session which add support processing Control-? on server side of OpenSSH

Some proprietary SSH servers use default code for backpace as 0x08 (^H) if ssh client does not provide option ERASE option.  Putty has option to specify code of  backspace but OpenSSH hasn't.  One solution is to replace 0x7F (^?) by 0x08(^H) on client side.
@tgauth
Copy link
Collaborator

tgauth commented Jan 29, 2024

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@tgauth tgauth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution, @kor44!

Could you elaborate on which proprietary SSH servers demonstrate this behavior?

This creates a slight change in behavior, as it erases the entire word, as opposed to erasing per character.
i.e. if I type "this is a test" within the ssh session, then I hit the backspace key 7 times, the entire line is erased.
Currently, hitting the backspace key 7 times would only erase " a test"

Therefore, I'd like to see if this could be made optional.

Also, I suggest creating a working branch off of latestw_all, as it's hard to build/test an outdated branch.

@kor44
Copy link
Author

kor44 commented Feb 5, 2024

I have synced my branch but it's very strange that not used latest from the beginning.

I made tests with different OpenSSH server versions (OpenSSH_8.4, OpenSSH_8.0 linux) type "this is a test" and backspace works as expected (delete single symbol) even when set console locate as UTF-8

@tgauth
Copy link
Collaborator

tgauth commented Feb 12, 2024

@kor44, ah ok - I tested with the latest Win32-OpenSSH (version 9.5) as the server & client (with the proposed change). My primary concern is the behavior on the latest versions for Windows/Linux.

@kor44
Copy link
Author

kor44 commented Feb 23, 2024

Should I make some actions to merge this pull request?

@tgauth
Copy link
Collaborator

tgauth commented Mar 11, 2024

Should I make some actions to merge this pull request?

No, the PR needs to be approved before it can be merged. I'm still concerned about the behavior change with the latest OpenSSH server versions (9.5 for Windows, and 9.7 for Linux (as of today))

I made tests with different OpenSSH server versions (OpenSSH_8.4, OpenSSH_8.0 linux)

Please test with the latest OpenSSH server versions mentioned above.

@kor44
Copy link
Author

kor44 commented Mar 23, 2024

Should I make some actions to merge this pull request?

No, the PR needs to be approved before it can be merged. I'm still concerned about the behavior change with the latest OpenSSH server versions (9.5 for Windows, and 9.7 for Linux (as of today))

I made tests with different OpenSSH server versions (OpenSSH_8.4, OpenSSH_8.0 linux)

Please test with the latest OpenSSH server versions mentioned above.

I have only access to Linux (OpenSSH_9.6p1, OpenSSL 3.2.1 30 Jan 2024)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants