Skip to content

Conversation

@foxfire52
Copy link
Contributor

@foxfire52 foxfire52 commented Oct 17, 2024

Issue
Interference API server doesn't support CORS protocol, so it can't handle OPTIONS preflight requests.
Issue #2006 shows a failed fetch request from browser and suggests a possible solution.
This feature is needed because local hosted browser apps can't call the API server, failing with CORS error mentioned in #2006.

Proposed Solution
Implement FastAPI CORSMiddleware handler.

Info: According to FastAPI documentation, if allow_origins is set to ['*'], credentials (Cookies, Authorization headers...) are not allowed, so origins must be specified to comply with fetch() Browser standard.
So I follow this alternative solution Puzzling error with FastAPI's CORS middleware - StackOverflow
and use allow_origin_regex=".*" instead, which sets a correct Access-Control-Allow-Origin header.

@foxfire52 foxfire52 changed the title Add support for CORS preflight OPTION requests (#2006) Add support for CORS preflight OPTIONS requests (#2006) Oct 17, 2024
@github-actions
Copy link

# Review for Pull Request: Add support for CORS preflight OPTIONS requests (#2006)

Hi there!

Thank you for contributing to the project! Your implementation of CORS support through the `FastAPI CORSMiddleware` is a great step forward in addressing the CORS issues raised in issue #2006. 

## What I Liked  
- **Clear Problem Statement**: You effectively outlined the issue with CORS and its impact on local hosted browser apps. This context is helpful for reviewers and future contributors.
- **Proposed Solution**: Using `allow_origin_regex=".*"` is a smart approach to handle the preflight requests while allowing necessary credentials. This solution directly addresses the challenge mentioned in the problem statement.
- **Well-Structured Code**: The diff is clean and easy to follow, making it straightforward to understand the changes you've made.

## Suggestions for Improvement  
- **Documentation**: Consider adding a brief comment above the middleware configuration to explain the rationale for using `allow_origin_regex=".*"` as opposed to listing specific origins. This could help future maintainers understand the choice quickly.
- **Tests**: If possible, it would be beneficial to include some tests that verify the CORS behavior, particularly for OPTIONS requests. This will ensure that the functionality works as intended and will help catch any regressions in the future.

Overall, this is a valuable contribution that will enhance the usability of the API for many developers. Thank you once again for your work!

Best,
g4f copilot  

@xtekky xtekky merged commit 6a1f88e into xtekky:main Oct 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants