Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

Web- Log in - Console error appears when navigating to URL staging.new.expensify.com #12943

Closed kbecciv closed 1 year ago

kbecciv commented 1 year ago

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`. Open the Incognito Chrome window

  1. Open the developer tool - Console tab
  2. Start typing staging.new.expensify.com

Expected Result:

No console error appears when navigating to URL staging.new.expensify.com

Actual Result:

Console error appears when navigating to URL staging.new.expensify.com

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.30

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/203417289-3cd7b475-974b-432c-b139-36b940e34d70.mp4

image (1)

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

melvin-bot[bot] commented 1 year ago

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

trjExpensify commented 1 year ago

👋 I can reproduce this on both staging and production. Here's prod:

image

I don't know enough about this specific error to make a call on whether it can be worked on externally or not. The fact that it mentions Plaid and Onfido gives me a feeling that it might not be the case, so I'd love a second opinion, please @MariaHCD @nkuoch @ctkochan22. Thanks!

MariaHCD commented 1 year ago

I think this might be more to do with our Content Security Policy. So I'm not quite sure this can be worked on externally either.

melvin-bot[bot] commented 1 year ago

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

trjExpensify commented 1 year ago

Ohh, nice find! Okay cool, I'm going to mark this as internal for the time being for someone to make a call on that or what (if anything) should be done about this.

CC: @mountiny for vis as you touched this code back in Oct.

mountiny commented 1 year ago

I can take this on if @techievivek would not mind

mountiny commented 1 year ago

Seems like sometimes the sentry request is has different format:

https://o104379.ingest.sentry.io/api/5495040/envelope/?sentry_key=5be7bb2583284b3ca5221379bc5517b8&sentry_version=7&sentry_client=sentry.javascript.react%2F7.16.0
image
mountiny commented 1 year ago

That might not be related to this actually https://stackoverflow.com/questions/72584295/how-do-i-fix-sentry-error-403-on-react-js

mountiny commented 1 year ago

Seems like we would need to add new env variable to make sure the inlined script is added to its won file during build https://drag13.io/posts/react-inline-runtimer-chunk/index.html. That is using INLINE_RUNTIME_CHUNK=false

mountiny commented 1 year ago

A PR to test one theory is in review. This would fix only staging.

mountiny commented 1 year ago

This seems like it did not work in staging web

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

mountiny commented 1 year ago

This is open for proposals, the fix I have tried above did not work. I am 90% sure this is not related to our backend CSP set up and this needs to be fixed in App but I dont know well enough what to do here except the fix I already tried and it did not work

techievivek commented 1 year ago

I can look into it. I do believe this has to be fixed inside the App and nothing in the backend.

techievivek commented 1 year ago

Seems like this PR introduced the inline script and as per our CSP rule we don't allow that and that's why it is throwing that error. Should we remove html-inline-script-webpack-plugin package and just load the file?

@roryabraham @rushatgabhane @vladamx Any thoughts?

Taking this internally so I can work on this.

mountiny commented 1 year ago

Niiiice, thanks @techievivek!

Out of curiosity and so I can learn too, did you just search for some inline/script changes or how did you know what specific change/package to look for?

rushatgabhane commented 1 year ago

CSP rule

what's CSP rule? @techievivek

mountiny commented 1 year ago

This @rushatgabhane https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP#:~:text=Content%20Security%20Policy%20(CSP)%20is,site%20defacement%2C%20to%20malware%20distribution.

techievivek commented 1 year ago

@mountiny I looked in the console where it was throwing the error when looking at the code I saw a bunch of inline JS code and I realized that might the reason it is showing the error because we don't allow that in our CSP rule so to trace it back I looked in index.html for web and went through the recent commits to the file to see if we added any inline script and found the above PR.

mountiny commented 1 year ago

Oh nice, I saw that line as well but just assumed that is what is added when the app is built as it was quite giberrish. Nice find 🙇

roryabraham commented 1 year ago

Thanks for reporting and investigating. I noticed this the other day and knew immediately it had to do with the splash screen. I'm not sure why this wasn't flagged as part of the QA of https://github.com/Expensify/App/pull/12334, because it doesn't work on staging or production due to this CSP. Maybe I just missed it but this should've been flagged as a QA failure either in the PR or linked issue. Incidentally, the splash screen doesn't work correctly on dev either 😓.

Should we remove html-inline-script-webpack-plugin package and just load the file?

No, I don't think we should remove the html-inline-script-webpack-plugin package. It's important that the JS for the splash screen is included inline in index.html, that's how we ensure that it's rendered as early as possible while other resources are loading.

So I think we need to adjust the CSP to allow inline scripts to run. According to https://content-security-policy.com/examples/allow-inline-script/, we can either:

Not sure exactly what that means in practice

roryabraham commented 1 year ago

Reading up here, I think that the hash solution is probably out because it would require us to update the CSP any time we update the splash screen JS

roryabraham commented 1 year ago

There's an article here regarding the nonce-based solution, trying to understand what that would look like for us

roryabraham commented 1 year ago

I am going to close this and reopen in an internal repo

roryabraham commented 1 year ago

https://github.com/Expensify/Expensify/issues/246097