[$1000] IOS - iOS app crashes when send message with very long invalid email

[kbecciv]

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. Go to any chat
2. Send this message:
   jaj@asjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfisjksksjsjssskssjskskssksksksksskdkddkddkdksskskdkdkdksskskskdkdkdkdkekeekdkddenejeodxkdndekkdjddkeemdjxkdenendkdjddekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdicdjejajasjjssjdjdjdjdjdjjeiwiwiwowkdjdjdieikdjfidekjcjdkekejdcjdkeekcjcdidjjcdkekdiccjdkejdjcjxisdjjdkedncicdjejejcckdsijcjdsodjcicdkejdi.cdjd

Expected Result:

iOS app does not crash when send message with very long invalid email

Actual Result:

iOS app crashes when send message with very long invalid email

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.3.27-6

Reproducible in staging?: y

Reproducible in production?:
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: Any additional supporting documentation

RPReplay_Final1687379650.MP4

Expensify/Expensify Issue URL:

Issue reported by: Situ Chandra Shil

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686758977117659

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d6eba6927987b0dc
• Upwork Job ID: 1673777985979330560
• Last Price Increase: 2023-07-18

[melvin-bot]

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

[melvin-bot]

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

[tjferriss]

I was not able to reproduce the issue on mobile web. The message was sent as expected.

I'm currently not able to access the iOS testflight app. The chat list is not loading. I will try again later.

[melvin-bot]

@tjferriss Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[tjferriss]

The app immediately crashed when I tried to send the message. Even when I click Refresh and try to open the chat back up it crashes again.

[melvin-bot]

@tjferriss, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[allroundexperts]

Still waiting for proposals.

[arthurmfgtab]

Hey, @tjferriss @allroundexperts
I'm Arthur from Callstack - expert contributor group - and I will start investigating this :)

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[arthurmfgtab]

@allroundexperts @tjferriss

Hey, peeps, here's an update on the issue

It seems that, more then the length of the email, what's really causing problem is its domain.
I tested it using "aaa@aaaaaaaaaaaaaaaaaaaaaaaaaaaaa.c" and the problem still persists. However, if I switch that ".c" at the end to ".com" it works fine.

Upon some investigation, I noticed that the URL_WEBSITE_REGEX (see image below) is made of, among other things, a lot of domains. It looks like if the domain of the email included in our chat message is not there, the urlRegex.exec(href) bit will throw.

I'll open a Slack convo tomorrow to discuss possible solutions for the issue :)

[melvin-bot]

@tjferriss, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@tjferriss @allroundexperts this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[arthurmfgtab]

@tjferriss @allroundexperts

I wonder if you guys had a change to take a look at this
I'm preparing a proposal to stop the crashing issue, but I'd like to first know from you guys if invalid domain should be allowed in the app

[shubham1206agra]

@tjferriss @allroundexperts

I wonder if you guys had a change to take a look at this I'm preparing a proposal to stop the crashing issue, but I'd like to first know from you guys if invalid domain should be allowed in the app

@allroundexperts @tjferriss

Hey, peeps, here's an update on the issue

It seems that, more then the length of the email, what's really causing problem is its domain. I tested it using "aaa@aaaaaaaaaaaaaaaaaaaaaaaaaaaaa.c" and the problem still persists. However, if I switch that ".c" at the end to ".com" it works fine.

Upon some investigation, I noticed that the URL_WEBSITE_REGEX (see image below) is made of, among other things, a lot of domains. It looks like if the domain of the email included in our chat message is not there, the urlRegex.exec(href) bit will throw.

I'll open a Slack convo tomorrow to discuss possible solutions for the issue :)

I don't think so its related to regex parsing. I think its something else, as it should not work on any device if there's a problem with regex

[arthurmfgtab]

Hey @shubham1206agra , thanks for your insight
I tested it by forcing a call to the getURLObject function (by putting the following code in the submitForm function at /src/pages/home/report/ReportActionCompose.js: urlUtils.getURLObject('aaa@aaaaaaaaaaaaaaaaaaaaaaaaaaaaa.c'); and then hitting the "send message" button in any chat). When I do it the app crashes and from what I can see it's because of the domain. Do you think that's reasonable? If not, do you have a suggestion on another way to check it?

Thanks in advance :)

[bondydaa]

merged PR

[tjferriss]

PR hit staging yesterday

[tjferriss]

The PR has been on production for seven days now, but I don't see the payment comment #23774

• @situchan reporting bonus $250
• @eh2077 contributor $1000
• @allroundexperts C+ $1000

@bondydaa can you double check this for me? Then I'll get payments processed in Upworks.

[allroundexperts]

I'll be paid through ND @tjferriss!

[tjferriss]

@eh2077 has been paid via Upworks and @situchan has been sent an offer

[allroundexperts]

Requested payment in ND!

[JmillsExpensify]

Reviewed the details for @allroundexperts. $1,000 approved for payment in NewDot based on BZ summary above.

[Antasel]

@JmillsExpensify @bondydaa @tjferriss @allroundexperts
I think this issue has not been resolved properly .
I mean we still have inconsistency between iOS and other platform, it's cased by solution here

new test steps:

1. send # _aaa.aaaaaaaaaaaaaa.aaaaaaaa.aaaaaaaa_ on web platform, then can see that 'heading1' and 'italic' have been applied .

2. send same text on iOS, then can check that 'heading1' and 'italic' have not been applied, still #, _ is remained . and more bad point is that app gets stuck for 5~10s

The cause is that, in iOS, autolink rule has not been parsed, but it's caught by 'maximum regex stack depth reached' Error, so heading1, italic can't be parsed.

[tjferriss]

@Antasel I agree, I'm able to reproduce the issue on iOS.

[Antasel]

@tjferriss Thanks for your rechecking.
So, This issue will be reopened ? or open another issue. I'd like to dig into it to resolve it.

[tjferriss]

Let's keep working on this issue here. @allroundexperts do you agree with the reasoning @Antasel has outlined above?

[allroundexperts]

I would agree. @eh2077 Can you check the issue here?

[agilejune]

@bondydaa @tjferriss
it's possible for me to propose a solution ? I mean you would accept new one?, because here @eh2077 had been assigned a long time ago, but his proposal is incomplete, and he's already got paid for it

[melvin-bot]

📣 @agilejune! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[Antasel]

Proposal

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

When some strings are parsed by URL_WEBSITE_REGEX, iOS app crashes.

What is the root cause of that problem ?

The URL_WEBSITE_REGEX pattern has tld part, which is consisted of possible predefined tlds, which is more than 1500 .
when string goes through every tld to be checked, maximum regex stack depth is reached, causing ios crashes

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

To avoid to be checked by a lot tlds, we should replace tlds part with general regex pattern (like [a-z0-9](?:[-a-z0-9]*[a-z0-9])+), and check tlds after regex pattern has been executed.

We need to add a few tweaks in all related to URL_WEBSITE_REGEX.

1. we need to add a few tweaks in expensify-common, by capturing group against tld part, and checking if captured tld candidates are contained in TLD_REGEX.

The specific changes can be checked in Expensify/expensify-common@main...Antasel:expensify-common:fix-#21266

2. also need to check tlds in https://github.com/Expensify/App/blob/main/src/libs/ValidationUtils.js#L259
   as a result, isValidWebsite(url) function would be as following

function isValidWebsite(url) {
    const urlRegex = new RegExp(`^${URL_REGEX_WITH_REQUIRED_PROTOCOL}$`, 'i');
    const match = urlRegex.exec(url);

    return (match !== null && _.contains(TLD_REGEX.split('|'), match[4].toUpperCase()));
}

with above tweaks, we are able to keep original functionality of TLD, and prevent iOS app to cause 'Regex maximum depth reached' error

PS: I can't find getUrlObject(href) function, maybe which has been removed with other PR .

[Antasel]

@tjferriss @bondydaa @allroundexperts
Could you please check my proposal ?

[allroundexperts]

@bondydaa @tjferriss it's possible for me to propose a solution ? I mean you would accept new one?, because here @eh2077 had been assigned a long time ago, but his proposal is incomplete, and he's already got paid for it

You can submit a proposal for sure!

[agilejune]

You can submit a proposal for sure!

ok, thanks for your reply

[eh2077]

I think the effects described by #21266 (comment) is the expected output of the solution we chose to implement previously.

Yes, we just catch the error and give up parsing the bad input(which triggers the error from regex engine) on iOS platform. The root cause of the regex execution error is by design according to facebook/hermes#281 (comment).

cc @allroundexperts @tjferriss

[Antasel]

@tjferriss @allroundexperts
I am curious if you ever checked my proposal

[bondydaa]

Ah okay so this isn't about iOS still crashing just that iOS still fails to properly parse the "markdown" and the update proposal is potentially a way to help prevent the regex from errorring?

I started a thread here about this b/c my regex knowledge isn't the best https://expensify.slack.com/archives/C01GTK53T8Q/p1693868279606089

but if we're able to prevent trying to parse the TLD regex if we don't need to then could be worth while.

[Antasel]

so this isn't about iOS still crashing just that iOS still fails to properly parse the "markdown" and the update proposal is potentially a way to help prevent the regex from errorring?

iOS is not crashing since parsing markdown has been wrapped by 'try-catch', but URL_WEBSITE_REGEX with TLD will still causing 'maximum regex depth reached' error which is caught by 'try-catch'. but it makes iOS app stuck for a few seconds, and all rules after autolink rule will not be parsed.
so we need to prevent to parse TLD regex in URL_WEBSITE_REGEX .

[melvin-bot]

@bondydaa, @tjferriss, @allroundexperts, @eh2077 Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Antasel]

@bondydaa @allroundexperts
Are you about to accept my solution ?

[melvin-bot]

@bondydaa, @tjferriss, @allroundexperts, @eh2077 Huh... This is 4 days overdue. Who can take care of this?

[tjferriss]

After some internal discussion, I think we can close out this issue for now. We don't anticipate this being a real issue for users presently.