Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mWeb - Error message displayed when trying to upload document on onfido flow and user is redirected to Company step #6085

Closed
isagoico opened this issue Oct 27, 2021 · 27 comments
Assignees
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2

Comments

@isagoico
Copy link

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. Navigate to add a bank account modal
  2. Enter some bank account credentials that trigger the Onfido flow
  3. Select drivers license
  4. Take a picture of anything

Expected Result:

Picture should be uploaded and then scanned by onfido

Actual Result:

There's an error just as the image is uploaded. User is redirected back to company step.

Workaround:

None found. Onfido is blocked on mWeb.

Platform:

Where is this issue occurring?

  • Mobile Web

Version Number: 1.1.10-0

Reproducible in staging?: Yes
Reproducible in production?: No

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: Applause
Slack conversation:

View all open jobs on GitHub

@isagoico isagoico added DeployBlockerCash This issue or pull request should block deployment Engineering Daily KSv2 labels Oct 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jasperhuangg (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@github-actions github-actions bot added Hourly KSv2 and removed Daily KSv2 labels Oct 27, 2021
@roryabraham
Copy link
Contributor

@jasperhuangg Did you mean to close this?

@roryabraham
Copy link
Contributor

I'm assuming that was a mistake because this was closed without a comment or PR, reopening.

@roryabraham roryabraham reopened this Oct 27, 2021
@kavimuru
Copy link

@jasperhuangg @roryabraham Adding video for the above bug.

20211027_183524.mp4

@roryabraham
Copy link
Contributor

@kavimuru Can you confirm the account(s) you used to test this? kavi.utest+chatd@gmail.com ?

@roryabraham
Copy link
Contributor

Logs don't appear to reveal much 😕 :

[hmmm] Onfido error in RequestorStep ~~ error: 'Unknown error' userAgent: 'Mozilla/5.0 (Linux; Android 10; SM-G973W) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/95.0.4638.50 Mobile Safari/537.36'

@jasperhuangg
Copy link
Contributor

Looks like there isn't enough information to go off of to debug this.

To summarize, the growl is how we handle an error returned by the Onfido API. Here's how that error is shaped (taken from the Onfido SDK npm package)

export declare type SdkError = {
	type: "exception" | "expired_token";
	message: string;
};

We only log the message here, and since the error we get back doesn't have a message to pass into the growl, we default to CONST.ERROR.UNKNOWN_ERROR i.e. Unknown error.

@roryabraham Is there any chance we can deploy something that changes the log to include the type parameter and get @kavimuru to retry? Or is it better at this point to revert the PR?

@roryabraham
Copy link
Contributor

I assume yes, but have we confirmed that this issue is not present on production?

@jasperhuangg
Copy link
Contributor

have we confirmed that this issue is not present on production?

Not yet, only verified on staging, on that right now.

@roryabraham
Copy link
Contributor

@roryabraham Is there any chance we can deploy something that changes the log to include the type parameter and get @kavimuru to retry?

Of course, this is possible.

Or is it better at this point to revert the PR?

What PR?

@isagoico
Copy link
Author

We were unable to reproduce this issue in production on our side.

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Oct 27, 2021

What PR?

@roryabraham Right sorry, forgot we don't know where this is coming from.

Just tested it myself on production and didn't run into it, so it looks like this is a regression. I'll start scanning through any relevant App PRs.

@roryabraham
Copy link
Contributor

One thing that's worth noting is that E/App staging points at the production Web-E API, but at the staging Web-S API. So the regression could have been caused in https://github.com/Expensify/Web-Secure

@roryabraham
Copy link
Contributor

I'm far from certain, but this PR seems like a potential suspect to me.

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Oct 27, 2021

@roryabraham After some investigation I think the error should be self-contained within the Onfido API, I don't think this part of the flow touches Web-S. Especially since switching between staging/prod for NewDot fixes the issue.

As for https://github.com/Expensify/App/pull/5975/files, it looks like the functions that were changed are only called within the onFieldChange prop for IdentityForm, which I also don't think is invoked by the Onfido flow.

I think we should deploy #6091 and get more information first.

@jasperhuangg
Copy link
Contributor

Also unrelated but this deploy blocker isn't showing up in my K2

@jasperhuangg
Copy link
Contributor

jasperhuangg commented Oct 28, 2021

Ah, after some poking around @roryabraham and I figured out that it's actually the browser blocking the request to upload the ID picture to this endpoint.

Screen Shot 2021-10-27 at 5 06 35 PM

If you're wondering why I have a screenshot of the Network tab from the Desktop version of Chrome (even though this is a mobile web issue): Onfido only allows us to upload documents from a mobile client. However Onfido doesn't actually check the user-agent–their JS just checks the screen size–meaning we can reproduce this on any browser.

@justinpersaud Looks like this has to do with the CSP we've enabled for staging. Do you think it's safe to get rid of this deploy blocker?

@marcaaron
Copy link
Contributor

Do you think it's safe to get rid of this deploy blocker?

Let's fix the CSP issue, retest, then remove the blocker?

@roryabraham
Copy link
Contributor

More details on the CSP errors:

image

index.js:26 Refused to load the script 'https://www.woopra.com/track/ce/?ra=rkQQmI5tHL4a&alias=onfido-js-sdk.com&instance=onfidojssdkwoopra&ka=24000&meta=&screen=375x812&language=en-US&app=js-client&referer=&idle=0&vs=r&cookie=e7m1bmxiirrV&event=started%20flow&cv_sdk_version=6.8.0&cv_client=staging.new.expensify.com&ce_is_custom_ui=true' because it violates the following Content Security Policy directive: "script-src 'self' https://polyfill.io https://cdn.plaid.com". Note that 'script-src-elem' was not explicitly set, so 'script-src' is used as a fallback.
...
...
Refused to connect to 'https://api.onfido.com/v3/documents' because it violates the following Content Security Policy directive: "connect-src 'self' https://*.pusher.com wss://*.pusher.com https://*.pusherapp.com https://staging-secure.expensify.com https://www.expensify.com wss://sync.onfido.com https://telephony.onfido.com blob:".
...
Refused to connect to 'https://sentry.io/api/109946/store/?sentry_key=6e3dc0335efc49889187ec90288a84fd&sentry_version=7' because it violates the following Content Security Policy directive: "connect-src 'self' https://*.pusher.com wss://*.pusher.com https://*.pusherapp.com https://staging-secure.expensify.com https://www.expensify.com wss://sync.onfido.com https://telephony.onfido.com blob:".

@justinpersaud
Copy link
Contributor

Let's fix it instead of just pushing to production. CSP was never enabled in production yet, and is on hold right now.

It can be updated here https://github.com/Expensify/Cloudflare-Workers/blob/master/new.expensify.com/wrangler.toml#L12

@roryabraham
Copy link
Contributor

roryabraham commented Oct 28, 2021

Got a PR up to hopefully fix this:

https://github.com/Expensify/Cloudflare-Workers/pull/15

It's approved, but we need it to be merged and deployed by ring0 before we can retest.

@fukawi2 fukawi2 closed this as completed Oct 28, 2021
@roryabraham
Copy link
Contributor

Reopening for testing

@roryabraham roryabraham reopened this Oct 28, 2021
@roryabraham
Copy link
Contributor

Verified this is fixed, closing this out 🎉

@marcaaron
Copy link
Contributor

Thanks @roryabraham !! 🙇‍♂️

@bondydaa
Copy link
Contributor

I think this was from Jasper testing this before the fix was out right? https://github.com/Expensify/Expensify/issues/182916

@roryabraham
Copy link
Contributor

I think this was from Jasper testing this before the fix was out right?

Yep, looks like it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants