[HOLD for payment 2023-06-08] [$1000] Copying HTML character entities from the composer and then pasting them in the composer converts them to a literal character i.e. > becomes >.

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Log into an account and open a chat.
2. Type an HTML character entity in the composer e.g. > or & or £
3. Highlight, copy, and then paste the HTML character into the composer.

Expected Result:

HTML character entity is pasted as is i.e. > is pasted as >.

Actual Result:

HTML character entity is converted i.e. > becomes > , & becomes & , etc.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.17.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

expensify-html-entities-bug.mov

Recording.732.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Victor-Nyagudi
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1684521621028419

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01a1fad58c2e2ecbef
• Upwork Job ID: 1661203707551158272
• Last Price Increase: 2023-05-24

[melvin-bot]

Triggered auto assignment to @jliexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[YousefKhawaga]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Copying HTML character entities from the composer and then pasting them in the composer converts them to a literal character

What is the root cause of that problem?

when we handle the paste event in case of plain text we decoded to HTML so the entities get decoded

App/src/components/Composer/index.js

Line 360 in e1daecc

this.paste(Str.htmlDecode(plainText));

What changes do you think we should make in order to solve the problem?

we could change this to pass the plain text and decode only in case of HTML

What alternative solutions did you explore? (Optional)

we can escape the entities first before passing to the decode that will return to the original but will be more calculations

[jliexpensify]

Can repro on Staging v1.3.17-1

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01a1fad58c2e2ecbef

[melvin-bot]

Current assignee @jliexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav (External)

[melvin-bot]

Triggered auto assignment to @danieldoglas (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[kamaljitSharma21]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Copying HTML character entities from the composer and then pasting them in the composer converts them to a literal character i.e. > becomes >.

What is the root cause of that problem?

When we paste the data into composer we have used Str.htmlDecode() function to decode the text to html.

What changes do you think we should make in order to solve the problem?

As we can see in the code below we are collecting the data as plain text so we don't need to decode it

App/src/components/Composer/index.js

Line 358 in e1daecc

const plainText = event.clipboardData.getData('text/plain');

existing code

App/src/components/Composer/index.js

Line 360 in e1daecc

this.paste(Str.htmlDecode(plainText));

Here is the updated line - 

this.paste(plainText);

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Copy and paste HTML entities convert it to it's character representation, for example & to &.

What is the root cause of that problem?

When we paste a text into composer, we decode the text. There are 3 cases where we decode the text.

1. Paste a text containing image of emoji. This is handled specifically for Slack. (make sure copy the emoji from the sent message, not from Slack composer)
   

   App/src/components/Composer/index.js

   Lines 318 to 322 in d70b746

   	if (embeddedImages[0].dataset && embeddedImages[0].dataset.stringifyType === 'emoji') {
   	const plainText = event.clipboardData.getData('text/plain');
   	this.paste(Str.htmlDecode(plainText));
   	return;
   	}

2. When the clipboard contains HTML. When we copy text from composer, it will contains HTML. For example, if we copy & from composer and paste later, we will receive <meta charset='utf-8'>&amp;amp; as the HTML.
   

   App/src/components/Composer/index.js

   Lines 284 to 287 in d70b746

   	handlePastedHTML(html) {
   	const parser = new ExpensiMark();
   	this.paste(parser.htmlToMarkdown(html));
   	}

   
   htmlToMarkdown will decode the HTML too.
   https://github.com/Expensify/expensify-common/blob/68abe48ad71a98604fdbf5e8e960023ed5807ec2/lib/ExpensiMark.js#L575
3. Paste a plain text. We can copy from Notepad. (or copy from composer, but paste later without formatting, i.e., CMD/CTRL+Shift+V)
   

   App/src/components/Composer/index.js

   Lines 358 to 360 in d70b746

   	const plainText = event.clipboardData.getData('text/plain');
   	this.paste(Str.htmlDecode(plainText));

What changes do you think we should make in order to solve the problem?

For 1st and 3rd case, as we are pasting a plain text, we can safely remove Str.htmlDecode.

For the 2nd case, we should definitely keep the HTML decoding (for case like this). The proper solution to solve it is to encode the text we copy from Composer. We have a code to handle copying from Composer here:

App/src/components/Composer/index.js

Lines 377 to 388 in d70b746

	putSelectionInClipboard(event) {
	// If anything happens that isn't cmd+c or cmd+x, ignore the event because it's not a copy command
	if (!event.metaKey || (event.key !== 'c' && event.key !== 'x')) {
	return;
	}
	// The user might have only highlighted a portion of the message to copy, so using the selection will ensure that
	// the only stuff put into the clipboard is what the user selected.
	const selectedText = event.target.value.substring(this.state.selection.start, this.state.selection.end);
	Clipboard.setHtml(selectedText, selectedText);
	}

So, we can simply encode (_.escape / Str.htmlEncode, idk which one is preferable) the selectedText of the first param of setHtml.

[mananjadhav]

@danieldoglas @jliexpensify I don't think I can get to this soon. I am unassigning myself. Can you please tag another C+ member?

[s77rt]

taking this

[melvin-bot]

📣 @s77rt You have been assigned to this job by @jliexpensify!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[jliexpensify]

@s77rt assigned you as C+

[s77rt]

@YousefKhawaga Thanks for the proposal. Your RCA is correct but not complete as it does not explain why the issue is only reproducible when you copy from the Composer i.e. if you copy from a Slack message or GH the bug won't be reproducible.

[s77rt]

@kamaljitSharma21 Thanks for the proposal. I think it's the same as @YousefKhawaga's proposal so same note above ^

[YousefKhawaga]

@YousefKhawaga Thanks for the proposal. Your RCA is correct but not complete as it does not explain why the issue is only reproducible when you copy from the Composer i.e. if you copy from a Slack message or GH the bug won't be reproducible.

it's related to the pasting i tried from to get from GH same problem as the plainText is decoded no matter from where it has been copied.

27.05.2023_01.54.15_REC.mp4

[YousefKhawaga]

@s77rt this will happen if taken from any composer or notepad as it's treated as plain text

2.mp4

[s77rt]

@bernhardoj Thanks for the proposal. Your RCA is correct. The bug here is in two parts:

1. Data that we send to Clipboard is corrupted: The html version of selected text is not properly escaped. This explains why the bug occurs when you copy from ND Composer but does not if you copy from Slack Message.
2. Data that we read from Clipboard as text is treated as html. This explains why the bug occurs when you copy from other places (places that set clipboard data in text only e.g. GH Composer, Slack Composer, another app like notepad, etc.)

The suggested solutions (respectively) looks good to me:

1. The html version of a text is an escaped text and not the text as is. htmlVersion = _.escape(textVersion).
2. The Clipboard text data should be read as is (do not try to decode it because it's already a raw text - nothing to decode here).

Sorry for restating those ^ I felt some clarification is needed here.

I have also noticed that putSelectionInClipboard relies on the metaKey modifier which is available only on macOS, let's fix that and add ctrlKey modifier as well (asked here #13739 (comment) to be sure).

And...
🎀 👀 🎀 C+ reviewed

cc @danieldoglas

[bernhardoj]

@s77rt or we can even remove putSelectionInClipboard. The initial implementation of it is to append extra new line to the text, which is not the case anymore (removed by this PR).

[s77rt]

@bernhardoj That's even better. I tested earlier and I was not able to reproduce #12769. If you are not able to reproduce it either (after removal putSelectionInClipboard) then yeah let's remove putSelectionInClipboard.

[bernhardoj]

Yep, unable to reproduce #12769 after removing putSelectionInClipboard.

[s77rt]

Awesome! 🚀

[melvin-bot]

📣 @bernhardoj You have been assigned to this job by @danieldoglas!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[danieldoglas]

PR merged.

[jliexpensify]

Cool, looks like you qualify for the bonus!

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.21-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Don't decode plain text on pasting text to composer #19738

If no regressions arise, payment will be issued on 2023-06-08. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@s77rt] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt] Determine if we should create a regression test for this bug.
• [@s77rt] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

• The PR that introduced the bug has been identified: format the text plain pasted #10804
• The offending PR has been commented on: format the text plain pasted #10804 (comment)
• A discussion in #expensify-bugs has been started: https://expensify.slack.com/archives/C049HHMV9SM/p1685613854309939
• Determine if we should create a regression test for this bug: Yes

---

Regression Test Proposal (Web/Desktop)

1. Open Notes App (or any text editor)
2. Type &
3. Copy the typed text
4. Open ND App
5. Go to any chat
6. Paste the copied text in the Composer
7. Verify the text is pasted correctly i.e you should see &

[Victor-Nyagudi]

Should I go ahead and send a proposal to this issue's Upwork job for the reporting bonus or wait for an invitation to be sent?

[s77rt]

@Victor-Nyagudi This is not due payment yet. You can just wait for now and it will become clear 😁 (I'm not sure if you will get invited or asked to apply)

[Victor-Nyagudi]

@s77rt Just thought I'd confirm so everything is ready for tomorrow.

[jliexpensify]

Hi @Victor-Nyagudi - I'm handling payments so I'll be sending out invites and paying sometime tomorrow.

EDIT: Everyone has been hired.

[jliexpensify]

@danieldoglas - all good with the regression test here?

[danieldoglas]

Yes!

[jliexpensify]

Regression test linked, everyone is paid, job closed in Upworks. Cheers everyone!