Fix CORS error for query-craft-frontend-758178119666#33
Fix CORS error for query-craft-frontend-758178119666#33
Conversation
- Move cloud_origins and cloud_origin_regex to module scope in backend/main.py - Ensure failing origin is in allow_origins list - Add CORSLoggingMiddleware to backend/common/middleware.py - Register CORSLoggingMiddleware in backend/main.py (outermost) to debug missing headers Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideAdjusts CORS configuration initialization order and adds logging middleware to detect missing CORS headers for Cloud Run origins, specifically covering the failing frontend origin. Sequence diagram for HTTP request flow with CORSLoggingMiddlewaresequenceDiagram
actor Browser
participant FrontendService
participant BackendService
participant CORSLoggingMiddleware
participant CORSMiddleware
participant ExceptionHandlingMiddleware
participant FastAPIApp
Browser->>FrontendService: HTTPS request
FrontendService->>BackendService: HTTPS request with Origin header
BackendService->>CORSLoggingMiddleware: Incoming request
activate CORSLoggingMiddleware
CORSLoggingMiddleware->>CORSMiddleware: call_next(request)
activate CORSMiddleware
CORSMiddleware->>ExceptionHandlingMiddleware: call_next(request)
activate ExceptionHandlingMiddleware
ExceptionHandlingMiddleware->>FastAPIApp: call_next(request)
activate FastAPIApp
FastAPIApp-->>ExceptionHandlingMiddleware: Response
deactivate FastAPIApp
ExceptionHandlingMiddleware-->>CORSMiddleware: Response
deactivate ExceptionHandlingMiddleware
CORSMiddleware-->>CORSLoggingMiddleware: Response with CORS headers
deactivate CORSMiddleware
CORSLoggingMiddleware->>CORSLoggingMiddleware: Check Origin header
CORSLoggingMiddleware->>CORSLoggingMiddleware: Check AccessControlAllowOrigin header
alt Origin present and AccessControlAllowOrigin missing
CORSLoggingMiddleware->>Logger: warning CORS header missing
end
CORSLoggingMiddleware-->>BackendService: Final response
deactivate CORSLoggingMiddleware
BackendService-->>FrontendService: HTTP response
FrontendService-->>Browser: HTTP response with CORS headers
Updated class diagram for CORSLoggingMiddlewareclassDiagram
class BaseHTTPMiddleware {
<<external>>
}
class CORSLoggingMiddleware {
+dispatch(request, call_next)
}
CORSLoggingMiddleware --|> BaseHTTPMiddleware
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
CORSLoggingMiddleware, you recreate the logger on every request; consider instantiatinglogger = get_logger("backend.middleware.cors")once at module scope and reusing it insidedispatchto avoid per-request overhead. - Since
CORSLoggingMiddlewareis intended to diagnose missing CORS headers, you might want to log additional context such as request method and query string (e.g.,request.method,request.url.query) to make the log entries more actionable when debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CORSLoggingMiddleware`, you recreate the logger on every request; consider instantiating `logger = get_logger("backend.middleware.cors")` once at module scope and reusing it inside `dispatch` to avoid per-request overhead.
- Since `CORSLoggingMiddleware` is intended to diagnose missing CORS headers, you might want to log additional context such as request method and query string (e.g., `request.method`, `request.url.query`) to make the log entries more actionable when debugging.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Refactor `tests/test_cors_config.py` to use `mock.patch.dict` and `importlib.reload` instead of setting `os.environ` globally, preventing `ENV='production'` from leaking into other tests. - Update `tests/test_integration.py` to defensively set `ENV='development'` using `mock.patch.dict` in `test_db_config_loads` and `TestPostgresIntegration` to ensure robustness against environment changes. - This resolves the `ValueError: POSTGRES_DSN is required in production environment` error in CI. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
This PR addresses the CORS policy error reported for
https://query-craft-frontend-758178119666.us-central1.run.app.Changes:
cloud_originsandcloud_origin_regexto the top ofbackend/main.py(beforelifespan) to ensure they are properly defined and initialized.cloud_originslist.CORSLoggingMiddlewareinbackend/common/middleware.pyand registered it as the outermost middleware inbackend/main.py. This middleware logs a warning if a request has anOriginheader but the response lacks theAccess-Control-Allow-Originheader, aiding in future debugging.Verified with
tests/test_cors_config.py.PR created automatically by Jules for task 11874747566118330029 started by @KnellBalm
Summary by Sourcery
Resolve CORS header issues for Cloud Run frontend origins and add logging to diagnose future CORS problems.
Bug Fixes:
Enhancements: