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

[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. Enter < or any other HTML character in the compose box
2. Click on send

Expected Result

<should remain the same (i.e. we don't want to convert HTML characters)

Actual Result

< becomes the < sign

Workaround:

Unknown

Platforms:

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

• [x] Android / native
• [x] Android / Chrome
• [x] iOS / native
• [x] iOS / Safari
• [x] MacOS / Chrome / Safari
• [x] MacOS / Desktop

Version Number: 1.2.78-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:

https://user-images.githubusercontent.com/43996225/221594543-a616c80e-1b39-4c57-b9b7-2dfdc2f0f3d8.mov

https://user-images.githubusercontent.com/43996225/221594593-0ce68ce7-dc75-49b4-b19a-118429c75a92.mp4

Expensify/Expensify Issue URL: Issue reported by: @esh-g Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677316598380029

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012cc7a7ec930277c6
• Upwork Job ID: 1631456392901574656
• Last Price Increase: 2023-03-14

[yuwenmemon]

FYI: this is the original PR (reviewer look familiar? 😄): https://github.com/Expensify/expensify-common/pull/232

[jliexpensify]

Hired @s77rt . @esh-g and @bernhardoj - I can't find you on Upworks, can you:

1. Send me your Upworks names or profiles
2. Edit your GH accounts to show your full names (so we can find you in the future)

[bernhardoj]

@yuwenmemon I see. But actually, I just realized that we may still need to escape it on E/App in case the text length is more than the max markup length.

https://github.com/Expensify/App/blob/9953eb896f60b676efd27541564a66326cbbcd34/src/libs/ReportUtils.js#L842-L845

@jliexpensify here is my upwork profile https://www.upwork.com/freelancers/~013e648527fa0266fd

[s77rt]

@bernhardoj Why would that be a problem for long text? The message will be sent directly to the backend as it is which is basically the expected behavior.

[bernhardoj]

@s77rt Yes, but the message is not escaped. So if we have html entities inside the long text, & will become & again.

[s77rt]

Ah yes but I think that goes for everything else, the behavior will be different as on long text we are not sending html but plaintext e.g. send a long text that contain *bold* the text won't be in bold. The only reason for that difference seems due to performance.

[esh-g]

@jliexpensify Here is my Upwork profile: https://www.upwork.com/freelancers/~0186b1d7cc69656f22

I'll update my name to match this

[bernhardoj]

Yes, it won't be converted to HTML for long text, but we still need to escape it.

Assume this is a long text: long text with *markdown* & HTML entities

If we don't escape it, the text we will see become long text with *markdown* & HTML entities .

Notice the & become & because we decode it inside ReportActionItemFragment.

[s77rt]

So the plan is: For short texts: parse the message (markdown) and encode the text For long texts: we can't parse it due to performance reasons but just encode it (assuming that encoding is not a heavy process)

[bernhardoj]

Yes, correct. We won't change the parsing part, just the encoding/escaping. So, should we reopen the PR?

[s77rt]

We still need to fix this upstream. The replace method (in expensify-common) should not prevent double-escape. And for E/App for texts that are too long we just encode them if that's not a slow process.

[bernhardoj]

Should we just remove the safeEscape from replace method? So,

1. Apply the changes from closed PR
2. Remove safeEscape in expensify-common

[s77rt]

1. expensify-common (replace method):

   -    let replacedText = shouldEscapeText ? Str.safeEscape(text) : text;
   +    let replacedText = shouldEscapeText ? _.escape(text) : text;

2. E/App

   -    return text.length < CONST.MAX_MARKUP_LENGTH ? parser.replace(text) : text;
   +    return text.length < CONST.MAX_MARKUP_LENGTH ? parser.replace(text) : _.escape(text);

I'm expecting something like that ^

[bernhardoj]

Got it 👍. I will open the PR in a few hours.

[bernhardoj]

PR is up.

[sobitneupane]

@s77rt We are expecting this issue to solve these issues as well.

cc: @bernhardoj

[bernhardoj]

@sobitneupane I don't know how to test the notification, but the LHN part, I have tested and the upcoming PR for this issue won't fix it. There is an issue reported for the LHN issue too here https://github.com/Expensify/App/issues/15920. You can take a look at the video and notice the LHN preview able to shows   for a while, then converted to empty space. I'm guessing there is 2 decoding happen, once from the BE and once from the front-end. So, when we send  ,

1. Encoded to  
2. Decoded at backend becomes  
3. Decoded at frontend becomes empty space

I take a look at the PR you linked and looks like the solution to it is to remove the decoding from FE. So, I think we should decide where we should do the decoding.

[s77rt]

@sobitneupane I'm not sure how those linked issues are related to what we are fixing here.

• What we are fixing here is related to what we send to the server (if user writes & we will send & so the returned message gets decoded correctly to reflect what the user initially sent).
• The linked PR have no relation to what to read from the server (it does not alter when to decode or not to decode).

[pecanoro]

The text fragment from the back-end is already decoded, so we send   and it should not change. There is definitely some extra decode somewhere that is decoding it to a blank space. This issue mentions HTML characters are converted in chat, so I would think we need to fix it here?

[bernhardoj]

I checked and yes, the last message text shown on LHN and text fragment both decoded from backend and for LHN last message, we decode it again here.

https://github.com/Expensify/App/blob/9241e61e6476d0ef816b4f9a3f304c837bd0ea05/src/libs/SidebarUtils.js#L250

which is actually not related to this issue. @s77rt first point explained what we are trying to fix here.

[s77rt]

@pecanoro This issue here is about what we send to the server and not about what receive from the server. So I'm not sure what you are referring to can be fixed here. Can you please create a new issue and assign me there? I'd like to verify the performed actions and actual/expected results of the new issue to better confirm if this can handled here or in a separated issue.

[s77rt]

Adding to @bernhardoj I have mentioned the same here https://github.com/Expensify/App/pull/15340#issuecomment-1469605794. It would be best to create a separate issue given that the root cause is not the same.

[bernhardoj]

I think I understand now.

open here to read the explanation

TLDR: The PR from this issue is needed to fully fix all 3 issues (Simple message, IOU, LHN) linked above by @sobitneupane. All 3 issues + this GH issue are caused by a text that is not properly escaped (because we use `safeEscape`) when we send it to the BE. When the PR of this issue is merged, the expectation is that all of those 3 issues will be solved. However, the double decoding prevents that to happen. So, we need to remove double decoding and the PR from this issue.

[pecanoro]

Hehe yes, I think we have a few issues and all of them are linked together. I think you can focus on your original fix and once that is merged, we can deal with whatever else is to handle so we don't undo any previous fix by mistake.

[s77rt]

I think decoding the text is what we are doing wrong here, but I'm expecting that to be fixed in this PR https://github.com/Expensify/App/pull/15340 Either way let's wait to get this issue fixed then we can see how to move forward.

[bernhardoj]

Expensify common PR is merged.

I have opened the App PR.

cc: @s77rt

[yuwenmemon]

Oops! I gotta create the Web-Expensify PR don't I?

[s77rt]

@yuwenmemon No, I don't think so.

[yuwenmemon]

@s77rt I do, because we use ExpensiMark in that code as well. Sorry I wasn't really asking I was saying I forgot to do it! (All taken care of now though!)

[s77rt]

Ah yes, I think this is only for Concierge?

[yuwenmemon]

Correctomundo!

[MelvinBot]

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

[MelvinBot]

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

• https://github.com/Expensify/App/pull/16380

If no regressions arise, payment will be issued on 2023-04-03. :confetti_ball:

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

[MelvinBot]

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:

• [x] [@s77rt / @yuwenmemon] The PR that introduced the bug has been identified. Link to the PR:
• [x] [@s77rt / @yuwenmemon] 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:
• [x] [@s77rt / @yuwenmemon] 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:
• [x] [@jliexpensify] Determine if we should create a regression test for this bug.
• [x] [@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.
• [x] [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/273028

[s77rt]

• The PR that introduced the bug has been identified: The bug is upstream. But I think the upstream PR is https://github.com/Expensify/expensify-common/pull/232
• The offending PR has been commented on: Not sure this applies here. cc @yuwenmemon
• A discussion in #expensify-bugs has been started: Same ^

---

Regression Test Proposal

1. Open any chat
2. Send & as a message
3. Verify the sent text shows &

[pecanoro]

I was going to close this issue since the problem was almost the same as here and got fixed already, but I am wondering if that one deserves the reporting bonus instead of this one since the other one was created before.

[jliexpensify]

I personally think both issues deserve the reporting bonus - the contributors did both report valid bugs (even if they're similar), and the reporting bonus isn't large (only $250). We can also bring this up in #bug-zero if you wanted a second opinion?

[jliexpensify]

Looks like there are no regressions, so creating the GH for regression test and paying everyone.

[jliexpensify]

All paid, and job closed!