Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for preflight requests with OPTIONS method #14991

Closed
mountiny opened this issue Feb 9, 2023 · 68 comments
Closed

Allow for preflight requests with OPTIONS method #14991

mountiny opened this issue Feb 9, 2023 · 68 comments
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Reviewing Has a PR in review

Comments

@mountiny
Copy link
Contributor

mountiny commented Feb 9, 2023

Links to comments explaining the CORS issue

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

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a085d801f68f616f
  • Upwork Job ID: 1628430561761431552
  • Last Price Increase: 2023-02-22
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Feb 9, 2023
@Expensify Expensify unlocked this conversation Feb 9, 2023
@mountiny
Copy link
Contributor Author

mountiny commented Feb 9, 2023

cc @kidroca

@MelvinBot
Copy link

Triggered auto assignment to @johnmlee101 (ring0), see https://stackoverflow.com/c/expensify/questions/6102 for more details.

@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Feb 9, 2023
@mountiny
Copy link
Contributor Author

mountiny commented Feb 9, 2023

👋 I have created this ring0 issue since we will have to update the way we handle requests with attachments so we dont run into CORS issues. I dont have much knowledge on how this can be updated in our backend, I think we need to allow for the OPTIONS request with the X-Chat-Attachment-Token in headers but dont know much about how to execute it.

Happy to learn and help you here! Making this issue Weekly as its not a blocker right now but we have some PRs which will need to be on hold until this will be enables in backend.

Thanks! Also @kidroca is probably best to explain the issue if there is anything not clear from the OP.

@mountiny
Copy link
Contributor Author

mountiny commented Feb 9, 2023

I think these are the related issues which will require this to be updated:

cc @Beamanator as well since you have worked closely with Kidroca on this.

@johnmlee101
Copy link
Contributor

Can you detail the pipeline for the request ideally and what is currently happening now?
Kind of like

Source: localhost
GET
Url: www.expensify.com/somepath
Headers:
    xxxx: xxxx
Body:
    xxx:xxx:

And where is it being rejected?

Is it the fact its making an expensify.com request from a new.expensify.com source?

@kidroca
Copy link
Contributor

kidroca commented Feb 9, 2023

This PR changes the way we load attachments using header for web/desktop and can be used to recreate the CORS issue

You can easily recreate the problem from the Desktop app

Recreating the problem for web is a bit more tricky as you have to host or simulate hosting the app on a staging URL
The requests against the API are still considered CORS because the staging app is hosted on staging.new.expensify.com while the API the requests are targeting is staging.expensify.com
Here's what I did to resolve the hostname new.expensify.com from my local server: #13036 (comment)
(Thought this only works in Safari)

To see how the backend reacts to an OPTIONS request (like the one used in preflight) right now you can make the following request

curl -X OPTIONS \
https://www.expensify.com/chat-attachments/4198796077944938521/w_6f21f50b3af2ee93c4e6e288ba5b986858166367.jpg.1024.jpg \
--dump-header -

@kidroca
Copy link
Contributor

kidroca commented Feb 9, 2023

@johnmlee101

Can you detail the pipeline for the request ideally and what is currently happening now?

Request to GET an attachment
Origin: new.expensify.com
Method: GET
URL: www.expensify.com/chat-attachments/{path}.jpg
Headers:
  - X-Chat-Attachment-Token: {TOKEN_VALUE}

Due to the difference in Origin and the target URL (and the presence of a header) browsers initiate a preflight request like:

Preflight Request
Origin: new.expensify.com
Method: OPTIONS
URL: www.expensify.com/chat-attachments/{path}.jpg
Headers:
  - access-control-request-headers: x-chat-attachment-token
  - access-control-request-method: GET
  - origin: http://new.expensify.com
  - referer: http://new.expensify.com/

It seems the backend doesn't expect an OPTIONS request for /chat-attachments/{path}.jpg and responds with a redirect to an error page

Response for the OPTIONS request
Status Code: 302
RESPONSE HEADERS
  - content-type: text/html; charset=UTF-8
  - server: cloudflare
  - location: https://www.expensify.com?exitTo=%2Fchat-attachments%2F4613573186077378841%2Fw_8c1e154469ef960408a0d7d2d6ce682fbe4e189d.jpg.1024.jpg

The browser considers this a non OK response and doesn't proceed with the initial GET request

Is it the fact its making an expensify.com request from a new.expensify.com source?

Yes that's the reason access control checks (CORS) are being made - www.expensify.com and new.expensify.com are not the same origin and requests between them are subject to CORS

@johnmlee101
Copy link
Contributor

johnmlee101 commented Feb 9, 2023

Ah cool I see what you mean. So the issue is that specific path doesn't support this bridge via OPTIONS, thanks for breaking that out for me

@johnmlee101
Copy link
Contributor

Okay a few things:
We currently set the CORS policy for known hosts like new.expensify.com and staging.new.expensify.com, not localhost so that's why we're hitting that.

So when I run an OPTIONS network request from staging.new.expensify.com from the console I get
access-control-allow-origin: https://staging.new.expensify.com in the response, which should be expected. However since I'm not passing a proper authToken its doing a 302 redirect to www.expensify.com which responds in CORS (as expected).

Are you suggesting we implement full OPTIONS support with our resources to determine if CORS is allowed with certain requests? We don't currently support that in any capacity and would require more work to implement.

@kidroca
Copy link
Contributor

kidroca commented Feb 9, 2023

We currently set the CORS policy for known hosts like new.expensify.com and staging.new.expensify.com, not localhost so that's why we're hitting that.

I'm not making the request from localhost

Here's an example from the browser console (on https://new.expensify.com/)

image

If we call fetch with a header, an OPTIONS request is being made (and stuff breaks)

fetch('https://www.expensify.com/chat-attachments/5307596164054552186/w_32652718d4f32c75792c468e94404d4eeb603bb6.pdf', {
  headers: {
    'X-Chat-Attachment-Token': 'aub12umOOqee9bbyQcLFXqVgyizNIT%2FRe2CGqgQ%2Bpu58NFMZ%2Faba3POWx9Bv04gApkd3aBnkoQowQ54iIDcqQu9szKyQA%2BXZpz4vkPLmYl%2FmYb2lEUAvRzzNuWPNGBOEsBTxWa7%2Bn5Dkk9J7JxzEgdgyegbiT1qrlbgjnkWKmYVgV9PLf4jKOcg%2FVMRqTA5h3kvA2CKgsoHVrc%2Ba2IiLzh6%2F%2FLPE1LVRVrGF5ar3QKZXb1jviLOq%2BwdVUOQJiaEWeKMatoxUPoa6RiRnp0hfcsTN2XGUEwl6iPq9OfbKtq1bx5DtToD%2FjMXqCok%2FGEsV5k%2F81pickHzmT%2BgokWSwvemtE%2FdTalz8ghVTQmm616LLTsRJBFh4liD6bjn3vd5qHJrlgKZHlGWeQO54AKYVdDKOuuZPKmuWEjUMyOmY8uyl%2Fr1%2FY2J5Z8%2FHGUWfzfwtCNAYE6c3infaT7YUo0sHHmoDSSIVciSDWo6TJwPFAQTCbIKRnYEk3Z6rkZ9NdjryQQSqgcS6wNXu4hrsrmls41ds7cB49DCwknlweCIxXTnB2wUNY46dw7l84HfFqwpps3pi6W7sQKkbfu5euUPXoRijWkBnLPjvVJoARoi9ewCB70jBvoq4hyfjEgOFFL1GvMopAg1MeMqOPIKZW%2F7Kx3mYG51alGJ9v01hfbz%2Boaq%2BDTJuRsfiXqGRfA3g2QARCG1LhfZMIpBIRu1PmRGYuzxOycA%2FAIiY2BC1O6WHlhLE1Ix6FVZXyuK41cIpOJBBzwQdHgMIvfJ5M%2BTtOTcFwEhQ54rRWZRjF1c%2BSZe2u0av%2FNqCnb2M3pabMjQPtE7%2BtyJ3%2FxewZBBATlSg8QBkdkKIkj7J1c8ylrR30h0A%2FtH8FMp1K2x%2FekksCICfiGD5XQbCfeFOBGgGbF5MAGQhQCzRk4ivfZvCRknoT5G%2FyoZSu0KUA9ETx%2BYXsqFNhSPr%2FGD48umfYsONz3rVa8X8lQ%3D%3D'
  }
})

If we instead call fetch without a header (the old way)

fetch('https://www.expensify.com/chat-attachments/5307596164054552186/w_32652718d4f32c75792c468e94404d4eeb603bb6.jpg.1024.jpg?encryptedAuthToken=AP8vz1B3TKytIYZ0BcuegJCDyD68Iqlf1jY3NHHp296%2B4mAPvRCW1MpRmlmnkXnWN9q4cxwYkJN%2FTJ%2FBdmqVQDxhetkoMOvqgs6txk6AzGVRmraXcQLpSoyAgyQ%2FV0nsX0%2BLKZdSuN%2BjX%2Bn8EkBJAi0wcuXKa9ZH0LNoAmASbook9PdCn7LJS2Gx5CoPogB4Y6%2BgCFcvMvgYgFIH18d0UoosIqG8IAhAW%2BABj20K4JBBR59G8%2FhKFyrefFKyoD%2BMcc47wJKNsa4wkkapQH1bsLpf%2Bro4pORXlQtjYtyX2vHxvPXtA%2B6lRqU9pX10ekQ1nkwpKOmYSyLhd%2FB%2BzV2BDGKbonpfOMDp4dT93wJy7FC%2BPxersm4BKnkiv8v0bTsa3muDOJNcRffQ8Wf%2FesJECLkYJhLaC2Uajf4EXhKsehLTrSrsaikbxiv9mihaMVLopIHaVUWm5dmrQAh%2BeHe%2Bu7gv8OcqAvmsZ0f4xZLqPTVBTjgT8PUZfU3FHbEAvfMz66hIuFNyllK986GquSapVEmqeCLGP3k7Syd4118CMqXZHxJIwKS4IAxGQJXcn4VoeR5e3plxDk%2BuhThR4GRX5V4WVoGaM%2B4xFlICSPcHThZ5KxDkRGkNUbpbTRWvzEeyc6Gt%2FKz1sqGr1S9T7uicfmuUNiDX5br9yE20ETQ2ERylwQyAFp96PV0O9taHfqn1frYvOSB%2Bx%2BPtPWFUNOoYKTw%2FAjhDclgc5o1jvmWiPk3Qc1LNfQk6lPXLsS8FbweeFAnJx%2BK4CRXWa8ynCdA7MoIOmQMwoX%2FRHd3A14zzpDmvhALZnz5YBGNUTwCRrevsECwNGDxcWDKsGglMzhL4Yd%2FmBeFUjFgJYVS0w0juJiSxVHHe5hQeSo7k9nzQKBHSZ7F60BA2DSsPZvFnpnldPA3XfA6gogWS2B%2FseJU%2BvqrERUsRhB0bZoqoFiaNy2AC%2FnsYNLaV0Gut2Sum6FW%2FxQ%3D%3D')

It's considered a SIMPLE CORS request - no preflight request is being made. The browser uses the GET response and searches for access-control-allow-origin on the response headers satisfying the CORS requirement
The backend responds correctly for this case

But when a header is involved, browsers first make OPTIONS request to ask the backend whether they can use x-chat-attachment-token in a follow-up CORS request
(access-control-request-headers: x-chat-attachment-token)

The backend (currently) responds with a redirect to an HTML page to the OPTIONS request
IMO OPTIONS requests cannot be answered with a redirect to a page - we're not responding to a human that can interpret the page, but to a machine that needs to know whether it can use the requested options or not

So when I run an OPTIONS network request from staging.new.expensify.com from the console I get
access-control-allow-origin: https://staging.new.expensify.com in the response, which should be expected. However since I'm not passing a proper authToken its doing a 302 redirect to www.expensify.com which responds in CORS (as expected).

Even if you use a valid token the OPTIONS request results in a redirect (add method: 'OPTIONS' to the above code sample), but browsers would not add the custom header (and it's value) to the OPTIONS request anyway - if we send the header it defeats the purpose of the options request
Browser uses OPTIONS to ask whether it can use a certain header(s) to make a GET request to for a certain resource

Are you suggesting we implement full OPTIONS support with our resources to determine if CORS is allowed with certain requests? We don't currently support that in any capacity and would require more work to implement.

Only for the resources that expect/allow requests from different origins

Usually CORS is configured on the server setup level and it's relatively easy to manage
But if the backend setup does not allow to change this easily you can bypass CORS altogether by making the API accessible from the same origin - the proxy idea
request to the API are made against the same origin, but a proxy server resolves them from the API. We do a similar thing for localhost development where a proxy server runs and channels requests

@johnmlee101
Copy link
Contributor

Gotcha, I've never done non-simple requests so that makes sense. And this is all coming from the need to add the custom header for caching.

@johnmlee101
Copy link
Contributor

When you call fetch manually it doesn't include your own authToken in the standard cookie request, right?

@kidroca
Copy link
Contributor

kidroca commented Feb 9, 2023

When you call fetch manually it doesn't include your own authToken in the standard cookie request, right?

IDK maybe it doesn't include cookies for CORS requests as a precaution
AFAIK you can tell fetch to send or not send cookies, but you can't select which ones


BTW the proxy idea is not about bypassing CORS, but to make requests use the same origins without CORS restrictions or the overhead of a preflight request
If the request is not coming from an allowed origin the proxy can reject it, or redirect it to the original API (and let it get a CORS error)

@johnmlee101
Copy link
Contributor

The backend (currently) responds with a redirect to an HTML page to the OPTIONS request

This happens because no valid authToken is passed with the request btw

@johnmlee101
Copy link
Contributor

So if we can somehow pass the authToken in the right parameter we expect (plus other base necessary cookies) I feel like we could simulate a request properly and the preflight would just work, since the redirect is basically seeing that the user is making an unauthenticated request outright. Alternatively we could try and fetch the x-chat-attachment-token and do some replace on the api side to then use that as the token itself

@johnmlee101
Copy link
Contributor

But in that case we'll still want to send the accountID and email for the account its associated with. There's no hard rules in our backend preventing this from happening, but it just doesn't like that this is an unauthenticated request without the expected token

@kidroca
Copy link
Contributor

kidroca commented Feb 9, 2023

The backend (currently) responds with a redirect to an HTML page to the OPTIONS request

This happens because no valid authToken is passed with the request btw

Browsers would not add body or headers (or cookies) to the OPTIONS request being made
They would not add the x-chat-attachment-token value
We don't have any control over the OPTIONS (preflight) request, can't alter it in any way

Preflight requests simply ask the server "what are the options for this resource, can I use this header"
Auth state shouldn't matter for the answer to this question
E.g. the OPTIONS request is supposed to be safe, and not include sensitive data, passing the token value as part of the request is a violation of that spec

It might be possible to return different OPTIONS for the same resource /chat-attachments/{path} based on context
But I don't see are reason to need authentication to get the OPTIONS for that particular resource

account IDs and any other information can be passed over the actual request, once the browser is satisfied by the OPTIONS response

@justinpersaud
Copy link
Contributor

Is this still important to do? The migration project has been de prioritized lately. If it is, happy to help come up with an alternate solution so we aren't waiting for a long time.

@trjExpensify
Copy link
Contributor

It would be great to finish the image improvements once and for all, the remainder of which as I understand it is blocked on image caching as the source of the problems left to fix from the OP here.

If you have an alternative to the migration project to unblock that it sounds good to me, though I'm sure others on this issue might have something to share on where we got to with exploring alts before.

@justinpersaud
Copy link
Contributor

There's a lot to read through. Maybe someone can summarize? It seems like we just aren't supporting OPTIONS method at the chat attachments path, or is there more to it? How does moving NewDot to our own environment help this? My gut has me thinking we can solve this via Cloudflare request transformations or our worker that we have setup?

If someone wants to catch me up here I can probably come up with a solution.

@trjExpensify
Copy link
Contributor

@kidroca & @johnmlee101 maybe?

@trjExpensify
Copy link
Contributor

Bump guys to maybe furnish Justin with a summary on why we ended up pausing to tackle the server migration project first!

@kidroca
Copy link
Contributor

kidroca commented Oct 12, 2023

Hey @trjExpensify, everybody

Here's a summary of our discussion so far:

Problem: The domain new.expensify.com is attempting to fetch chat attachments from www.expensify.com but encounters CORS (Cross-Origin Resource Sharing) issues.

Details:

  • Our goal is to shift attachment authentication to a custom header, eliminating the need to append a token to the URL. This change allows for more efficient caching since the URL remains consistent.
  • However, when new.expensify.com initiates a GET request to www.expensify.com for chat attachments, browsers trigger a preflight OPTIONS request. This is due to differing origins and the introduction of the custom header.
  • The server's reaction to the OPTIONS request is a 302 redirect. Since the browser sees this as a non-OK response, the initial GET request is halted.
  • To elaborate, the server/attachments endpoint isn't set up to handle the preflight OPTIONS request. Instead, it perceives it as an error and redirects to an error page.
  • Browsers differentiate between www.expensify.com and new.expensify.com, which results in the CORS (OPTIONS) checks.

Potential Solutions:

  1. CORS Config: Servers or routes usually allow for basic CORS configurations in place. This supports the utilization of specific headers, origins, and other CORS-related elements.
  2. Proxy Server: We could establish a straightforward proxy server that presents both the App and the API from an identical origin. When both entities operate from the same origin, CORS are notbeing made.
  3. Custom Caching: We could develop a unique caching method, potentially necessitating distinct implementations across platforms (e.g., web vs. native). This method poses challenges like manual cache invalidation and eviction. However, the server code would remain unchanged.

From my perspective, the custom header strategy appears to be the most beneficial for inherent caching. Nevertheless, if the architectural obstacles are sizable, it might be wise to consider alternative solutions.

@justinpersaud
Copy link
Contributor

The server's reaction to the OPTIONS request is a 302 redirect. Since the browser sees this as a non-OK response, the initial GET request is halted.

It sounds like this is the main issue.

Is there an example request, perhaps via cURL I can use to reproduce this?

@kidroca
Copy link
Contributor

kidroca commented Oct 12, 2023

Is there an example request, perhaps via cURL I can use to reproduce this?

Check out this console log capture:

image

  1. Open a chat with attachment

  2. Select the image and inspect the html to see the src attribute

  3. Move the auth token from the url query params to headers like the sample fetch call bellow

       fetch(`https://www.expensify.com/chat-attachments/${reportId}/${attachmentPath}`, {
         headers: {
           'X-Chat-Attachment-Token': authToken,
         },
       })

    I guess you can even use made up attachmentPath and X-Chat-Attachment-Token because they would trigger an OPTION request and the request will fail, the actual attachment doesn't yet metter at that point it's all about the OPTIONS request

@kidroca
Copy link
Contributor

kidroca commented Oct 12, 2023

perhaps via cURL I can use to reproduce this?

Keep in mind that CORS is particular to browsers and cURL won't initiate an OPTIONS request, browsers do that as a precaution before the actual request, cURL and other utils just perform the actual request

The OPTIONS request should be a plain GET OPTIONS https://www.expensify.com/chat-attachments/${reportId}/${attachmentPath} for the resource (I guess you can run that in cURL)

@justinpersaud
Copy link
Contributor

Question: would this still be a problem if the chat attachments were served off the new.expensify.com domain instead of www.expensify.com? I think that is what is presenting the problem in the browser right? We can do some sever/cloudflare magic to manipulate the hostname and have the attachments be accessible off the NewDot domain as well if that would help?

@kidroca
Copy link
Contributor

kidroca commented Oct 12, 2023

I've just tried it, now and I no longer get a CORS error, neither an OPTION request is being made, but instead of getting an attachment I'm receiving an html document with Expensify App's logo

From origin https://staging.new.expensify.com/

var response await fetch('www.expensify.com/chat-attachments/5720532303966235030/w_862e89b6f6f0afeec9a6986a366c786aa4ef17d7.png', {
    headers: {
        'X-Chat-Attachment-Token': 'HznVDJAe%2Be8ta%2FjAePLypOqkQtxmk5SOR6bN0OEU%2F6kvfmg5%2BvpfqeV9K%2BbuqADS7OLULKrcVx7iIX5XrYp1vpVYiKpNKSlxyFORgB1DgLaFHas58cKmyllrIBR4T9hQXm98ywDLiGTqKt1WgcLT2vu3FUeA3FBpj7s%2FcHc9wYIdaro89Mx5qaO9xa9hrh2kXWEqRzX19PLtRUzqLPvjcb8qWzeyPNotjMb2rrdMcA2XEQYgqJ3baezC8TaR5vzihoXJTsTCmaX7gtFfDvnL7nboFdrYE2lUewuGkutr%2BKw7cmRxUYYlWh%2F9TIbBIRG6RNZirAZFvJee8t1UEAwev78zs%2BW5RMHpf2su4sei4fqndq0G1NrWIiINnJj7h2xwqychUqQ5pmmEbJ80Hj4PT9WTfCOGRCaZD0JhK3cofqA95i6a7pph8wQXC7Emahs%2BZ%2B71USBRQ6yy1PnARQsl3j1fRgq1Db1iAJipYJEBV0co2zHmGQQydcBhsYJLsuo95QrL9Gia5csNrjs12MRvKwASnv1XVdLXlqlUBFAWG6j4Ly%2BKh4jGARPSgZTixVa14BF2SlOW83byqr4UaRFApjpnAim64%2B4j2HltONEr1x4d6d8JtUcb%2BosAbAy3506ofUM02vGuXXDB256e23ig%2BwQeN9N4dLZMfWQcZyGpiFG19DcEzrRgZ%2BT3FkO9WmTOW1IHuSL3Bd%2BfIZEatOXipRdwO5Kt1erdHkkrnhARuxpDDCU%2BzUPuxLqzQPbaTwIHOfm6XB8bVYIB9kWQByjQ9vmh6pI%2BRp%2BzT15uMpn92wUHI89ZrFOjV3bV1324fNTsRdGQHOvRGIHQpxjcVdZLnMKLokyFFpJx0SsYQT%2BpxlfucEv2Mhnkz%2FWmIc7WGdZY1P%2FwbA%2BU0nDux3VqZESyxAiRZw1ZeX0MVvCzL2z3%2Bj%2Fekab8SmB2gxqhGVEk55JIhxPx38Qe29wvDELvheQPxWrEV0ShnZaGV9xIaKNrubRGZVi7%2FSkaFjT%2B05j6PeTP'
    }
})

console.log(response.status) // 200
console.log(response.headers.get('content-type')) // text/html

For reference in the past this used to result in a preflight OPTIONS request and then an error: #14991 (comment)

Now it seems the request is re-routed to the path on the same origin, which does not result in CORS error, but also doesn't return the attachment, but a document like "One moment while we connect you to Expensify"

@kidroca
Copy link
Contributor

kidroca commented Oct 12, 2023

Question: would this still be a problem if the chat attachments were served off the new.expensify.com domain instead of www.expensify.com?

It should no longer be a problem
When both entities operate from the same origin, CORS request are not being made.
But this means staging should serve attachments from staging and new should serve attachments from new

@trjExpensify
Copy link
Contributor

Thanks, @kidroca for the overview. Super helpful! @justinpersaud, do you a better idea of where we're at now and any suggestions for our next steps? Thanks!

@justinpersaud
Copy link
Contributor

Yeah, I have some ideas. Let me put them together and run them by some other infra folks to validate them. What's the priority on this? Is it still monthly or are you hoping for sooner?

@justinpersaud justinpersaud self-assigned this Oct 20, 2023
@trjExpensify
Copy link
Contributor

I'm always hoping for sooner 😆 ! Jests aside, somewhere in the ballpark of weekly to monthly sounds about right. It's not attached to a wave, but it unblocks fixing our image upload experience, which isn't really premium as we gear up to get existing customers migrated across.

@justinpersaud
Copy link
Contributor

Got it. I was able to reproduce the issue using @kidroca's steps. I have an idea on how to solve this and posted it for some feedback https://expensify.slack.com/archives/C094TGUTZ/p1698098986137179

I will try and test it out locally tomorrow and see if it works.

@justinpersaud
Copy link
Contributor

I have a working fix for this in my dev environments

https://github.com/Expensify/Expensidev/pull/809

Once reviewed/approved, I'll work on getting it out to staging/production to test further.

@justinpersaud
Copy link
Contributor

I submitted a PR to get this into staging/production. Once deployed, maybe someone here can help test this out again and help validate.

@justinpersaud justinpersaud changed the title [HOLD Server Migration E/E #277714] Allow for preflight requests with OPTIONS method Allow for preflight requests with OPTIONS method Oct 26, 2023
@justinpersaud
Copy link
Contributor

This is working in staging and production now using the example @kidroca provided.

Can anyone else help confirm and see if we can close this?

Request:

OPTIONS /chat-attachments/8923768874934211380/w_2bbeb8dd4d2843915d9bb8c6c0e085c14dfa8225.png.1024.jpg HTTP/2
Host: www.expensify.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/118.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Access-Control-Request-Method: GET
Access-Control-Request-Headers: x-chat-attachment-token
Referer: https://new.expensify.com/
Origin: https://new.expensify.com
Connection: keep-alive
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: same-site
TE: trailers

Response:

HTTP/2 204 
date: Thu, 26 Oct 2023 14:19:29 GMT
access-control-allow-origin: https://new.expensify.com
access-control-allow-methods: GET, OPTIONS
access-control-allow-headers: X-Chat-Attachment-Token
cf-cache-status: DYNAMIC
set-cookie: __cf_bm=LiO2iiAyH5JuJoh0KH2wVGGlZru0oDRx2b1SAOJzhFk-1698329969-0-AciTxSxU2tcuHgwnMVBDsIO4KLiIrfky8/J4Br4MACcI+cicej0bsangkTWXiE1n4tuwEhU9CHB5Pi/1zLEoGLM=; path=/; expires=Thu, 26-Oct-23 14:49:29 GMT; domain=.expensify.com; HttpOnly; Secure; SameSite=None
set-cookie: _cfuvid=mLPpEYXWmCK8lOb6vUCUZrFu7pwfDkdIbfLI7MqE3.w-1698329969788-0-604800000; path=/; domain=.expensify.com; HttpOnly; Secure; SameSite=None
strict-transport-security: max-age=31536000; includeSubDomains; preload
server: cloudflare
cf-ray: 81c355a679c939c3-YYZ
cf-team: 1c0df3dc08000039c35c1cd400000001
X-Firefox-Spdy: h2

@justinpersaud justinpersaud added the Reviewing Has a PR in review label Oct 26, 2023
@trjExpensify
Copy link
Contributor

Niceeeee! @Beamanator @mollfpr perhaps for a second confirmation?

@justinpersaud
Copy link
Contributor

bump @Beamanator

@Beamanator
Copy link
Contributor

Looking at where we left off in this PR, I think the purpose of this issue has been completed so let's close! We'll continue the App work / discussion in #12603. Thanks @justinpersaud & everyone involved!

Copy link

melvin-bot bot commented Dec 7, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

9 participants