[HOLD for payment 2023-04-26] [HOLD for payment 2023-04-21] [$1000] Special html characters like &nbsp are not escaped in name and can bug out the UI

[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. Change your name to &nbsp
2. Notice that all the reports and IOUs just have a blank space

Expected Result

There should be no blank space and the &nbsp should be shown normally

Actual Result

There is blank space for the name.

Workaround:

unknown

Platforms:

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

• MacOS / Chrome / Safari
• MacOS / Desktop

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

Recording.1515.mp4

Screen.Recording.2023-02-14.at.1.05.13.AM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01823be2bad9582bf7
• Upwork Job ID: 1625814162742865920
• Last Price Increase: 2023-02-15

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01823be2bad9582bf7

[MelvinBot]

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

[MelvinBot]

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

[MelvinBot]

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

[khashayar-bo]

Restating the Problem

HTML special characters are not rendered as plain text and are rather rendered to their corresponding character

Proposal

There are two possible solutions:

1. The sections that have the possibility to contain the special characters must have <xmp> html tag
2. When the user wants to save the data, the data should be parsed and if there is a character which is corresponding to an HTML special character, the value should be prefixed with & so that when the value is interpreted by the browser, it would show as text and not the special character

[tienifr]

Proposal

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

If the user name coincides with HTML entities like &nbsp, 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 user name in the chat. We should not do this because the user name should be shown as is.

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

We should remove Str.htmlDecode in some places like

App/src/pages/home/report/ReportActionItemFragment.js

Line 145 in f40f9c9

{Str.htmlDecode(props.fragment.text)}

(here we show the user name in the message header),

App/src/components/ReportActionItem/IOUQuote.js

Lines 73 to 80 in f40f9c9

	{Str.htmlDecode(fragment.text.split(' ')[0])}
	</Text>
	<Text style={[styles.chatItemMessage, props.shouldAllowViewDetails
	? styles.cursorPointer
	: styles.cursorDefault]}
	>
	{/* Get remainder of IOU message */}
	{Str.htmlDecode(fragment.text.substring(fragment.text.indexOf(' ')))}

(here we show the user name in the IOU message),
and other places if needed (basically any places that display the user name should not be using Str.htmlDecode)

What alternative solutions did you explore? (Optional)

For some places like the IOU message, we might want to escape the HTML entity inside the user name when we generate the message, and we can keep the Str.htmlDecode logic there.

So essentially if the user name is User &nbsp, we will store Requested ₫35,555 from User &nbsp as the lastMessageHtml
And then the Str.htmlDecode in the IOU message logic will unescape it to Requested ₫35,555 from User &nbsp which is correct.

We should not do this for the user name itself because I think we should store the raw text input of the user name in the db, and it's not a html by right.

Result

[dgaisan]

Restating the Problem

There should be no blank space and the &nbsp should be shown normally

Proposal

Right now the text(representing display name) that's being passed into Report component is explicitly html-escaped.
Instead we will need to render it as is(w/o escaping it). It's already done this way(no escaping) in other places, for instance on the top of Settings component (right panel), under the thumbnail

[eh2077]

Proposal

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

Changing username to contain special characters, like &nbsp, makes App unable to display username correctly in chats and IOU messages.

What is the root cause of that problem?

The root cause of this issue is that

1. we don't check if the username input contains many special characters
2. we save the username contains special characters to backend
3. in chats and IOU messages, we do html decode for username

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

To fix this issue, we can improve the validation rule of username. Currently, we only check , and ; for username through this function

App/src/libs/ValidationUtils.js

Lines 359 to 367 in f839c56

	/**
	* Checks that the provided name doesn't contain any commas or semicolons
	*
	* @param {String} name
	* @returns {Boolean}
	*/
	function isValidDisplayName(name) {
	return !name.includes(',') && !name.includes(';');
	}

We can improve it to check other special characters we don't want for username, like

function isValidDisplayName(name) {
    const format = /[!@#$%^&*()_+\-=\[\]{};':"\\|,.<>\/?]+/; // We can customize it for the App if needed
    return !format.test(name);
}

And update validation error hints. We can add similar checking at backend as well.

We can also use this method to check for other inputs that we don't want special characters.

What alternative solutions did you explore? (Optional)

None

[sobitneupane]

Thanks for your proposal @khashayar-bo and @dgaisan.

Please do make use of our proposal template to post proposals.

Also it is good to use permalinks to explain root cause of the problem and the solution.

[sobitneupane]

Thanks for the proposal @tienifr.

The proposal looks good to me.

@pecanoro First thing, I am not sure if it is really a bug.

If we want to allow the use of special html characters like &nbsp and want it to be shown as it is, we should get rid of str.htmlDecode as suggested in the proposal. str.htmlDecode was being used from very beginning.

🎀👀🎀 C+ reviewed
cc: @pecanoro

[eh2077]

Hi @pecanoro , does it make sense to just avoid using some special html characters in username as mentioned in my proposal?

It's a common choice to avoid using special characters in username among popular Apps, like Facebook, IG, Github, and etc.

By avoiding special characters

1. username will become more readable
2. codebase will be more concise for now and future

Note that currently we do avoid using , and ;

[tienifr]

@eh2077 nice suggestion but I think it's more like a feature request.

With that change existing names that have the HTML entities will still show incorrectly.

Also users who use special characters in the name before will suddenly see their name showing error when they try to edit the name.

Also & seems not a very uncommon choice to put in the name, for example Tienifr & partners,... so I don't think we should strip it.

[eh2077]

Hi @tienifr, thanks for the comment.

I think what I proposed #15131 (comment) is the right fix if using html entities in username is unexpected from product perspective. I really don't think we would expect to use html tags in username initially, like having a username <h1>David</h1>

While if this is an expected feature, then I agree with you it'll be a feature request.

[flaviadefaria]

@sobitneupane @pecanoro any further thoughts here?

[pecanoro]

Taking a look at what it would be the desirable behaviour here so we can go with one proposal or another.

[pecanoro]

Ok, so yeah, to avoid potential weird injections, we should never decode the name and if we input &nbsp, we should show &nbsp as the name in the chats. I agree with @sobitneupane, @tienifr's proposal sounds good so assigning it to you.

[MelvinBot]

@pecanoro, @sobitneupane, @zanyrenney, @tienifr Still overdue 6 days?! Let's take care of this!

[zanyrenney]

How are we doing with the regression @pecanoro ? Are we ready to pay this out now?

[MelvinBot]

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

[MelvinBot]

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

• fix: 15131 html decode in lhn and iou #16957
• Fix null issue in lastMessageText #17449

If no regressions arise, payment will be issued on 2023-04-26. 🎊

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:

• [@sobitneupane] The PR that introduced the bug has been identified. Link to the PR:
• [@sobitneupane] 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:
• [@sobitneupane] 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:
• [@zanyrenney] Determine if we should create a regression test for this bug.
• [@sobitneupane] 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.
• [@zanyrenney] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[zanyrenney]

bump @pecanoro can you confirm the status of this one? I am not clear whether this is ready to pay out and I took this over when @flaviadefaria reassigned it so not clear here. Thanks!

[pecanoro]

@zanyrenney Yes, sorry! Now it's ready to be paid since it's past 2023-04-26. I think BugBot got confused with the timeline due to the regression. There was a regression after merging so take that into account when calculating the payouts and such 🤗

[pecanoro]

@zanyrenney Did it get paid?

[pecanoro]

Moving back to daily so we can this sort out soon.

[zanyrenney]

Hey @pecanoro I was at a conference, getting this paid out now!

[zanyrenney]

Had to create a new job post as Flavia's one was too old. https://www.upwork.com/jobs/~01ff4a14f72d218529

Invited all. please accept invite for payout @tienifr @sobitneupane @esh-g

[jayeshmangwani]

@zanyrenney I am eligible for reporting regression here, Can you please send me an offer for reporting regression?

[zanyrenney]

hey @jayeshmangwani - invited you.

[zanyrenney]

hired @sobitneupane @esh-g, waiting on @tienifr to accept job.

[tienifr]

@zanyrenney I submitted proposal to the job, thanks!

[jayeshmangwani]

hey @jayeshmangwani - invited you.

@zanyrenney I submitted a proposal for the job, thanks!

[zanyrenney]

Cool, thanks everyone. All the jobs have now been paid out. CLosing the issue!