App icon indicating copy to clipboard operation
App copied to clipboard

[$1000] Using backtick in the description in send/request money changes to '&#x60' in the report

Open kavimuru opened this issue 2 years ago • 18 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Go to web chrome
  2. Go to any chat
  3. Send/Request money and in the description , type backticks
  4. Send/request money
  5. Notice that in the report, it displays &#x60 for backticks.
  6. Follow the same steps in the chat box, and see that we don't get such problem. Also, use other special characters, problem is not seen.

Expected Result:

Using backtick in the description in send/request money should not change to '&#x60' in the report

Actual Result:

Using backtick in the description in send/request money changes to '&#x60' in the report (works well for other special characters)

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
  • [x] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop

Version Number: 1.3.3-1 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

https://user-images.githubusercontent.com/43996225/233723623-ef7ae3f5-cc86-49e9-86c1-d6f79323eccc.mp4

https://user-images.githubusercontent.com/43996225/233723662-a96cd92b-294a-4202-8428-0ad2242e4ac8.mp4

Expensify/Expensify Issue URL: Issue reported by: @priya-zha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1682056771077709

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~010020e818c61501ab
  • Upwork Job ID: 1650782203779108864
  • Last Price Increase: 2023-04-25

kavimuru avatar Apr 21 '23 19:04 kavimuru

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

MelvinBot avatar Apr 21 '23 19:04 MelvinBot

Bug0 Triage Checklist (Main S/O)

  • [x] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • [x] 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
  • [x] 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.
  • [x] 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.
  • [x] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot avatar Apr 21 '23 19:04 MelvinBot

Reproduced

bfitzexpensify avatar Apr 25 '23 08:04 bfitzexpensify

Job added to Upwork: https://www.upwork.com/jobs/~010020e818c61501ab

MelvinBot avatar Apr 25 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 25 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 25 '23 08:04 MelvinBot

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

MelvinBot avatar Apr 25 '23 08:04 MelvinBot

Proposal

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

Using backtick in the description in send/request money is changing to '&#x60' in the report

What is the root cause of that problem?

  1. User request/sent money with description `12``
  2. API is called with comment is converted to <code>12</code>&#x60; because of these lines

https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/libs/ReportUtils.js#L1391-L1396

and

https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/libs/actions/IOU.js#L362-L369

  1. API response returns the comment is 12&#x60;
Screenshot 2023-05-18 at 15 58 59

We use this comment value to display on UI

Screenshot 2023-05-18 at 16 10 33

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

  1. We should send the description to BE as what users enter change https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/libs/actions/IOU.js#L362-L369

to

API.write(
        'RequestMoney',
        {
            debtorEmail: payerEmail,
            amount,
            currency,
            comment,

apply above logic to other functions: splitBill, getSendMoneyParams

  1. Update optimistic data as well

change

https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/libs/ReportUtils.js#L1391-L1396

to

 const originalMessage = {
        amount,
        comment,
...

and

https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/libs/ReportUtils.js#L1426

to

        message: getIOUReportActionMessage(type, amount, comment, currency, paymentType, isSettlingUp),
  1. Be will return the comment as what we pass to, that why we can remove Str.htmlDecode here (prevent the case users type &nbsp)

https://github.com/Expensify/App/blob/a5c47370e4367f652e3e0ccc4568e212abcd88f3/src/components/ReportActionItem/IOUPreview.js#L151

Result

https://github.com/Expensify/App/assets/129500732/5bd31beb-8ae3-4250-bdae-1b8b71834f01

dukenv0307 avatar Apr 25 '23 10:04 dukenv0307

Looks like have the same cause - https://github.com/Expensify/App/issues/17658

alitoshmatov avatar Apr 25 '23 12:04 alitoshmatov

This is likely a back-end issue where specifically for backticks, they send HTML-encoded character (&#x60) back to the front-end rather than backtick characters. This doesn't happen for other special characters like & so we need to look in the back-end to resolve it.

The backend actually returns &amp; for & as well, we are just parsing that correctly image

Looks like have the same cause - https://github.com/Expensify/App/issues/17658

Thanks, looking into this

alex-mechler avatar Apr 25 '23 17:04 alex-mechler

Looks like have the same cause - https://github.com/Expensify/App/issues/17658

This appears to be similar, but with that fix applied, I am still able to reproduce this issue

alex-mechler avatar Apr 25 '23 18:04 alex-mechler

Proposal

Problem Statement: When using a backtick in the description while sending or requesting money, the backtick is displayed as '&#x60' (the HTML entity representation) in the report, instead of the expected backtick. Root Cause: The issue stems from the incorrect/unnecessary use of text parsing logic (ExpensiMark()), which converts backticks and other markdown entities to their HTML entity representation in this case for backticks '`' or into code blocks when in pairs <"code">...</"code">. In ReportUtils.js, unnecessary conversion and reversions result in the comment being incorrectly formatted/converted and thus when used later in the code causes the errors. https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/ReportUtils.js#L1096-L1099 Here is what the conversions look like: //Sample Text: `123`456` //commentText: <"code">123</"code">456"&#x60"; //textForNewComment: 123456` //textForNewCommentDecoded: 123456` Therefore, when using these variables to save comments, the comments are saved incorrectly because of the incorrect usage of textForNewComment and textForNewCommentDecoded. https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/ReportUtils.js#L1100-L1107 https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/ReportUtils.js#L1130-L1131 The unnecessary parsing in the API calls in IOU.js, specifically /api/RequestMoney and /api/SendMoneyElsewhere also cause the issue, as these calls are responsible for saving the comments to the backend. https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/actions/IOU.js#L910-L914 https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/actions/IOU.js#L668-L670 https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/actions/IOU.js#L675 https://github.com/Expensify/App/blob/32fbb2c44aeea0e112f8eb0ca9ea4911e160f499/src/libs/actions/IOU.js#L196-L201 Proposed Solution: To address this issue, the parsing can be bypassed as it is not necessary to parse the comments/text being passed down to these functions. What alternative solutions did you explore? (Optional) Implementing a new parsing function specifically for comments. Although this could be done, it would mean building a new parser and also changing how the comments are saved, which is more complex and changes to the comment UI.

AngelNBazan avatar Apr 25 '23 22:04 AngelNBazan

📣 @AngelNBazan! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

MelvinBot avatar Apr 25 '23 22:04 MelvinBot

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~0189d2d1b473ae382d

AngelNBazan avatar Apr 25 '23 22:04 AngelNBazan

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

MelvinBot avatar Apr 25 '23 22:04 MelvinBot

The backend actually returns & for & as well, we are just parsing that correctly

@alex-mechler thanks for the feedback! I think we're not using the data returned in your screenshot for the report comment.

We're using the text field of the reportAction instead, see below where I sent a description of "&&&&&& ``````". The & is returned as is while the "`" is encoded before returning.

Can see below in the OpenReport command after requesting money.

Screenshot 2023-04-26 at 19 38 50

dukenv0307 avatar Apr 26 '23 12:04 dukenv0307

Screenshot 2023-04-26 at 21 02 27

We send &#x60; from the front end. In the back end, it strips out the HTML tag.

Screenshot 2023-04-26 at 21 03 58

mollfpr avatar Apr 26 '23 14:04 mollfpr

hi @mollfpr in this line https://github.com/Expensify/App/blob/f8acfae37e181c66ab9dbf232772bb293c9b159b/src/libs/actions/IOU.js#L196, if we change to const parsedComment = _.escape(comment); to avoid adding the incorrect <code> html tag to the comment. It will send the &#x60;&#x60;&#x60;&#x60;&#x60;&#x60; to the back-end and back-end returns as is in the text field rather than convert back to "``````" (this is different from, say, & character), so I think there's still something wrong with the back-end.

dukenv0307 avatar Apr 27 '23 17:04 dukenv0307

@dukenv0307 Why we send to the backend &#x60;&#x60;&#x60;&#x60;&#x60;&#x60;?

Some markdown character pairs like _ _, * *, when sending in IOU Description, will disappear. This also happens to backtick if sending it in offline mode (6 consecutive backticks will become 2 backticks).

@alex-mechler @bfitzexpensify Are we on purpose striping the HTML tag in the backend?

mollfpr avatar Apr 28 '23 01:04 mollfpr

Proposal Updated https://github.com/Expensify/App/issues/17798#issuecomment-1522519111

AngelNBazan avatar Apr 28 '23 03:04 AngelNBazan

@alex-mechler @bfitzexpensify Are we on purpose striping the HTML tag in the backend?

Hmmm, it might be getting caught up in xss filters. I'll take a look next week. I'll be OOO monday so I'll take a look tuesday

alex-mechler avatar Apr 28 '23 23:04 alex-mechler

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

MelvinBot avatar May 02 '23 16:05 MelvinBot

@alex-mechler, @mollfpr, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

MelvinBot avatar May 02 '23 19:05 MelvinBot

@alex-mechler, @mollfpr, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

MelvinBot avatar May 02 '23 20:05 MelvinBot

Will be checked later Melv

mollfpr avatar May 03 '23 01:05 mollfpr

Sorry for the delay, was sick and unexpectedly OOO, looking today

alex-mechler avatar May 04 '23 19:05 alex-mechler

So it is getting caught up in XSS filters, but even with those disabled locally, we are not rendering the html at all in the IOU request. image

alex-mechler avatar May 04 '23 23:05 alex-mechler

We actually sent the same HTML for the AddComment API and got the same response from the API, but it's displayed differently.

Screenshot 2023-05-06 at 00 38 06

So this should be only a problem in showing the correct message in the IOU description.

mollfpr avatar May 05 '23 17:05 mollfpr

@alex-mechler @mollfpr @bfitzexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] avatar May 05 '23 20:05 melvin-bot[bot]

@alex-mechler, @mollfpr, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] avatar May 08 '23 20:05 melvin-bot[bot]