-
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
OPTIONSmethod matcher exists for the route - Set
Access-Control-Allow-Methodson 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
OPTIONShandlers for this middleware to work at all, and they might want to do other things in theOPTIONShandler (such as set other CORS headers), it doesn't make sense to return - Do not append
OPTIONSto the allowed methods. This is currently a bug. If there isn't anOPTIONSmatcher the middleware will only set the header on non-OPTIONSrequests (which is invalid) and it will say that there is anOPTIONSmethod when there isn't. If there is anOPTIONSmatcher it will return anAccess-Control-Allow-Methodswith 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.
Reactions are currently unavailable