prevent pasting large input on secure compose by martgil · Pull Request #4914 · FlowCrypt/flowcrypt-browser

martgil · 2023-01-30T06:19:17Z

This PR adds a check when pasting a text input with a length greater than the limit and forbids pasting if it the condition is true.

close #4818

For manual test, please use: 55k.txt - paste it on intro and input body.

Tests (delete all except exactly one):
Difficult to test (#4914 (comment))

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

addresses the issue it closes (if any)
code is readable and understandable
is accompanied with tests, or tests are not needed
is free of vulnerabilities
is documented clearly and usefully, or doesn't need documentation

martgil · 2023-01-30T06:21:49Z

sir @rrrooommmaaa here is the change that works. currently, it doesn't have a test. I'd like to hear your suggestion regarding adding a test. is it necessary for us to inform the user about preventing the paste action via modal? If not, can I simply invoke the paste event in the input body and input intro then checks if both inputs received the data from the paste event?

rrrooommmaaa · 2023-01-31T17:31:42Z

extension/chrome/elements/compose-modules/compose-input-module.ts

      const div = document.createElement('div');
      div.appendChild(e.fragment);
      const html = div.innerHTML;
+      this.checkInputLengthBeforePasting(div.innerText, this.view.S.cached('input_text').get(0), e);


Instead of copying this.view.S.cached('input_text').get(0) it'd be more clean to extract the element from the squire object itself. SquireEditor actually has a method getRoot() for this. It was missing in our squire.d.ts type definitions, so I added it. You can now use this.squire.getRoot()

Another remark: is there a good reason to use div.innerText for the check because what actually used is sanitized div.innerHTML. Can it be that we inserting a big load of tags with little text? Can we perform the check on the sanitized value instead?

Also, when you preventDefault, I guess, further manipulations on e.fragment serve no purpose? If so, they should be skipped. It'd be more convenient to have the check method to return a boolean value and then we can either ev.preventDefault() or do the rest steps. The method may be called willInputLimitBeExceeded

Thank you for all the remarks. I apply some changes according to your helpful insight.

rrrooommmaaa · 2023-01-31T17:33:13Z

extension/chrome/elements/compose-modules/compose-input-module.ts

+  public checkInputLengthBeforePasting = (textToPaste: string, targetInputField: HTMLElement, ev: ClipboardEvent) => {
+    const currentLength = targetInputField.innerText.length;
+    const limit = 50000;
+    if (textToPaste.length + currentLength > limit) {


Do we want to gracefully handle the situation when the paste (partially) replaces the existing content? This will happen if there is an active selection at the moment of pasting.

Not sure, sir, since we only want to test the input limit. But if the process in this pr affects the performance significantly I think we could consider that.

We want to check the resulting element size, e.g. replacing a 40k text with another 40k text should trigger no error, right?
82782ac -- is it looking good?

the changes from 82782ac look good to me. I see that I missed that scenario for subtracting content. thank you.

rrrooommmaaa · 2023-01-31T17:39:28Z

sir @rrrooommmaaa here is the change that works. currently, it doesn't have a test. I'd like to hear your suggestion regarding adding a test.

As I understand, clipboard-related tests are not easy to develop. I guess @tomholub will suggest to skip the test for this issue.

is it necessary for us to inform the user about preventing the paste action via modal?

That's a question for @tomholub

tomholub · 2023-01-31T18:29:55Z

I think we should indeed do a modal, and as Roman says, ok to skip test if difficult to do.

rrrooommmaaa · 2023-02-03T17:59:40Z

extension/chrome/elements/compose-modules/compose-input-module.ts

+      if (this.willInputLimitBeExceeded(sanitized.trim(), this.squire.getRoot())) {
+        e.preventDefault();
+        return;
+      }


Xss.setElementContentDANGEROUSLY(div, sanitized); // xss-sanitized
can also be moved after if {...}, right?

right - you are correct. So that Xss.set... will execute only when needed.

rrrooommmaaa · 2023-02-03T18:01:11Z

extension/chrome/elements/compose-modules/compose-input-module.ts

      const html = div.innerHTML;
      const sanitized = this.isRichText() ? Xss.htmlSanitizeKeepBasicTags(html, 'IMG-KEEP') : Xss.htmlSanitizeAndStripAllTags(html, '<br>', false);
      Xss.setElementContentDANGEROUSLY(div, sanitized); // xss-sanitized
+      if (this.willInputLimitBeExceeded(sanitized.trim(), this.squire.getRoot())) {


Do we need trim()? Why? sanitized is HTML. Did you notice any extra space appear during testing?

Yes, the secure compose has an extra space.

(before)

(after)

…put-on-paste-for-contenteditable

martgil · 2023-02-16T10:20:25Z

@rrrooommmaaa ready for review. thank you!

rrrooommmaaa · 2023-02-17T19:36:45Z

I think we should indeed do a modal, and as Roman says, ok to skip test if difficult to do.

@martgil Please add a modal saying something like The paste operation can't be completed because the resulting text size would exceed the allowed limit of 50K.

martgil · 2023-02-18T03:59:22Z

@rrrooommmaaa I just added the modal. I'm thinking to put the reusable warning message in ComposeInputModule but I think it was inappropriate and decided to use the same warning message in two separate places. Is it okay?

extension/chrome/elements/compose-modules/compose-input-module.ts

extension/chrome/elements/compose.ts

…#4943) * build(deps-dev): bump @openpgp/web-stream-tools from 0.0.11 to 0.0.13 Bumps [@openpgp/web-stream-tools](https://github.com/openpgpjs/web-stream-tools) from 0.0.11 to 0.0.13. - [Release notes](https://github.com/openpgpjs/web-stream-tools/releases) - [Commits](openpgpjs/web-stream-tools@v0.0.11...v0.0.13) --- updated-dependencies: - dependency-name: "@openpgp/web-stream-tools" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> * #4282 fix: duplicated contact search result (#4944) * fix: dupliated contact search result * feat: added ui test * fix: pr review * #4940 Update build script to not produce .bak files (#4945) * update build script * rename STREAMS_OUTDIR to STREAMS_FILES * #4844 feat: renew id token when expires (#4949) * feat: renew id token when expires * fix: pr reviews * Employ user verification mechanisms from OpenPGP v5 (#4946) * reuse getPrimeryUser method * verifying key users * allow localhost domain for email address * fix * fix and test * use verified users in key-import-ui * PR review fixes --------- Co-authored-by: Roman Shevchenko <roman@flowcrypt.com> * prevent pasting large input on secure compose (#4914) * prevent pasting large input on secure compose * Added type definition for SquireEditor.getRoot() * apply requested change * update * cleanup * update * consider selection * add warning modal * update * update * update --------- Co-authored-by: Roman Shevchenko <rrrooommmaaa@mail.ru> Co-authored-by: Roman Shevchenko <roman@flowcrypt.com> * fix --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Ioan Moldovan <mobilestar108@outlook.com> Co-authored-by: Roma Sosnovsky <roma.sosnovsky@gmail.com> Co-authored-by: Roman <rrrooommmaaa@mail.ru> Co-authored-by: Roman Shevchenko <roman@flowcrypt.com> Co-authored-by: Mart G <46025304+martgil@users.noreply.github.com>

prevent pasting large input on secure compose

db62744

martgil requested a review from rrrooommmaaa as a code owner January 30, 2023 06:19

Added type definition for SquireEditor.getRoot()

9646d45

rrrooommmaaa reviewed Jan 31, 2023

View reviewed changes

martgil added 3 commits February 3, 2023 17:10

apply requested change

3349f70

update

b90f9a5

cleanup

f61d1e5

rrrooommmaaa reviewed Feb 3, 2023

View reviewed changes

martgil added 2 commits February 16, 2023 17:32

Merge remote-tracking branch 'origin/master' into issue-4818-check-in…

fae387d

…put-on-paste-for-contenteditable

update

0d21743

consider selection

82782ac

martgil added 2 commits February 18, 2023 11:23

add warning modal

8b394b3

update

7188655

rrrooommmaaa suggested changes Feb 18, 2023

View reviewed changes

extension/chrome/elements/compose-modules/compose-input-module.ts Outdated Show resolved Hide resolved

rrrooommmaaa reviewed Feb 18, 2023

View reviewed changes

extension/chrome/elements/compose-modules/compose-input-module.ts Show resolved Hide resolved

rrrooommmaaa reviewed Feb 18, 2023

View reviewed changes

extension/chrome/elements/compose.ts Show resolved Hide resolved

update

d1f23e7

martgil marked this pull request as draft February 18, 2023 08:45

update

691c5df

martgil marked this pull request as ready for review February 18, 2023 09:09

martgil requested a review from rrrooommmaaa February 18, 2023 09:14

rrrooommmaaa approved these changes Feb 20, 2023

View reviewed changes

rrrooommmaaa merged commit 09b17a8 into master Feb 20, 2023

rrrooommmaaa deleted the issue-4818-check-input-on-paste-for-contenteditable branch February 20, 2023 07:59

Conversation

martgil commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To be filled by reviewers

Uh oh!

martgil commented Jan 30, 2023

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!

Choose a reason for hiding this comment

Uh oh!

rrrooommmaaa commented Jan 31, 2023

Uh oh!

tomholub commented Jan 31, 2023

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!

martgil commented Feb 16, 2023

Uh oh!

rrrooommmaaa commented Feb 17, 2023

Uh oh!

martgil commented Feb 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martgil commented Jan 30, 2023 •

edited

Loading