-
-
Notifications
You must be signed in to change notification settings - Fork 397
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
fix(ASGI mounts): Prevent accidental scope overrides by mounted ASGI apps #3945
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3945 +/- ##
=======================================
Coverage 98.34% 98.34%
=======================================
Files 347 347
Lines 15727 15743 +16
Branches 1738 1740 +2
=======================================
+ Hits 15467 15483 +16
Misses 124 124
Partials 136 136 ☔ View full report in Codecov by Sentry. |
dumb question maybe but what would happen if you mount a litestar app in a litestar app, won't you end up with the same issue you're fixing for other asgi apps ? |
True, didn't think of that case as it's not something we really advertise and I can't immediately think of a use case.
If we want to handle this one case in particular, we could just "restore" the original |
The use case to is pretty much the same as this one: you mount something you have no control on ? Realistically we'll have more Litestar users mounting FastAPI for the time being but you never know ;) This was just a small remark, every asgi app is a squatter of the same namespace, but I'm all for squatts generally speaking so probably leaving this as is is a good option if ths copy_scope let the user solve the issue, it gives Litestar another edge other probably don't have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent :)
Co-authored-by: Jacob Coffee <[email protected]>
Quality Gate passedIssues Measures |
Documentation preview will be available shortly at https://litestar-org.github.io/litestar-docs-preview/3945 |
When mounting ASGI apps, there's no guarantee they won't overwrite some key in the
scope
that we rely on, e.g.scope["app"]
, which is what caused #3934.To prevent this, I've implemented two things:
app
key, but the more specificlitestar_app
key. I've also added aLitestar.from_scope
method, which can be used to safely access the current app from the sopecopy_scope
to the ASGI route handler, which, when set toTrue
will copy the scope before calling into the mounted ASGI app. This should make things behave more as expected, since it truly give the called app its own environment without causing any side-effects. Since this change might break some things, I've left it with a default ofNone
, which does not copy the scope, but will issue a warning if the mounted app modified it, enabling users to decide how to deal with that situationFixes #3934