[HOLD for payment 2023-04-04] Attachments (w/o preview: mp4, docx etc) use Prod domain in case of download

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Issue found when executing PR #15178

Action Performed:

1. Prod side: upload attachment without preview docx. mp4 etc to the chat
2. On staging click on the download button
3. check from which domain the file is download

Expected Result:

All attachment URLs to be resolved relative to current environment

Actual Result:

If file without preview like docx or mp4 sent from the Prod and it will be downloaded from the Staging via www.expensify.com
domain instead of staging.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.87.0

Reproducible in staging?: Yes

Reproducible in production?: yes

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: Any additional supporting documentation

Bug5985447_Screen_Recording_2023-03-20_at_19.21.21.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

@kidroca Can you have a look at this one? This came from testing your PR, thanks!

[kidroca]

Proposal

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

Downloading attachments with no preview is not respecting the API in use - being on staging would load an attachment from the prod URL (when the attachment was originally uploaded on prod)

What is the root cause of that problem?

When we upload an attachment the backend saves the address including the host where the attachment was originally uploaded from (uploading an attachment from prod, saves the prod address to the attachment)

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

1. (backend) Don't save the host which uploaded the attachment, only the path for the attachment relative to API root
   • ❌ www.expensify.com/chat-attachments/{path}
   • ✔️ /chat-attachments/{path}
2. (backend) Migrate existing attachment URLs - remove the host portion from them
3. (app) App logic can prefix attachments with the current API

What alternative solutions did you explore? (Optional)

Because of Old Dot, a migration would probably not be possible ATM (migration would break Old Dot)

We have reusable logic that remmaps hosts to the current environment

App/src/libs/tryResolveUrlFromApiRoot.js

Lines 17 to 30 in 44f9871

	/**
	* When possible this function resolve URLs relative to API ROOT
	* - Absolute URLs like `/{path}`, become: `https://{API_ROOT}/{path}`
	* - Similarly for prod or staging URLs we replace the `https://www.expensify`
	* or `https://staging.expensify` part, with `https://{API_ROOT}`
	* - Unmatched URLs (non expensify) are returned with no modifications
	*
	* @param {String} url
	* @returns {String}
	*/
	export default function tryResolveUrlFromApiRoot(url) {
	const apiRoot = ApiUtils.getApiRoot({shouldUseSecure: false});
	return url.replace(ORIGIN_PATTERN, apiRoot);
	}

We can use it in order to substitute the API portion of the address with the API we're currently using, by updating the code here

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

Line 27 in 743ae89

const attrHref = htmlAttribs.href || '';

With something like

const attrHref = ApiUtils.tryResolveUrlFromApiRoot(htmlAttribs.href);

---

The ideal solution would be if we don't have to do anything with the URL at all
If the API and the website were hosted from the same origin a path like /chat-attachments/{path} would be resolved from {origin}/chat-attachments/{path}

• might track this, because there's a possibility we'd serve api from the same origin: Allow for preflight requests with OPTIONS method #14991 (comment)

The next best thing is, manually prefixing with the current api
{API_ROOT}/chat-attachments/{path}

And 3rd, the only thing possible ATM is, replacing the origin dynamically
{PROD_OR_STAGING/chat-attachments/{path} -> {API_ROOT}/chat-attachments/{path}

[mountiny]

Lets fix this @kidroca feel free to create a PR

[kidroca]

BTW the issue is not tied to Desktop - it affects all platforms, though I don't think QA would be able to detect this on mobile

[mountiny]

Makes sense, updated the title and issue body

[mountiny]

@kidroca Feel free to make a PR for this!

[kidroca]

Created a PR, ready for review, I'll be able to add test steps and screens on Monday

[kidroca]

PR updated and ready for review

[kidroca]

✅ PR was merged last week

[parasharrajat]

@maddylewis Please move this back to Weekly and assign me as C+ as I reviewed the PR. Also, the header did not update to reflect payments, etc.

Thanks.

[mountiny]

@maddylewis could you please pay @parasharrajat $1000 for his work on this issue, thank you very much!

[maddylewis]

on it!

[maddylewis]

paid!

[parasharrajat]

Question: Is this Internal or external @mountiny? Or this was handled as regression.

If this is external, is it valid for a bonus?

[mountiny]

I would say this was handled as a regression/ oversight since we implemented these changes before and this hsould have been done in the original PR

Are we good to close?