-
-
Notifications
You must be signed in to change notification settings - Fork 50
Add circuit breakers #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add circuit breakers #167
Changes from all commits
8fbb002
3a9d235
0e73272
12f1118
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| """Request deadline middleware — caps request time below the platform timeout.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
|
|
||
| from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint | ||
| from starlette.requests import Request | ||
| from starlette.responses import JSONResponse, Response | ||
|
|
||
| from infrastructure.logging import get_logger | ||
| from shared.ip_utils import get_client_ip | ||
|
|
||
| log = get_logger("spoo.timeout") | ||
|
|
||
| EXEMPT_PATHS: frozenset[str] = frozenset({"/metric"}) | ||
|
|
||
|
|
||
| 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), | ||
| ) | ||
|
Comment on lines
+33
to
+39
|
||
| return JSONResponse( | ||
| status_code=504, | ||
| content={"error": "Request timed out", "code": "request_timeout"}, | ||
| ) | ||
|
Comment on lines
+19
to
+43
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -67,8 +67,22 @@ def resolve_app(self, app_id: str) -> AppEntry | None: | |||||||||||||||||||
| return entry if entry and entry.is_live_device_app() else None | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def validate_redirect_uri(self, redirect_uri: str, app: AppEntry) -> bool: | ||||||||||||||||||||
| """Return True if redirect_uri is empty or in the app's allowlist.""" | ||||||||||||||||||||
| return not redirect_uri or redirect_uri in app.redirect_uris | ||||||||||||||||||||
| """Return True if redirect_uri is empty, exact-matches an allowlist | ||||||||||||||||||||
| entry, or prefix-matches an entry ending with ``*``. | ||||||||||||||||||||
|
|
||||||||||||||||||||
| The ``*`` suffix is used for OAuth clients (e.g. Raycast) that append | ||||||||||||||||||||
| a varying query string to a fixed redirect URL. Only exact or | ||||||||||||||||||||
| explicit-wildcard entries are accepted; implicit wildcards are not. | ||||||||||||||||||||
| """ | ||||||||||||||||||||
| if not redirect_uri: | ||||||||||||||||||||
| return True | ||||||||||||||||||||
| for allowed in app.redirect_uris: | ||||||||||||||||||||
| if allowed.endswith("*"): | ||||||||||||||||||||
| if redirect_uri.startswith(allowed[:-1]): | ||||||||||||||||||||
| return True | ||||||||||||||||||||
|
Comment on lines
+79
to
+82
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reject bare This branch also accepts 🔒 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| elif redirect_uri == allowed: | ||||||||||||||||||||
| return True | ||||||||||||||||||||
| return False | ||||||||||||||||||||
|
|
||||||||||||||||||||
| async def create_device_auth_code( | ||||||||||||||||||||
| self, user_id: ObjectId, email: str, app_id: str | None = None | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -107,7 +107,13 @@ def device_auth_svc(): | |||||||||||||||||||||||||||||||||||
| # 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 | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
109
to
+115
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): Consider at least one integration test that uses the real The mock now copies the wildcard logic from the implementation, so existing tests will keep passing even if the real Please add a single integration test that:
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:
|
||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+110
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redirect URI mock incorrectly allows empty URI when allowlist is empty Line 111 short-circuits 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| return svc | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the new Mongo timeout settings at startup.
These env-backed fields currently accept
0and 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
🤖 Prompt for AI Agents