[HOLD for payment 2024-11-22] Add a step to the Issue New Card form that collects a magic code

[tgolen]

Problem

Someone can issue a physical or virtual Expensify card without verifying they are the owner of the account. This relates to an internal security issue.

Why this is important to solve

This is a security vulnerability which can be taken advantage of if an account is compromised.

Solution

Collect a magic code when issuing physical or virtual Expensify cards. In a little more detail:

• The two API commands are here
• They should each have a new parameter called validateCode that is passed to the server
• In order to do this, the IssueNewCardPage needs a new step to gather a magic code from the user (we should have existing components that can be reused for this)

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

Add a step to the Issue New Card form that collects a magic code

What is the root cause of that problem?

This is a new feature request

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

• Add a validation step in IssueNewCardPage like ValidationStep. If this step is after the confirmation step, we should move the logic call API to the ValidationStep. If this step is before the confirmation step we will store the validation code data in issueNewCard?.data then pass it here

App/src/pages/workspace/expensifyCard/issueNew/IssueNewCardPage.tsx

Line 33 in cff3e86

const getCurrentStep = () => {

• Add the validationCode param here

App/src/libs/actions/Card.ts

Line 628 in cff3e86

const parameters = {

• To create ValidationStep, we can use ValidateCodeActionModal with isVisible as true. The way to use it as we do here

The details can be discussed in the PR phrase. I can create a test branch if we need

What alternative solutions did you explore? (Optional)

NA

[allgandalf]

Should we work on this issue under Workspace Feed project instead @trjExpensify ?

[tgolen]

@nkdengineer thanks! I was thinking the magic code should probably be the first step. My thinking is that there isn't much sense in letting the user fill out the rest of the form if they can't get past the magic code.

[nkdengineer]

@tgolen If we do that we need an API to help the user verrify before moving to the next step.

[twilight2294]

@tgolen I am working on a simpler proposal for this one, please wait assignment for few minutes

[twilight2294]

Proposal

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

Add a step to the Issue New Card form that collects a magic code

What is the root cause of that problem?

This is a feature request.

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

We can follow the same pattern we used for delegates here, we already have the requestValidationCode api command:

App/src/libs/actions/Delegate.ts

Lines 177 to 179 in 9490328

	function requestValidationCode() {
	API.write(WRITE_COMMANDS.RESEND_VALIDATE_CODE, null);
	}

NOTE: We always ask for validation code at the confirm step (Like delegates), so to match the app existing approach, we should ask it at the last.

1. When we click on confirm button, we need to trigger requestValidationCode.
2. Then like we do with delgates, we should create a RHP page to take form input like here. We need to create a base form logic (much can be reused from there) and then we need to create a separate file for android paltform.
3. Then like we do in delegates, we can send the validation code with the api:
   

   App/src/pages/settings/Security/AddDelegate/ValidateCodeForm/BaseValidateCodeForm.tsx

   Line 152 in 4c90d62

   Delegate.addDelegate(delegate, role, validateCode);

    if (cardType === CONST.EXPENSIFY_CARD.CARD_TYPE.PHYSICAL) {
        API.write(WRITE_COMMANDS.CREATE_EXPENSIFY_CARD, {...parameters, feedCountry, validateCode});
        return;
    }

    const domainAccountID = PolicyUtils.getWorkspaceAccountID(policyID);

    // eslint-disable-next-line rulesdir/no-multiple-api-calls
    API.write(WRITE_COMMANDS.CREATE_ADMIN_ISSUED_VIRTUAL_CARD, {...parameters, domainAccountID, validateCode});

Or we can include that code in the parameters itself. We ned to add a validateCode prop to issueExpensifyCard, if the code validation fails we already have the checks in the BaseValidateCodeForm so we can use most of the logic from there

NOTE: we also need to make changes to the number of steps back to routes etc. all that can be done in the PR phase

What alternative solutions did you explore? (Optional)

[nkdengineer]

• Using ValidateCodeActionModal is useful because

1. It had the logic to call requestValidateCodeAction API when it is rendered
2. It had a separate file for the Android platform
3. It's also used in other places of the app

If this step is after the confirmation step, we should move the logic call API to the ValidationStep

To create ValidationStep, we can use ValidateCodeActionModal with isVisible as true. The way to use it as we do here

• I notice that the idea of the last proposal is similar to what I proposed
• If we don't want to create a new step we can add ValidateCodeActionModal in the confirmation page and will open this modal when we click on the confirmation button

[twilight2294]

• Note:

• The RHP is what is followed throughout the application (While validating unvalidated accounts, bank account validation step) can be confirmed with design team as well

[hungvu193]

• Using ValidateCodeActionModal is useful because

1. It had the logic to call requestValidateCodeAction API when it is rendered
2. It had a separate file for the Android platform
3. It's also used in other places of the app

If this step is after the confirmation step, we should move the logic call API to the ValidationStep

To create ValidationStep, we can use ValidateCodeActionModal with isVisible as true. The way to use it as we do here

• I notice that the idea of the last proposal is similar to what I proposed
• If we don't want to create a new step we can add ValidateCodeActionModal in the confirmation page and will open this modal when we click on the confirmation button

I also agree with @nkdengineer at this point. We also have a PR to reuse ValidateCodeModal as much as possible here. (#49445)

[tgolen]

Yeah, that's right, we should reuse those existing components as much as possible. It sounds like this should work:

If we don't want to create a new step we can add ValidateCodeActionModal in the confirmation page and will open this modal when we click on the confirmation button

[hungvu193]

Yeah. I think we can go with @nkdengineer 's proposal here

[mountiny]

I want to highlight that @getusha and @hungvu193 are working on making the component for the magic code easier to reuse #49445, but it has been proving tricky and taking a bit of time

[tgolen]

Thanks for the heads up @mountiny. Do you want to hold off on doing anything here until that's settled? Or is there something that we can do in this PR that maybe helps the progress of making it easier to reuse?

[mountiny]

I think it would make sense to wait on the refactor, or then we have to double the work. I hope the PR can be wrapped up soon. I have started a thread for this here to hopefully get to faster completion.

[tgolen]

@nkdengineer This should be ready for the frontend PR to be created.

[tgolen]

bump @nkdengineer are you able to get a PR going for this now?

[nkdengineer]

@tgolen I'm investigating some bugs in the PR. Will give an update on Monday.

[tgolen]

@nkdengineer Do you have an update on this? If you're not able to prioritize this, I suggest you unassign it so that we can have someone else work on it.

[nkdengineer]

@tgolen I still update it daily, will give an update soon.

[melvin-bot]

This issue has not been updated in over 15 days. @tgolen, @trjExpensify, @hungvu193, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[hungvu193]

PR is still in progress

[tgolen]

PR was merged this morning.

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

• fix: Add a step validate to the Issue New Card form #50864

If no regressions arise, payment will be issued on 2024-11-22. 🎊

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

• @hungvu193 requires payment through NewDot Manual Requests
• @nkdengineer requires payment (Needs manual offer from BZ)

[melvin-bot]

@hungvu193 @trjExpensify @hungvu193 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[trjExpensify]

👋 Let's get the checklist done @hungvu193 ahead of Friday!

[hungvu193]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on both staging and production
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] 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: There's no offending PR. We added this feature to prevent internal security issue (as mentioned in the OP).

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source 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: This is not regression, I don't think we should create slack convo for it.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

IMO, this is a new feature, I don't think we need a regression test.

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @hungvu193 owed $250 via NewDot
• ROLE: @nkdengineer paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@trjExpensify)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[trjExpensify]

Payment summary:

• $250 to @hungvu193 (go ahead and request!)
• $250 to @nkdengineer (offer sent!)

[trjExpensify]

IMO, this is a new feature, I don't think we need a regression test.

On this, I agree we don't need a new regression test here. We have one for the issue card flow, and it would necessitate going through the magic code validation by design.

[trjExpensify]

Paid, closing!

[garrettmknight]

$250 approved for @hungvu193