[$2000] IOU Request message changes & removes some characters after sending. #15026

kavimuru · 2023-02-10T03:40:22Z

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:

Go to any chat & request funds with a message "Tokyo <> Mexico"
Notice the request message changes & removes "<>" from the original message.

Expected:

Request payment messages should not change after sending, just like send payment messages.

Actual:

Request message changes & removes some characters such as "<>" from the original message.

Workaround:

unknown

Platforms:

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

Version Number: v1.2.69-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:

text.mov

Recording.1492.mp4

Expensify/Expensify Issue URL:
Issue reported by: @priyeshshah11
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1675781361590699

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~0123dd9e5d8f2edaa2
Upwork Job ID: 1625152776725245952
Last Price Increase: 2023-02-20

MelvinBot · 2023-02-10T03:40:28Z

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

MelvinBot · 2023-02-10T03:40:33Z

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

lschurr · 2023-02-10T18:54:28Z

I was able to reproduce. Adding Eng for another set of eyes. Should this be internal or external?

MelvinBot · 2023-02-10T18:54:38Z

Triggered auto assignment to @tgolen (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

tgolen · 2023-02-13T15:19:37Z

Looks like a legit bug.

MelvinBot · 2023-02-13T15:19:53Z

Job added to Upwork: https://www.upwork.com/jobs/~0123dd9e5d8f2edaa2

eh2077 · 2023-02-13T15:19:53Z

Proposal

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

IOU Request money message changes and removes characters <> after sending. Characters <> should be shown in the request message as normal text. Send money and Split bill have the same issue.

What is the root cause of that problem?

The root cause of this problem is that

in the frontend, we don't escape HTML characters for this.state.comment.trim() of IOUModal and pass message contains HTML characters to the backend. See

App/src/pages/iou/IOUModal.js

Line 289 in d42e443

const comment = this.state.comment.trim();

App/src/pages/iou/IOUModal.js

Line 333 in 39d9b66

const comment = this.state.comment.trim();
the backend strips HTML tag for message.html to get message.text

For example, if we request funds with a message "Tokyo <> Mexico", then

Before merging returned data from backend, the frontend will show below message without escaping HTML characters

{
  "html": "Requested SGD 1.00 from hanyl_fight+123@126.com for Tokyo <> Mexico",
  "text": "Requested SGD 1.00 from hanyl_fight+123@126.com for Tokyo <> Mexico",
}

html and text are same, see

App/src/libs/ReportUtils.js

Lines 913 to 916 in d42e443

    
           return [{ 
        
               html: iouMessage, 
        
               text: iouMessage, 
        
               isEdited: false,

The request message shows normally.

But after the backend returning message text with HTML tag stripped
```
{
  "html": "Requested SGD 1.00 from hanyl_fight+123@126.com for Tokyo <> Mexico",
  "text": "Requested SGD 1.00 from hanyl_fight+123@126.com for Tokyo  Mexico",
}
```
the IOUQuote will change request message to text without HTML characters, see

App/src/components/ReportActionItem/IOUQuote.js

Line 80 in 39d9b66

{Str.htmlDecode(fragment.text.substring(fragment.text.indexOf(' ')))}

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

Note that, in the chat, we can send normal messages contain HTML characters and it works well, like sending message

Tokyo <> Mexico

This is because we call Str.safeEscape in ExpensiMark#replace which supports escaping HTML characters to format the message before sending. See

https://github.com/Expensify/expensify-common/blob/5cdfe388a759e063bd86d00440a4bf277b89a3c6/lib/ExpensiMark.js#L326-L328

So, to fix this issue, we can also use Str.safeEscape to escape HTML characters for this.state.comment.trim() in IOUModal. We can change line

App/src/pages/iou/IOUModal.js

Line 289 in d42e443

const comment = this.state.comment.trim();

and

App/src/pages/iou/IOUModal.js

Line 333 in d42e443

const comment = this.state.comment.trim();

to

const comment = Str.safeEscape(this.state.comment.trim());

Then we can fix this issue for Request money, Send money and Split bill together.

This solution needs backend to fix historical data if necessary.

What alternative solutions did you explore? (Optional)

N/A

Result

Working well after the fix

15026.mp4

MelvinBot · 2023-02-13T15:19:57Z

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

MelvinBot · 2023-02-13T15:20:08Z

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

MelvinBot · 2023-02-13T15:20:13Z

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

PrashantMangukiya · 2023-02-13T15:20:25Z

Proposal

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

Whenever we enter some text like "Hello <> World" etc. It will remove <> from original message while display IOU Request message. i.e. It will not show some characters e.g. <> after sending.

What is the root cause of that problem?

Here is the thing, from input iou message we are striping html tag during save etc. So it will never show html tag pair like <b></b> etc. Because for security reason etc. it is removing such html tag from input.

But we can show characters that exist within fragment.html part. e.g. single bracket < or " or <> etc. As such char not stripped during save and it is available within fragment.html so we can show it if wish.

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

We are displaying reminder of IOU message at line 80 as shown below:

App/src/components/ReportActionItem/IOUQuote.js

Lines 79 to 80 in 14ebe26

    
           {/* Get remainder of IOU message */} 
        
           {Str.htmlDecode(fragment.text.substring(fragment.text.indexOf(' ')))}

So we can show chars that was not stripped during save by changing the code at line 80 as shown below. i.e. use fragement.html.substring Instead of fragment.text.substring while display reminder of IOU message.

{/* Get remainder of IOU message */}
- {Str.htmlDecode(fragment.text.substring(fragment.text.indexOf(' ')))}
+ {Str.htmlDecode(fragment.html.substring(fragment.text.indexOf(' ')))}

Note: It will not show any html tag pair like <b> </b> etc. because it was already stripped during save and not available within fragment.html So whatever char exist within fragment.html will be rendered via this solution.

s77rt · 2023-02-13T16:11:41Z

This issue is also reproducible if you send a message that is 10000 chars or longer.

thesahindia · 2023-02-13T18:32:24Z

@eh2077, I think we will have to decode the html here -

other then that the proposal looks good to me.

C+ reviewed 🎀👀🎀

cc: @tgolen

thesahindia · 2023-02-13T18:34:01Z

This issue is also reproducible if you send a message that is 10000 chars or longer.

I am not sure if we wanna change something here.

s77rt · 2023-02-13T19:59:00Z

@thesahindia We should apply the same fix there.

eh2077 · 2023-02-14T01:15:18Z

@eh2077, I think we will have to decode the html here -

other then that the proposal looks good to me.

C+ reviewed 🎀👀🎀

cc: @tgolen

Yes, I agree. I think we can decode message text here

App/src/components/ReportTransaction.js

Line 79 in e31cb53

{this.props.action.message[0].text}

tgolen · 2023-02-14T15:27:36Z

I want to be very cautious here with this change, just because of the XSS nature of this. Please be sure to include tests on the PR which show that XSS cannot happen.

🟢 for the proposal 👍

@Julesssss do you happen to have any context about why this has always displayed the fragment.text and not the fragment.html? It looks like it's been that way from the beginning:

6870e69#diff-a3328ec5d9b5b7526e668f601b713a91821426315993b3e93a31b91f0ec0e38dR18

tgolen · 2023-02-28T16:32:54Z

cc @Julesssss @marcaaron @AndrewGable @luacmartins for some additional thoughts on this validation.

luacmartins · 2023-02-28T19:31:06Z

I think that we should be consistent with our validation whether inputs are controlled by Form or not. It seems like we discussed and decided to check for invalid characters and display an error message in Form, so I think that we should do that same here.

eh2077 · 2023-03-01T12:51:24Z

Just brought the discussion to a Slack thread

eh2077 · 2023-03-02T02:53:46Z

As the current backend escapes HTML characters for IOU messages instead of stripping it away and the merged PR #15578 has fixed the decoding issue, I think we can just hold the issue for a while and check it again after #15578 is in production. If all good, then we can just close this issue.

pecanoro · 2023-03-03T18:03:34Z

Just posting because while fixing the profile names here, even though we know that messages will be probably be fixed here with the linked PR, the notifications seem to have the same issue so just making sure we test them as well to decide if this can be closed or not.

eh2077 · 2023-03-06T15:50:15Z

PR #15578 is in production but the issue still exists if we use HTML characters like Tokyo <==> Mexico. See

Screen.Recording.2023-03-06.at.11.33.37.PM.mov

I think, for IOU message, we should fix this from frontend by escaping it before sending and decoding the message text from backend. Also see discussions in slack

cc @tgolen @luacmartins @thesahindia

pecanoro · 2023-03-06T16:21:47Z

@eh2077 I think we are fixing that bug in this PR #15340

tgolen · 2023-03-08T15:34:29Z

So... are we thinking of just closing out this issue and deleting the PR then? It sounds like everything is covered?

pecanoro · 2023-03-08T15:48:09Z

@tgolen I think the notifications are still not fixed.

pecanoro · 2023-03-08T15:48:57Z

Though I would wait until the other PR gets merged to see if all cases reported here are fixed. I think <==> might still be a problem?

MelvinBot · 2023-03-15T21:02:38Z

@tgolen, @lschurr, @thesahindia, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot · 2023-03-17T18:22:31Z

@tgolen, @lschurr, @thesahindia, @eh2077 Huh... This is 4 days overdue. Who can take care of this?

MelvinBot · 2023-03-21T18:21:55Z

@tgolen, @lschurr, @thesahindia, @eh2077 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

lschurr · 2023-03-21T21:07:06Z

@tgolen - are we ready to close this one?

priyeshshah11 · 2023-03-21T21:20:32Z

@tgolen - are we ready to close this one?

@lschurr Can the reporting bonus be paid out before the issue is closed?

MelvinBot · 2023-03-29T18:22:35Z

@tgolen, @lschurr, @thesahindia, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

pecanoro · 2023-03-29T19:00:53Z

This has been fixed already by other PRs, but I wonder if this qualifies for reporting bonus. I think this one is the oldest of all of them so maybe we need to pay the bonus for this one and not here.

s77rt · 2023-03-29T20:59:18Z

The other PR fix messages that are sent to AddComment API endpoint. If IOU messages goes through the same process then it should be fixed already otherwise we will have to _.escape IOU messages too.

PS: It's important to use _.escape and not Str.safeEscape

pecanoro · 2023-03-29T22:49:06Z

We used ReportUtils.getParsedComment which uses _.escape. PR was created here.

lschurr · 2023-03-30T15:22:03Z

Alright, so just a reporting bonus on this GH for @priyeshshah11, correct?

lschurr · 2023-03-30T15:25:42Z

I created a new job since the old one expired. Can you apply to this job @priyeshshah11? https://www.upwork.com/jobs/~01748dd7f27e5bdfae

priyeshshah11 · 2023-03-30T17:28:15Z

I created a new job since the old one expired. Can you apply to this job @priyeshshah11? https://www.upwork.com/jobs/~01748dd7f27e5bdfae

@lschurr Applied

thesahindia · 2023-03-31T19:24:25Z

@lschurr, I think according to the C+ process doc I am also eligible for the review. I think we also compensate the contributor for their work even when we decide to close the PR. So I think @eh2077 is also eligible according to some previous similar cases e.g. #7438 (comment), #14232 (comment), #11427 (comment)

lschurr · 2023-04-04T18:11:13Z

After reviewing internally, we've agreed that payment for $1000 should be paid to @eh2077 and $500 to @thesahindia. Could you both apply to this job: https://www.upwork.com/jobs/~012483cf80c76af86b

eh2077 · 2023-04-05T00:41:11Z

@lschurr Thank you and I applied the job!

thesahindia · 2023-04-05T12:01:02Z

Applied, thanks!

lschurr · 2023-04-06T14:43:40Z

All paid. I think we can close this one out.

kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Feb 10, 2023

melvin-bot bot assigned lschurr Feb 10, 2023

melvin-bot bot locked and limited conversation to collaborators Feb 10, 2023

lschurr added the Engineering label Feb 10, 2023

melvin-bot bot assigned tgolen Feb 10, 2023

melvin-bot bot added the Overdue label Feb 13, 2023

melvin-bot bot removed the Overdue label Feb 13, 2023

tgolen added the External Added to denote the issue can be worked on by a contributor label Feb 13, 2023

melvin-bot bot unlocked this conversation Feb 13, 2023

melvin-bot bot changed the title ~~IOU Request message changes & removes some characters after sending.~~ [$1000] IOU Request message changes & removes some characters after sending. Feb 13, 2023

melvin-bot bot assigned thesahindia Feb 13, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 13, 2023

pecanoro mentioned this issue Mar 2, 2023

Fix/15131: Special html characters like &nbsp are not escaped in name and can bug out the UI #15340

Merged

57 tasks

pecanoro mentioned this issue Mar 29, 2023

[HOLD for payment 2023-04-03] [$1000] HTML characters are converted in chat (they shouldn't be converted) #15508

Closed

6 tasks

lschurr closed this as completed Apr 6, 2023

[$2000] IOU Request message changes & removes some characters after sending. #15026

[$2000] IOU Request message changes & removes some characters after sending. #15026

Comments

kavimuru commented Feb 10, 2023 • edited by melvin-bot bot Loading

Action Performed:

Expected:

Actual:

Workaround:

Platforms:

MelvinBot commented Feb 10, 2023

MelvinBot commented Feb 10, 2023 • edited by lschurr Loading

Bug0 Triage Checklist (Main S/O)

lschurr commented Feb 10, 2023

MelvinBot commented Feb 10, 2023

tgolen commented Feb 13, 2023

MelvinBot commented Feb 13, 2023

eh2077 commented Feb 13, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Result

MelvinBot commented Feb 13, 2023

MelvinBot commented Feb 13, 2023

MelvinBot commented Feb 13, 2023

PrashantMangukiya commented Feb 13, 2023

Proposal

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

What is the root cause of that problem?

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

s77rt commented Feb 13, 2023

thesahindia commented Feb 13, 2023

thesahindia commented Feb 13, 2023

s77rt commented Feb 13, 2023

eh2077 commented Feb 14, 2023

tgolen commented Feb 14, 2023

tgolen commented Feb 28, 2023

luacmartins commented Feb 28, 2023

eh2077 commented Mar 1, 2023

eh2077 commented Mar 2, 2023

pecanoro commented Mar 3, 2023

eh2077 commented Mar 6, 2023

pecanoro commented Mar 6, 2023

tgolen commented Mar 8, 2023

pecanoro commented Mar 8, 2023

pecanoro commented Mar 8, 2023

MelvinBot commented Mar 15, 2023

MelvinBot commented Mar 17, 2023

MelvinBot commented Mar 21, 2023

lschurr commented Mar 21, 2023

priyeshshah11 commented Mar 21, 2023

MelvinBot commented Mar 29, 2023

pecanoro commented Mar 29, 2023

s77rt commented Mar 29, 2023

pecanoro commented Mar 29, 2023

lschurr commented Mar 30, 2023

lschurr commented Mar 30, 2023

priyeshshah11 commented Mar 30, 2023 • edited Loading

thesahindia commented Mar 31, 2023

lschurr commented Apr 4, 2023 • edited Loading

eh2077 commented Apr 5, 2023

thesahindia commented Apr 5, 2023

lschurr commented Apr 6, 2023

kavimuru commented Feb 10, 2023 •

edited by melvin-bot bot

Loading

MelvinBot commented Feb 10, 2023 •

edited by lschurr

Loading

priyeshshah11 commented Mar 30, 2023 •

edited

Loading

lschurr commented Apr 4, 2023 •

edited

Loading