Add initial support for paste filtering and bracketed paste mode #7508

skyline75489 · 2020-09-02T12:39:58Z

Summary of the Pull Request

This adds "bracketed paste mode" to the Windows Terminal.

References

PR Checklist

Closes bracketed paste in conhost #395
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manually tested.

Unit testing TBD.

skyline75489 · 2020-09-02T12:43:13Z

src/types/PasteConverter.cpp

+#include "precomp.h"
+#include "inc/PasteConverter.h"
+
+using namespace Microsoft::Console::Utils;


I don't honestly know where to put this class. Right now it is heavily VT-related. But in the future maybe other features will be added, making it less VT-related. So perhaps Types is also OK?

I think we should keep this VT-specific for now. It can be refactored later if need be.

skyline75489 · 2020-09-02T13:10:59Z

src/terminal/adapter/adaptDispatch.cpp

+// - enabled - true to enable, false to disable.
+// Return value:
+// True if handled successfully. False otherwise.
+bool AdaptDispatch::EnableBracketedPasteMode(const bool /*enabled*/)


I'm not sure what to do with conhost. Those legacy code always scare me . Call we just leave it like this?

skyline75489 · 2020-09-02T13:14:27Z

src/types/PasteConverter.cpp

+    {
+        // For security reasons, control characters should be filtered.
+        // Here ASCII control characters will be removed, except HT(0x09), LF(0x0a), CR(0x0d) and DEL(0x7F).
+        converted.erase(std::remove_if(converted.begin(), converted.end(), [](wchar_t c) {


I try to minimize the copy opertion so I choose erase instead of regex_replace. Performance-wise, I think this should be enough for most cases.

skyline75489 · 2020-09-02T13:50:30Z

src/types/PasteConverter.cpp

+                return false;
+            }
+
+            return c >= L'\x00' && c <= L'\x08' ||


The best resource I can find is here https://security.stackexchange.com/a/52655. It suggest removing every control characters except:

CR(0x0d) \r

LF(0x0a) \n

HT(0x09) \t,

DEL(0x7F)

Then there's some other opinions in https://bugzilla.gnome.org/show_bug.cgi?id=753197, regarding removing:

0x80-0x9F

And here https://bugzilla.gnome.org/show_bug.cgi?id=794653, regarding removing:

BS(0x08)

DEL(0x7F)

Since security is my thing even more than dev stuff, I would agree all of these should be disabled by default. I would also agree we should probably put it behind a setting for certain control characters to be allowed such as BS and DEL with nice warning basically saying "you're on your own if you get hacked".

Thanks @WSLUser. That’s a great idea, to give users more options controlling paste behavior. In fact many terminals offer these kind of options so I figure a dedicated class for pasting is a must.

I think there's some confusion here about what bracketed paste is actually meant to do. On most terminals I've tested, it doesn't do any additional filtering - it just adds the escape sequences at the beginning and end of the paste (that's the "bracketing").

While some terminals do filter out certain control characters, or convert them in some way, they do that all the time - not just when bracketed paste mode is enabled. So I think you're kind of mixing two different features here - paste filtering (which should have nothing to do with bracketed paste mode), and then the bracketed paste mode itself.

Thanks now I realize that. It’s just we don’t have a setting for people to enable/disable filtering pasted content. So I think that can be added later with another flag SanitizeContent or something. Or should we just filter them all without letting people choose.

In my imagination there will be more flags coming in the future and do various kinds of things

I can understand wanting to add support for paste filtering (although I would have expected that in a PR named something like "Add paste filtering"). I also understand leaving the option to enable or disable it for a follow-up PR. What I don't understand is why we want to tie this to bracketed paste - it's a completely separate concept.

If we want to make the paste filtering controllable via an escape sequence for some reason, then we should be inventing our own escape sequence for that - not repurposing an existing sequence. I don't know if I've just misunderstood the intention of this PR, but it make no sense to me.

Yeah I kind misunderstand the relation between filtering escape sequence and adding support for bracketed paste mode. I’ll try to make this better.

DHowett · 2020-09-02T16:40:25Z

So, full disclosure: I tried to implement bracketed paste support in conhost before Terminal was even a thing. There's a number of interrelated issues here, and I'm not sure what percentage of problem space coverage I want for this.

We decided to hold off on my initial implementation (https://github.com/microsoft/terminal/tree/dev/duhowett/bracketed_paste (wow: last updated "2 years ago")) because we're lacking a higher level abstraction for input types in conhost. There's a class that handles the translation of clipboard input into raw key events, but it does so at paste time instead of at read time. It can't necessarily know that the terminal is in VT input mode & that bracketed paste is turned on.

We considered introducing a "ClipboardPasteEvent" that would be unspooled into individual input records on the input buffer when the event was read, and simultaneously we would move VT translation from queue insertion to queue removal to unify around a single input buffer paradigm.

For various reasons -- some of which are documented in #4954 (comment) -- we never did this.

I'm wondering if it makes sense to leverage our ability to do VT passthrough directly from an application to the terminal (and back) and keep the bracketed paste implementation in terminal only.

WSLUser · 2020-09-02T17:58:36Z

I'm wondering if it makes sense to leverage our ability to do VT passthrough directly from an application to the terminal (and back) and keep the bracketed paste implementation in terminal only.

I think I prefer it that way actually. Due to the nature of this particular feature, it makes more sense for it to be an app-specific feature for terminals utilizing passthrough mode.

DHowett · 2020-09-02T18:03:27Z

@WSLUser it bears understanding that conhost is a terminal as well, we just suppress the use of many of its terminal features when we run it inside WT

skyline75489 · 2020-09-03T00:54:01Z

It was also my first thought, to add something like PasteEvent to make bracketed paste happen in both conhost and WT. Then the complexity of it just stoped me from actually implementing it. So I changed my mind to make a minimium but working one only for WT.

This is a feature that I've been craving for in WT. It works smooth for iTerm2 on macOS and many terminals on Linux.

eyalz800 · 2020-10-03T13:34:24Z

@skyline75489 I don't know what stage you're in implementing this, but I built windows terminal with your commits and it works. I'm already using this. Just wanted to say thanks.

skyline75489 · 2020-10-08T11:55:51Z

@eyalz800 I'm glad my work helps. Unfortunately I think this PR is still in its very early stage. Besides this issue itself belongs to the "Console Backlog" category which means it will not be prioritized until more urgent issues are resolved. I will keep an eye on this one and try to push my work forward. But it's going to take some time.

asford · 2020-10-14T23:58:43Z

@skyline75489 Would you be open to splitting the PR into an initial pull for bracketed paste mode, and a follow-up for control-code filtering? Would that help this land faster? Bracketed paste alone would be an excellent usability improvement for Terminal.

musm · 2020-11-07T22:01:37Z

@skyline75489 Many thanks for the effort here. I do echo @asford as I think it would be great if we could at least get bracketed paste mode in. Perhaps following the suggestion to split the PR would help accelerate review and a merge in the repo code base?

skyline75489 · 2020-11-08T01:08:38Z

Thanks guys for your interest in this project. Right now we are on our way towards 1.5 release. I feel that bracketed paste might be able to land in 1.6 or 1.7. I understand your eagerness. I really do. I am the one who filed this PR, after all. Still this is a backlog issue which means we have more important things to work on, for example, settings UI. I will consider reconstructing this PR later. Thanks again for your enthusiasm in this project. 获取 Outlook for Android<https://aka.ms/ghei36>

skyline75489 · 2021-01-21T03:12:16Z

Closed in favor of #8840 .

This adds the skeleton code for "bracketed paste mode" to the Windows Terminal. No actual functionality is implemented yet, just the wiring for handling DECSET/DECRST 2004. References #395 Supersedes #7508

This adds the skeleton code for "bracketed paste mode" to the Windows Terminal. No actual functionality is implemented yet, just the wiring for handling DECSET/DECRST 2004. References microsoft#395 Supersedes microsoft#7508

This adds "paste filtering" & "bracketed paste mode" to the Windows Terminal. I've moved the paste handling code in `TerminalControl` to `Microsoft::Console::Util` to be able to easily test it, and the paste transformer from `TerminalControl` to `TerminalCore`. Supersedes #7508 References #395 (overall bracketed paste support request) Tests added. Manually tested.

skyline75489 · 2021-02-16T09:46:03Z

FYI with #9034 merged the bracketed paste mode support in Windows Terminal should land in 1.7 preview. Thanks everyone for the support and suggestions. Cheers! 🎉

skyline75489 added 3 commits September 2, 2020 18:14

Init

9b4b08b

It works!

8cf4ccb

Clean

50f8e77

ghost added Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase labels Sep 2, 2020

skyline75489 commented Sep 2, 2020

View reviewed changes

Audit

1fdf4de

skyline75489 commented Sep 2, 2020

View reviewed changes

Typo

a360eec

skyline75489 commented Sep 2, 2020

View reviewed changes

Add FilterControlCodes flag

56d46ac

skyline75489 changed the title ~~Add initial support for bracketed paste mode~~ Add initial support for paste filtering and bracketed paste mode Sep 3, 2020

Merge branch 'master' into chesterliu/dev/bracketed-paste

0399056

Merge branch 'master' into chesterliu/dev/bracketed-paste

6dcbd57

Merge branch 'master' into chesterliu/dev/bracketed-paste

2fdf153

skyline75489 force-pushed the chesterliu/dev/bracketed-paste branch from 103ea8d to 2fdf153 Compare October 19, 2020 10:43

microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27

skyline75489 mentioned this pull request Dec 17, 2020

Multi-line paste warning doesn't detect \r, only \n #8601

Closed

skyline75489 closed this Jan 21, 2021

skyline75489 mentioned this pull request Jan 21, 2021

Add skeleton code for bracketed paste mode #8840

Merged

skyline75489 deleted the chesterliu/dev/bracketed-paste branch January 25, 2021 02:57

This was referenced Jan 25, 2021

Sanitize paste content by filtering out control chars #8875

Closed

Add initial support for bracketed paste mode #8909

Closed

skyline75489 mentioned this pull request Feb 4, 2021

Add support for paste filtering and bracketed paste mode #9034

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for paste filtering and bracketed paste mode #7508

Add initial support for paste filtering and bracketed paste mode #7508

skyline75489 commented Sep 2, 2020

skyline75489 Sep 2, 2020

WSLUser Sep 2, 2020

skyline75489 Sep 2, 2020

skyline75489 Sep 2, 2020

skyline75489 Sep 2, 2020

WSLUser Sep 2, 2020 •

edited

Loading

skyline75489 Sep 2, 2020

j4james Sep 3, 2020

skyline75489 Sep 3, 2020

j4james Sep 3, 2020

skyline75489 Sep 3, 2020

DHowett commented Sep 2, 2020 •

edited

Loading

WSLUser commented Sep 2, 2020

DHowett commented Sep 2, 2020

skyline75489 commented Sep 3, 2020

eyalz800 commented Oct 3, 2020

skyline75489 commented Oct 8, 2020

asford commented Oct 14, 2020

musm commented Nov 7, 2020

skyline75489 commented Nov 8, 2020 via email •

edited by ghost

Loading

skyline75489 commented Jan 21, 2021

skyline75489 commented Feb 16, 2021

Add initial support for paste filtering and bracketed paste mode #7508

Add initial support for paste filtering and bracketed paste mode #7508

Conversation

skyline75489 commented Sep 2, 2020

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WSLUser Sep 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Sep 2, 2020 • edited Loading

WSLUser commented Sep 2, 2020

DHowett commented Sep 2, 2020

skyline75489 commented Sep 3, 2020

eyalz800 commented Oct 3, 2020

skyline75489 commented Oct 8, 2020

asford commented Oct 14, 2020

musm commented Nov 7, 2020

skyline75489 commented Nov 8, 2020 via email • edited by ghost Loading

skyline75489 commented Jan 21, 2021

skyline75489 commented Feb 16, 2021

WSLUser Sep 2, 2020 •

edited

Loading

DHowett commented Sep 2, 2020 •

edited

Loading

skyline75489 commented Nov 8, 2020 via email •

edited by ghost

Loading