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

Cors middleware does not send Allow Origin Headers in OPTIONS request #538

Closed
mmatthebi opened this issue Sep 15, 2022 · 8 comments
Closed
Labels
bug Something isn't working duplicate This issue or pull request already exists feature Code based project improvement

Comments

@mmatthebi
Copy link

Introduced with revision b986d1e

Test program:

#include "Crow/include/crow/middlewares/cors.h"
#include "crow.h"

crow::response crypto_put(const crow::request& req) {
    std::cout << "Crypto PUT" << std::endl;
    return crow::response(200, "PUT");
}

crow::response crypto_get(const crow::request& req) {
    std::cout << "Crypto GET" << std::endl;
    return crow::response(200, "GET");
}



int main() {
    crow::App<crow::CORSHandler>* app = new crow::App<crow::CORSHandler>{};
    auto& cors = app->get_middleware<crow::CORSHandler>();

    cors.global()
        .methods("OPTIONS"_method, "POST"_method, "GET"_method, "PUT"_method)
        .prefix("/").origin("*");


		CROW_ROUTE((*app), "/crypto").methods(crow::HTTPMethod::PUT)(crypto_put);
		CROW_ROUTE((*app), "/crypto").methods(crow::HTTPMethod::GET)(crypto_get);
		app->port(8000);
		app->bindaddr("0.0.0.0");
		app->signal_clear();

                app->run();
		//boost::thread(boost::bind(&crow::App<crow::CORSHandler>::run, app));
		return 0;
}

then compiling with

g++ main.cpp -ICrow/include -lpthread -o main

and executing a curl command

curl -v -XOPTIONS localhost:8000/crypto

I receive these outputs:

# Revision b986d1e38a853277bc31e4b628e0b7160c0af4e1
*   Trying 127.0.0.1:8000...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8000 (#0)
> OPTIONS /crypto HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 204 No Content
< Allow: OPTIONS, HEAD, GET, PUT
< Content-Length: 0
< Server: Crow/master
< Date: Thu, 15 Sep 2022 06:46:49 GMT
< 
* Connection #0 to host localhost left intact
# Revision 5f49f9ad (which is git checkout b986d1e38a853277bc31e4b628e0b7160c0af4e1^)
*   Trying 127.0.0.1:8000...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8000 (#0)
> OPTIONS /crypto HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.68.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 204 No Content
< Access-Control-Allow-Headers: *
< Access-Control-Allow-Methods: *
< Access-Control-Allow-Origin: *
< Allow: OPTIONS, HEAD, GET, PUT
< Content-Length: 0
< Server: Crow/master
< Date: Thu, 15 Sep 2022 06:46:59 GMT
< Connection: Keep-Alive
< 
* Connection #0 to host localhost left intact

As visible, the Allow-Origin-Headers are not sent in the OPTIONS request. The bug still exists in current master.

@dranikpg
Copy link
Member

Hi! Indeed, Crow currently doesn't run middleware for OPTIONS/HEAD requests.

Briefly, middleware runs for handlers. If there is no handler, then no middleware is run

@dranikpg
Copy link
Member

@The-EDev: What do you think? What would be the best way of handling OPTIONS without breaking backwards compatibility.

Maybe we can run local & global middleware if it has some option like IGNORE_CORS = false. We'd run local middleware only if the handler is registered with a OPTIONS method, but we would not invoke the handler itself... Or maybe we should invoke it?

In any case, running marked global middleware would be simple enough to be quickly implemented and solve this issue.

Currently Crow just builds the options response manually by allowing all

@kiner-shah
Copy link
Contributor

kiner-shah commented Jun 20, 2023

@dranikpg @The-EDev are there any updates about this? This seems to be quite an important change. If there is any REST API written using Crow that only needs to accept requests from a certain origin, then it will not work since the browsers will never receive those headers in the response.
In fact, I need this fix for a project that I am working on. Or maybe anyone can suggest some workaround for this until a fix is ready?

@The-EDev what do you think of @dranikpg's suggestion?

@itz-arnav
Copy link

I'm facing the same issue with a code:

``
#include "crow.h"
#include "crow/middlewares/cors.h"
#include

std::map<std::string, std::string> users;

int main() {
users["abc"] = "123";
users["def"] = "234";

crow::App<crow::CORSHandler> app;

auto& cors = app.get_middleware<crow::CORSHandler>();
cors
    .global()
    .origin("*") // Allow all origins to access your API
    .methods("POST"_method, "OPTIONS"_method)
    .headers("Content-Type");

CROW_ROUTE(app, "/api/login")
    .methods("OPTIONS"_method)
    ([] {
        crow::response res;
        res.set_header("Access-Control-Allow-Origin", "*");
        res.set_header("Access-Control-Allow-Methods", "POST");
        res.set_header("Access-Control-Allow-Headers", "Content-Type");
        res.end();
        return res;
    });


CROW_ROUTE(app, "/api/login")
    .methods("POST"_method)
    ([&](const crow::request& req) {
        crow::response res;
        res.set_header("Access-Control-Allow-Origin", "*");

        auto x = crow::json::load(req.body);
        if (!x) {
            res.code = 400;
            res.body = "Bad request";
            return res;
        }
        std::string username = x["username"].s();
        std::string password = x["password"].s();

        if(users.find(username) == users.end()) {
            res.code = 404;
            res.body = "User does not exist";
            return res;
        }

        if(users[username] != password) {
            res.code = 401;
            res.body = "Invalid password";
            return res;
        }

        res.code = 200;
        res.body = "User logged in successfully";
        return res;
    });


app.bindaddr("127.0.0.1").port(5500).multithreaded().run();

}
``
Any idea how to fix this please? I keep getting CORS error on my browser.

@kiner-shah
Copy link
Contributor

kiner-shah commented Jun 23, 2023

I managed to do a HACK to get this working.

Observations:

  • For requests other than OPTIONS and HEAD, in routing.h inside handle_initial(), it first tries to find the route in the Trie.
  • A valid route will have a non-zero rule index. Since there is no explicit route defined for OPTIONS for a given URL, for OPTIONS the rule index will always be zero (For example, if there is a route for POST method /write, then there is no OPTIONS method present for /write because of which it can't file the rule and rule index is zero).
  • For OPTIONS, res.end() is called after adding "Allow" headers, this prevents adding any more data to response.

Changes inside Crow: crow_hack.patch
Changes inside code using Crow: Added a OPTIONS method explicitly to get this working.

crow::App<crow::CORSHandler> app; //define your crow application

// Customize CORS
auto &cors = app.get_middleware<crow::CORSHandler>();

// clang-format off
cors
  .global()
    .methods("POST"_method, "GET"_method)
  .prefix("/")
    .origin("origin-urls-you-wish-to-allow")
    .allow_credentials();
// clang-format on

CROW_ROUTE(app, "/write")
    .methods(crow::HTTPMethod::OPTIONS)
([](const crow::request& req) {
    return crow::response(crow::status::OK);
});

CROW_ROUTE(app, "/write")
    .methods(crow::HTTPMethod::GET)
([](const crow::request& req) {
    CROW_LOG_INFO << "Sending response";
    return crow::response(crow::status::OK, "This is a response");
});

This needs to be done for all route URLs probably. Can get a bit redundant, but seems to be working.

kiner-shah added a commit to kiner-shah/mykmoments that referenced this issue Jun 23, 2023
@whaliendev
Copy link

Any progress on this issue now?

@broose-goose
Copy link

Just tried the patch from @kiner-shah and can confirm it's working great! Luckily, I only need this functionality for development, but it would feel super bad to need this for a production build...

@gittiver gittiver added bug Something isn't working feature Code based project improvement labels Jan 3, 2024
@gittiver
Copy link
Member

probably a duplicate of #764 764

@gittiver gittiver added the duplicate This issue or pull request already exists label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists feature Code based project improvement
Projects
None yet
Development

No branches or pull requests

7 participants