microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.32k stars 8.29k forks source link

Support xterm VT sequence (OSC 52) for setting the clipboard #2946

Closed fpqc closed 4 years ago

fpqc commented 5 years ago

Mintty and xterm support options to allow setting the clipboard by VT sequences, a feature used by tmux to sync its internal clipboard with the system clipboard. I also wrote the patch for st to add support for setting Xorg's PRIMARY clipboard from VT as well as supporting OSC 52 clipboard setting in tmux if it is emitted from something like vim or a remote nested tmux instance (see patch).

The current code in st is under

strhandle(void)
{
[...]
case 52

in st.c, where it is implemented in a few lines (and the low-level logic is clearer than in my tmux snippet). Notice that the first argument is ignored (since it is only used to read back the system clipboard, which is insecure for reasons spelled out below).

Implementation: The documentation for this control sequence in xterm is available on invisible-island (see OSC Ps for Ps = 52). The documentation isn't amazing, but essentially the data is emitted as base64 and is decoded back to UTF-8 by base64dec. I don't know how you would implement this into the Windows clipboard, which iirc still uses USC2/UTF-16, but base64dec does give you back a UTF-8 string, and reencoding that to a string compatible with the Windows clipboard is probably something that I think is just handled nowadays by a standard Windows API.

Potential security implications: I would leave this setting as optional and on a profile-by-profile basis (gated behind an option 'allowClipboardSharing', in case a nasty script emits uses this to try to write data to the clipboard (which at least in the past had been the source of an exploit in an older version of Windows). Note: I would not add support for the read portion of the control sequence (my tmux patch for example simply ignores this case), since this would allow unauthorized reading of the system clipboard.

Example Use Case: https://sunaku.github.io/tmux-yank-osc52.html

Note that X11 forwarding is not really a desirable option just for clipboard sharing in Windows, given that the Windows clipboard is not an X clipboard, and X11 forwarding theoretically has the same security implications (in fact worse implications).

egmontkob commented 5 years ago

Let me also ephasize the security aspects of this feature.

in case a nasty script emits

It doesn't necessarily mean a malicious script. It could be a poorly coded application combined with an attacker-supplied input, such as OpenSSH's SCP client (bullet points 3 & 4) as discovered recently, or another example mentioned in VTE's issue tracker.

I would leave this setting as optional [...] I would not add support for the read portion

Sounds to me the most reasonable approach, if this feature is implemented at all. Another idea from the VTE bug is to present a popup dialog to confirm setting the clipboard (although it's sure annoying after some time).

fpqc commented 5 years ago

Whenever this spec is implemented, and clipboard-setting from within the terminal is implemented, I think even I (a non-programmer) could write this patch. My only concern is with mixing levels, since I'm not sure if this spec is for the terminal application or for plain conhost, since I think VT processing is done at the level of conhost, rather than at the level of the Terminal, and if the option will be controlled by the profiles.json, I don't know how to turn on a setting at the level of conhost.

fpqc commented 5 years ago

@DHowett-MSFT I was looking through OutputStateMachine.cpp in reference to this comment https://github.com/microsoft/terminal/issues/2958#issuecomment-536315829 .

You suggested that we actually parse the escape sequence, then fail.

Do you mean in this snippet:

https://github.com/microsoft/terminal/blob/1caece74abdae9e57642fdc3761bb432238c75f8/src/terminal/parser/OutputStateMachineEngine.cpp#L689-L713

to add

//Let this control sequence pass through to the attached terminal (if it exists)
case OscActionCodes::SetClipboard:
fSuccess = false;
break;

then just let this this piece of the method dump the whole string through to the attached terminal anyway?

https://github.com/microsoft/terminal/blob/1caece74abdae9e57642fdc3761bb432238c75f8/src/terminal/parser/OutputStateMachineEngine.cpp#L751-L756

Where in the cascadia/TerminalApp can we then perform secondary parsing to parse, decode, and then insert this into the clipboard? Does TerminalApp have its own secondary parser or at least a secondary parsing stage that calls the original parser? I don't think I'm experienced enough to add a whole new stage to TerminalApp's program logic if that doesn't exist yet.

If a secondary parsing stage doesn't exist yet, I think I'll have to wait on https://github.com/microsoft/terminal/issues/1173

zadjii-msft commented 5 years ago

Actually, I'm just going to yank conhost off this one. This seems like it's easiest for us to just handle in the Terminal layer, by "ignoring" it in the conpty layer, effectively causing the string to passthrough straight to the Terminal.

That way, we can have the Terminal implement more granular settings to enable/disable this feature.

IMO we could probably get away without the "query the clipboard contents" part of this feature. That would at least alleviate some of the concerns.

chiefjester commented 4 years ago

I just learned this today, this is the most straightforward way to sync clipboard on tmux and vim. Right now I'm using X11 forwarding just to handle my clipboard syncing. I hope this land soon as this will greatly simplify things and give us out of the box support for clipboard syncing. 🙏🏻

hh commented 4 years ago

Another windows option:

Minitty in 2.6.1 (Included with https://git-scm.com/download/win) https://github.com/mintty/mintty/pull/586 #Merged Sept 20th, 2016

I can't find the commit/pr for it, but I think putty supports OSC 52 as well.

chiefjester commented 4 years ago

@zadjii-msft , @DHowett-MSFT, @cinnamon-msft is there a chance this can be added in 1.0 since it’s a low hanging fruit?

hh commented 4 years ago

Imagine for a moment, given Docker Destkop for windows within Microsoft/terminal + osc 52:

kind cluster create
kubectl run -ti --generator=run-pod/v1 pod-with-osc52 --image kubemacs/kubemacs:latest -- echo "data within cluster" \| osc52.sh

And the data is available within the windows clipboard (or in our use case, a cloud-native development environment running in-cluster in-kind in-docker in-Microsoft/terminal).

We use this workflow on iTerm2, xterm, and others, but I'm having trouble onboarding windows folks.

zadjii-msft commented 4 years ago

@thisguychris Unless a community member is the one to contribute this, then unfortunately, this won't be making the 1.0 cut. However, if you do think this is low-hanging fruit and were willing to contribute a PR, we'd happily review and accept it by 1.0 😄

chiefjester commented 4 years ago

@zadjii-msft I hope I didn't hit a nerve when I said low hanging fruit. I've been following a trail of this issue for a while now.

Quoting @miniksa said:

_@fpqc, we're not concerned about the coding proficiency of our own noses. :P We realize that the implementation of this particular sequence is easy._

And a quote by you:

Actually, I'm just going to yank conhost off this one. This seems like it's easiest for us to just handle in the Terminal layer

This suggests it's going to be much easier since you don't have to deal with conhost anymore and just deal with Terminal layer? Or am I reading to it too much?

While I don't want to make promises, if someone can point me where to add those sequences, I would gladly try.

hh commented 4 years ago

I'm also willing to dig into this with some gentle guidance in the right direction.

zadjii-msft commented 4 years ago

Sure - here's an outline that's largely copy pasted from https://github.com/microsoft/terminal/issues/3004#issuecomment-590984729:

I think is involved here is roughly:

And I think it might just work.

I know that @DHowett-MSFT had a WIP prototype for this long ago when this project was just conhost, and might have some additional thoughts on the matter. EDIT: apparently I was wrong about this

DHowett-MSFT commented 4 years ago

Sorry, my WIP prototype was for bracketed paste mode :smile:

uzxmx commented 4 years ago

Hi, I just made a PR to add such support. Please give it a try.

hh commented 4 years ago

Thanks uzxmx!

I haven't been able to test yet, but I think the big target software here is:

Chromium has an etc folder with osc52 scripts for both editors and a quick shell script to test under tmux/tmate.

They should allow you to select text with it the editor and it show up in your windows clipboard, even over ssh.

This gist may help in configuring tmux/tmate.

osc52.sh is a quick script to test, which I use quite often over ssh. cat /some/file | osc52.sh and it's in my clipboard.

The editors should both work when run directly within terminal AND within the muxers when OSC52 support is enabled.

I'll try and take a look soon and test.

ghost commented 4 years ago

:tada:This issue was addressed in #5823, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

unxed commented 2 weeks ago

Please tell me, is OSC52 support enabled by default, or does it need to be enabled in some special way? Also, please tell me, from which version does it work?

lhecker commented 2 weeks ago

Support for copying to the clipboard was added in v1.2.2022.0. Pasting with OSC 52 was not implemented as it is a bit of a security hole (as mentioned in the linked PR).

unxed commented 2 weeks ago

Thanks! Should it be enabled manually somehow or it is enabled by default?

unxed commented 2 weeks ago

Also, v1.2.2022.0 is a preview release. Corresponding stable release is v1.2.2381.0, am I right?

DHowett commented 2 weeks ago

Yes, that's correct.

DHowett commented 2 weeks ago

(It is enabled by default.)