Conversation
- Added `CORSLoggingMiddleware` to `backend/common/middleware.py` to log CORS failures and origins. - Registered `CORSLoggingMiddleware` in `backend/main.py`. - This change will trigger a redeployment to ensure the latest CORS configuration (which correctly includes the user's origin) is active. - Verified locally with reproduction scripts and existing tests. 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 GuideAdds a dedicated CORS logging middleware to detect responses missing CORS headers when an Origin is present, and wires it into the FastAPI app to aid debugging CORS issues after redeploying the backend. Sequence diagram for HTTP request flow with new CORSLoggingMiddlewaresequenceDiagram
actor Client
participant FastAPIApp as FastAPIApp
participant CORSMiddleware as CORSMiddleware
participant CORSLoggingMiddleware as CORSLoggingMiddleware
participant Endpoint as EndpointHandler
Client->>FastAPIApp: HTTP request
activate FastAPIApp
FastAPIApp->>CORSLoggingMiddleware: dispatch(request)
activate CORSLoggingMiddleware
CORSLoggingMiddleware->>CORSMiddleware: call_next(request)
activate CORSMiddleware
CORSMiddleware->>Endpoint: call_next(request)
activate Endpoint
Endpoint-->>CORSMiddleware: response
deactivate Endpoint
CORSMiddleware-->>CORSLoggingMiddleware: response with CORS headers
deactivate CORSMiddleware
CORSLoggingMiddleware->>CORSLoggingMiddleware: check origin and access_control_allow_origin
alt Origin present and CORS header missing
CORSLoggingMiddleware->>Logger_backend_middleware_cors: warning CORS Missing Header with origin and path
end
CORSLoggingMiddleware-->>FastAPIApp: response
deactivate CORSLoggingMiddleware
FastAPIApp-->>Client: HTTP response
deactivate FastAPIApp
Class diagram for new CORSLoggingMiddlewareclassDiagram
class BaseHTTPMiddleware {
<<abstract>>
+dispatch(request, call_next) async
}
class CORSLoggingMiddleware {
+dispatch(request, call_next) async
}
class Request {
+headers
+url
}
class Response {
+headers
}
class Logger {
+warning(message)
}
CORSLoggingMiddleware --|> BaseHTTPMiddleware
CORSLoggingMiddleware ..> Request : uses
CORSLoggingMiddleware ..> Response : uses
CORSLoggingMiddleware ..> Logger : logs via_get_logger
CORSLoggingMiddleware : +origin
CORSLoggingMiddleware : +dispatch(request, call_next) async
CORSLoggingMiddleware : origin = request.headers.get(origin)
CORSLoggingMiddleware : response = await call_next(request)
CORSLoggingMiddleware : if origin and no access_control_allow_origin
CORSLoggingMiddleware : logger = get_logger(backend.middleware.cors)
CORSLoggingMiddleware : logger.warning(CORS Missing Header ...)
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:
- Consider creating the
backend.middleware.corslogger at module level instead of insidedispatchto avoid re-instantiating it on every request. - You may want to include additional context (e.g., HTTP method, status code, and request headers like
Access-Control-Request-Method) in the CORS warning log to make diagnosing intermittent CORS issues easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider creating the `backend.middleware.cors` logger at module level instead of inside `dispatch` to avoid re-instantiating it on every request.
- You may want to include additional context (e.g., HTTP method, status code, and request headers like `Access-Control-Request-Method`) in the CORS warning log to make diagnosing intermittent CORS issues easier.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Fixed `tests/test_integration.py` to properly mock the environment during testing, resolving the CI failure that prevented deployment. - Added `CORSLoggingMiddleware` to `backend/common/middleware.py` and registered it in `backend/main.py` to provide visibility into CORS rejections. - Ensured `backend/main.py` has a clean definition of allowed origins, explicitly including the reported Cloud Run domain. - Verified imports in middleware to prevent runtime errors. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
This PR addresses the reported CORS issue by adding a logging middleware to debug future occurrences and forcing a redeployment of the backend service. The investigation confirmed that the reported origin IS present in the configuration, suggesting the deployed version might be stale. This commit ensures the correct configuration is deployed and provides better visibility into CORS rejections.
PR created automatically by Jules for task 8522996915867058504 started by @KnellBalm
Summary by Sourcery
Add middleware to log missing CORS response headers for requests with an Origin header and register it in the FastAPI application to aid debugging of CORS issues.
Enhancements: