Add CORSLoggingMiddleware and verify Cloud Run origins#34
Add CORSLoggingMiddleware and verify Cloud Run origins#34
Conversation
- Added `CORSLoggingMiddleware` to `backend/common/middleware.py` to log CORS success/failure. - Updated `backend/main.py` to register `CORSLoggingMiddleware` as the outermost middleware (so it sees headers added by inner middlewares like `CORSMiddleware`). - Explicitly defined and cleaned `cloud_origins` list in `backend/main.py` to ensure the problematic origin is correctly included. - Added `tests/test_cors_logging.py` to verify CORS behavior and logging. 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 GuideIntroduces a CORSLoggingMiddleware to log CORS success/failure, refactors CORS origin configuration in backend/main.py so Cloud Run origins are defined before app creation and ensures middleware ordering, and adds tests to validate allowed/disallowed origins and logging behavior in production mode. Sequence diagram for CORSLoggingMiddleware around CORSMiddlewaresequenceDiagram
actor Browser
participant FastAPIApp
participant CORSLoggingMiddleware
participant ExceptionHandlingMiddleware
participant CORSMiddleware
participant Endpoint
Browser->>FastAPIApp: HTTP request with Origin header
FastAPIApp->>CORSLoggingMiddleware: dispatch(request)
CORSLoggingMiddleware->>ExceptionHandlingMiddleware: call_next(request)
ExceptionHandlingMiddleware->>CORSMiddleware: call_next(request)
CORSMiddleware->>Endpoint: call_next(request)
Endpoint-->>CORSMiddleware: response
CORSMiddleware-->>ExceptionHandlingMiddleware: response with ACAO header
ExceptionHandlingMiddleware-->>CORSLoggingMiddleware: response
CORSLoggingMiddleware->>CORSLoggingMiddleware: read origin and ACAO header
alt ACAO present
CORSLoggingMiddleware->>Logger: info CORS Success
else ACAO missing
CORSLoggingMiddleware->>Logger: warning CORS Failed
end
CORSLoggingMiddleware-->>FastAPIApp: response
FastAPIApp-->>Browser: response with CORS headers
Class diagram for new CORSLoggingMiddleware and related middlewareclassDiagram
class BaseHTTPMiddleware {
<<abstract>>
+dispatch(request, call_next) async
}
class ExceptionHandlingMiddleware {
+dispatch(request, call_next) async
}
class CORSLoggingMiddleware {
+dispatch(request, call_next) async
-log_cors_success(origin, acao)
-log_cors_failure(origin, status_code)
}
BaseHTTPMiddleware <|-- ExceptionHandlingMiddleware
BaseHTTPMiddleware <|-- CORSLoggingMiddleware
class FastAPIApp {
+add_middleware(middleware_class)
+middleware_stack
}
FastAPIApp ..> ExceptionHandlingMiddleware : uses
FastAPIApp ..> CORSLoggingMiddleware : uses
class CORSMiddleware {
+dispatch(request, call_next) async
+allow_origins
+allow_origin_regex
+allow_credentials
+allow_methods
+allow_headers
}
FastAPIApp ..> CORSMiddleware : uses
class CORSConfig {
+cloud_origins : list
+cloud_origin_regex : str
}
CORSConfig ..> CORSMiddleware : configures
CORSLoggingMiddleware ..> Logger : logs
class Logger {
+info(message)
+warning(message)
}
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,get_loggeris called on every request; consider instantiating the logger once at module or class level to avoid repeated lookups on the hot path. - Logging every successful CORS request at INFO may generate a lot of noise in production; consider downgrading success logs to DEBUG or adding sampling/feature flags so that high-volume paths remain manageable.
- The
cloud_origin_regexis defined near the top ofbackend/main.py; ensure it is actually wired into theCORSMiddlewareconfiguration (e.g., viaallow_origin_regex) or remove it to avoid confusion about unused CORS rules.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CORSLoggingMiddleware`, `get_logger` is called on every request; consider instantiating the logger once at module or class level to avoid repeated lookups on the hot path.
- Logging every successful CORS request at INFO may generate a lot of noise in production; consider downgrading success logs to DEBUG or adding sampling/feature flags so that high-volume paths remain manageable.
- The `cloud_origin_regex` is defined near the top of `backend/main.py`; ensure it is actually wired into the `CORSMiddleware` configuration (e.g., via `allow_origin_regex`) or remove it to avoid confusion about unused CORS rules.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Updated `tests/test_cors_logging.py` to use `mock.patch.dict` and `importlib.reload` instead of setting `os.environ` globally, preventing environment pollution. - Updated `tests/test_integration.py` to defensively set `ENV='development'` using `mock.patch.dict` for tests that check config loading, ensuring they don't fail if `ENV` was previously set to `production`. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
This PR addresses a reported CORS issue in the production environment.
It introduces a
CORSLoggingMiddlewareto provide visibility into CORS failures by logging whether theAccess-Control-Allow-Originheader is present in the response.It also refactors
backend/main.pyto explicitly define the allowed origins list and ensures the middleware is registered in the correct order to capture headers.Tests were added to verify the fix and logging behavior.
PR created automatically by Jules for task 1671057333786473719 started by @KnellBalm
Summary by Sourcery
Add middleware to log CORS behavior and ensure production CORS configuration correctly recognizes Cloud Run origins.
New Features:
Bug Fixes:
Tests: