[HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from

[kavimuru]

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:

1. Use https://staging.new.expensify.com/ to chat with somebody
2. Send an attachment
3. Use https://new.expensify.com/ to open the same conversation
4. Inspect the URL for the attachment
5. It will say it's loaded from https://staging.expensify.com/chat-attachments/ even though you are on new.expensify

Expected Result:

all attachment URLs to be resolved relative to current environment
E.g. when I’m on staging.new I expect all attachments to be resolved from https://staging.new.expensify.com/chat-attachments/…
Or when Im on prod I expect all attachments to be resolved from https://new.expensify.com/chat-attachments/…

Actual Result:

some attachment urls are resolved from staging.expensify.com others from new.expensify.com

Workaround:

unknown

Platforms:

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

• MacOS / Chrome / Safari

Version Number: 1.2.52-4
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Expensify/Expensify Issue URL:
Issue reported by: @kidroca
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673289214225929

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01911d83399c4bca37
• Upwork Job ID: 1613477091071528960
• Last Price Increase: 2023-01-20

[melvin-bot]

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

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

[melvin-bot]

Current assignee @bfitzexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

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

[getusha]

This happens when you upload the attachment on staging and view it on production vice versa, and i don't think this is an issue.

cc @bfitzexpensify

[Prince-Mendiratta]

Proposal

RCA

When the attachment is uploaded, the HTML is sent to the backend using the staging URL. When the same is received via the API, the HTML remains same and the src is not updated.

Solution

This should be handled on the backend, the file links should have the base URL updated depending on the request host. Not sure if this can be handled externally.

[kidroca]

@Prince-Mendiratta

When the attachment is uploaded, the HTML is sent to the backend using the staging URL

Can you point where this is happening? I think the HTML is formed on the backend

If the html is formed locally and then sent to the backend, I think it's should be ok to set the image src as

<img src="/chat-attachments/{path}" />

[kidroca]

Proposal

Exclude the origin (the protocol and the domain name) for chat attachment links
This would make the browser use the same protocol and same domain name as the one where App is loaded from

Instead of saving an html containing <img src="https://{ENV}.com/chat-attachments/{attachment}" /> we should only save the absolute path for the attachment - <img src="/chat-attachments/{attachment}" /> (the / prefix is intentional - denoting an absolute path)

---

More Info

excluding the origin from src (e.g. https://staging.expensify.com) and prefixing the src with a slash/ would resolve the src from the current origin

When I'm on new.expensify.com/r/100 this source <img src="/chat-attachments/..." /> would make the following request

GET https://new.expensify.com/chat-attachments/...

While if I view the same from staging.new.expensify.com/r/100 it would make the following request

GET https://staging.new.expensify.com/chat-attachments/...

Wherever the HTML is generated I think we should skip hardcoding the origin from the source as explained above

Here's more info on how and why that would work: https://developer.mozilla.org/en-US/docs/Learn/Common_questions/What_is_a_URL#absolute_urls_vs._relative_urls

Implicit domain name
This is the most common use case for an absolute URL within an HTML document. The browser will use the same protocol and the same domain name as the one used to load the document hosting that URL.

[mountiny]

Asking in Slack for clarification https://expensify.slack.com/archives/C049HHMV9SM/p1673539365623209?thread_ts=1673289214.225929&cid=C049HHMV9SM

[jatinsonijs]

I think it's ok to have same url for both staging and production as we are already sharing user data between staging and production. And if from backend we remove base url from html it will not work for Android and iOS. we have to explicitly set base url.

[kidroca]

This happens when you upload the attachment on staging and view it on production vice versa, and i don't think this is an issue.

This is inconsistent behavior - if it should be possible to load staging attachments from production and production attachments from staging, why not simply always capture all attachments under https://www.expensify.com - at least all would be prefixed by the same origin URL

[getusha]

That means we have to send the picture both to production and staging to achieve this consistency?

[kidroca]

That means we have to send the picture both to production and staging to achieve this consistency?

We don't need to send to both, if staging can access attachments from www.expensify... and prod can access attachments from the same url, then we can at least only use that one

---

I think attachments are stored in the same place and can be accessed with different urls, like I've pointed out here: https://expensify.slack.com/archives/C049HHMV9SM/p1673541181055489?thread_ts=1673289214.225929&cid=C049HHMV9SM

You can open the sam attachment by changing www.expensify... to staging.expensify...

[kidroca]

For more context, we're trying to change the way we load attachments and start to use an authentication header
HTML images (src attribute) do not support headers, so to load and image we'll have to first make a fetch request which is subject to CORS

When we are on new.expensify.com but try to load something from staging.expensify.com/... a CORS request will be made. Image elements handle this well, but fetch would not expose the underlying blob unless the server responds in a CORS friendly way

So an alternative might be to configure CORS and allow requests from new.staging to www.expensify and the various combinations:

• new.expensify -> www.expensify
• new.expensify -> staging.expensify
• staging.new. -> www.expensify
• staging.new. -> staging.expensify

And keep in mind to update this if in the future attachments can be saved under new.expensify

I think it would be simpler to save the attachment and excluding the origin and allow to load it from all these domains
web would use the current origin, while native apps can use the origin stored in App Config

[kidroca]

After further investigation I found logic inside App that expects attachment URLs to be coming from Config.EXPENSIFY.EXPENSIFY_URL and replace them to be loaded from Config.EXPENSIFY.URL_API_ROOT

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

Lines 38 to 45 in 81855c2

	// Update the image URL so the images can be accessed depending on the config environment
	previewSource = previewSource.replace(
	Config.EXPENSIFY.EXPENSIFY_URL,
	Config.EXPENSIFY.URL_API_ROOT,
	);
	source = source.replace(
	Config.EXPENSIFY.EXPENSIFY_URL,
	Config.EXPENSIFY.URL_API_ROOT,

So it seems the intended way to render user attachments (that were uploaded to Expensify) it so load them from EXPENSIFY.URL_API_ROOT

Of course EXPENSIFY.EXPENSIFY_URL is a constant so it can only match staging or prod so if the attachment urls are stuck with the host they were made on this logic would not always work (and some attachments would not be loaded from URL_API_ROOT when they should have)

Traced the original change to this commit: bcca418

---

There seems to be 3 ways to solve this

1. Handle multiple expensify domains and rewrite the source

(No backend changes - handle more than one origins on the client side)

/**
 * Update the image URL so images can be accessed depending on the config environment
 *
 * @param {String} urlString
 * @returns {*|String}
 */
function mapSource(urlString) {
    const url = new URL(urlString);
    if (/expensify/i.test(url.origin)) {
        return `${Config.EXPENSIFY.URL_API_ROOT}${url.pathname.slice(1)}`;
    }

    return urlString;
}

const previewSource = mapSource(htmlAttribs.src);
const source = mapSource(isAttachment
    ? htmlAttribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]
    : htmlAttribs.src);

2. Always save attachments to www.exepnsify.com

Update backend and the logic here to always expect attachments from this URL
This would keep almost the same logic for ImageRenderer sources

3. Let the backend sort this out

I think it might be best to just handle this on the backend and drop the client logic

Backend flow:

1. Upload attachment request arrives
2. Attachment is saved to storage
3. Absolute path (excluding host) is saved to database (e.g. /chat-attachments/123.../...)

Then when report actions are returned the backend can return them with the abs path
(like originally proposed here: #14227 (comment))

[mountiny]

thanks @kidroca

continuing the discussion in slack as I would love to get more people to agree with this change before updating anything

[kidroca]

Discovered a related problem regarding PDF document previews

If a PDF was uploaded on staging and later viewed on prod, clicking on the attachment would result in an Error

New.Expensify.-.Google.Chrome.2023-01-16.16-19-38.mp4

Clicking the [Download] button would also print the same error, but it would open the link in a new window (it should just download the file instead of opening a window)

[kidroca]

@mountiny
Regarding my comment and the remaining CORS issue, should we open a separate ticket?
Or maybe there's already an existing internal issue?

I don't think I can do anything about it in the scope of the current ticket (needs backend update, can't be fixed with client side changes)

[mountiny]

Yeah I think this might be better to abstract into its own issue. Would you be bale to write down exactly what we need to allow in the servers to avoid the CORS issues? I could try to get someone from infra team involved to help with this.

If you post it here as and issue body, I can create the new issue and take it from there for clarity

[getusha]

@mountiny we want to disable the CORS completely from the client side?

[kidroca]

@mountiny
Links to comments explaining the CORS issue

• [HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from #14227 (comment)
• [HOLD for payment 2023-02-08] [$1000] Backend - Attachment links capture the environment they are from #14227 (comment)

Summary

We are removing the auth token from the URL part of an attachment/image and instead placing it in a HTTP header called X-Chat-Attachment-Token
We're doing this to improve file caching (when the token is part of the URL, the URL changes often and cannot be cached)

As a consequence (for adding this header) browsers start to make preflight (OPTIONS method) requests
(This also applies to Desktop since under the hoods it's using Chromium)

The preflight requests are being made because the API url (we request the image from) is from a different origin
than the place where webapp is hosted
e.g. website is new.expensify.com while API is www.expensify.com

Right now the OPTIONS request, seems unexpected and the API redirects with a response to an error page and we get the following error

Web:

[Error] Preflight response is not successful. Status code: 302
[Error] Fetch API cannot load https://www.expensify.com/chat-attachments/4198796077944938521/w_6f21f50b3af2ee93c4e6e288ba5b986858166367.jpg.1024.jpg due to access control checks.
[Error] Failed to load resource: Preflight response is not successful. Status code: 302 (w_6f21f50b3af2ee93c4e6e288ba5b986858166367.jpg.1024.jpg, line 0)
[Error] Preflight response is not successful. Status code: 302

Desktop:

Access to fetch at 'https://www.expensify.com/chat-attachments/962115596/w_e989bfe450cb8e872f52dac2d082c391f35e2e0a.jpg.1024.jpg' from origin 'http://localhost:8081' 
has been blocked by CORS policy: Response to preflight request doesn't pass access control check: Redirect is not allowed for a preflight request.

What can we do

I think this needs to be planned internally
We can start a conversation in Slack to get a general consensus on how to deal with the CORS issue

Perhaps the simplest thing would be to update the backend to handle cross origin request coming from these origins

• localhost
• desktop app
• staging url
• prod url

Typically servers support a CORS configuration that can specify to allow and properly respond to request (including preflight) from these origin

Alternatively we can avoid CORS altogether if the API is resolved from the same origin where the app is hosted
this can be a simple proxy that takes requests coming to new.expensify.com/api and resolves them from www.expensify.com/api

Perhaps more alternative exist and someone can propose something better

[mountiny]

Thank you @kidroca, I dont think we want to get rid of CORS altogether @getusha

[mountiny]

Created a new issue here, keeping this open for payment to @mananjadhav

[kidroca]

Hey @mountiny I noticed something else while inspecting actual staging & prod environments

URL_API_ROOT is https://www.expensify.com for both staging and prod environments, you can see the same if you put a breakpoint here:

App/src/libs/tryResolveUrlFromApiRoot.js

Lines 22 to 24 in 72a2950

	export default function tryResolveUrlFromApiRoot(url) {
	return url.replace(ORIGIN_PATTERN, Config.EXPENSIFY.URL_API_ROOT);
	}

So attachment requests are always made against https://www.expensify.com, while the expected behavior was that

• staging would make requests against https://staging.expensify.com
• prod would make requests against https://www.expensify.com

The I saw this in .env.staging

App/.env.staging

Line 3 in 8bc6f52

EXPENSIFY_URL=https://www.expensify.com/

URL_API_ROOT resolves from EXPENSIFY_URL, and as we can see it's www.expensify.com even in the staging config

But then I've asked myself "if URL_API_ROOT is not staging.expensify.com, how come the staging app is making COMMAND requests against staging.expensify.com and not www.expensify.com

The answer is here:

App/src/libs/HttpUtils.js

Lines 112 to 114 in 7c53f31

	if (shouldUseStagingServer(stagingServerToggleState)) {
	apiRoot = shouldUseSecure ? CONFIG.EXPENSIFY.STAGING_SECURE_EXPENSIFY_URL : CONFIG.EXPENSIFY.STAGING_EXPENSIFY_URL;
	}

I'm not sure what the reason for this comment

App/src/libs/HttpUtils.js

Lines 11 to 12 in 7c53f31

	// Desktop and web use staging config too so we we should default to staging API endpoint if on those platforms
	const shouldDefaultToStaging = _.contains([CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP], getPlatform());

Why should desktop and web default to staging? IMO they should default to whatever .env.staging says should be the API url

---

All that said, we see that resolving attachments from https://www.expensify.com works on both staging and prod, so I'm not sure if we should do something about this

But if we do make the following a fact

• staging would make requests against https://staging.expensify.com
• prod would make requests against https://www.expensify.com

We'd need to place the same shouldUseStagingServer in tryResolveUrlFromApiRoot.js

I'm not a fan of this, because we already missed it once - it's not obvious unless someone points out that URL_API_ROOT is not really the address that will be used and that we should use shouldUseStagingServer whenever we have such logic

I think this should be redesigned in such a way so that we import URL_API_ROOT or SECURE_API_ROOT and these URLs are already taken care of - they point to staging when they need to

[mountiny]

@kidroca

Why should desktop and web default to staging? IMO they should default to whatever .env.staging says should be the API url

This means we would like web staging and web desktop to point to staging APIs by default, the toggle in settings can change it to production if we want to yes.

I am also not sure if we should actively be fixing this now as it worked like this for a while :D

I am wondering though why the .env.staging has non-staging url defined, not sure.

@kidroca Could we use the shouldUseStagingServer in tryResolveUrlFromApiRoot as well to get around this prblem and fetch from the correct URL?

[mountiny]

These staging/ production things are super annoying especially since native works a bit differently with them too

[kidroca]

Why should desktop and web default to staging? IMO they should default to whatever .env.staging says should be the API url

This means we would like web staging and web desktop to point to staging APIs by default, the toggle in settings can change it to production if we want to yes.

Hmm, I didn't understand the comment like that
What I thought it meant is the default value (for web/desktop) is always staging (even if we're on prod)

Only after, seeing your comment and tracing the code I see that this is meant as the default value for the toggle if Onyx doesn't have any value for it at the moment

And not meaning that by default web/desktop should be using staging api

My question though is more about, what's the problem with the other platforms - why can't we simple use CONFIG as the default value:

Instead of this:

const shouldDefaultToStaging = _.contains([CONST.PLATFORM.WEB, CONST.PLATFORM.DESKTOP], getPlatform());

Have this:

const defaultStagingServerToggleState = CONFIG.IS_IN_STAGING;

---

@kidroca Could we use the shouldUseStagingServer in tryResolveUrlFromApiRoot as well to get around this prblem and fetch from the correct URL?

Inspecting the code, I see the toggle shouldUseStagingServer is something that only affects STAGING where you can toggle between staging api and prod api from settings
This is very similar to a platform check - it's a inline ENV check that alters logic, and as such I think it should be captured in a separate file, loaded only when ENV is staging - the same thing we do for platform specific code

We could apply a workaround and use shouldUseStagingServer in tryResolveUrlFromApiRoot, but I think such code leads to problems. I already missed it entirely here (same as everyone involved in the PR)

IMO instead of imperative checks testing for certain conditions, the logic used here (and other URL dealing places) should be setup in a declarative way where it just says

const apiRoot = getApiRoot(request);

Where getApiRoot is making the necessary checking and returns the staging/prod or secure URLs based on the request

This also solves another problem (at least for me),
I can't trace what logic would ever set the shouldUseSecure to true - I've digged through Apps code and followed usages and just can't find anything that would set that value to true

The flag value is probably based on the command or something like that, this can be made more transparent if the conditions for a secure URL are declared in the same place e.g. use secure API for a list of command names

---

I think we should discuss this on Slack and agree whether we should

• patch this up with another shouldUseStagingServer check
• do nothing
• refactor and create a function like getApiRoot then update usages

I also would like to either continue in a new ticket or increase the scope/payment for the current one
I've applied and accepted a job in Upwork and haven't tracked any of the work done here under my hourly contract

[mountiny]

My question though is more about, what's the problem with the other platforms - why can't we simple use CONFIG as the default value:

Because native apps are only build once using the staging config, then after QA is done, the staging build is pushed to production, but there is no new compilation occurring. Hence for native the Config on production is still staging so that is why we need a different approach for those.

This also solves another problem (at least for me),
I can't trace what logic would ever set the shouldUseSecure to true - I've digged through Apps code and followed usages and just can't find anything that would set that value to true

The flag value is probably based on the command or something like that, this can be made more transparent if the conditions for a secure URL are declared in the same place e.g. use secure API for a list of command names

Yeah I am not sure straight from top of my head which commands use it in NewDot its possible we are deprecating this too and we are moving the secure commands to normal API, it is a relict and it will be deprecated down the line I believe.

I think we should discuss this on Slack and agree whether we should

Would you start that discussion? I think we would like to make sure that if the staging toggle is turned to staging we should resolve the attachments form there too. Declarative way makes sense to me, this part of the code is not super clear because of the platform dependency.

I also would like to either continue in a new ticket or increase the scope/payment for the current one
I've applied and accepted a job in Upwork and haven't tracked any of the work done here under my hourly contract

We can create a new ticket after the discussion and after we decide what is needed, does that work for you?

[mountiny]

@bfitzexpensify would you be able to also pay out @kidroca for this job?

[mountiny]

@bfitzexpensify Bumping this one!

[mountiny]

Created a new ticket for the issue mentioned here

[kidroca]

@mountiny
Thanks for the explanations man, Kudos!

Saw the new ticket
I'll start discussion on Slack and propose to have getApiRoot(), but if we decide something else in the end I'll work with that

[spreadmycode]

Solution:
The following condition should meet for this issue.
Backend and staging.new.expensify.com should be running on the same machine (i.e VPS or EC2 AWS).

1. Backend receive the uploading request from https://new.expensify.com/ and store the files in staging.new.expensify.com 's public folder. The reason why uploaded files should be stored in public folder of staging.new.expensify.com is to allow anyone can see (only read) these uploaded images (or digital assets).

2. Uploaded digital asset's access link (https://staging.new.expensify.com/1.png) will be stored in database as a reference to attachment for the specific chat item. You can check this public URL can access the uploaded asset to load on browser.
   If needs, we should store these files on the specific folder which indicates the unique chat room or some thing like that to avoid overwriting or conflicts. It would be much better for us to create Git ignored folder in public of staging.new.expensify.com base path.

3. Finally, all assets can be fetched from staging.new.expensify.com, not new.expensify.com.

[mountiny]

@spreadmycode thanks for the proposal, can you check issues which have Help Wanted label though, there you can be assigned to work on them, this one is being closed.

I think @bfitzexpensify has paid out what was needed here so closing