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.

Sign up for GitHub

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

Jump to bottom

[Bug]: may fail to ignore escape sequences for kitty progressive enhancements #4338

Open
krobelus opened this issue Jan 3, 2025 · 5 comments · May be fixed by #4417
Open

[Bug]: may fail to ignore escape sequences for kitty progressive enhancements #4338

krobelus opened this issue Jan 3, 2025 · 5 comments · May be fixed by #4417
Labels
bug report

Comments

@krobelus
Copy link

krobelus commented Jan 3, 2025 •
edited
Loading

Problem description

termux seems to fail to parse certain CSI escape sequences, and ends up echoing them.
This is unfortunate because it prevents programs like text editors and shells from using the kitty keyboard protocol

This was originally reported in the fish-shell matrix channel
I have not had a chance to reproduce this but it should be easy to do so.

Steps to reproduce the behavior.

printf '\x1b[=5u

This supposedly echoes 5u

What is the expected behavior?

Nothing should be echoed, since the string is a CSI-prefixed escape sequence

System information

not sure
krobelus added a commit to fish-shell/fish-shell that referenced this issue Jan 3, 2025
@krobelus
Probe for kitty keyboard protocol support
e49dde8 
We unconditionally request kitty keyboard protocol's progressive
enhancements.

It seems that a lot of terminals fail to parse CSI commands that
contain '=' such as \x1b[=5u.

1. [Midnight Commander](MidnightCommander/mc@0ea77d2)
2. Prompt 3 App (private bug tracker)
3. JetBrains IDEs https://youtrack.jetbrains.com/issue/IJPL-166234
4. Termux termux/termux-app#4338
5. Amazon Linux Web Console amazonlinux/amazon-linux-2023#871

It is difficult to fix the four remaining ones in a
timely manner, so let's query for support as described in
https://sw.kovidgoyal.net/kitty/keyboard-protocol/#detection-of-support-for-this-protocol
This uses CSI 5 n (device status report), which is the older brother
of CSI 6 n (cursor position report) we use as of recently.

Query asynchronously and enable progressive enhancements as soon
as we get a response. In theory this allow `cat malicious-file.txt`
leading us to believe the protocol is supported.

See #10994
@2-4601
Copy link

2-4601 commented Mar 1, 2025

I can confirm that printf '\x1b[=5u' prints 5u on Termux v0.118.1 running on Android 15.

Since Fish v4.0.0. is not yet packaged for Termux, I don't experience real issues yet.

However, when I ssh from Termux to a host that has Fish v4.0.0 set as the user's shell, I get these unexpected character sequences printed. For example, the prompt has 5u at the beginning of the line. Using the shell further outputs more and more garbage.

The workaround makes the shell usable again:

set -Ua fish_features no-keyboard-protocols

@TomJo2000
Copy link
Member

TomJo2000 commented Mar 1, 2025 •
edited
Loading

There is an open PR for it,

However the 32 Bit builds aren't currently working and I haven't had the time to look into that yet.
You can use the DEB package from the CI artifact for the AArch64 build if you want to do some preliminary testing.
It would probably be a good idea to ship that workaround as part of the system config file for Fish 4.0.0 if it's going to be a problem.

@krobelus
Copy link
Author

krobelus commented Mar 1, 2025 •
edited
Loading

It would probably be a good idea to ship that workaround as part of the system config file for Fish 4.0.0 if it's going to be a problem.

Why not fix the CSI parser - termux owns that code, no?
That fix could be created and tested independent of fish.

@TomJo2000
Copy link
Member

We currently have 1 (one) maintainer for the terminal emulator and main Termux App part of the project.
Bandwidth is sliced a little thin on this part.
If we can ship a workaround as part of the config in the meantime that helps take load off the development for the main application.

@krobelus
Copy link
Author

krobelus commented Mar 1, 2025

I'll create a PR later today. If someone else could test it that would be great - the main reason why I didn't fix it already is probably that I don't have toolchain set up to test it.
The advantage of fixing the parser is that it will also fix the SSH use case.

krobelus added a commit to krobelus/termux-app that referenced this issue Mar 1, 2025
@krobelus
Fixed: fully consume unknown CSI sequences containing non-numeric par…
9161c07 
…ameter byte

To-do: this is untested, would appreciate help.

Standard ECMA-48: Control Functions for Coded Character Sets specifies
the format of CSI commands.  Wikipedia has a concise description:
https://en.wikipedia.org/wiki/ANSI_escape_code#Control_Sequence_Introducer_commands

Do this for at least for sequences like "CSI = 5 u" that
contain non-numeric parameter bytes.

This fixes a problem with fish shell 4.0.0 which (for better or worse)
uses that sequence.

This patch introduces a slight inconsistency: for the above example,
"unknownSequence('u')" will be called.  The resulting log message may
be confusing, because we *do* support "CSI u". We should fix this to
log the entire unknown sequence. I can try doing that.

In future we should also fully consume all other unknown CSI commands.
A contrived example would be "CSI ! ! a".  If desired, I'm happy to
fix those as well, as I have some context already (but I don't think
that would block this patch).

Closes termux#4338
@krobelus krobelus linked a pull request Mar 1, 2025 that will close this issue
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug report
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed: fully consume unknown CSI sequences containing non-numeric parameter byte krobelus/termux-app
3 participants
@2-4601 @krobelus @TomJo2000