[$500] CORS error is displayed when opening attachment

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v9.0.56-5 Reproducible in staging?: Yes Reproducible in production?: Unknown (we assume yes)

Email or phone of affected tester (no customers): [email protected] Logs: https://stackoverflow.com/c/expensify/questions/4856

Issue reported by: @quinthar Slack conversation #quality

Action Performed:

1. Open dev tools > Network tab
2. Send a image to a conversation
3. Open the image to view it's preview

Expected Result:

There's no error displayed in the network tab when opening a image.

Actual Result:

There's a CORS error when opening an image sent by the current user. (check screenshot) Issue is not constantly reproducible.

Workaround:

N/A

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [ ] Android: Standalone
• [ ] Android: HybridApp
• [ ] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021854953413121158499
• Upwork Job ID: 1854953413121158499
• Last Price Increase: 2024-11-27

Issue Owner

Current Issue Owner: @hungvu193

[melvin-bot[bot]]

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

[MelvinBot]

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

[greg-schroeder]

I couldn't reproduce this

[justinpersaud]

We have had similar reports of this before. I won't be able to help here unless it's still open when I'm back on the 11th, but I think the issue is has something to do with the exitTo being inserted into the request for the image from NewDot. I could be wrong, but that suggests to me that the token being used to request the image has expired and now we're serving back the exitTo to reauthenticate the user.

The CORS headers that it says are missing should be inserted here in nginx: https://github.com/Expensify/Salt/blob/main/shared_pkgs/nginx/files/conf.d/www.conf.template#L289-L302

but that line above does not match if the request is something like https://www.expensify.com/?exitTo=/chat-attachments/...

I think that is the problem.

cc @luacmartins since we were discussing a similar issue in another GH

[justinpersaud]

@Beamanator I think I worked with you on getting this CORS stuff working awhile back when we started using X-Chat-Attachment-Token so maybe you have some insight to the above

[Kalydosos]

Edited by proposal-police: This proposal was edited at 2024-11-03 18:47:58 UTC.

Proposal

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

CORS error is displayed when opening attachment

What is the root cause of that problem?

There are in fact 2 causes to this issue

cause 1 Some attachments are seen as of external source These attachments miss the data-expensify-source attribute and then are requested from the BE without authentication token. Per example the attachment in the image below (see in video below also)

"html": "fixpdf10 updated<br /><br /><img src="https://www.expensify.com/chat-attachments/3502317922698459004/w_b0b3af58e7d67648f4a81469e82f9e0f409960d4.jpg.1024.jpg" alt="w_b0b3af58e7d67648f4a81469e82f9e0f409960d4.jpg.1024.jpg" />", "text": "fixpdf10 updated\n\n[Attachment]",

they are requested without authentication token from the following lines

https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/components/Image/index.tsx#L50-L57

The BE then send a redirect as exitTo url but which is requested without the proper "Access-Control-Allow-Origin" header. it doesnt reproduce all the time because sometimes the image is in the cache and is not requested from the BE

https://github.com/user-attachments/assets/4d11d4cf-6471-4c4a-9688-804e592817c6

cause 2 The second cause is that the images are, at the very beginning of the screen loading (deeplinking to report), requested with an expired authentication token even though in most cases a reauthentication have been made. That should not happen but in fact the images loading were disconnected from direct session changes because of a bug in the past https://github.com/Expensify/App/issues/26034

https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/components/Image/index.tsx#L60-L62

Then even though a reauthentication happens like in the images below (after reconnectApp gets a 407 from the BE) some of the images are firstly requested with an expired authentication token

These 2 causes are well sumed up in the video below

https://github.com/user-attachments/assets/47f88fe7-2d10-4e1c-bca8-95b6358ebc4d

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

For cause 1 We should change the code below to include these attachments, from

https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L56

into

const attachmentSourceAttribute = htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]??(htmlAttribs.src?.includes("expensify.com/chat-attachments")?htmlAttribs.src:null);

For cause 2 After evaluating other options, the safer is to apply the principle of not loading the images when the session is older then a certain amount of time (in fact the max idle time to expire a session in the BE. To be determined with the relevant Expensify team. I have setted 3 hours in this code and use 3 minutes for testing purpose). For that we should induce a notion of session age and if the session is older than the max idle time allowed, we display a loading-spinner-like icon (i have used hourglass in the code. To be determined with Design team) on the images until the session change have been propagated to induce a redraw of the screen. For that we add the following

in type Session.tsx https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/types/onyx/Session.ts we added

we change the following lines

https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/components/Image/index.tsx#L44-L62

into

we force useMemo to recalculate and rerender when the session is outdated and we cover cases like the first outdated session without age and the first still valid session without age. Based on the experience working on this issue, we think that any update of the session by Onyx in less than 60s after the previous update may be related to a reauthentication and thus we must recalculate the image source and rerender the image

we also change https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/libs/actions/Session/updateSessionAuthTokens.ts into this

export default function updateSessionAuthTokens(authToken?: string, encryptedAuthToken?: string) { Onyx.merge(ONYXKEYS.SESSION, {authToken, encryptedAuthToken, age: new Date().getTime()}); }

and https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/libs/E2E/actions/e2eLogin.ts#L54-L66

into

in Session/index.tsx https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/libs/actions/Session/index.ts we added

RESULT

It works like a charm, the attachments are properly displayed in both cases .

video before https://github.com/user-attachments/assets/72295591-3162-4acd-8431-98c8da2ef195

video after https://github.com/user-attachments/assets/367d36b8-1f72-4047-8a35-d67c6114b195

video testing the 2 Hours expired token https://github.com/user-attachments/assets/ccd73db5-d00a-49cd-83b2-0b3fb7388577

What alternative solutions did you explore? (Optional)

None

[Kalydosos]

it doesnt reproduce all the time because sometimes the image is in the cache and is not requested from the BE

[andriivitiv]

@justinpersaud you are correct, see details here https://github.com/Expensify/App/issues/51699#issuecomment-2453773986

@greg-schroeder let's close this as a dupe of https://github.com/Expensify/App/issues/51699 ?

[Beamanator]

This def does look like a dupe 👍 Also @CyberAndrii 's reproduction steps look pretty good in the other issue, would be nice if @Kalydosos could try to reproduce w/out their fix & with their fix to see if that solves the issue every time 🙏

[Kalydosos]

This def does look like a dup

@Beamanator https://github.com/Expensify/App/issues/51699 is about the reproduction steps but this ticket is about the solution to the problem. So this is not a duplicate. I will provide simpler repro steps in https://github.com/Expensify/App/issues/51699

would be nice if @Kalydosos could try to reproduce w/out their fix & with their fix

They provide no fix afaik in https://github.com/Expensify/App/issues/51699, they just propose some potential repro steps. Let me know if i'm wrong.

I have read the recents edits https://github.com/Expensify/App/issues/51699#issuecomment-2450789710 and recents comments https://github.com/Expensify/App/issues/51699#issuecomment-2453773986 of @CyberAndrii. I will provide simpler reproduction step. And in fact his statement is not correct as the issue has nothing to do with "expired authentication token", the token are just not sent. We cannot send expired token because the token that is send is from the session and the user session is refresh way before retrieving the images. (EDIT : @CyberAndrii is right in fact on this point, cf https://github.com/Expensify/App/issues/51888#issuecomment-2465273720)

The image url is passed directly to RN's <Image /> component bypassing our middleware so a new token is not issued.

this part is not correct neither as the token is not just sent and it is a deliberate choice, see ligne 50 of https://github.com/Expensify/App/blob/bef062b4caa7f665159dc107911e708031e648c4/src/components/Image/index.tsx#L44-L58

and what proves even more my point and made my proposed fix correct is the deliberate intent of not sending the token made clear in the following comments from the code itself. It is just the some attachments were not taken in consideration as of "local" source cf https://github.com/Expensify/App/issues/51888#issuecomment-2453519021

https://github.com/Expensify/App/blob/f2e295fd174c87f7a516800c22028b637746b22c/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx#L39-L55

[Beamanator]

would be nice if @Kalydosos could try to reproduce w/out their fix & with their fix

They provide no fix afaik in https://github.com/Expensify/App/issues/51699, they just propose some potential repro steps. Let me know if i'm wrong.

Yes that's my bad on phrasing - I was talking to the air, but i should have asked you specifically if you could reproduce consistently & if your fix solves all cases 🙏 I def agree there were no solutions proposed in the linked issue

[Kalydosos]

@Beamanator yes i could reproduce the issue consistently and yes my fix solve all cases. I will provide a repro steps here and in the linked issue asap

[Beamanator]

Thanks very much for confirming 🙏

[greg-schroeder]

Okay so I should set this to External and have us assign @Kalydosos?

[Kalydosos]

@Beamanator i've been having some issue with my test account since yesterday, i will make a proposal for the repro steps in https://github.com/Expensify/App/issues/51699 once i get that fixed, this proposal here anyway still the fix for the problem

[andriivitiv]

@Kalydosos this happens sometimes. Create a new account with the + postfix as described in CONTRIBUTING.md

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[Kalydosos]

@Beamanator @greg-schroeder i have updated my proposal, it was a tough issue with multiple causes. I have given a sumup in the proposal details. Reproduction and evolution on the resolution was not easy as eachstep required waiting some hours but i can say that the different aspects of the issue are taken in consideration by my proposal. Some information (the maximum session idle time allowed by the BE) and a choice of Design for a loading spinner must be made if my proposal is accepted.

[Kalydosos]

@CyberAndrii my bad, you were right on the point that expired token was also send as a cause of the issue. It shouldn't have happen but an old bug allow the code to do so https://github.com/Expensify/App/issues/26034

[Kalydosos]

@mvtglobally the repro steps will be more like https://github.com/Expensify/App/issues/51699#issuecomment-2459151144

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[hungvu193]

@Kalydosos What's the best way to reproduce this one?

[Kalydosos]

@hungvu193 have you tried these steps https://github.com/Expensify/App/issues/51699#issuecomment-2459151144 ?

[greg-schroeder]

This was initially reported in #quality by DB so I'm adding to that project accordingly

[melvin-bot[bot]]

@hungvu193, @greg-schroeder Whoops! This issue is 2 days overdue. Let's get this updated quick!

[hungvu193]

Not overdue.

[melvin-bot[bot]]

@hungvu193 @greg-schroeder this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[hungvu193]

I've been a bit under the water today, but def can give a decision this week.

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸