[HOLD for payment 2024-06-28] [HOLD for payment 2024-06-20][$250] Room-User is allowed to invite via mentions invalid number.

[izarutskaya]

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

---

Version Number: 1.4.66-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: https://expensify.testrail.io/index.php?/tests/view/4513241
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to https://staging.new.expensify.com/home
2. Tap on a room report
3. Enter @+91 and send the message
4. Tap invite
5. Note invited message
6. Tap header--members---invite
7. Enter +91
8. Note no options to select and invite the contact is shown

Expected Result:

User must not be allowed to invite via mentions invalid number.

Actual Result:

User is allowed to invite via mentions invalid number.
Tapping header -- members -- invite, user unable to invite +91 but via mentions can invite.

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6462540_1714115844874.mev.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01cdf72ac69795bc85
• Upwork Job ID: 1784976134411239424
• Last Price Increase: 2024-05-09
• Automatic offers:
• hungvu193 | Reviewer | 0
• nkdengineer | Contributor | 0

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @lschurr

[melvin-bot]

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01cdf72ac69795bc85

[melvin-bot]

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

[hungvu193]

@lschurr I want to discuss about the expected behavior here. In this case, when we sent an invalid phone number, the whisper message shouldn't be displayed or it's displayed but we can't click the Invite button?

[lschurr]

I'm not 100% sure - Would you mind starting a Slack thread to discuss @hungvu193?

[hungvu193]

I raised on slack:
https://expensify.slack.com/archives/C01GTK53T8Q/p1714617971723469

[hungvu193]

Coming from Slack, I think we should handle this issue from BE (BE won't send the whisper action if that's invalid mention), @lschurr can you please add the Internal label?

[melvin-bot]

Current assignee @hungvu193 is eligible for the Internal assigner, not assigning anyone new.

[lschurr]

Asked in the Slack thread about best ways to move forward here. I think we need an Eng volunteer to assign this to.

[blimpich]

I'll pick this up, I think it'll be a quick fix.

[blimpich]

Notes for myself:

Looks like we have a couple ways in Web for validating a phone number, though we should probably only have 1. We also have ways of validating it from the client; one way to validate it is via a command called IsValidPhoneNumber (which doesn't appear to be used ever in App) but we also have ways that leverage other libraries as shown here. Auth doesn't seem to have methods for validating phone numbers, which seems odd to me.

Another funny thing is that we also seem to be making accounts for these invalid phone numbers. Like when you comment @+91 it actually creates an account with the username [email protected], even though we would reject making that account if a user tried to sign up with it. Not sure if we care about that or not. We probably don't care.

It seems like this will be easiest to fix in Web. But that may be problematic because I think these actionable mention whispers are made in Auth. We may need to port over our TwilioUtils.php file to Auth, or at least partly port it. That would take a little bit of time. But I think that would be the "best" solution since a Web solution will probably end up being quite hacky. Those are my thoughts after doing an initial inspection.

Will return to this tomorrow.

[blimpich]

Ugh, this is more complicated than expected. Twilio uses the E164 format as their standard for what is and isn't a valid phone number, but this standard actually doesn't have a minimum number of digits necessary for it to be valid. See https://www.twilio.com/docs/api/errors/21211. This means that I don't think we can actually say that these phone numbers are invalid.

So, technically, +91 is a valid phone number. And if you can believe it, according to the leading SMS api in the world, even + is a valid phone number. Kinda ridiculous.

I tested this by directing hitting Twilio's api using our own TwilioUtils->isValidPhoneNumber method in Web-Expensify
.

But we don't allow numbers like that to make accounts! If you type +94 into the login page it'll tell you that's not a valid number!

The reason is because we use the awesome-phonenumber npm package to partly validate our phone numbers. And for some reason it seems to understand that +94 is not a valid phone number, probably due to a very long if/else chain checking country code against phone number length. For example, Sweden only has to have 7 digits in their phone numbers (source).

Funnily enough this package is actually based off a C++ package from Google called libphonenumber. So we could download that into our C++ layer and start using that, but this is getting a little ridiculous.

[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:

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

[hungvu193]

• The PR that introduced the bug has been identified. Link to the PR: No offending PR
• 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: N/A
• 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
• Determine if we should create a regression test for this bug.

We don't need regression test for this one imo, this was how our markdown worked, we can treat this issue as a improvement.

[nkdengineer]

@hungvu193 The PR to bump Expensify-common in live-markdown here

[nkdengineer]

@hungvu193 The App PR is here.

[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.4.82-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [CP Staging] Bump react-native-live-markdown to 0.1.82 #43255

If no regressions arise, payment will be issued on 2024-06-20. 🎊

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

• @hungvu193 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

[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:

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

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[lschurr]

Payment summary:

• Contributor: @nkdengineer $250 (paid in Upwork)
• Reviewer: @hungvu193 $250 (paid in Upwork)

[lschurr]

Do we need to leave this open for the Deploy Blocker message? #41078 (comment)

@nkdengineer @hungvu193

[nkdengineer]

It's not a blocker from our PR here. Ref: #44085 (comment).

[lschurr]

Great! Closing this one out since it's paid.

[melvin-bot]

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

• Bump react-native-live-markdown and expensify-common version #43644

If no regressions arise, payment will be issued on 2024-06-28. 🎊

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

• @hungvu193 requires payment automatic offer (Reviewer)
• @nkdengineer requires payment automatic offer (Contributor)

[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:

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