[HOLD for payment 2023-09-15] [$1000] Adding any invalid text/string as contact method through URL

[m-natarajan]

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

---

Action Performed:

1. Go to the following link and write anything you want in "HERE"
   https://staging.new.expensify.com/settings/profile/contact-methods/HERE/details
   For example:
2. https://staging.new.expensify.com/settings/profile/contact-methods/Yooooo/details
3. Notice it shows a magic code validation tab
4. Click on resend code quickly to stay in the incorrect validation page and bypass error page.
5. If you don't click an error page will be thrown later

Expected Result:

It should immediately says PageNotFound and not preview Contact Validation with Magic Code sent

Actual Result:

It shows validation page with magic code and you can stay in it if you click on resend code

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.53-1
Reproducible in staging?: yes
Reproducible in production?: yes
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

Recording.2394.mp4

Expensify/Expensify Issue URL:
Issue reported by: @makiour
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1691207772114549

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~013d8b6c25b7efa6cc
• Upwork Job ID: 1691192278548029440
• Last Price Increase: 2023-08-21
• Automatic offers:
• makiour | Contributor | 26292086
• MAkiour | Reporter | 26292087

[melvin-bot]

Triggered auto assignment to @joekaufmanexpensify (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

[makiour]

Here is my proposal as original reporter: I posted it in slack when I reported the bug:

Proposal

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

Send magic code to any possible string/number as contact method through URL. Changing the URL for ContactMethodDetailsPage will allow us to see the magic code sent page, and clicking quickly on Did not receive a magic code will allow us to stay in it.

What is the root cause of that problem?

First, I had to compare it the normal flow of adding a new contact method. We first access it through settings/profile/contact-methods which allow us to add a new contact method with validation present here:

After adding a new contact method to our login list, we can clearly see that we have added a new login to the user loginList in ONYX.LOGIN_LIST through merging, and this one before validating the magic code:

After validation, we can clearly see another request has been sent, which shows that our new login is validated in the loginList

Now, this allow us to conclude:
We have a validation for the text entered to see if it is a valid number or email.
Adding the new contact method is reflected immediately in the loginList. Additionally, after confirming magic code, it shows it in the ONYX data.

This whole process is done through:
First, adding first the new contact-details to show the form. Second, we have magic code validation page to confirm the new contact method.
The second step is the one triggering this bug as we are accessing it without any sort of validation through URL

Adding any text as contact method in the URL means that either no validation is present within the component itself, or validation is not fully covered. I started first investigating the data returned through ONYX in the main component ContactMethodDetailsPage:

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 306 to 325 in 4230f28

	export default compose(
	withLocalize,
	withOnyx({
	loginList: {
	key: ONYXKEYS.LOGIN_LIST,
	},
	session: {
	key: ONYXKEYS.SESSION,
	},
	myDomainSecurityGroups: {
	key: ONYXKEYS.MY_DOMAIN_SECURITY_GROUPS,
	},
	securityGroups: {
	key: `${ONYXKEYS.COLLECTION.SECURITY_GROUP}`,
	},
	isLoadingReportData: {
	key: `${ONYXKEYS.IS_LOADING_REPORT_DATA}`,
	},
	}),
	)(ContactMethodDetailsPage);

As you can see, we are getting loginList, and what I found is the validation is indeed present:

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 212 to 216 in 4230f28

	const loginData = this.props.loginList[contactMethod];
	if (!contactMethod || !loginData) {
	return <NotFoundPage />;
	}

We are checking here if we do have the email entered in the URL among the list of contact methods => We basically check here if the first step of adding a new contact method has been followed! This is a right approach to check and it should return the page not found. However, this doesn't happen only after a few seconds, which means that we have something updating preventing us from rendering within this condition.
After I traced the loginList while accessing the component, I found that at the first render: the contact method input in the URL is indeed not present among the loginList:

This is the intended behavior, and it shows only the ones that were added correctly through the normal flow.
However, immediately the next LoginList seems to include also the one sent through the URL, which means that something in the re-render triggered the ONYX data to merge the new invalid contact method!

Indeed it has been merged, and it also shows a useful information that it not validated, so we can use this information later to fix the bug as well among the options.
My first intuition here is to check the ComponentDidMount() as the changes shows up immediately, and it would be the first function triggered after render

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 110 to 112 in 4230f28

	componentDidMount() {
	User.resetContactMethodValidateCodeSentState(this.getContactMethod());
	}

If we check User.resetContactMethodValidateCodeSentState():

App/src/libs/actions/User.js

Lines 266 to 272 in 4230f28

	function resetContactMethodValidateCodeSentState(contactMethod) {
	Onyx.merge(ONYXKEYS.LOGIN_LIST, {
	[contactMethod]: {
	validateCodeSent: false,
	},
	});
	}

We find exactly the same data merged a second after we have added the new contact method into the URL. Which means that the root cause here is the following:

We are sending here

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Line 111 in 4230f28

User.resetContactMethodValidateCodeSentState(this.getContactMethod());

this.getContactMethod() and directly we merge it into our data without having any form of validation within the mount function. This triggers the loginList to also include the invalid contact method and bypasses the checks we have here as the contact method indeed present.

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 212 to 214 in 4230f28

	const loginData = this.props.loginList[contactMethod];
	if (!contactMethod || !loginData) {
	return <NotFoundPage />;

The solution to this issue is to add validation in one of the functions and not merge the ONYX data until we know that it is indeed a valid input and followed the correct flow.

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

As we are concerned now about validating the login list and it is not fully covering the contact methods, I propose different options:

Option 1

We can simply modify the ComponentDidMount() to include the check for the this.getContactMethod() and not directly sending it to the other function. This can be done through changing:

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 110 to 112 in 4230f28

	componentDidMount() {
	User.resetContactMethodValidateCodeSentState(this.getContactMethod());
	}

To:

    componentDidMount() {
        const contactMethod = this.getContactMethod();
        const loginData = this.props.loginList[contactMethod];
        if (loginData) {
            User.resetContactMethodValidateCodeSentState(contactMethod);
        }
    }

Option 2

We can fix the resetContactMethodValidateCodeSentState in User.js. We can see that no validation is present to the contact method so we have here two flows we can approach:

Option 2.1

We can first have validation in the getContactMethod() component:

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 129 to 131 in 4230f28

	getContactMethod() {
	return decodeURIComponent(lodashGet(this.props.route, 'params.contactMethod'));
	}

To:

    getContactMethod() {
        const contactMethod =  decodeURIComponent(lodashGet(this.props.route, 'params.contactMethod'));
        return this.props.loginList[contactMethod] ? contactMethod : undefined;
    }

Then we will add a simple check to

App/src/libs/actions/User.js

Lines 266 to 272 in 4230f28

	function resetContactMethodValidateCodeSentState(contactMethod) {
	Onyx.merge(ONYXKEYS.LOGIN_LIST, {
	[contactMethod]: {
	validateCodeSent: false,
	},
	});
	}

:
By changing it to:

function resetContactMethodValidateCodeSentState(contactMethod) {
    if (contactMethod) {
        Onyx.merge(ONYXKEYS.LOGIN_LIST, {
            [contactMethod]: {
                validateCodeSent: false,
            },
        });
    }
}

Option 2.2

We can check the function itself:
https://github.com/Expensify/App/blob/4230f289341c8d6d9a8b780d54da0725d4764b2b/src/libs/actions/User.js#L266-L272to include the check:

  function resetContactMethodValidateCodeSentState(contactMethod) {
    const existingLoginData = Onyx.get(ONYXKEYS.LOGIN_LIST, {});
    if (existingLoginData.hasOwnProperty(contactMethod)) {
        return;
    }    Onyx.merge(ONYXKEYS.LOGIN_LIST, {
        [contactMethod]: {
            validateCodeSent: false,
        },
    });
}

Result

Screencast.from.05.2023.+01.04.49.30.webm

[joekaufmanexpensify]

Reproduced. I kind of wonder whether we should be including the specific contact method as part of the URL at all? It doesn't seem realistic that an actual user would ever need to deep link to this specific page.

And if we removed deep linking to a specific contact method, that would largely obviate this issue.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~013d8b6c25b7efa6cc

[melvin-bot]

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

[melvin-bot]

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

[Thanos30]

Proposal

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

Hardcoding contact method details URL with a non existent contact method shows magic code verification for a brief amount of time before showing not found page.

What is the root cause of that problem?

The root cause of this problem is on ContactMethodDetailsPage.js file, lines 212-215:

const loginData = this.props.loginList[contactMethod];
        if (!contactMethod || !loginData || !loginData.partnerUserID) {
            return <NotFoundPage />;
        }

An existent contact method object will have a bunch of properties when created, one of them being partnerUserID which I will use for the solution of the problem. A non existent contact method object will only have the validateCodeSent property set to false, getting it from our resetContactMethodValidateCodeSentState method.

On the above code section, we are only checking if the loginData object exists, and it does, with the validateCodeSent property I mentioned above.

This of course is not the correct check to do to determine if the contact method is found, which leads to showing the Magic Code validation page later on the code, which we show when that condition is true:
!loginData.validatedDate && !isFailedAddContactMethod, and of course, since loginData only has the validateCodeSent it becomes true.

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

On lines 212-215 , we should add a check for partnerUserID in the condition:

if (!contactMethod || !loginData || !loginData.partnerUserID) {
            return <NotFoundPage />;
        }

Screen.Recording.2023-08-15.at.4.36.53.AM.mov

[joekaufmanexpensify]

Proposals pending review.

[joekaufmanexpensify]

@eVoloshchak Mind taking a look at the proposal here when you have a chance?

[joekaufmanexpensify]

Proposals still pending review

[kaushiktd]

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

Adding any invalid text/string as a contact method through URL. It goes to the magic code validation tab but it should immediately says PageNotFound and not preview Contact Validation
with Magic Code sent

What is the root cause of that problem?

When we go through the URL using an invalid string, initially it will be rendered the component and check for validation on below file:
https://github.com/Expensify/App/blob/main/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js#L245-L259

{!loginData.validatedDate && !isFailedAddContactMethod && (
<View style={[styles.ph5, styles.mt3, styles.mb7]}>
<DotIndicatorMessage
type="success"
style={[styles.mb3]}
messages={{0: ['contacts.enterMagicCode', {contactMethod: formattedContactMethod}]}}
/>
<ValidateCodeForm
contactMethod={contactMethod}
hasMagicCodeBeenSent={hasMagicCodeBeenSent}
loginList={this.props.loginList}
ref={this.validateCodeFormRef}
/>
</View>
)}

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

To solve this issue you need to wrap up the complete component in below mentioned condition on
https://github.com/Expensify/App/blob/main/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

<>
{!loginData.validatedDate ? (
<NotFoundPage />
) : (
<ScreenWrapper onEntryTransitionEnd={() => this.validateCodeFormRef.current && this.validateCodeFormRef.current.focus()}>
<!-- all content from page inside screen wrapper -->
</ScreenWrapper>
)}
</>

Video
https://drive.google.com/file/d/1EJ5oEk_3MTHlBjNxWxrru3-ozDweRemo/view?usp=sharing

[joekaufmanexpensify]

Bumped in Slack

[eVoloshchak]

I kind of wonder whether we should be including the specific contact method as part of the URL at all? It doesn't seem realistic that an actual user would ever need to deep link to this specific page.
And if we removed deep linking to a specific contact method, that would largely obviate this issue.

@joekaufmanexpensify, there are legitimate use cases in theory. For example, if you want to open the current page (in this case details for a contact method) in a different browser (we do it all the time when testing PRs). Of course, this is an edge case (there are no other pages like this as far as I know) and even if we remove the deep linking, it will only add one extra click in this specific scenario, but I think we might want to fix this, even the rare cases are possible

[eVoloshchak]

@makiour, wow, thank you for such a thorough explanation!
I think we should proceed with option 1.1 of @makiour's proposal, which is changing

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 110 to 112 in 4230f28

	componentDidMount() {
	User.resetContactMethodValidateCodeSentState(this.getContactMethod());
	}

to

componentDidMount() {
    const contactMethod = this.getContactMethod();
    const loginData = this.props.loginList[contactMethod];
    if (loginData) {
        User.resetContactMethodValidateCodeSentState(contactMethod);
    }
}

I think we should also set NotFoundPage to say "Back to contact methods" instead of "Back to home" (and change the actual navigation action). If we navigate back from contact details, we should end up on the contacts page, not just dismiss the modal

🎀👀🎀 C+ reviewed!

[melvin-bot]

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

[joekaufmanexpensify]

Pending internal engineering sign off

[melvin-bot]

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

[joekaufmanexpensify]

@AndrewGable mind taking a look at this proposal when you have a sec? TY!

[joekaufmanexpensify]

Bumped Andrew 1:1

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @makiour got assigned: 2023-08-23 19:48:00 Z
• when the PR got merged: 2023-09-05 20:49:25 UTC
• days elapsed: 9

On to the next one 🚀

[makiour]

@joekaufmanexpensify

I just want you to take into consideration my particular situation: I have got an LGTM from @eVoloshchak within the three days timeline. However, @AndrewGable was OOO, and he reviewed the PR right after he is back and managed to merge the PR!

This is why it seems that the days elapsed are 9!

Thank you so much and happy to collaborate with you guys!

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

• Fix adding invalid contact method through URL #25814

If no regressions arise, payment will be issued on 2023-09-15. 🎊

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

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

• @eVoloshchak does not require payment (Eligible for Manual Requests)
• @makiour requires payment offer (Contributor)
• @makiour 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:

• [@eVoloshchak] The PR that introduced the bug has been identified. Link to the PR: Fix "Link has been re-sent" text in verify secondary login is not cleared even reload the page #23601
• [@eVoloshchak] 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/23601/files#r1326128384
• [@eVoloshchak] 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: I don't think an additional discussion is needed, we already have a checklist item to cover failure scenarios: I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
• [@eVoloshchak] Determine if we should create a regression test for this bug.
• [@eVoloshchak] 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.
• [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/317532

[joekaufmanexpensify]

@eVoloshchak mind completing the BZ checklist so we can prep to issue payment?

[makiour]

@joekaufmanexpensify

I just want you to take into consideration my particular situation: I have got an LGTM from @eVoloshchak within the three days timeline. However, @AndrewGable was OOO, and he reviewed the PR right after he is back and managed to merge the PR!

This is why it seems that the days elapsed are 9!

Thank you so much and happy to collaborate with you guys!

@joekaufmanexpensify replying here if you missed my comment!
Thank you!!

[joekaufmanexpensify]

Thanks for bumping! I'll review, and we'll definitely take any internal delays into account when calculating whether a bonus is warranted.

[makiour]

Thank you so much @joekaufmanexpensify! Appreciate it!

[joekaufmanexpensify]

Bumped BZ checklist in Slack so we can prep to issue payment soon.

[eVoloshchak]

• The PR that introduced the bug has been identified. Link to the PR: Fix "Link has been re-sent" text in verify secondary login is not cleared even reload the page #23601
• 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/23601/files#r1326128384
• 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: I don't think an additional discussion is needed, we already have a checklist item to cover failure scenarios: I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
• Determine if we should create a regression test for this bug.
  Is it easy to test for this bug? Yes
  Is the bug related to an important user flow? Yes
  Is it an impactful bug? No
  It's kind of an edge case since it requires a user to manually edit a URL with an invalid contact method, but the same can happen in case there are issues with the back end, so I think we need a regression test

[eVoloshchak]

Regression Test Proposal

1. Go to Settings > Profile > Contact Methods
2. Verify there is at least one contact method
3. Open the following link: https://staging.new.expensify.com/settings/profile/contact-methods/invalidContactMethodName/details
4. Verify that you're presented with 'Hmm... it's not here' page
5. Press on 'Go back to contact methods'
6. Select a contact method, verify you're navigated to an according contact method page

Do we agree 👍 or 👎

[joekaufmanexpensify]

Great, ty! I'll finish up the checklist so we can issue payment asap.

[joekaufmanexpensify]

Checklist is all set!

[joekaufmanexpensify]

We discussed this here and confirmed this qualifies for a speed bonus. This means we need to issue the following payments:

• @makiour - $1,750 ($1,000 for PR, $500 speed bonus, $250 reporting bonus). Paid via upwork.
• @eVoloshchak - $1,500 ($1,000 for C+ and $500 speed bonus). Paid via NewDot.

[joekaufmanexpensify]

@eVoloshchak could you please request $1,500 via NewDot and confirm here once complete?

[joekaufmanexpensify]

@makiour I sent your $1,750 in two payments, since you had two roles for this one.

[eVoloshchak]

@joekaufmanexpensify, requested the payment!

[joekaufmanexpensify]

Awesome, ty! @eVoloshchak closing this out for now. If your request hasn't been paid in 7 days LMK, and I'll look into it on my end.

[joekaufmanexpensify]

Payment summary message is here.

[JmillsExpensify]

$1,500 payment for @eVoloshchak approved based on BZ summary.