Add circuit breakers - #167
Open
Zingzy wants to merge 4 commits into
Open
Conversation
- Updated Raycast extension description to clarify link management functionality. - Added redirect URI allowlist validation to ensure secure handling of redirect URIs in device authentication. - Improved the `validate_redirect_uri` method to support exact and prefix matching for redirect URIs, enhancing security for OAuth clients.
- Changed the status of the Raycast app in the configuration file to reflect its current availability, enhancing clarity for users and developers.
- Updated the redirect URI format in the configuration for the Raycast app to support query parameters. - Refactored the redirect URI validation logic to utilize a service method for improved security and maintainability. - Added comprehensive unit tests for the new validation logic, including support for wildcard matching and various edge cases.
…dpoint to a liveness probe
Contributor
Reviewer's GuideRefactors the /health endpoint into a simple liveness probe, introduces request-level timeouts and MongoDB socket/circuit timeouts, and adds wildcard-aware redirect URI validation (used for Raycast device auth), with accompanying config, app wiring, and tests. Sequence diagram for request timeout middleware behaviorsequenceDiagram
actor Client
participant FastAPIApp
participant RequestTimeoutMiddleware
participant RequestHandler
Client->>FastAPIApp: HTTP request
FastAPIApp->>RequestTimeoutMiddleware: dispatch(request, call_next)
alt Exempt path (/metric)
RequestTimeoutMiddleware->>RequestHandler: call_next(request)
RequestHandler-->>RequestTimeoutMiddleware: Response
RequestTimeoutMiddleware-->>FastAPIApp: Response
FastAPIApp-->>Client: 200 OK or other
else Non-exempt path
RequestTimeoutMiddleware->>RequestTimeoutMiddleware: asyncio.wait_for(call_next(request), timeout_seconds)
alt Handler completes before timeout
RequestTimeoutMiddleware->>RequestHandler: call_next(request)
RequestHandler-->>RequestTimeoutMiddleware: Response
RequestTimeoutMiddleware-->>FastAPIApp: Response
FastAPIApp-->>Client: 2xx/4xx/5xx
else Timeout exceeded
RequestTimeoutMiddleware-->>FastAPIApp: JSONResponse 504 request_timeout
FastAPIApp-->>Client: 504 Gateway Timeout
end
end
Sequence diagram for device auth redirect URI validation with wildcardssequenceDiagram
actor Browser
participant DeviceAuthRoute
participant DeviceAuthSvc
participant AppEntry
participant RedirectResponse
Browser->>DeviceAuthRoute: GET /auth/device/login?redirect_uri=...
DeviceAuthRoute->>DeviceAuthSvc: create_device_auth_code(user_id, email, app_id)
DeviceAuthSvc-->>DeviceAuthRoute: code
DeviceAuthRoute->>DeviceAuthRoute: _build_callback_redirect(code, state, redirect_uri, app, svc)
DeviceAuthRoute->>DeviceAuthSvc: validate_redirect_uri(redirect_uri, app)
alt redirect_uri empty
DeviceAuthSvc-->>DeviceAuthRoute: True
DeviceAuthRoute->>RedirectResponse: /auth/device/callback?code, state
RedirectResponse-->>Browser: 302 to default callback
else redirect_uri exact or wildcard match in app.redirect_uris
DeviceAuthSvc-->>DeviceAuthRoute: True
DeviceAuthRoute->>RedirectResponse: redirect_uri with code, state appended
RedirectResponse-->>Browser: 302 to redirect_uri
else redirect_uri not allowed
DeviceAuthSvc-->>DeviceAuthRoute: False
DeviceAuthRoute->>RedirectResponse: /auth/device/callback?code, state
RedirectResponse-->>Browser: 302 to default callback
end
Updated class diagram for timeout middleware, settings, health response, and device auth serviceclassDiagram
class RequestTimeoutMiddleware {
- float _timeout
+ RequestTimeoutMiddleware(app, timeout_seconds: float)
+ dispatch(request: Request, call_next: RequestResponseEndpoint) Response
}
class DatabaseSettings {
+ str mongodb_uri
+ str db_name
+ int max_pool_size
+ int min_pool_size
+ int server_selection_timeout_ms
+ int connect_timeout_ms
+ int socket_timeout_ms
}
class AppSettings {
+ int max_active_api_keys
+ int max_date_range_days
+ float http_client_timeout
+ float request_timeout_seconds
+ float blocked_url_regex_timeout
+ float shortlink_regex_timeout
+ float reserved_path_regex_timeout
+ float reserved_subdomain_regex_timeout
+ float reserved_host_regex_timeout
+ _must_be_positive_float(v: float, info) float
}
class DeviceAuthSvc {
+ resolve_app(app_id: str) AppEntry
+ validate_redirect_uri(redirect_uri: str, app: AppEntry) bool
+ create_device_auth_code(user_id: ObjectId, email: str, app_id: str) str
}
class AppEntry {
+ str id
+ list~str~ redirect_uris
+ bool is_live_device_app()
}
class HealthResponse {
+ str status
}
class FastAPIApp {
+ middleware RequestTimeoutMiddleware
+ DatabaseSettings db_settings
+ AppSettings settings
}
FastAPIApp "1" o-- "1" DatabaseSettings
FastAPIApp "1" o-- "1" AppSettings
FastAPIApp "1" o-- "1" RequestTimeoutMiddleware
DeviceAuthSvc "1" --> "1" AppEntry
RequestTimeoutMiddleware ..> HealthResponse : returns on timeout
DeviceAuthSvc ..> HealthResponse : unaffected by change
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR introduces request timeout middleware to enforce maximum request handling duration, simplifies the health endpoint from a dependency-aware check to a liveness probe, enhances redirect URI validation with wildcard pattern support, and updates database connection timeout configuration and Raycast app metadata. ChangesRequest Timeout Infrastructure
Health Endpoint as Liveness Probe
Redirect URI Validation with Wildcard Support
Sequence DiagramsequenceDiagram
participant Client
participant RequestTimeoutMiddleware
participant Handler as App Handler
participant Response
Client->>RequestTimeoutMiddleware: HTTP Request
Note over RequestTimeoutMiddleware: Check if path in EXEMPT_PATHS
alt Path exempted (e.g., /metric)
RequestTimeoutMiddleware->>Handler: call_next(request)
else Path not exempted
RequestTimeoutMiddleware->>RequestTimeoutMiddleware: Wrap with asyncio.wait_for(timeout=8.0s)
RequestTimeoutMiddleware->>Handler: call_next(request) [with deadline]
alt Request completes within timeout
Handler->>Response: Normal response
Response->>Client: 200/201/etc.
else Request exceeds timeout
RequestTimeoutMiddleware->>RequestTimeoutMiddleware: asyncio.TimeoutError caught
RequestTimeoutMiddleware->>RequestTimeoutMiddleware: Log with request metadata
RequestTimeoutMiddleware->>Response: 504 JSON error
Response->>Client: {"error": "Request timed out", "code": "request_timeout"}
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The request timeout middleware currently exempts only exact path matches (e.g. "/metric"); if you ever serve the same endpoint under a prefix (versioned or mounted app) you may want to switch this to a prefix or pattern match instead of a hard-coded exact path set.
- The 504 timeout response uses a bespoke JSON shape (
{"error": ..., "code": ...}); consider wiring this through your existing error-handling/response schema so timeout errors are consistent with other API error responses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The request timeout middleware currently exempts only exact path matches (e.g. "/metric"); if you ever serve the same endpoint under a prefix (versioned or mounted app) you may want to switch this to a prefix or pattern match instead of a hard-coded exact path set.
- The 504 timeout response uses a bespoke JSON shape (`{"error": ..., "code": ...}`); consider wiring this through your existing error-handling/response schema so timeout errors are consistent with other API error responses.
## Individual Comments
### Comment 1
<location path="tests/integration/test_health.py" line_range="21-30" />
<code_context>
return app
class TestHealthEndpoint:
- def test_healthy_when_both_ok(self):
- app = _build_test_app(mongo_ok=True, redis_ok=True)
- with TestClient(app) as client:
- resp = client.get("/health")
- assert resp.status_code == 200
- body = resp.json()
- assert body["status"] == "healthy"
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `test_no_db_dependency` to explicitly prove that `/health` does not touch app state
Currently `test_returns_ok` and `test_no_db_dependency` both just assert a 200 and static body, and `test_no_db_dependency` only *implicitly* checks that `app.state` is not used. To encode that contract more clearly, consider making the test fail if `app.state` is touched, e.g. by assigning sentinels that raise on access:
```python
app = _build_test_app()
class Sentinel:
def __getattr__(self, name):
raise AssertionError("health endpoint must not touch DB/Redis")
app.state.db = Sentinel()
app.state.redis = Sentinel()
with TestClient(app) as client:
resp = client.get("/health")
assert resp.status_code == 200
assert resp.json() == {"status": "ok"}
```
This ensures any future reintroduction of DB/Redis access in `/health` is caught for the right reason, instead of just duplicating `test_returns_ok`.
Suggested implementation:
```python
class TestHealthEndpoint:
def test_no_db_dependency(self):
app = _build_test_app()
class Sentinel:
def __getattr__(self, name):
raise AssertionError("health endpoint must not touch DB/Redis")
app.state.db = Sentinel()
app.state.redis = Sentinel()
with TestClient(app) as client:
resp = client.get("/health")
assert resp.status_code == 200
assert resp.json() == {"status": "ok"}
```
1. Ensure `TestClient` is imported at the top of `tests/integration/test_health.py`, e.g.:
`from starlette.testclient import TestClient` or `from fastapi.testclient import TestClient`, matching the rest of your test suite.
2. If your `/health` endpoint returns a different JSON body (e.g. `{"status": "healthy"}` or includes additional keys), adjust the `assert resp.json() == {"status": "ok"}` accordingly to match the actual contract you want to enforce.
</issue_to_address>
### Comment 2
<location path="tests/smoke/test_startup.py" line_range="19-23" />
<code_context>
def test_health_endpoint_responds(smoke_client: TestClient) -> None:
- """Health endpoint should return a response (mock DB will show unhealthy, but no crash)."""
+ """Liveness probe — always 200, no dependency checks."""
resp = smoke_client.get("/health")
- assert resp.status_code in (200, 503)
- data = resp.json()
- assert "status" in data
- assert "checks" in data
+ assert resp.status_code == 200
+ assert resp.json() == {"status": "ok"}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the new `RequestTimeoutMiddleware`, including timeout and exempt paths
The new `RequestTimeoutMiddleware` wired in `app.py` isn’t covered by tests. Please add coverage for at least:
1. A handler that sleeps longer than the configured timeout and returns a 504 with the expected JSON body.
2. An exempt path (e.g. `/metric`) that does not time out even if it sleeps longer than the global timeout.
3. (Optional) Using a very short timeout (e.g. 0.01s) so the tests stay fast.
These can go in a small dedicated integration test module (e.g. `tests/integration/test_timeout_middleware.py`) or be folded into the smoke tests, so regressions in the timeout behavior are caught.
Suggested implementation:
```python
def test_health_endpoint_responds(smoke_client: TestClient) -> None:
"""Liveness probe — always 200, no dependency checks."""
resp = smoke_client.get("/health")
assert resp.status_code == 200
def test_request_timeout_returns_504(timeout_client: TestClient) -> None:
"""
Requests exceeding the configured timeout should return 504 with a JSON body.
The /timeout-test handler is expected to sleep longer than the configured timeout.
"""
resp = timeout_client.get("/timeout-test")
assert resp.status_code == 504
data = resp.json()
# Be flexible on the exact schema but ensure we have a timeout indication.
assert isinstance(data, dict)
# Common patterns: {"detail": "...timeout..."} or {"error": "...timeout..."}
text = " ".join(str(v) for v in data.values()).lower()
assert "timeout" in text
def test_timeout_middleware_does_not_affect_exempt_path(timeout_client: TestClient) -> None:
"""
Exempt paths (e.g. /metrics) should not be subject to the global timeout,
even if their handler sleeps longer than the configured timeout.
"""
resp = timeout_client.get("/metrics")
assert resp.status_code == 200
```
To make these tests pass, the following additional work is needed elsewhere in the codebase:
1. **Provide a timeout-aware TestClient fixture**:
- Add a `timeout_client: TestClient` fixture (e.g. in `tests/conftest.py`) that:
- Constructs the FastAPI/Starlette app using the normal app factory.
- Configures `RequestTimeoutMiddleware` with a very short timeout (e.g. 0.01s) for fast tests, via settings/env or direct kwargs.
- Ensure this fixture is independent from `smoke_client` so it can override timeout-specific configuration.
2. **Add a slow test handler for the timeout scenario**:
- In the app used by tests (or in a test-only router), add a route such as:
- `GET /timeout-test` that performs `await asyncio.sleep(x)` where `x` is significantly larger than the configured timeout, so it reliably triggers the middleware.
3. **Mark the exempt path in the middleware**:
- Ensure that the timeout middleware is configured so that `/metrics` (or whatever path is used for metrics) is in the middleware’s exempt-path list.
- If your metrics endpoint uses a different path (e.g. `/metric`), update the test to use the correct exempt path.
4. **Align JSON error shape if needed**:
- If your `RequestTimeoutMiddleware` returns a specific JSON schema (e.g. `{"detail": "Request timed out"}`), you can tighten the assertion in `test_request_timeout_returns_504` to match it exactly.
</issue_to_address>
### Comment 3
<location path="tests/integration/test_device_auth.py" line_range="109-115" />
<code_context>
# resolve_app and validate_redirect_uri are sync — must not be coroutines
svc.resolve_app = MagicMock(side_effect=_resolve_app)
svc.validate_redirect_uri = MagicMock(
- side_effect=lambda uri, app: not uri or uri in app.redirect_uris
+ side_effect=lambda uri, app: (
+ not uri
+ or any(
+ uri.startswith(a[:-1]) if a.endswith("*") else uri == a
+ for a in app.redirect_uris
+ )
+ )
)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider at least one integration test that uses the real `validate_redirect_uri` for end-to-end coverage
The mock now copies the wildcard logic from the implementation, so existing tests will keep passing even if the real `validate_redirect_uri` later changes. You already have good unit coverage, but there’s no integration‑level check that `_build_callback_redirect` works with the real `DeviceAuthService.validate_redirect_uri`.
Please add a single integration test that:
- Uses the real `svc.validate_redirect_uri` (no override),
- Sets up an app with a wildcard redirect URI, and
- Asserts that the redirect response uses that wildcard redirect with the appended `code`/`state`.
This will give you one end‑to‑end check tying together the route, service, and redirect behaviour.
Suggested implementation:
```python
# resolve_app and validate_redirect_uri are sync — must not be coroutines
svc.resolve_app = MagicMock(side_effect=_resolve_app)
svc.validate_redirect_uri = MagicMock(
side_effect=lambda uri, app: (
not uri
or any(
uri.startswith(a[:-1]) if a.endswith("*") else uri == a
for a in app.redirect_uris
)
)
)
return svc
@pytest.mark.integration
def test_device_callback_uses_wildcard_redirect_with_real_validation(
client,
app_factory,
):
"""
Integration-level check that _build_callback_redirect cooperates with the real
DeviceAuthService.validate_redirect_uri for wildcard redirect URIs.
"""
# Arrange: create an app that allows a wildcard redirect URI
app = app_factory(redirect_uris=["https://example.com/callback/*"])
# This is the specific redirect URI we want to use; it should be accepted
redirect_uri = "https://example.com/callback/flow-123"
# Act: call the device callback endpoint, which should internally:
# - resolve the app
# - validate the redirect_uri using DeviceAuthService.validate_redirect_uri
# - build a redirect with appended code/state
response = client.get(
"/device/callback",
query_string={
"client_id": app.client_id,
"redirect_uri": redirect_uri,
},
)
# Assert: we got redirected to the provided redirect_uri with code/state
assert response.status_code == 302
location = response.headers["Location"]
assert location.startswith(redirect_uri)
assert "code=" in location
assert "state=" in location
```
To make this compile and pass, you’ll likely need to:
1. Ensure `pytest` is imported (`import pytest`) at the top of `tests/integration/test_device_auth.py` if it is not already.
2. Adjust the fixture names to match your existing test helpers:
- If your factory for creating client applications is not called `app_factory`, rename the parameter and usage accordingly (e.g. `application_factory`, `make_app`, etc.).
- If your device callback route is not `/device/callback`, update the `client.get(...)` path to the actual callback endpoint that triggers `_build_callback_redirect`.
- If your application model does not expose `client_id` as `app.client_id`, replace that access with the appropriate attribute.
3. Make sure the app created by `app_factory` is persisted/visible to the running application under test (e.g. committing to the DB or registering in whatever storage `resolve_app` uses), consistent with how other integration tests create apps.
4. If you want this test to bypass the `MagicMock` override of `validate_redirect_uri`, introduce a separate fixture that *does not* patch `svc.validate_redirect_uri` and use that fixture’s client in this test, or scope the existing mocking fixture so it doesn’t apply here (e.g. via a different marker or fixture layering).
</issue_to_address>
### Comment 4
<location path="tests/unit/services/test_redirect_uri.py" line_range="29-33" />
<code_context>
+ return AppEntry(name="test", description="test app", redirect_uris=redirect_uris)
+
+
+class TestValidateRedirectUri:
+ """Tests for DeviceAuthService.validate_redirect_uri."""
+
+ def setup_method(self):
+ self.svc = DeviceAuthService.__new__(DeviceAuthService)
+
+ def test_empty_uri_allowed(self):
+ app = _app(["https://example.com/callback"])
+ assert self.svc.validate_redirect_uri("", app) is True
+
+ def test_exact_match(self):
+ app = _app(["https://example.com/callback"])
+ assert (
+ self.svc.validate_redirect_uri("https://example.com/callback", app) is True
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for patterns where `*` appears but not as a suffix, to document literal behaviour
One missing edge case is a redirect URI that contains `*` not at the end (e.g. `"https://exa*mple.com/callback"`). Since only entries ending with `*` are treated as wildcards, such patterns are effectively literals and will rarely match real URIs. A test like:
```python
def test_internal_asterisk_is_literal(self):
app = _app(["https://exa*mple.com/callback"])
assert self.svc.validate_redirect_uri("https://exa*mple.com/callback", app) is True
assert self.svc.validate_redirect_uri("https://example.com/callback", app) is False
```
would document and lock in this behavior around non-suffix `*`.
```suggestion
def test_exact_no_match(self):
app = _app(["https://example.com/callback"])
assert self.svc.validate_redirect_uri("https://evil.com/callback", app) is False
def test_internal_asterisk_is_literal(self):
app = _app(["https://exa*mple.com/callback"])
assert (
self.svc.validate_redirect_uri("https://exa*mple.com/callback", app) is True
)
assert (
self.svc.validate_redirect_uri("https://example.com/callback", app) is False
)
def test_wildcard_query_match(self):
```
</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
21
to
+30
| class TestHealthEndpoint: | ||
| def test_healthy_when_both_ok(self): | ||
| app = _build_test_app(mongo_ok=True, redis_ok=True) | ||
| with TestClient(app) as client: | ||
| resp = client.get("/health") | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["status"] == "healthy" | ||
| assert body["checks"]["mongodb"] == "ok" | ||
| assert body["checks"]["redis"] == "ok" | ||
|
|
||
| def test_unhealthy_when_mongo_fails(self): | ||
| app = _build_test_app(mongo_ok=False, redis_ok=True) | ||
| with TestClient(app) as client: | ||
| resp = client.get("/health") | ||
| assert resp.status_code == 503 | ||
| body = resp.json() | ||
| assert body["status"] == "unhealthy" | ||
| assert body["checks"]["mongodb"] == "error" | ||
|
|
||
| def test_degraded_when_redis_fails(self): | ||
| app = _build_test_app(mongo_ok=True, redis_ok=False) | ||
| with TestClient(app) as client: | ||
| resp = client.get("/health") | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["status"] == "degraded" | ||
| assert body["checks"]["redis"] == "error" | ||
|
|
||
| def test_degraded_when_redis_not_configured(self): | ||
| app = _build_test_app(mongo_ok=True, redis_configured=False) | ||
| def test_returns_ok(self): | ||
| app = _build_test_app() | ||
| with TestClient(app) as client: | ||
| resp = client.get("/health") | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["status"] == "degraded" | ||
| assert body["checks"]["redis"] == "not_configured" | ||
| assert resp.json() == {"status": "ok"} | ||
|
|
||
| def test_response_shape(self): | ||
| def test_no_db_dependency(self): | ||
| """Health must not touch app.state.db or app.state.redis.""" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen test_no_db_dependency to explicitly prove that /health does not touch app state
Currently test_returns_ok and test_no_db_dependency both just assert a 200 and static body, and test_no_db_dependency only implicitly checks that app.state is not used. To encode that contract more clearly, consider making the test fail if app.state is touched, e.g. by assigning sentinels that raise on access:
app = _build_test_app()
class Sentinel:
def __getattr__(self, name):
raise AssertionError("health endpoint must not touch DB/Redis")
app.state.db = Sentinel()
app.state.redis = Sentinel()
with TestClient(app) as client:
resp = client.get("/health")
assert resp.status_code == 200
assert resp.json() == {"status": "ok"}This ensures any future reintroduction of DB/Redis access in /health is caught for the right reason, instead of just duplicating test_returns_ok.
Suggested implementation:
class TestHealthEndpoint:
def test_no_db_dependency(self):
app = _build_test_app()
class Sentinel:
def __getattr__(self, name):
raise AssertionError("health endpoint must not touch DB/Redis")
app.state.db = Sentinel()
app.state.redis = Sentinel()
with TestClient(app) as client:
resp = client.get("/health")
assert resp.status_code == 200
assert resp.json() == {"status": "ok"}- Ensure
TestClientis imported at the top oftests/integration/test_health.py, e.g.:
from starlette.testclient import TestClientorfrom fastapi.testclient import TestClient, matching the rest of your test suite. - If your
/healthendpoint returns a different JSON body (e.g.{"status": "healthy"}or includes additional keys), adjust theassert resp.json() == {"status": "ok"}accordingly to match the actual contract you want to enforce.
Comment on lines
19
to
+23
| def test_health_endpoint_responds(smoke_client: TestClient) -> None: | ||
| """Health endpoint should return a response (mock DB will show unhealthy, but no crash).""" | ||
| """Liveness probe — always 200, no dependency checks.""" | ||
| resp = smoke_client.get("/health") | ||
| assert resp.status_code in (200, 503) | ||
| data = resp.json() | ||
| assert "status" in data | ||
| assert "checks" in data | ||
| assert resp.status_code == 200 | ||
| assert resp.json() == {"status": "ok"} |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for the new RequestTimeoutMiddleware, including timeout and exempt paths
The new RequestTimeoutMiddleware wired in app.py isn’t covered by tests. Please add coverage for at least:
- A handler that sleeps longer than the configured timeout and returns a 504 with the expected JSON body.
- An exempt path (e.g.
/metric) that does not time out even if it sleeps longer than the global timeout. - (Optional) Using a very short timeout (e.g. 0.01s) so the tests stay fast.
These can go in a small dedicated integration test module (e.g. tests/integration/test_timeout_middleware.py) or be folded into the smoke tests, so regressions in the timeout behavior are caught.
Suggested implementation:
def test_health_endpoint_responds(smoke_client: TestClient) -> None:
"""Liveness probe — always 200, no dependency checks."""
resp = smoke_client.get("/health")
assert resp.status_code == 200
def test_request_timeout_returns_504(timeout_client: TestClient) -> None:
"""
Requests exceeding the configured timeout should return 504 with a JSON body.
The /timeout-test handler is expected to sleep longer than the configured timeout.
"""
resp = timeout_client.get("/timeout-test")
assert resp.status_code == 504
data = resp.json()
# Be flexible on the exact schema but ensure we have a timeout indication.
assert isinstance(data, dict)
# Common patterns: {"detail": "...timeout..."} or {"error": "...timeout..."}
text = " ".join(str(v) for v in data.values()).lower()
assert "timeout" in text
def test_timeout_middleware_does_not_affect_exempt_path(timeout_client: TestClient) -> None:
"""
Exempt paths (e.g. /metrics) should not be subject to the global timeout,
even if their handler sleeps longer than the configured timeout.
"""
resp = timeout_client.get("/metrics")
assert resp.status_code == 200To make these tests pass, the following additional work is needed elsewhere in the codebase:
-
Provide a timeout-aware TestClient fixture:
- Add a
timeout_client: TestClientfixture (e.g. intests/conftest.py) that:- Constructs the FastAPI/Starlette app using the normal app factory.
- Configures
RequestTimeoutMiddlewarewith a very short timeout (e.g. 0.01s) for fast tests, via settings/env or direct kwargs.
- Ensure this fixture is independent from
smoke_clientso it can override timeout-specific configuration.
- Add a
-
Add a slow test handler for the timeout scenario:
- In the app used by tests (or in a test-only router), add a route such as:
GET /timeout-testthat performsawait asyncio.sleep(x)wherexis significantly larger than the configured timeout, so it reliably triggers the middleware.
- In the app used by tests (or in a test-only router), add a route such as:
-
Mark the exempt path in the middleware:
- Ensure that the timeout middleware is configured so that
/metrics(or whatever path is used for metrics) is in the middleware’s exempt-path list. - If your metrics endpoint uses a different path (e.g.
/metric), update the test to use the correct exempt path.
- Ensure that the timeout middleware is configured so that
-
Align JSON error shape if needed:
- If your
RequestTimeoutMiddlewarereturns a specific JSON schema (e.g.{"detail": "Request timed out"}), you can tighten the assertion intest_request_timeout_returns_504to match it exactly.
- If your
Comment on lines
109
to
+115
| svc.validate_redirect_uri = MagicMock( | ||
| side_effect=lambda uri, app: not uri or uri in app.redirect_uris | ||
| side_effect=lambda uri, app: ( | ||
| not uri | ||
| or any( | ||
| uri.startswith(a[:-1]) if a.endswith("*") else uri == a | ||
| for a in app.redirect_uris | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Consider at least one integration test that uses the real validate_redirect_uri for end-to-end coverage
The mock now copies the wildcard logic from the implementation, so existing tests will keep passing even if the real validate_redirect_uri later changes. You already have good unit coverage, but there’s no integration‑level check that _build_callback_redirect works with the real DeviceAuthService.validate_redirect_uri.
Please add a single integration test that:
- Uses the real
svc.validate_redirect_uri(no override), - Sets up an app with a wildcard redirect URI, and
- Asserts that the redirect response uses that wildcard redirect with the appended
code/state.
This will give you one end‑to‑end check tying together the route, service, and redirect behaviour.
Suggested implementation:
# resolve_app and validate_redirect_uri are sync — must not be coroutines
svc.resolve_app = MagicMock(side_effect=_resolve_app)
svc.validate_redirect_uri = MagicMock(
side_effect=lambda uri, app: (
not uri
or any(
uri.startswith(a[:-1]) if a.endswith("*") else uri == a
for a in app.redirect_uris
)
)
)
return svc
@pytest.mark.integration
def test_device_callback_uses_wildcard_redirect_with_real_validation(
client,
app_factory,
):
"""
Integration-level check that _build_callback_redirect cooperates with the real
DeviceAuthService.validate_redirect_uri for wildcard redirect URIs.
"""
# Arrange: create an app that allows a wildcard redirect URI
app = app_factory(redirect_uris=["https://example.com/callback/*"])
# This is the specific redirect URI we want to use; it should be accepted
redirect_uri = "https://example.com/callback/flow-123"
# Act: call the device callback endpoint, which should internally:
# - resolve the app
# - validate the redirect_uri using DeviceAuthService.validate_redirect_uri
# - build a redirect with appended code/state
response = client.get(
"/device/callback",
query_string={
"client_id": app.client_id,
"redirect_uri": redirect_uri,
},
)
# Assert: we got redirected to the provided redirect_uri with code/state
assert response.status_code == 302
location = response.headers["Location"]
assert location.startswith(redirect_uri)
assert "code=" in location
assert "state=" in locationTo make this compile and pass, you’ll likely need to:
- Ensure
pytestis imported (import pytest) at the top oftests/integration/test_device_auth.pyif it is not already. - Adjust the fixture names to match your existing test helpers:
- If your factory for creating client applications is not called
app_factory, rename the parameter and usage accordingly (e.g.application_factory,make_app, etc.). - If your device callback route is not
/device/callback, update theclient.get(...)path to the actual callback endpoint that triggers_build_callback_redirect. - If your application model does not expose
client_idasapp.client_id, replace that access with the appropriate attribute.
- If your factory for creating client applications is not called
- Make sure the app created by
app_factoryis persisted/visible to the running application under test (e.g. committing to the DB or registering in whatever storageresolve_appuses), consistent with how other integration tests create apps. - If you want this test to bypass the
MagicMockoverride ofvalidate_redirect_uri, introduce a separate fixture that does not patchsvc.validate_redirect_uriand use that fixture’s client in this test, or scope the existing mocking fixture so it doesn’t apply here (e.g. via a different marker or fixture layering).
Comment on lines
+29
to
+33
| def test_exact_no_match(self): | ||
| app = _app(["https://example.com/callback"]) | ||
| assert self.svc.validate_redirect_uri("https://evil.com/callback", app) is False | ||
|
|
||
| def test_wildcard_query_match(self): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test for patterns where * appears but not as a suffix, to document literal behaviour
One missing edge case is a redirect URI that contains * not at the end (e.g. "https://exa*mple.com/callback"). Since only entries ending with * are treated as wildcards, such patterns are effectively literals and will rarely match real URIs. A test like:
def test_internal_asterisk_is_literal(self):
app = _app(["https://exa*mple.com/callback"])
assert self.svc.validate_redirect_uri("https://exa*mple.com/callback", app) is True
assert self.svc.validate_redirect_uri("https://example.com/callback", app) is Falsewould document and lock in this behavior around non-suffix *.
Suggested change
| def test_exact_no_match(self): | |
| app = _app(["https://example.com/callback"]) | |
| assert self.svc.validate_redirect_uri("https://evil.com/callback", app) is False | |
| def test_wildcard_query_match(self): | |
| def test_exact_no_match(self): | |
| app = _app(["https://example.com/callback"]) | |
| assert self.svc.validate_redirect_uri("https://evil.com/callback", app) is False | |
| def test_internal_asterisk_is_literal(self): | |
| app = _app(["https://exa*mple.com/callback"]) | |
| assert ( | |
| self.svc.validate_redirect_uri("https://exa*mple.com/callback", app) is True | |
| ) | |
| assert ( | |
| self.svc.validate_redirect_uri("https://example.com/callback", app) is False | |
| ) | |
| def test_wildcard_query_match(self): |
There was a problem hiding this comment.
Pull request overview
This PR improves operational resilience for the FastAPI service by adding request/DB timeouts, simplifying /health into a liveness probe, and expanding device-auth redirect URI allowlisting to support explicit wildcard entries (e.g., Raycast’s query-string variations).
Changes:
- Add a request-deadline middleware (504 on timeout) and wire it into app startup via a new
request_timeout_secondssetting. - Add MongoDB client timeouts and cap the
/metricaggregation viamaxTimeMS. - Update device-auth redirect URI validation to support allowlist entries ending in
*, and update tests/config accordingly. - Simplify
/healthresponse/schema to{"status": "ok"}and update tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
middleware/timeout.py |
New request timeout middleware returning 504 on deadline exceeded. |
app.py |
Wires in RequestTimeoutMiddleware and Mongo client timeout options; updates lifespan type. |
config.py |
Adds DB timeout settings and request_timeout_seconds app setting with validation. |
routes/legacy/url_shortener.py |
Adds maxTimeMS to the /metric aggregation query. |
routes/health_routes.py |
Converts /health into a liveness-only endpoint always returning 200. |
schemas/dto/responses/common.py |
Removes dependency checks from HealthResponse. |
services/auth/device.py |
Adds explicit *-suffix prefix matching for redirect URI allowlists. |
routes/auth/device.py |
Uses the service’s redirect URI validation when building callback redirects. |
config/apps.yaml |
Marks Raycast app live and adds wildcard redirect URI allowlist entry. |
tests/unit/services/test_redirect_uri.py |
Adds unit tests for wildcard redirect URI matching behavior. |
tests/integration/test_device_auth.py |
Updates mocked redirect URI validation to match new wildcard behavior. |
tests/integration/test_health.py |
Updates integration tests for new liveness-only /health behavior. |
tests/smoke/test_startup.py |
Updates smoke test expectations for /health response shape. |
tests/unit/schemas/dto/test_common.py |
Updates HealthResponse DTO serialization test for new shape. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+20
to
+21
| """Liveness probe. Public, unauthenticated, no dependency checks.""" | ||
| return JSONResponse(status_code=200, content={"status": "ok"}) |
Comment on lines
+29
to
+34
| def test_no_db_dependency(self): | ||
| """Health must not touch app.state.db or app.state.redis.""" | ||
| app = _build_test_app() | ||
| with TestClient(app) as client: | ||
| resp = client.get("/health") | ||
| body = resp.json() | ||
| assert "status" in body | ||
| assert "checks" in body | ||
| assert "mongodb" in body["checks"] | ||
| assert "redis" in body["checks"] | ||
| assert resp.status_code == 200 |
Comment on lines
+33
to
+39
| log.error( | ||
| "request_deadline_exceeded", | ||
| method=request.method, | ||
| path=request.url.path, | ||
| timeout_seconds=self._timeout, | ||
| client_ip=get_client_ip(request), | ||
| ) |
Comment on lines
228
to
+247
| # ── Middleware (registered in reverse execution order) ──────────────── | ||
| # 1. Session — outermost, needed by Authlib OAuth for state storage | ||
| app.add_middleware(SessionMiddleware, secret_key=settings.secret_key) | ||
| # 2. Security headers — must be outer so HSTS/CSP/nosniff apply to | ||
| # all responses including CORS preflights (204) and body-limit (413) | ||
| app.add_middleware(SecurityHeadersMiddleware, hsts_enabled=settings.is_production) | ||
| # 2a. Long-lived cache headers for /static/* | ||
| app.add_middleware(StaticCacheHeadersMiddleware) | ||
| # 3. CORS | ||
| configure_cors(app, settings) | ||
| # 4. Body size limit | ||
| app.add_middleware( | ||
| MaxContentLengthMiddleware, max_content_length=settings.max_content_length | ||
| ) | ||
| # 5. Request logging — innermost, logs all requests with request_id | ||
| # 5. Request logging — logs all requests with request_id | ||
| app.add_middleware(RequestLoggingMiddleware) | ||
| # 6. Request deadline — innermost | ||
| app.add_middleware( | ||
| RequestTimeoutMiddleware, timeout_seconds=settings.request_timeout_seconds | ||
| ) |
Comment on lines
+19
to
+43
| class RequestTimeoutMiddleware(BaseHTTPMiddleware): | ||
| def __init__(self, app, timeout_seconds: float = 8.0) -> None: | ||
| super().__init__(app) | ||
| self._timeout = timeout_seconds | ||
|
|
||
| async def dispatch( | ||
| self, request: Request, call_next: RequestResponseEndpoint | ||
| ) -> Response: | ||
| if request.url.path in EXEMPT_PATHS: | ||
| return await call_next(request) | ||
|
|
||
| try: | ||
| return await asyncio.wait_for(call_next(request), timeout=self._timeout) | ||
| except asyncio.TimeoutError: | ||
| log.error( | ||
| "request_deadline_exceeded", | ||
| method=request.method, | ||
| path=request.url.path, | ||
| timeout_seconds=self._timeout, | ||
| client_ip=get_client_ip(request), | ||
| ) | ||
| return JSONResponse( | ||
| status_code=504, | ||
| content={"error": "Request timed out", "code": "request_timeout"}, | ||
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
routes/legacy/url_shortener.py (1)
561-561: ⚡ Quick winKeep the
/metricquery deadline configurable.
maxTimeMS=90000is now coupled toDatabaseSettings.socket_timeout_msonly by convention. If a self-hoster lowers the socket timeout, this route can start failing before Mongo hits its own deadline. Please source the aggregation cap from settings, or derive it from the socket timeout with some headroom.♻️ Possible adjustment
- cursor = await db["urls"].aggregate(METRIC_PIPELINE_V1, maxTimeMS=90000) + metric_query_timeout_ms = max(1000, settings.db.socket_timeout_ms - 5000) + cursor = await db["urls"].aggregate( + METRIC_PIPELINE_V1, + maxTimeMS=metric_query_timeout_ms, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/legacy/url_shortener.py` at line 561, The hardcoded maxTimeMS in the aggregation call (cursor = await db["urls"].aggregate(METRIC_PIPELINE_V1, maxTimeMS=90000)) should be sourced from configuration or derived from DatabaseSettings.socket_timeout_ms with headroom; update the aggregation invocation to use a settings value (e.g., settings.metric_max_time_ms) or compute maxTimeMS = int(settings.socket_timeout_ms * 0.9) (or similar) and pass that variable into db["urls"].aggregate, and ensure METRIC_PIPELINE_V1 and the route handler that performs this aggregation read the same settings object so the deadline is configurable and consistent with socket timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config.py`:
- Around line 26-29: Add startup validation to ensure the new Mongo timeout
config ints (server_selection_timeout_ms, connect_timeout_ms, socket_timeout_ms)
are positive values just like the existing float timeouts: check each value > 0
during config initialization and raise a clear ValueError (or log+exit) if any
are zero or negative. Locate the validation block used for the float timeout
fields and replicate the same positive-value check for these three integer
fields in the same config initialization path (the config class or function that
reads env vars) so bad deployments fail fast.
In `@services/auth/device.py`:
- Around line 79-82: The wildcard branch currently treats allowed == "*" as a
valid prefix match, allowing any redirect_uri; update the logic that iterates
over app.redirect_uris so that wildcard entries require a non-empty prefix
before treating the trailing "*" as a wildcard (i.e., only accept when
allowed.endswith("*") and the prefix part allowed[:-1] is non-empty), then
perform redirect_uri.startswith(prefix) using that prefix; use the existing loop
variables (app.redirect_uris, allowed, redirect_uri) to locate and change the
check.
In `@tests/integration/test_device_auth.py`:
- Around line 110-116: The mock side_effect currently short-circuits with "not
uri" which allows empty URIs when app.redirect_uris is empty; update the
side_effect lambda so it first requires a truthy uri and then checks any(match)
against app.redirect_uris (i.e., replace "not uri or any(...)" with "bool(uri)
and any(...)" or equivalent) so an empty allowlist (app.redirect_uris==[]) will
not validate an empty URI; locate the side_effect lambda and ensure it uses
app.redirect_uris and wildcard handling exactly as in the existing generator
logic.
---
Nitpick comments:
In `@routes/legacy/url_shortener.py`:
- Line 561: The hardcoded maxTimeMS in the aggregation call (cursor = await
db["urls"].aggregate(METRIC_PIPELINE_V1, maxTimeMS=90000)) should be sourced
from configuration or derived from DatabaseSettings.socket_timeout_ms with
headroom; update the aggregation invocation to use a settings value (e.g.,
settings.metric_max_time_ms) or compute maxTimeMS =
int(settings.socket_timeout_ms * 0.9) (or similar) and pass that variable into
db["urls"].aggregate, and ensure METRIC_PIPELINE_V1 and the route handler that
performs this aggregation read the same settings object so the deadline is
configurable and consistent with socket timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7103bab3-6df1-42c0-b202-76187c9ab7d3
📒 Files selected for processing (14)
app.pyconfig.pyconfig/apps.yamlmiddleware/timeout.pyroutes/auth/device.pyroutes/health_routes.pyroutes/legacy/url_shortener.pyschemas/dto/responses/common.pyservices/auth/device.pytests/integration/test_device_auth.pytests/integration/test_health.pytests/smoke/test_startup.pytests/unit/schemas/dto/test_common.pytests/unit/services/test_redirect_uri.py
Comment on lines
+26
to
+29
| # socket_timeout sized for /metric aggregation; hot-path cap lives in RequestTimeoutMiddleware. | ||
| server_selection_timeout_ms: int = 3000 | ||
| connect_timeout_ms: int = 3000 | ||
| socket_timeout_ms: int = 120000 |
There was a problem hiding this comment.
Validate the new Mongo timeout settings at startup.
These env-backed fields currently accept 0 and negative values, so a bad deployment config can disable or break the new DB-side timeout behavior at runtime. Please add the same positive-value validation you already apply to the float timeouts.
🛡️ Suggested validation
class DatabaseSettings(BaseSettings):
model_config = SettingsConfigDict(env_file=".env", extra="ignore")
@@
server_selection_timeout_ms: int = 3000
connect_timeout_ms: int = 3000
socket_timeout_ms: int = 120000
+
+ `@field_validator`(
+ "server_selection_timeout_ms", "connect_timeout_ms", "socket_timeout_ms"
+ )
+ `@classmethod`
+ def _must_be_positive_timeout_ms(cls, v: int, info) -> int:
+ if v <= 0:
+ raise ValueError(f"{info.field_name} must be > 0, got {v}")
+ return v📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # socket_timeout sized for /metric aggregation; hot-path cap lives in RequestTimeoutMiddleware. | |
| server_selection_timeout_ms: int = 3000 | |
| connect_timeout_ms: int = 3000 | |
| socket_timeout_ms: int = 120000 | |
| # socket_timeout sized for /metric aggregation; hot-path cap lives in RequestTimeoutMiddleware. | |
| server_selection_timeout_ms: int = 3000 | |
| connect_timeout_ms: int = 3000 | |
| socket_timeout_ms: int = 120000 | |
| `@field_validator`( | |
| "server_selection_timeout_ms", "connect_timeout_ms", "socket_timeout_ms" | |
| ) | |
| `@classmethod` | |
| def _must_be_positive_timeout_ms(cls, v: int, info) -> int: | |
| if v <= 0: | |
| raise ValueError(f"{info.field_name} must be > 0, got {v}") | |
| return v |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config.py` around lines 26 - 29, Add startup validation to ensure the new
Mongo timeout config ints (server_selection_timeout_ms, connect_timeout_ms,
socket_timeout_ms) are positive values just like the existing float timeouts:
check each value > 0 during config initialization and raise a clear ValueError
(or log+exit) if any are zero or negative. Locate the validation block used for
the float timeout fields and replicate the same positive-value check for these
three integer fields in the same config initialization path (the config class or
function that reads env vars) so bad deployments fail fast.
Comment on lines
+79
to
+82
| for allowed in app.redirect_uris: | ||
| if allowed.endswith("*"): | ||
| if redirect_uri.startswith(allowed[:-1]): | ||
| return True |
There was a problem hiding this comment.
Reject bare * redirect allowlist entries.
This branch also accepts allowed == "*", and redirect_uri.startswith("") then approves every callback URI. In this flow that turns one bad app entry into an open redirect for the device auth code. Require a non-empty prefix before treating * as a wildcard.
🔒 Suggested fix
for allowed in app.redirect_uris:
if allowed.endswith("*"):
- if redirect_uri.startswith(allowed[:-1]):
+ prefix = allowed[:-1]
+ if prefix and redirect_uri.startswith(prefix):
return True
elif redirect_uri == allowed:
return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for allowed in app.redirect_uris: | |
| if allowed.endswith("*"): | |
| if redirect_uri.startswith(allowed[:-1]): | |
| return True | |
| for allowed in app.redirect_uris: | |
| if allowed.endswith("*"): | |
| prefix = allowed[:-1] | |
| if prefix and redirect_uri.startswith(prefix): | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/auth/device.py` around lines 79 - 82, The wildcard branch currently
treats allowed == "*" as a valid prefix match, allowing any redirect_uri; update
the logic that iterates over app.redirect_uris so that wildcard entries require
a non-empty prefix before treating the trailing "*" as a wildcard (i.e., only
accept when allowed.endswith("*") and the prefix part allowed[:-1] is
non-empty), then perform redirect_uri.startswith(prefix) using that prefix; use
the existing loop variables (app.redirect_uris, allowed, redirect_uri) to locate
and change the check.
Comment on lines
+110
to
+116
| side_effect=lambda uri, app: ( | ||
| not uri | ||
| or any( | ||
| uri.startswith(a[:-1]) if a.endswith("*") else uri == a | ||
| for a in app.redirect_uris | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Redirect URI mock incorrectly allows empty URI when allowlist is empty
Line 111 short-circuits not uri to True, so apps with redirect_uris=[] are treated as valid for empty redirect URIs. That conflicts with the new unit contract (tests/unit/services/test_redirect_uri.py, Line 62-Line 65) and can hide redirect-validation regressions in integration tests.
Suggested fix
svc.validate_redirect_uri = MagicMock(
side_effect=lambda uri, app: (
- not uri
- or any(
- uri.startswith(a[:-1]) if a.endswith("*") else uri == a
- for a in app.redirect_uris
- )
+ bool(app.redirect_uris)
+ and (
+ not uri
+ or any(
+ uri.startswith(a[:-1]) if a.endswith("*") else uri == a
+ for a in app.redirect_uris
+ )
+ )
)
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| side_effect=lambda uri, app: ( | |
| not uri | |
| or any( | |
| uri.startswith(a[:-1]) if a.endswith("*") else uri == a | |
| for a in app.redirect_uris | |
| ) | |
| ) | |
| side_effect=lambda uri, app: ( | |
| bool(app.redirect_uris) | |
| and ( | |
| not uri | |
| or any( | |
| uri.startswith(a[:-1]) if a.endswith("*") else uri == a | |
| for a in app.redirect_uris | |
| ) | |
| ) | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/test_device_auth.py` around lines 110 - 116, The mock
side_effect currently short-circuits with "not uri" which allows empty URIs when
app.redirect_uris is empty; update the side_effect lambda so it first requires a
truthy uri and then checks any(match) against app.redirect_uris (i.e., replace
"not uri or any(...)" with "bool(uri) and any(...)" or equivalent) so an empty
allowlist (app.redirect_uris==[]) will not validate an empty URI; locate the
side_effect lambda and ensure it uses app.redirect_uris and wildcard handling
exactly as in the existing generator logic.
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
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.
Summary by Sourcery
Introduce request-level timeouts and simplify health checks to act as a pure liveness probe while expanding redirect URI validation for device auth integrations.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Improvements