[HOLD for payment 2023-08-10] [$1000] Handle mentions renderer not receiving props

[puneetlath]

Problem

If for some reason the back-end doesn't return props that the mentions renderer expects, this causes the app to crash.

Solution

In the mentions renderer, handle the case where the expected props aren't provided.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0125cc67eabd576779
• Upwork Job ID: 1684978455578161152
2023-07-28
• Automatic offers:
• aimane-chnaif | Reviewer | 25816064
• tienifr | Contributor | 25816067

[melvin-bot]

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

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0125cc67eabd576779

[melvin-bot]

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

[melvin-bot]

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

[puneetlath]

I'm going to assign this to @tienifr as we have already discussed this over email. @tienifr can you please post your proposal here?

[tienifr]

Proposal

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

The MentionUserRenderer makes the app crash when rendering nested mention. This could be exploited to make any user’s app crash

What is the root cause of that problem?

In

App/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.js

Line 35 in e32ff5e

const loginWithoutLeadingAt = props.tnode.data.slice(1);

we are removing the first character of the mention text with props.tnode.data.slice(1). This would work when the markdown component is a single component - which will be understood as a TText node. However, when an attacker purposely send a nested mention, the props.tnode value is a TPhrasing object, and this does not have the data value, which will cause the app to crash since props.tnode.data is undefined.

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

We need to check if props.tnode.data is actually valid before applying the slice function

- const loginWithoutLeadingAt = props.tnode.data.slice(1);
+ const loginWithoutLeadingAt = props.tnode.data ? props.tnode.data.slice(1) : null;

or something like

- const loginWithoutLeadingAt = props.tnode.data.slice(1);
- const isOurMention = loginWithoutLeadingAt === props.currentUserPersonalDetails.login;
+ const isOurMention = props.tnode.data && props.tnode.data.slice(1) === props.currentUserPersonalDetails.login;

[tienifr]

@puneetlath I've posted my proposal above. Can you help take a look at it?

[aimane-chnaif]

can anyone please share repro step to crash?

[tienifr]

STEPS TO REPRODUCE

1. In
   buildOptimisticAddCommentReportAction

Change from

const commentText = getParsedComment(text);

to

const commentText = text;

By doing this, we can customize and send any markdown texts to any users.

2. Open any report to any user, send this text: <mention-user><mention-here>here</mention-here></mention-user>
3. Notice the app crashes in both the sending and receiving accounts.

[tienifr]

@aimane-chnaif Here you are #23811 (comment)

[saalimzafar]

function MentionUserRenderer(props) {
// Check if the required props exist
if (!props.tnode || !props.currentUserPersonalDetails) {
// Return a fallback UI or null when required props are missing
return null;
}

// Rest of the component's logic

[aimane-chnaif]

does nested mention happen in real world?

[aimane-chnaif]

@tienifr null causes another crash. Empty string is fine.

const loginWithoutLeadingAt = props.tnode.data ? props.tnode.data.slice(1) : '';

I am still curious if we can send that weird message without any dev code change.
Anyway, good one to fix as bad guy can send this to any public room which makes everyone crashes. Even not able to delete because of crash.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[puneetlath]

@danieldoglas I'll take this since it deals with mentions and I'm already assigned here: https://github.com/Expensify/Expensify/issues/302711

[melvin-bot]

📣 @aimane-chnaif 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[tienifr]

The PR #23845 is ready for review

[melvin-bot]

🎯 ⚡️ Woah @aimane-chnaif / @tienifr, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

• when @tienifr got assigned: 2023-07-28 19:48:22 Z
• when the PR got merged: 2023-08-01 15:58:35 UTC

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• fix: 23811 handle mentions renderer not receiving props #23845

If no regressions arise, payment will be issued on 2023-08-10. 🎊

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 @tienifr
• Contributor that fixed the issue @tienifr
• Contributor+ that helped on the issue and/or PR @aimane-chnaif

For reference, here are some details about the assignees on this issue:

• @aimane-chnaif requires payment offer (Reviewer)
• @tienifr requires payment offer (Contributor)

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

[melvin-bot]

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:

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

[puneetlath]

@aimane-chnaif friendly bump on the checklist.

@tienifr has been paid.

[aimane-chnaif]

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:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR: [No QA] create UserMentionRenderer for rendering mentions inside chat #18495
• [@aimane-chnaif] 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: https://github.com/Expensify/App/pull/18495/files#r1290553222
• [@aimane-chnaif] 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: N/A
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] 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.

As this is extreme edge case, regression test is not needed

[puneetlath]

All paid. Thanks y'all!