[$250] CORS error is displayed when opening attachment

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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): dbarret@expensify.com 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-22

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

[CyberAndrii]

@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 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

[CyberAndrii]

@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? 💸

[mvtglobally]

Issue not reproducible during KI retests. (Second week)

[Kalydosos]

@mvtglobally maybe you can try these steps https://github.com/Expensify/App/issues/51699#issuecomment-2450789710 even though i think it's a bit technical but it can be inspired from for a PR test steps

[hungvu193]

I don't think I can reproduce the case, even if I added an invalid token:

https://github.com/user-attachments/assets/d0ca5723-a7cf-4d0a-97c3-f1f517f18fdf

[Kalydosos]

@hungvu193 you shouldnt click on the attachment image, the bug is that the image doesnt display as you can see it in 0:09 of your video.

Here is the video of such test i made based on the solution i proposed https://github.com/Expensify/App/issues/51888#issuecomment-2453519021 (setting an age to sessions) based on the reproduction steps proposed https://github.com/Expensify/App/issues/51699#issuecomment-2450789710. Instead of that gray icon (not found) we now display a spinner while waiting for a valid session

https://github.com/user-attachments/assets/ccd73db5-d00a-49cd-83b2-0b3fb7388577

[hungvu193]

OK. I see your point here. @Kalydosos I think your proposal makes sense but it will also require BE change right? I don't think we have age in our Session?

[Kalydosos]

@hungvu193 i thought about it and at least for this particular issue a BE change is not required. But if we must make the BE change for some reason, meaning receiving the session age from the BE, we need to make sure that the BE takes in consideration the timezone of the FE device

[hungvu193]

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);

To be clear, this part of your proposal will fix the CORS issue? Correct? If so, I think I still can reproduce the issue:

[Kalydosos]

@hungvu193 cause1 or let's say causeA occurs when you have some legacy attachments that miss the 'data-expensify-source' attribute. A BE fix must have prevented such situation for new attachments (see image below), but if you can found some attachments like mix of updated text + pdf (pdfs turn image attachments) dating around 2 months ago (mines are from last september, see in the video) you will see causeA occur. Here is a video of these attachments after and before applying solution1. Those attachments trigger also the same cors error as you can see in the video below, because they are seen by the code as of external source and requested without authentication token which also result in a 302 redirection and then in a cors error. So to say, the only way to reproduce causeA is to have legacy attachments like updated pdf+text dating by last September or before.

The videos focus on the following attachment "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]"

Video of causeA triggering cors error https://github.com/user-attachments/assets/4d11d4cf-6471-4c4a-9688-804e592817c6

Video of the attachments after applying solution 1 https://github.com/user-attachments/assets/84a96b77-e043-4486-9815-54295cb616a7

Video of the attachments before applying solution 1 https://github.com/user-attachments/assets/b40b4579-b81c-4ba8-a2b8-9213371df503

image of not reproducing the kind of attachments for cause1 now

[CyberAndrii]

@Kalydosos Hmm, perhaps you copied a markdown representation of an attachment (which looks like below) and sent it as a new message? In this case, it's expected that data-expensify-source will be missing as the markdown only includes a link to the image.

123

![w_437ae01cd161793b78230838f5258002d1bbea73.jpg.1024.jpg](https://www.expensify.com/chat-attachments/2396479887808340341/w_437ae01cd161793b78230838f5258002d1bbea73.jpg.1024.jpg)

Also, even with your fix, it won't be possible to view the pdf as we don't have its link, so there's pretty much no point in making the preview work. I guess we just ignore this data-expensify-source part of the proposal :+1: :-1: ?

[Kalydosos]

@CyberAndrii no i didn't explicitly copy the markdown. These attachments were generated back then through the edition/update of text+attchachment already sent as you can see in the images (the "edited" footnote).

Yes back then, once the comment text modified it was not also possible to view the pdf. This is a legacy situation. Unless a patch went through the production data to correct these comments it is possible that some of these comments are still viewed with cors errors in production (or not, the cache helping). I think we are not losing anything by keeping that fix but we can miss "fixing" these comments (if any) by not including this part in the fix imho

[hungvu193]

cause1 or let's say causeA occurs when you have some legacy attachments that miss the 'data-expensify-source' attribute. A BE fix must have prevented such situation for new attachments (see image below), but if you can found some attachments like mix of updated text + pdf (pdfs turn image attachments) dating around 2 months ago (mines are from last september, see in the video) you will see causeA occur

hmm not sure if this is correct, I checked the pdf + text message, or image + text message but none of theses messages don't have the data-expensify-source. Is there any other way to re-create that case?

[Kalydosos]

@hungvu193 were the pdf/images + text you checked dating around 2 months and they were edited ? It has to be edited ones. Anyway, i tried some other ways but as i said the BE now systematically returns data-expensify-source for new attachments. We can now either keep in solution1 (it's just a change on 1 line) or we can let down solution1 and just implemented solution2. But i think our solution must adress this issue globally, so i advise we integrate solution1 (but the repro steps and PR repro tests adress just causeB), as imagine if a tester or in production cors issues raised up again after our PR have been merged and this time caused by legacy attachments.

[hungvu193]

Can you post a video that you log the value of htmlAttribs when the data-expensify-source is empty?

[Kalydosos]

@hungvu193 it turns out that one of my test earlier did reproduce causeA, like @CyberAndrii was saying if i directly copy the markdown the attachment lose its data-expensify-source and then the bug occurs. Check the videos of the repro below

https://github.com/user-attachments/assets/66e5f38b-40a0-470d-b1f2-5437d918118f

https://github.com/user-attachments/assets/9d448756-cc1a-48e7-9f73-b8f495eb9bbf

[hungvu193]

@hungvu193 it turns out that one of my test earlier did reproduce causeA, like @CyberAndrii was saying if i directly copy the markdown the attachment lose its data-expensify-source and then the bug occurs. Check the videos of the repro below

Thanks. This is correct and your changes from For cause 1 fixed this issue for image preview (Note that we still need to fix the issue when user opens the preview modal).

These attachments miss the data-expensify-source attribute and then are requested from the BE without authentication token

I don't think this can convince me. In the description of this issue, you can see that the user only sent an image and opened it, and the CORS appeared. Do you have any explanation for that?

[Kalydosos]

@greg-schroeder this ticket (3 weeks already) has and will used up a lot of time and brain power before a merged solution PR, what do you think of raising the bounty a bit ?

[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? 💸

[mvtglobally]

Issue not reproducible during KI retests. (Third week)

[hungvu193]

I think we should fix the bug when we copied markdown of image preview and paste into composer cc @greg-schroeder

[hungvu193]

Not overdue.