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

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

Closed
kbecciv opened this issue Nov 22, 2022 · 28 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@kbecciv
Copy link

kbecciv commented Nov 22, 2022

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
2. Open the developer tool - Console tab
3. 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?

  • Web

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

12924.Web.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

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 22, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 22, 2022

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

@trjExpensify
Copy link
Contributor

👋 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
Copy link
Contributor

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.

@trjExpensify trjExpensify added Internal Requires API changes or must be handled by Expensify staff Engineering labels Nov 23, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 23, 2022

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

@trjExpensify
Copy link
Contributor

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
Copy link
Contributor

I can take this on if @techievivek would not mind

@mountiny mountiny assigned mountiny and unassigned techievivek Nov 24, 2022
@mountiny
Copy link
Contributor

mountiny commented Nov 25, 2022

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
Copy link
Contributor

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

@mountiny
Copy link
Contributor

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
Copy link
Contributor

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

@mountiny
Copy link
Contributor

This seems like it did not work in staging web

@mountiny mountiny added External Added to denote the issue can be worked on by a contributor and removed Reviewing Has a PR in review Internal Requires API changes or must be handled by Expensify staff labels Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

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

@melvin-bot melvin-bot bot changed the title Web- Log in - Console error appears when navigating to URL staging.new.expensify.com [$1000] Web- Log in - Console error appears when navigating to URL staging.new.expensify.com Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 30, 2022

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

@mountiny
Copy link
Contributor

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
Copy link
Contributor

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

@techievivek
Copy link
Contributor

techievivek commented Nov 30, 2022

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.

@techievivek techievivek self-assigned this Nov 30, 2022
@techievivek techievivek removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Nov 30, 2022
@techievivek techievivek changed the title [$1000] Web- Log in - Console error appears when navigating to URL staging.new.expensify.com Web- Log in - Console error appears when navigating to URL staging.new.expensify.com Nov 30, 2022
@trjExpensify trjExpensify added the Internal Requires API changes or must be handled by Expensify staff label Nov 30, 2022
@mountiny
Copy link
Contributor

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
Copy link
Member

CSP rule

what's CSP rule? @techievivek

@mountiny
Copy link
Contributor

@techievivek
Copy link
Contributor

techievivek commented Nov 30, 2022

@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
Copy link
Contributor

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
Copy link
Contributor

roryabraham commented Dec 1, 2022

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 #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:

  • Use a nonce
  • Use a hash of the inline script

Not sure exactly what that means in practice

@roryabraham
Copy link
Contributor

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
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

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

@roryabraham
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
None yet
Development

No branches or pull requests

8 participants