[HOLD for payment 2025-01-18] [$500] 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
• 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: @greg-schroeder

[melvin-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

App/src/components/Image/index.tsx

Lines 50 to 57 in f2e295f

	if (isAuthTokenRequired && authToken) {
	return {
	...propsSource,
	headers: {
	[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken,
	},
	};
	}

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

video_cause1.mp4

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

App/src/components/Image/index.tsx

Lines 60 to 62 in f2e295f

	// The session prop is not required, as it causes the image to reload whenever the session changes. For more information, please refer to issue #26034.
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [propsSource, isAuthTokenRequired]);

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

requetes_302_200_302.mp4

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

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx

Line 56 in f2e295f

const attachmentSourceAttribute = htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE];

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

App/src/components/Image/index.tsx

Lines 44 to 62 in f2e295f

	const source = useMemo(() => {
	if (typeof propsSource === 'object' && 'uri' in propsSource) {
	if (typeof propsSource.uri === 'number') {
	return propsSource.uri;
	}
	const authToken = session?.encryptedAuthToken ?? null;
	if (isAuthTokenRequired && authToken) {
	return {
	...propsSource,
	headers: {
	[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken,
	},
	};
	}
	}
	return propsSource;
	// The session prop is not required, as it causes the image to reload whenever the session changes. For more information, please refer to issue #26034.
	// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
	}, [propsSource, isAuthTokenRequired]);

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

App/src/libs/E2E/actions/e2eLogin.ts

Lines 54 to 66 in f2e295f

	Authenticate(e2eUserCredentials)
	.then((response) => {
	Onyx.merge(ONYXKEYS.SESSION, {
	authToken: response.authToken,
	email: e2eUserCredentials.email,
	});
	console.debug('[E2E] Signed in finished!');
	return waitForBeginSignInToFinish();
	})
	.catch((error) => {
	console.error('[E2E] Error while signing in', error);
	reject(error);
	});

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 #51699 (comment)

@greg-schroeder let's close this as a dupe of #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 #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 #51699

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

They provide no fix afaik in #51699, they just propose some potential repro steps. Let me know if i'm wrong.

I have read the recents edits #51699 (comment) and recents comments #51699 (comment) 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 #51888 (comment))

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

App/src/components/Image/index.tsx

Lines 44 to 58 in bef062b

	const source = useMemo(() => {
	if (typeof propsSource === 'object' && 'uri' in propsSource) {
	if (typeof propsSource.uri === 'number') {
	return propsSource.uri;
	}
	const authToken = session?.encryptedAuthToken ?? null;
	if (isAuthTokenRequired && authToken) {
	return {
	...propsSource,
	headers: {
	[CONST.CHAT_ATTACHMENT_TOKEN_KEY]: authToken,
	},
	};
	}
	}

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 #51888 (comment)

App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx

Lines 39 to 55 in f2e295f

	// There are two kinds of images that need to be displayed:
	//
	// - Chat Attachment images
	//
	// Images uploaded by the user via the app or email.
	// These have a full-sized image `htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]`
	// and a thumbnail `htmlAttribs.src`. Both of these URLs need to have
	// an authToken added to them in order to control who
	// can see the images.
	//
	// - Non-Attachment Images
	//
	// These could be hosted from anywhere (Expensify or another source)
	// and are not protected by any kind of access control e.g. certain
	// Concierge responder attachments are uploaded to S3 without any access
	// control and thus require no authToken to verify access.
	//

[Beamanator]

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

They provide no fix afaik in #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 #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 #26034

[Kalydosos]

@mvtglobally the repro steps will be more like #51699 (comment)

[greg-schroeder]

Same as above

[Kalydosos]

the issue in #44445 , added in steps3 of proposed repro steps #51699 (comment) is more related to a cache policy/navigator behavior. ND or the navigator tries to load the image from the cache (but the image is not in ND cache for cache partionning reasons) and the navigator does not send the request (therefore the connection only stalling).

One of the images ☝️ displays correctly because i disable the cache at the moment of its download and then i enabled the cache again and the other image couldnt be downloaded.

In the same time i can perfectly have both images correctly displayed when i access from another navigator

here are some illustrations of the issue

BUT the issue is not reproducing anymore in the current stage of changes (i tested using our current PR) but we see requests for the images that are canceled from time to time and because they dont reach the server they raise no errors and thus no gray icons.

So we need to found a proper fix for this issue (even though it doesnt impact the UX anymore) as our fix adressed more the session tokens problems. I will then suggest (so thinks @hungvu193) that this issue which seems related to a core feature as caching policy could be adressed in a ticket dedicated to it

[melvin-bot]

This issue has not been updated in over 15 days. @Kalydosos, @rlinoz, @hungvu193, @greg-schroeder eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[hungvu193]

Approved the PR.

[greg-schroeder]

Thanks @hungvu193

Work continues on linked PR as Rodrigo requested changes

[greg-schroeder]

Merged. Awaiting staging deploy

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• Fix 51888 cors errors are displayed for attachments #53407

If no regressions arise, payment will be issued on 2025-01-18. 🎊

For reference, here are some details about the assignees on this issue:

• @Kalydosos requires payment (Needs manual offer from BZ)
• @hungvu193 requires payment through NewDot Manual Requests

[melvin-bot]

@hungvu193 @greg-schroeder @hungvu193 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[greg-schroeder]

@hungvu193 Nearing the payment date here - can you finish up the checklist?

[greg-schroeder]

Sent you an offer for this job @Kalydosos - can you please accept it so I can prepare to pay you on the payment date? Thanks!

[hungvu193]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other: This is more like a feature we didn't implement before.

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: N/A, we didn't have a mechanism to detect expired session on FE before, there's no offending PR.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

Regression Test Proposal

Test:

Test 1:

1. Open a chat
2. Send a text + image as a comment if you don't have it already
3. Click to edit the comment
4. Copy the comment
5. Send the copy as a new comment
6. Verify that the preview image is displayed correctly

Test 2:

1. Open any chat.
2. Send a few attachments.
3. Open dev tools (Command + D) and choose Invalidate with delay.
4. Verify that attachments display correctly.
5. Click on one of these attachments.
6. Verify that there's no CORS error is displayed.

Do we agree 👍 or 👎

[greg-schroeder]

Created issue to add regression tests

[Kalydosos]

@greg-schroeder offer accepted. Thanks a lot 👍

[greg-schroeder]

Payment summary:

Contributor: @Kalydosos - $500 - Via upwork
C+: @hungvu193 - $500 - You can make a manual request via ND

[garrettmknight]

$500 approved for @hungvu193