Add settings to warn about large or multiline pastes by beviu · Pull Request #6631 · microsoft/terminal

beviu · 2020-06-22T13:13:08Z

Before sending calling the HandleClipboardData member function on
the PasteFromClipboardEventArgs object when we receive a request
from the TermControl to send it the clipboard's text content, we
now display a warning to let the user choose whether to continue or
not if the text is larger than 5 KiB or contains the new line
character, which can be a security issue if the user is pasting the
text in a shell.

These warnings can be disabled with the largePasteWarning and
multiLinePasteWarning global settings respectively.

Closes #2349

beviu · 2020-06-22T13:27:04Z

src/cascadia/TerminalApp/TerminalPage.cpp

+            const bool hasNewLine = std::find(text.cbegin(), text.cend(), L'\n') != text.cend();
+            const bool warnMultiLine = hasNewLine && _settings->GlobalSettings().WarnAboutMultiLinePaste();
+
+            constexpr const std::size_t minimumSizeForWarning = 1024 * 5; // 5 KiB


I put 5 KiB here for now, but I don't know what should be the optimal threshold for the warning.

I think five kilobytes is fine!

zadjii-msft · 2020-06-22T13:31:26Z

These warnings can be disabled with the largePasteWarning and
multiLinePasteWarning global settings respectively.

I haven't read any of the code and I already love it

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft

My immediate concern is does there need to be a _RejectPasteButtonOnClick that clears _acceptPaste?

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/TerminalPage.h

beviu · 2020-06-22T13:51:09Z

My immediate concern is does there need to be a _RejectPasteButtonOnClick that clears _acceptPaste?

Yes, you're right, I will fix this.

EDIT: Done.

beviu · 2020-06-22T14:54:41Z

This is weird, I pushed a new commit on my branch (e4ccf28) but GitHub did not update the PR, and the CI did not run. Maybe someone at GitHub pasted a multi line command in a terminal accidentally and broke something...

EDIT: I fixed it by squashing my two commits and force pushing.

…haracter Before sending calling the `HandleClipboardData` member function on the `PasteFromClipboardEventArgs` object when we receive a request from the `TermControl` to send it the clipboard's text content, we now display a warning to let the user choose whether to continue or not if the text is larger than 5 KiB or contains the _new line_ character, which can be a security issue if the user is pasting the text in a shell. These warnings can be disabled with the `largePasteWarning` and `multiLinePasteWarning` global settings respectively. I merged `TerminalPage::_PasteFromClipboardHandler` and `TerminalPage::_PasteFromClipboardHandler` because I think that the code is simpler this way.

beviu · 2020-06-22T15:19:36Z

src/cascadia/TerminalApp/GlobalAppSettings.h

+    GETSET_PROPERTY(bool, WarnAboutLargePaste, true);
+    GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true);


Should these be enabled by default?

I think I was initially reluctant to enabled by default for both, but I've sat on it for a bit and I concur, enabled by default seems sensible.

carlos-zamora

Approving. I just have a few discussion points but I don't think they should be blocking.

Should we display the text that's going to be pasted for reference?

carlos-zamora · 2020-06-23T16:39:25Z

doc/cascadia/SettingsSchema.md

+| `largePasteWarning` | Optional | Boolean | `true` | When set to `true`, trying to paste text with more than 5 KiB of characters will display a warning asking you whether to continue or not with the paste. |
+| `multiLinePasteWarning` | Optional | Boolean | `true` | When set to `true`, trying to paste text with a _new line_ character will display a warning asking you whether to continue or not with the paste. |


For Discussion:
I'm wondering if we'll be adding more "warning" features over time. If so, what's everybody's thoughts on having a "warning." prefix? So these settings would be called warning.largePaste and warning.multiLinePaste.

And we could override some others like confirmCloseAllTabs to be warning.closeWithMultipleTabs.

oh wow, I actually like that a lot. I'd love another ACK on that design, but I'm on board.

I'll ACK that design. Should we do it in post so we can let greg904 in peace to merge his PR?

Making it a separate PR allows us to also add a compatibility lookup for warning.closeAllTabs

I third this :)

carlos-zamora · 2020-06-23T16:42:58Z

src/cascadia/TerminalApp/GlobalAppSettings.h

+    GETSET_PROPERTY(bool, WarnAboutLargePaste, true);
+    GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true);


DHowett · 2020-06-23T19:36:50Z

Should we display the text that's going to be pasted for reference?

Are you sure that's what you want

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

DHowett · 2020-06-23T19:41:02Z

src/cascadia/TerminalApp/TerminalPage.cpp

+            const bool hasNewLine = std::find(text.cbegin(), text.cend(), L'\n') != text.cend();
+            const bool warnMultiLine = hasNewLine && _settings->GlobalSettings().WarnAboutMultiLinePaste();
+
+            constexpr const std::size_t minimumSizeForWarning = 1024 * 5; // 5 KiB


I think five kilobytes is fine!

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/TerminalPage.h

zadjii-msft

Yea I'm cool with this. I won't block on the resume_foreground thing, I trust it'll get sorted out before Dustin signs off ☺️

zadjii-msft · 2020-06-24T13:39:01Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+    <value>Cancel</value>
+  </data>
+  <data name="LargePasteDialog.Content" xml:space="preserve">
+    <value>You are about to paste text that is longer than 5 KiB. This can make the terminal unresponsive while the text is being received on the other end. Are you sure you want to continue?</value>


nit:

Suggested change

<value>You are about to paste text that is longer than 5 KiB. This can make the terminal unresponsive while the text is being received on the other end. Are you sure you want to continue?</value>

<value>You are about to paste text that is longer than 5 KiB. Are you sure you want to continue?</value>

@cinnamon-msft for confirmation on the wording here though.

@zadjii-msft's wording looks great

Okay actually, can we change "Are you sure you want to continue?" to "Do you wish to continue?" to match the other message?

zadjii-msft · 2020-06-24T13:39:35Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+    <value>Cancel</value>
+  </data>
+  <data name="MultiLinePasteDialog.Content" xml:space="preserve">
+    <value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>


nit:

Suggested change

<value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>

<value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) might be executed automatically upon paste. Are you sure you want to continue?</value>

again, @cinnamon-msft to ACK this wording

I might rephrase as "You are about to paste text that contains multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Are you sure you want to continue?"

edit: "are you sure you wish to continue?" ("wish" stuck with me a bit more than "want")

That's my go at wordsmithing

I think I like:

You are about to paste text that contains multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Do you wish to continue?

src/cascadia/TerminalApp/TerminalPage.cpp

carlos-zamora · 2020-06-24T16:46:14Z

We also need a PR in the docs repo (or at the very least an issue please).

DHowett

I LOVE THIS SO MUCH
THANK YOU

Just minor nits. I'll let my red-x from before ride, but I'd like to have out the discussion about the coroutine returns

src/cascadia/TerminalApp/AppLogic.cpp

DHowett · 2020-06-24T23:47:09Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+    <value>Cancel</value>
+  </data>
+  <data name="MultiLinePasteDialog.Content" xml:space="preserve">
+    <value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>


I might rephrase as "You are about to paste text that contains multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Are you sure you want to continue?"

edit: "are you sure you wish to continue?" ("wish" stuck with me a bit more than "want")

DHowett · 2020-06-24T23:47:19Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

+    <value>Cancel</value>
+  </data>
+  <data name="MultiLinePasteDialog.Content" xml:space="preserve">
+    <value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>


That's my go at wordsmithing

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/TerminalPage.idl

DHowett · 2020-06-25T23:46:40Z

Paging @cinnamon-msft We need you in this PR to handle some dialog wording.

skyline75489 · 2020-06-27T00:04:56Z

I absolutely love this! Next maybe warn users about pasting binary text into the terminal? I think I’ve seen this in some terminals. Xfce Terminal warns me every time when I paste something with “sudo” command. That’s also kinda sweet but I don’t think we should go that far.

…aste-warning

beviu · 2020-06-27T08:29:19Z

~~It doesn't build (even on my machine) right now but I don't know why. I will look into this later, but do not merge it for now.~~

EDIT: Fixed.

ghost · 2020-07-01T19:42:04Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett

Thanks so much for this. :)

ghost · 2020-07-22T16:23:20Z

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

beviu commented Jun 22, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

zadjii-msft requested changes Jun 22, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 22, 2020

beviu commented Jun 22, 2020

View reviewed changes

src/cascadia/TerminalApp/TerminalPage.h Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 22, 2020

beviu commented Jun 22, 2020

View reviewed changes

carlos-zamora approved these changes Jun 23, 2020

View reviewed changes

DHowett requested changes Jun 23, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 23, 2020

Greg Depoire--Ferrer added 4 commits June 24, 2020 11:23

Fix formatting in Resources.resw

0e515f5

Make dialog presentation waitable

2c5e78c

Use the IAsyncOperation returned by ShowDialog

6385175

Use a weak reference to prevent circular dependency

5d73dc7

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 24, 2020

Add 'IDialog' to spell checker's apis.txt

6006394

zadjii-msft approved these changes Jun 24, 2020

View reviewed changes

Greg Depoire--Ferrer added 2 commits June 24, 2020 17:39

Only resume_foreground if we actually need to display a warning dialog

21eb079

Fix invalid name in FindName

b8152fc

beviu mentioned this pull request Jun 24, 2020

Add settings for paste warnings MicrosoftDocs/terminal#75

Merged

DHowett reviewed Jun 24, 2020

View reviewed changes

Greg Depoire--Ferrer added 2 commits June 25, 2020 13:17

Replace return by co_return to fix warning

aaca27c

Fix DialogPresenter comment

0861584

Greg Depoire--Ferrer added 2 commits June 27, 2020 10:00

Change wording

0f083cf

Merge branch 'master' of https://github.com/microsoft/terminal into p…

33196af

…aste-warning

Add missing close tag in Resources.resw

b7dfde2

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 1, 2020

DHowett changed the title ~~Add settings to warn when pasting text too large or with a new line character~~ Add settings to warn about large pastes or pastes w/ multiple lines Jul 1, 2020

DHowett changed the title ~~Add settings to warn about large pastes or pastes w/ multiple lines~~ Add settings to warn about large or multiline pastes Jul 1, 2020

DHowett approved these changes Jul 1, 2020

View reviewed changes

ghost merged commit 985f85d into microsoft:master Jul 1, 2020

ghost mentioned this pull request Jul 22, 2020

Paste size warnings #2349

Closed

This pull request was closed.

		GETSET_PROPERTY(bool, WarnAboutLargePaste, true);
		GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true);

		\| `largePasteWarning` \| Optional \| Boolean \| `true` \| When set to `true`, trying to paste text with more than 5 KiB of characters will display a warning asking you whether to continue or not with the paste. \|
		\| `multiLinePasteWarning` \| Optional \| Boolean \| `true` \| When set to `true`, trying to paste text with a _new line_ character will display a warning asking you whether to continue or not with the paste. \|

	<value>You are about to paste text that is longer than 5 KiB. This can make the terminal unresponsive while the text is being received on the other end. Are you sure you want to continue?</value>
	<value>You are about to paste text that is longer than 5 KiB. Are you sure you want to continue?</value>

Conversation

beviu commented Jun 22, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Jun 22, 2020

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

beviu commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beviu commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented Jun 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora commented Jun 24, 2020

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beviu commented Jun 22, 2020 •

edited by DHowett

Loading

beviu commented Jun 22, 2020 •

edited

Loading

beviu commented Jun 22, 2020 •

edited

Loading

DHowett Jun 24, 2020 •

edited

Loading

DHowett Jun 24, 2020 •

edited

Loading

beviu commented Jun 27, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.