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

access-control-allow-methods is "DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT" while only post defined #99

Closed
slavugan opened this issue Apr 10, 2022 · 7 comments
Assignees
Labels
Bug 🐛 This is something that is not working as expected

Comments

@slavugan
Copy link
Contributor

My controller:

class BrandController(Controller):
    path = '/brand'

    @post()
    async def create(self, data: BrandNew, edb: AsyncIOClient) -> str:
        await edb.query("INSERT Brand { name := <str>$name }", name=data.name)
        return 'ok'

On preflight request it returns access-control-allow-methods header equal to "DELETE, GET, HEAD, OPTIONS, PATCH, POST, PUT" but only "OPTIONS, POST" should be there.

@Goldziher
Copy link
Contributor

Thanks for the report.

Can you add a failing test case? Would be great.

@slavugan
Copy link
Contributor Author

Not sure, but I can try.

@Goldziher Goldziher added the Bug 🐛 This is something that is not working as expected label Apr 10, 2022
@Goldziher Goldziher self-assigned this Apr 10, 2022
@slavugan
Copy link
Contributor Author

slavugan commented Apr 10, 2022

Seems like this issue is related to Starlette CORSMiddleware
I've added a discussion in Starlette encode/starlette#1582

@slavugan
Copy link
Contributor Author

About test, you can just update existing one test_setting_cors_middleware by changing this line
assert cors_middleware.allow_methods == ("DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT") 🙂

@Goldziher
Copy link
Contributor

ok, I will check this out during the weekend.

@Goldziher
Copy link
Contributor

So, i tested this as you suggested by adjusting that test. This is working correctly:

def test_setting_cors_middleware() -> None:
    cors_config = CORSConfig(allow_methods=["OPTIONS", "POST"])
    assert cors_config.allow_credentials is False
    assert cors_config.allow_headers == ["*"]
    assert cors_config.allow_methods == ["OPTIONS", "POST"]
    assert cors_config.allow_origins == ["*"]
    assert cors_config.allow_origin_regex is None
    assert cors_config.max_age == 600
    assert cors_config.expose_headers == []

    client = create_test_client(route_handlers=[handler], cors_config=cors_config)
    unpacked_middleware = []
    cur = client.app.middleware_stack
    while hasattr(cur, "app"):
        unpacked_middleware.append(cur)
        cur = cur.app  # type: ignore
    assert len(unpacked_middleware) == 2
    cors_middleware = unpacked_middleware[0]
    assert isinstance(cors_middleware, CORSMiddleware)
    assert cors_middleware.allow_headers == ["*", "accept", "accept-language", "content-language", "content-type"]
    assert cors_middleware.allow_methods == ['OPTIONS', 'POST']
    assert cors_middleware.allow_origins == cors_config.allow_origins
    assert cors_middleware.allow_origin_regex == cors_config.allow_origin_regex

Note that ['OPTIONS', 'POST'] are used everywhere.

Now, I understand what the issue is as far as you are concerned. You want the allowed methods to be based not on a global configuration but be per endpoint. I am afraid this is not possible using the middleware pattern because middleware is unaware of the endpoints. While it might be possible to modify this, it would be a complex architectural change to do.

The alternative approaches you can take are to use either a dependency that receives some parameters, or a decorator that takes the route handler and does something similar. These you will need to create on your end and if you think they are generic enough, you might add a PR to suggest them.

Inside Starlite the only possibility is to create a CORS layer separate from the middleware that is able to parse individual route_handlers and respond. This though is not a small change and will require substanital work. If you want to give it a try, you are welcome to do so.

@slavugan
Copy link
Contributor Author

There is work going in this direction in Starlette encode/starlette#1286, encode/starlette#685, so I think it is better to wait.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Development

No branches or pull requests

2 participants