-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Comments
cc @kidroca |
Triggered auto assignment to @johnmlee101 ( |
👋 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 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. |
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. |
Can you detail the pipeline for the request ideally and what is currently happening now?
And where is it being rejected? Is it the fact its making an expensify.com request from a new.expensify.com source? |
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 To see how the backend reacts to an OPTIONS request (like the one used in preflight) right now you can make the following request
|
Request to GET an attachment
Due to the difference in Origin and the target URL (and the presence of a header) browsers initiate a preflight request like: Preflight Request
It seems the backend doesn't expect an OPTIONS request for Response for the OPTIONS request
The browser considers this a non OK response and doesn't proceed with the initial GET request
Yes that's the reason access control checks (CORS) are being made - www.expensify.com and |
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 |
Okay a few things: So when I run an OPTIONS network request from staging.new.expensify.com from the console I get 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. |
I'm not making the request from localhost Here's an example from the browser console (on https://new.expensify.com/) If we call 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('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 But when a header is involved, browsers first make OPTIONS request to ask the backend whether they can use The backend (currently) responds with a redirect to an HTML page to the OPTIONS request
Even if you use a valid token the OPTIONS request results in a redirect (add
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 |
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. |
When you call |
IDK maybe it doesn't include cookies for CORS requests as a precaution 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 |
This happens because no valid authToken is passed with the request btw |
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 |
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 |
Browsers would not add body or headers (or cookies) to the OPTIONS request being made Preflight requests simply ask the server "what are the options for this resource, can I use this header" It might be possible to return different OPTIONS for the same resource account IDs and any other information can be passed over the actual request, once the browser is satisfied by the OPTIONS response |
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. |
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. |
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. |
@kidroca & @johnmlee101 maybe? |
Bump guys to maybe furnish Justin with a summary on why we ended up pausing to tackle the server migration project first! |
Hey @trjExpensify, everybody Here's a summary of our discussion so far: Problem: The domain Details:
Potential Solutions:
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. |
It sounds like this is the main issue. Is there an example request, perhaps via cURL I can use to reproduce this? |
Check out this console log capture:
|
Keep in mind that CORS is particular to browsers and The OPTIONS request should be a plain |
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? |
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 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" |
It should no longer be a problem |
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! |
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? |
I'm always hoping for sooner 😆 ! Jests aside, somewhere in the ballpark of |
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. |
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. |
I submitted a PR to get this into staging/production. Once deployed, maybe someone here can help test this out again and help validate. |
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:
Response:
|
Niceeeee! @Beamanator @mollfpr perhaps for a second confirmation? |
bump @Beamanator |
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! |
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. |
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 iswww.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:
Desktop:
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
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 fromwww.expensify.com/api
Perhaps more alternative exist and someone can propose something better
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: