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

refactor: CORS handling #3395

Closed
wants to merge 4 commits into from
Closed

refactor: CORS handling #3395

wants to merge 4 commits into from

Conversation

peterschutt
Copy link
Contributor

@peterschutt peterschutt commented Apr 16, 2024

Description

Applies three refactors to our CORS handling:

  1. Move pre-flight response generation from the generated OPTIONS handlers to the CORS middleware.
  2. Make CORS outermost middleware.
  3. Deprecate CORS middleware from public API.

Fixes case where pre-flight response not applied to mounted ASGI apps. Previously, we would only generate OPTIONS handlers for HTTP routes, not mounts, so pre-flight requests would be passed through to the mounted ASGI app.

Fixes the case where a user defined OPTIONS handler may not include CORS logic. In fact, this is they only way we can be sure that we honor CORS pre-flight if the config object is provided.

Remove ExceptionHandlerMiddleware's dependence on CORSMiddleware by making CORSMiddleware the outermost middleware in the stack if the app has CORS configured. This makes responses sent via the exception middleware pass through the CORS middleware send wrapper if CORS is configured, which is behaviourally equal to the current behavior of wrapping the send coro inside the exception handler middleware with the CORS middleware send wrapper.

Closes

Move pre-flight response generation from the generated OPTIONS handlers to the CORS middleware.

Fixes case where pre-flight response not applied to mounted ASGI apps.

Fixes the case where a user defined OPTIONS handler may not include CORS logic. In fact, this is they only way we can be sure that we honor CORS pre-flight if the config object is provided.
@github-actions github-actions bot added the area/middleware This PR involves changes to the middleware label Apr 16, 2024
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.27%. Comparing base (a0d1109) to head (17c3960).

❗ Current head 17c3960 differs from pull request most recent head 2d95328. Consider uploading reports for the commit 2d95328 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3395      +/-   ##
==========================================
- Coverage   98.28%   98.27%   -0.01%     
==========================================
  Files         323      323              
  Lines       14772    14765       -7     
  Branches     2346     2343       -3     
==========================================
- Hits        14518    14511       -7     
  Misses        116      116              
  Partials      138      138              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Remove `ExceptionHandlerMiddleware`'s dependence on `CORSMiddleware` by making `CORSMiddleware` the outermost middleware in the stack if the app has CORS configured.

This makes responses sent via the exception middleware pass through the CORS middleware send wrapper if CORS is configured, which is behaviourally equal to the current behavior of wrapping the `send` coro inside the exception handler middleware with the CORS middleware send wrapper.
@peterschutt peterschutt changed the title test: for OPTIONS requests on asgi handler. refactor: CORS handling Apr 16, 2024
@peterschutt peterschutt marked this pull request as ready for review April 16, 2024 05:27
@peterschutt peterschutt requested review from a team as code owners April 16, 2024 05:27

await send(message)

return wrapped_send

def create_preflight_response(self, origin: str, request_headers: Headers) -> Response[str | None]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to be private?

Copy link
Contributor Author

@peterschutt peterschutt Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an interesting question, because while this class is public interface, there isn't really any way for users to customize the way it works. So IMO this whole class should be private.

Prior to this PR, there needed to be a CORSConfig passed to the application object in order for pre-flight responses to be generated (this is generally still the case after this PR, but the refactors supported by this PR would allow the middlware to be added to the application, without the config object also being added to the application):

cors_config = scope["app"].cors_config
request_headers = Headers.from_scope(scope=scope)
origin = request_headers.get("origin")
if cors_config and origin:

There is no way to only supply the CORSMiddleware to a subset of routes via a Router.middleware or similar, because of the fact that the pre-flight response generation is conditional upon the application having access to the CORSConfig object. In other words, if a user didn't supply the CORSConfig object to the application, but did supply DefineMiddleware(CORSMiddleware, config=CORSConfig(...)) to a router, then the pre-flight responses for that router wouldn't have worked anyway because there's no CORSConfig on the app. And if the CORSConfig is added to the app, then there is no point adding the middleware to a sub-router, because we'd then apply it to the whole application anyway.

Also, the CORSMiddleware class is hard coded as a wrapper around the ASGIRouter:

litestar/litestar/app.py

Lines 842 to 844 in a0d1109

asgi_handler: ASGIApp = self.asgi_router
if self.cors_config:
asgi_handler = CORSMiddleware(app=asgi_handler, config=self.cors_config)

There is no way for a user to sub-class the middleware to alter its behavior, as there is no way to pass that sub-classed middleware to us.

So IMO, CORSMiddleware is implementation detail - the whole thing should be undocumented and we should probably deprecate it from public use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@litestar-org/maintainers, can I deprecate CORSMiddleware from our public interface based on ⬆️?

Copy link

Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3395

@peterschutt
Copy link
Contributor Author

Splitting this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/middleware This PR involves changes to the middleware pr/internal size: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants