[$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
• 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]

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)

[melvin-bot]

Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[Kalydosos]

@hungvu193 ok to discuss the BE change if it can be done. We will also need Design

[rlinoz]

Thanks for all the hard work on this issue!

What is the BE change that we might need to fix the second issue? Is it sending back when the token expires?

[hungvu193]

What is the BE change that we might need to fix the second issue? Is it sending back when the token expires?

Yeah, we need an expiredTime for our token, @Kalydosos can you confirm?

[Kalydosos]

@rlinoz thanks for helping out !

If possible, we will need the BE, when it send a new session to the FE (like in response to Authenticate), to add the session creation date timestamp. That prop of the session can be named "creation" or "creationTime" per example and its value must take in consideration the timezone of the FE device.

What do you think of it ?

cc @hungvu193

[rlinoz]

Let's discuss here https://expensify.slack.com/archives/C01GTK53T8Q/p1732798975726319

[mvtglobally]

Issue not reproducible during KI retests. (Fourth week)

[melvin-bot]

@rlinoz @hungvu193 @greg-schroeder this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[hungvu193]

Still discussing

[melvin-bot]

❌ There was an error making the offer to @Kalydosos for the Contributor role. The BZ member will need to manually hire the contributor.

[rlinoz]

assigning @Kalydosos per thread https://expensify.slack.com/archives/C01GTK53T8Q/p1732798975726319

[Kalydosos]

thanks for helping out @rlinoz

@hungvu193 we will also need Design team's help

[hungvu193]

thanks for helping out @rlinoz

@hungvu193 we will also need Design team's help

About the loading image right? Let's use a temporary image and create the PR. We can discuss about it during PR phrase

[Kalydosos]

@hungvu193 sounds good 👍

[hungvu193]

@Kalydosos any ETA for the PR?

[Kalydosos]

The PR will be available in a few hours

[greg-schroeder]

Draft PR created

[melvin-bot]

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

[hungvu193]

Draft PR is up. We're still working on it

[greg-schroeder]

Thanks @hungvu193

[mvtglobally]

Issue not reproducible during KI retests. (Fifth week)

[greg-schroeder]

Work on linked PR continues