fix: add CORS logging to diagnose production issue#30
Open
fix: add CORS logging to diagnose production issue#30
Conversation
- Added `CORSLoggingMiddleware` to `backend/common/middleware.py` to log Origin headers and check for missing Access-Control-Allow-Origin headers. - Registered the middleware in `backend/main.py` as the outermost middleware. - Added `tests/test_cors_logging.py` to verify the logging behavior. - Verified that `backend/main.py` correctly includes the reported origin in `cloud_origins` and matches the regex. - This change aims to diagnose why the production environment reports CORS errors despite correct configuration, and ensures any stale deployment is updated. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
Contributor
|
👋 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 CORS logging middleware to help diagnose missing CORS headers in production and verifies behavior with targeted tests for allowed and disallowed origins. Sequence diagram for CORSLoggingMiddleware request and response flowsequenceDiagram
actor Client
participant App
participant CORSLoggingMiddleware
participant CORSMiddleware
participant RouteHandler
Client->>App: HTTP request
App->>CORSLoggingMiddleware: dispatch(request, call_next)
CORSLoggingMiddleware->>CORSLoggingMiddleware: Read origin header
alt Origin header present
CORSLoggingMiddleware->>CORSLoggingMiddleware: Log CORS Request info
end
CORSLoggingMiddleware->>CORSMiddleware: call_next(request)
CORSMiddleware->>RouteHandler: Process request
RouteHandler-->>CORSMiddleware: Response
CORSMiddleware-->>CORSLoggingMiddleware: Response with CORS headers (if configured)
alt Origin header present
CORSLoggingMiddleware->>CORSLoggingMiddleware: Check access control allow origin header
alt Header missing
CORSLoggingMiddleware->>CORSLoggingMiddleware: Log warning about missing CORS header
end
end
CORSLoggingMiddleware-->>App: Response
App-->>Client: HTTP response
Class diagram for new CORSLoggingMiddleware and FastAPI app integrationclassDiagram
class BaseHTTPMiddleware {
+dispatch(request, call_next)
}
class CORSLoggingMiddleware {
+dispatch(request, call_next)
}
class FastAPIApp {
+add_middleware(middleware_class)
}
CORSLoggingMiddleware --|> BaseHTTPMiddleware
FastAPIApp o--> CORSLoggingMiddleware
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider instantiating the
backend.middleware.corslogger once (e.g., as a module- or class-level variable) instead of callingget_loggermultiple times inCORSLoggingMiddleware.dispatchto avoid repeated lookup overhead and keep the code cleaner. - The tests in
test_cors_logging.pysetos.environ["ENV"] = "production"at import time, which can leak into other tests; it would be safer to scope this via a fixture ormonkeypatchwithin the individual tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider instantiating the `backend.middleware.cors` logger once (e.g., as a module- or class-level variable) instead of calling `get_logger` multiple times in `CORSLoggingMiddleware.dispatch` to avoid repeated lookup overhead and keep the code cleaner.
- The tests in `test_cors_logging.py` set `os.environ["ENV"] = "production"` at import time, which can leak into other tests; it would be safer to scope this via a fixture or `monkeypatch` within the individual tests.
## Individual Comments
### Comment 1
<location> `tests/test_cors_logging.py:7-8` </location>
<code_context>
+import pytest
+from fastapi.testclient import TestClient
+
+# Set ENV to production
+os.environ["ENV"] = "production"
+
+try:
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid setting ENV at import time; use a per-test fixture/monkeypatch instead
Mutating `os.environ` at module import can leak state across tests and make execution order matter. Instead, use a pytest fixture (optionally `autouse=True`) that monkeypatches `os.environ["ENV"] = "production"` for the duration of each relevant test and restores it afterward to keep tests isolated and deterministic.
Suggested implementation:
```python
import logging
import os
import sys
import pytest
from fastapi.testclient import TestClient
try:
from backend.main import app
except ImportError:
sys.path.append(os.getcwd())
from backend.main import app
@pytest.fixture
def env_production(monkeypatch):
"""Ensure ENV is set to production for the duration of a test."""
monkeypatch.setenv("ENV", "production")
class TestCORSLogging:
def test_cors_logging_success(self, caplog, env_production):
"""Test that CORS requests are logged and valid responses don't trigger warnings."""
caplog.set_level(logging.INFO)
```
If there are other tests in this file that rely on `ENV` being `"production"`, they should also accept the `env_production` fixture as a parameter (e.g. `def test_x(..., env_production):`). Alternatively, you can change the fixture to `@pytest.fixture(autouse=True)` if you want all tests in this module to automatically run with `ENV="production"` without modifying each test signature.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+7
to
+8
| # Set ENV to production | ||
| os.environ["ENV"] = "production" |
There was a problem hiding this comment.
suggestion (testing): Avoid setting ENV at import time; use a per-test fixture/monkeypatch instead
Mutating os.environ at module import can leak state across tests and make execution order matter. Instead, use a pytest fixture (optionally autouse=True) that monkeypatches os.environ["ENV"] = "production" for the duration of each relevant test and restores it afterward to keep tests isolated and deterministic.
Suggested implementation:
import logging
import os
import sys
import pytest
from fastapi.testclient import TestClient
try:
from backend.main import app
except ImportError:
sys.path.append(os.getcwd())
from backend.main import app
@pytest.fixture
def env_production(monkeypatch):
"""Ensure ENV is set to production for the duration of a test."""
monkeypatch.setenv("ENV", "production")
class TestCORSLogging:
def test_cors_logging_success(self, caplog, env_production):
"""Test that CORS requests are logged and valid responses don't trigger warnings."""
caplog.set_level(logging.INFO)If there are other tests in this file that rely on ENV being "production", they should also accept the env_production fixture as a parameter (e.g. def test_x(..., env_production):). Alternatively, you can change the fixture to @pytest.fixture(autouse=True) if you want all tests in this module to automatically run with ENV="production" without modifying each test signature.
- Modified `tests/test_integration.py` to explicitly set `ENV=development` using `mock.patch.dict` to prevent `ValueError: POSTGRES_DSN is required` when running in a polluted environment. - Refactored `tests/test_cors_logging.py` to use `importlib.reload` and `mock.patch.dict` for safe testing of production settings without global side effects. - Added `CORSLoggingMiddleware` to `backend/common/middleware.py` and registered it in `backend/main.py` to diagnose production CORS issues. - Verified that `backend/main.py` already contains the correct allowed origin, suggesting the original issue was due to a stale deployment or environment mismatch, which this commit will resolve by triggering a redeploy. Co-authored-by: KnellBalm <90038472+KnellBalm@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Diagnose and fix production CORS issues by adding detailed logging and ensuring deployment update.
The user reported a CORS error for a specific origin in production. Code analysis confirmed the origin is correctly configured in
backend/main.py. To investigate further and rule out stale deployments or environment-specific header stripping,CORSLoggingMiddlewarewas added.Changes:
CORSLoggingMiddlewareinbackend/common/middleware.py.backend/main.py.tests/test_cors_logging.py.PR created automatically by Jules for task 12351911378339454831 started by @KnellBalm
Summary by Sourcery
Add middleware to log CORS requests and missing response headers and verify its behavior with tests.
New Features:
Tests: