App: [HOLD for payment 2023-04-03] [$1000] HTML characters are converted in chat (they shouldn't be converted)
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:
- Enter
<or any other HTML character in the compose box - 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?
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- 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:
Expensify/Expensify Issue URL: Issue reported by: @esh-g Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1677316598380029
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~012cc7a7ec930277c6
- Upwork Job ID: 1631456392901574656
- Last Price Increase: 2023-03-14
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 89 (73 by maintainers)
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.
Fun fact: I've separated each word of this sentence with an
While I understand the viewpoint that what you type into the input is what should be displayed back to you. It’s certainly not a paradigm that’s universal across the internet.
I do think that it shouldn’t be escaped within a code snippet, which is currently happening:
All paid, and job closed!
Regression Test Proposal
&as a message&Expensify common PR is merged.
I have opened the App PR.
cc: @s77rt
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.
PR is up.
Got it 👍. I will open the PR in a few hours.
@jliexpensify Here is my Upwork profile: https://www.upwork.com/freelancers/~0186b1d7cc69656f22
I’ll update my name to match this
@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
Hired @s77rt . @esh-g and @bernhardoj - I can’t find you on Upworks, can you:
FYI: this is the original PR (reviewer look familiar? 😄): https://github.com/Expensify/expensify-common/pull/232
yeah the RCA thing was a bug - and agreed to revert to $1000. Thanks!
Re-assigning to @yuwenmemon (discussed 1:1) as he has more context here
Proposal
Please re-state the problem that we are trying to solve in this issue.
If the message has HTML entities like <,  , it’s not showing as is in the chat.
What is the root cause of that problem?
This is because we’re unescaping HTML entities when displaying the message in the chat in here https://github.com/Expensify/App/blob/ce229bc1812ded576ce29d007fa3c03be8d1cd75/src/pages/home/report/ReportActionItemFragment.js#L123. The message was not properly encoded before so this decoding means the HTML entities in our message will be unexpectedly changed.
What changes do you think we should make in order to solve the problem?
We can
htmlEncodethe message when we parse it to HTML right after this line https://github.com/Expensify/expensify-common/blob/2e6d46d1c6227974f27475aa00d8cd7963bdde8b/lib/ExpensiMark.js#L308 We can addreplacedText = Str.htmlEncode(replacedText);, then the message will be html-encoded and subsequently decoded properly when we display it.What alternative solutions did you explore? (Optional)
We should remove
Str.htmlDecodein https://github.com/Expensify/App/blob/ce229bc1812ded576ce29d007fa3c03be8d1cd75/src/pages/home/report/ReportActionItemFragment.js#L123 and other places if needed (basically any places that display the message should not be usingStr.htmlDecode)