-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Closed
Labels
Description
I was the one that added CORSMethodMiddleware
, and I feel bad about it looking back, because it's implementation is terrible. I didn't understand enough about CORS—just had some half-baked idea about something that might be cool and decided to PR it. It's understandably caused confusion. I would like to change it in the following ways so it's actually useful:
- Only set the header if an
OPTIONS
method matcher exists for the route - Set
Access-Control-Allow-Methods
on all requests (not just preflight, see https://stackoverflow.com/questions/24264574/cors-headers-present-only-on-preflight-or-every-request) - Do not return after setting the header. Since people need to create their own
OPTIONS
handlers for this middleware to work at all, and they might want to do other things in theOPTIONS
handler (such as set other CORS headers), it doesn't make sense to return - Do not append
OPTIONS
to the allowed methods. This is currently a bug. If there isn't anOPTIONS
matcher the middleware will only set the header on non-OPTIONS
requests (which is invalid) and it will say that there is anOPTIONS
method when there isn't. If there is anOPTIONS
matcher it will return anAccess-Control-Allow-Methods
with twoOPTIONS
(not really invalid, but bad) - Vastly improve the documentation to document all this behavior
- Add more (better documented) tests
- Add an example test
I believe making these changes would both make the middleware more intuitive and practical to use. It is a fairly breaking change but I think it's worth it since the current behavior is practically useless.