Skip to content

Make CORSMethodMiddleware actually make sense #476

@fharding1

Description

@fharding1

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 the OPTIONS 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 an OPTIONS matcher the middleware will only set the header on non-OPTIONS requests (which is invalid) and it will say that there is an OPTIONS method when there isn't. If there is an OPTIONS matcher it will return an Access-Control-Allow-Methods with two OPTIONS (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.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions