[HOLD for payment 2022-07-25] [$250] Statement does not load in NewDot Desktop App

[thienlnam]

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:

Break down in numbered steps

1. Click on the statement from Concierge on NewDot Desktop

Expected Result:

Describe what you think should've happened
Should show the statement

Actual Result:

Describe what actually happened
Shows a blank screen

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Will have to view the statement on a non desktop application

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App x
• Mobile Web

Version Number:
Reproducible in staging?:
Reproducible in production?:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Upwork job link: https://www.upwork.com/jobs/~01993059e1414b86b4

View all open jobs on GitHub

[thienlnam]

cc @justinpersaud since you've helped us on previous configuration issues - do we just need to include https://www.expensify.com/ in the frame-ancestors list?

https://github.com/Expensify/Salt/blob/main/shared_pkgs/nginx/files/conf.d/www.conf.template#L50-L50

[justinpersaud]

www.expensify.com is in the list already. Is it not valid without the protocol? Maybe that was a typo?

[thienlnam]

I assume it's not valid without the protocol, www.expensify.com is put there from the hostname here. We explicity imply it in these other configs https://github.com/Expensify/Salt/blob/3c6ac677d3a3e6d0dd7ee57b04fab523957525b6/shared_pkgs/nginx/files/conf.d/integrations.conf.template#L20-L20 so I'm guessing we can just start by adding it?

[justinpersaud]

yeah let's try that and see? according to the spec it should work though: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-ancestors

Warning: If no URL scheme is specified for a host-source and the iframe is loaded from an https URL, the URL for the page loading the iframe must also be https, per the Does URL match expression in origin with redirect count? section of the CSP spec.

[justinpersaud]

wait, are you using the staging server on newdot? could it be because the request is coming from https://staging.new.expensify.com --> www.expensify.com

[thienlnam]

Not on the staging server on newDot, unless it's something that's automatically added to my expensify account. Good point on the staging though, we'd also want this to work on staging so I will also add https://staging.expensify.com/ as well

[parasharrajat]

@thienlnam This one needs to be done on front end. Doing this change will fix it.

1. Add the following line after

   App/desktop/main.js

   Line 166 in 35a41d9

   callback({responseHeaders: details.responseHeaders});

   .

if(details.responseHeaders['content-security-policy']){
                        details.responseHeaders['content-security-policy']  = details.responseHeaders['content-security-policy'].map(v => v.startsWith('frame-ancestors') ? v + ` app://*` : v);
                    }

2. Add the following line after

   App/desktop/main.js

   Line 170 in 35a41d9

   details.responseHeaders['access-control-allow-origin'] = [`http://localhost:${process.env.PORT}`];

   .

  if(details.responseHeaders['content-security-policy']){
                        details.responseHeaders['content-security-policy']  = details.responseHeaders['content-security-policy'].map(v => v.startsWith('frame-ancestors') ? v + ` http://localhost:${process.env.PORT}` : v);
                    }

Other solution could be to remove the CSP response headers all along. I am not sure of the security implications so I won't suggest it.

[thienlnam]

cc @parasharrajat

Add the following line after

What's the reasoning behind this, how come it isn't being recognized currently?

Other solution could be to remove the CSP response headers all along. I am not sure of the security implications so I won't suggest it.

Yeah this is unlikely what we'd want to do

[parasharrajat]

On the desktop app, the app is loading with localhost URL on dev and with app:// on production. CSP policy will fail if URLs do not match what is specified in frame-ancestors.

Previously we never used frames anywhere AFAIK so frame-ancestors CSP was never applicable to any API calls.
As we are using electron, we can do anything with the API calls, either disable the policy completely before building the app(very bad) Or intercept the requests calls and manage the headers before that calls are received on the frontend.

[thienlnam]

Interesting, thanks for the context. I agree this actually seems to be an issue we can fix on the front-end. Seems like we've already been running into this with access-control-allow-origin.

We could just update our config to allow app://expensify... but I think the solution you provided makes more sense. Do you want to take this issue?

[parasharrajat]

Yup, I can do that.

[mvtglobally]

@thienlnam not sure if this is related #9670

[thienlnam]

@mvtglobally Yup, good catch - this is the same issue so I'm closing the other one

[melvin-bot]

Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[thienlnam]

cc @michaelhaxhiu

This is now an external job - please also hire @parasharrajat as the contributor completing the PR
I'll be reviewing the PR

[parasharrajat]

@thienlnam I also found one issue #9712 (comment). Should I report it or you are aware of this?

I also suggested a solution to that.

[thienlnam]

We've been aware of the blank page - we don't return anything in various scenarios and we've talked about adding some kind of error message so it doesn't look like a bug. The way you mentioned is one way we could handle it, we've also thought about just returning an error dialog but we haven't really fleshed out the design of what this would look like yet so it's something that we need to do first

[melvin-bot]

Current assignee @parasharrajat is eligible for the Exported assigner, not assigning anyone new.

[melvin-bot]

Current assignee @thienlnam is eligible for the Exported assigner, not assigning anyone new.

[michaelhaxhiu]

Job posted here - https://www.upwork.com/jobs/~01993059e1414b86b4, and invited Rajat :)

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.84-13 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [Prod QA] Fix CSP for statement frames on Desktop #9804

If no regressions arise, payment will be issued on 2022-07-25. 🎊

[michaelhaxhiu]

paid