refactor: Centralize CORS business logic into Tangle middleware #65
Conversation
maxy-shpfy
left a comment
maxy-shpfy
left a comment
There was a problem hiding this comment.
LGTM, but havent had a chance to test
| # Parse the comma-separated list and strip whitespace from each origin | ||
| allowed_origins = [ | ||
| origin.strip() | ||
| for origin in cors_origins_str.split(",") |
There was a problem hiding this comment.
Do we want to add more sanity-checks (aka valid URLs) since it is a user-input?
There was a problem hiding this comment.
Great idea. I am sure this will help some people.
I have added some validation and useful messages when invalid formats are detected.
| ) | ||
|
|
||
|
|
||
| def setup_global_middleware(app: fastapi.FastAPI) -> None: |
There was a problem hiding this comment.
NIT: No idea what naming conventions usually are, but since the usage is middleware.setup_global_middleware , maybe we simplify to middlware.setup? Not sure what not "global" middleware we may have.
There was a problem hiding this comment.
I think setup is fine for now as well. Thank you!
Until we have a group of middleware to call, I am just calling setup_cors_middleware directly now (1 less function).
morgan-wowk
force-pushed
the
01-08-refactor_centralize_cors_business_logic_into_tangle_middleware
branch
from
3d90d50 to
9d18a3c
Compare
Thanks for reviewing. I have added tests to the PR now as well to verify the middleware is meeting expectations, then will test again on staging with actual requests. |
2 participants
Why is this change needed:
localhost:3000, backend onlocalhost:8000)