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

Crow does not reply to browser preflight messages correctly when using CORs middleware #721

Closed
agribov opened this issue Nov 9, 2023 · 15 comments
Labels
bug Something isn't working

Comments

@agribov
Copy link

agribov commented Nov 9, 2023

Hello,
I set up a project with CrowCpp using the CORs middleware, and found an issue with the OPTIONS reply. When using CORs in some modern browsers, the browser sends a preflight OPTIONS message before sending the actual request (it doesn't do this with "simple" requests, such as GET, but does with some other request types). It expects the response to contain a CORs header, but with CrowCpp, the response does not have CORs header even when using the CORs middleware. I took a look at the code in routing.h, and saw that the OPTIONS and HEAD responses go through their own special pathways, and seemingly do not get affected by the middleware, so they don't get the CORs header added on to the response.

As a test, I added the CORs headers manually underneath else if (req.method == HTTPMethod::Options), and after that my browser allowed the frontend to communicate with the CrowCpp backend. Though the actual solution would probably be more complicated (and involve running the OPTIONS replies through the middleware).

I believe this was brought up previously in issue #417. Though it's marked as resolved, it does seem a code change is still needed to fix this.

Mozilla info about preflight requests, and how they should be responded to
https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request

@gittiver gittiver added the bug Something isn't working label Jan 3, 2024
@u7i
Copy link

u7i commented Jan 3, 2024

I've been exploring this issue for some time. The first idea that comes to my mind is to add CORS support directly into the server (omitting the middleware). Due to the unique nature of preflight requests, it would be impossible to send OPTION requests through middleware without breaking its functionality.

@KkemChen
Copy link

KkemChen commented Apr 3, 2024

Hello, I set up a project with CrowCpp using the CORs middleware, and found an issue with the OPTIONS reply. When using CORs in some modern browsers, the browser sends a preflight OPTIONS message before sending the actual request (it doesn't do this with "simple" requests, such as GET, but does with some other request types). It expects the response to contain a CORs header, but with CrowCpp, the response does not have CORs header even when using the CORs middleware. I took a look at the code in routing.h, and saw that the OPTIONS and HEAD responses go through their own special pathways, and seemingly do not get affected by the middleware, so they don't get the CORs header added on to the response.

As a test, I added the CORs headers manually underneath else if (req.method == HTTPMethod::Options), and after that my browser allowed the frontend to communicate with the CrowCpp backend. Though the actual solution would probably be more complicated (and involve running the OPTIONS replies through the middleware).

I believe this was brought up previously in issue #417. Though it's marked as resolved, it does seem a code change is still needed to fix this.

Mozilla info about preflight requests, and how they should be responded to https://developer.mozilla.org/en-US/docs/Glossary/Preflight_request

Did you solve it temporarily by adding directly in

else if (req.method == HTTPMethod::Options)
/* .... */
res.set_header("Access-Control-Allow-Origin", "*");
res.set_header("Access-Control-Allow-Headers", "*");
res.set_header("Access-Control-Allow-Methods", "*");

I tried to add this code directly here, but when I used postman to make an options request, the socket hungup directly.😥

@rohitsutreja
Copy link

It is working fine in august 2022 release, but not in latest release.

@gittiver
Copy link
Member

gittiver commented Apr 6, 2024

I think this was already fixed: #764
Would you give the master branch a try?

IIRC this fix was after the release.

If you can confirm that it works on master, then we could prepare a new 1.1.x release.

@rohitsutreja
Copy link

I tried on master branch, it is working fine.

However it is not setting allow_credential flag to true in the response to option request even if I have set it via global prefix.

Other headers are applied successfully.

@witcherofthorns
Copy link
Contributor

@rohitsutreja hi, here is my CORS in main.cpp, version crow 1.1.0 from Conan, maybe this will help you

cors.global()
.origin("http://localhost:8080")  //frontend host
.allow_credentials()
.headers(
    "Accept",
    "Origin",
    "Content-Type",
    "Authorization",
    "Refresh"
)
.methods(
    crow::HTTPMethod::GET,
    crow::HTTPMethod::POST,
    crow::HTTPMethod::OPTIONS,
    crow::HTTPMethod::HEAD,
    crow::HTTPMethod::PUT,
    crow::HTTPMethod::DELETE
);

Anyway it works, but I had problems setting max_age(int value) to cache OPTIONS queries, don't know if this may have already been fixed

@TheDelta
Copy link

TheDelta commented May 8, 2024

Just ran into this issue with v1.1.0 and #771 fixes it for me. (using latest master)
A new version would be great ❤️

Edit: using max_age() on cors will remove for some reason the headers attribute, despite following the Warning in the example (defining headers).

@witcherofthorns
Copy link
Contributor

Hi @TheDelta, yes, it's true, i think i wrote about this earlier, but it seems it still hasn't been fixed 👀
But, unfortunately, I don’t know when this will be fixed by someone, I wanted to go back to this moment myself and fix it, but I don’t have much time for this yet

@witcherofthorns
Copy link
Contributor

CORS in Crow is implemented correctly, I tested it again on a real frontend and Postman, Crow returns all headers correctly, but the browser rejects "Access-Control-Allow-Origin" if "allow_credentials" is enabled - please keep this in mind, most of the problems are related to the web frontend

@TheDelta
Copy link

v1.1.0 works fine if I do a direct GET request with postman.
I have a third party frontend which does an OPTION preflight request and this one won't work with v1.1.0 because of the missing fix from #771
Without this fix, the headers are missing in the OPTION response, which will fail the preflight and therefore the GET request.

So a new version (like 1.1.1) with the fix would be much appreciated.

@gittiver
Copy link
Member

New Version 1.2.0 added which contains all fixes from master.

@maurocolella
Copy link

maurocolella commented Jul 10, 2024

This is not resolved. Whether using the CORS middleware or a custom endpoint for the OPTIONS method, if a response is served, it does not contain the custom headers. Whether specified exactly or using widlcard. Furthermore, in many cases, I have seen socket hangups, again only with the OPTIONS method.

The POST version of the same endpoint works as intended.

I have had to revert to v1.0 for my own purposes.

@dr3mro
Copy link

dr3mro commented Jul 22, 2024

Same issue with me!

@gittiver
Copy link
Member

Can you provide exactly what does not work , maybe with a sample to reproduce?

@dr3mro
Copy link

dr3mro commented Jul 22, 2024

 curl -X OPTIONS  http://localhost:8080/v1/hello -i
HTTP/1.1 204 No Content
Access-Control-Allow-Headers: Content-Type, Accept-Encoding, Origin, Access-Control-Request-Method
Access-Control-Allow-Methods: GET, OPTIONS
Allow: OPTIONS, HEAD, GET
curl -X GET  http://localhost:8080/v1/hello -i
HTTP/1.1 200 OK
Access-Control-Allow-Headers: Content-Type, Accept-Encoding, Origin, Access-Control-Request-Method
Access-Control-Allow-Origin: *
Access-Control-Max-Age: 3600
Access-Control-Allow-Methods: GET, OPTIONS
Content-Length: 36
Server: Valhalla
Date: Mon, 22 Jul 2024 15:09:41 GMT
Connection: Keep-Alive

{
"Message" : "Welcome to ASGARD."
}%
#pragma once

#include "crow/middlewares/cors.h"
#include <crow.h>

class CORSHandler {
public:
    CORSHandler(crow::CORSHandler& corsHandler)
    {
        corsHandler.global()
            .origin("*")
            .methods(crow::HTTPMethod::GET, crow::HTTPMethod::OPTIONS)
            .headers("Content-Type", "Accept-Encoding", "Origin", "Access-Control-Request-Method")
            .max_age(3600);

        corsHandler.prefix("/v1/patient")
            .origin("*")
            .methods(crow::HTTPMethod::GET, crow::HTTPMethod::POST, crow::HTTPMethod::DELETE, crow::HTTPMethod::PATCH, crow::HTTPMethod::SEARCH, crow::HTTPMethod::OPTIONS)
            .headers("Content-Type", "Accept-Encoding", "Origin", "Access-Control-Request-Method", "Authorization")
            .max_age(7200)
            .allow_credentials();

        corsHandler.prefix("/v1/user")
            .origin("*")
            .methods(crow::HTTPMethod::GET, crow::HTTPMethod::POST, crow::HTTPMethod::OPTIONS)
            .headers("Content-Type", "Accept-Encoding", "Origin", "Access-Control-Request-Method", "Authentication")
            .max_age(7200)
            .allow_credentials();
    }
    ~CORSHandler() = default;
};

option method does not return Access-Control-Allow-Origin: "*"
but other method works just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants