Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions backend/druks/api/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

from fastapi import Depends, FastAPI, HTTPException, Request
from fastapi.exceptions import RequestValidationError
from fastapi.responses import JSONResponse
from fastapi.responses import HTMLResponse, JSONResponse
from fastmcp.utilities.lifespan import combine_lifespans
from starlette.datastructures import MutableHeaders
from starlette.routing import Route
Expand All @@ -20,6 +20,7 @@
from druks.api.subjects import router as subjects_router
from druks.browser.exceptions import BrowserApiError
from druks.browser.routes import router as browser_sessions_router
from druks.core.templates import render_page
from druks.database import configure_session, create_engine_from_url, db_session, session_scope
from druks.durable.engine import init_dbos, launch, shutdown
from druks.durable.exceptions import AgentCallNotFound
Expand All @@ -34,7 +35,7 @@
from druks.notifications.routes import external_router as notifications_external_router
from druks.notifications.routes import router as notifications_router
from druks.redis import close_client
from druks.services.exceptions import ServiceNotConnectedError
from druks.services.exceptions import OauthPageError, ServiceNotConnectedError
from druks.services.routes import oauth_router
from druks.services.routes import router as service_identities_router
from druks.settings import Settings, ensure_data_dirs, load_settings, setup_logging
Expand Down Expand Up @@ -190,6 +191,13 @@ async def _service_not_connected_handler(
return JSONResponse(status_code=409, content={"error": "HTTP_409", "detail": str(exc)})


# The connect and callback doors are reached by full-page browser navigation,
# so a failure renders an operator page, not the JSON envelope every fetch gets.
@app.exception_handler(OauthPageError)
async def _oauth_page_error_handler(request: Request, exc: OauthPageError) -> HTMLResponse:
return render_page("service_oauth_error.html", message=str(exc), status_code=exc.status_code)


# Browser routes raise their typed error and let this name the status, so no
# route hand-maps one.
@app.exception_handler(BrowserApiError)
Expand Down
9 changes: 4 additions & 5 deletions backend/druks/core/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@
)


def render_page(template: str, **context: Any) -> HTMLResponse:
"""One of the operator-facing pages in ``core/templates`` — server-rendered
browser stops (connect callbacks, the GitHub App manifest form) that share
the dashboard's chrome."""
return HTMLResponse(_templates.get_template(template).render(context))
def render_page(template: str, *, status_code: int = 200, **context: Any) -> HTMLResponse:
"""An operator-facing page from ``core/templates`` — a server-rendered
browser stop that shares the dashboard's chrome."""
return HTMLResponse(_templates.get_template(template).render(context), status_code=status_code)
5 changes: 5 additions & 0 deletions backend/druks/core/templates/service_oauth_error.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{% extends "page.html" %}
{% block content %}
<p>{{ message }}</p>
<p><a href="/">Return to druks</a></p>
{% endblock %}
9 changes: 9 additions & 0 deletions backend/druks/services/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,15 @@ class ServiceConnectError(Exception):
and safe to show; it never quotes anything the operator pasted."""


class OauthPageError(Exception):
"""A failure on a browser-navigated OAuth door — the connect and callback
routes, whose failures render an operator page instead of the JSON envelope."""

def __init__(self, message: str, *, status_code: int) -> None:
super().__init__(message)
self.status_code = status_code


class OauthExchangeError(Exception):
"""Completing an OAuth connect flow failed — an unknown or expired state,
or a rejected code exchange. Nothing is stored on failure, so re-running
Expand Down
31 changes: 15 additions & 16 deletions backend/druks/services/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from druks.extensions.registry import services
from druks.services.exceptions import (
OauthExchangeError,
OauthPageError,
ServiceConnectError,
ServiceNotConnectedError,
)
Expand Down Expand Up @@ -69,7 +70,7 @@ async def connect_service(slug: str, payload: dict[str, str]) -> ServiceResponse
def _get_oauth_service(slug: str):
service = services.get(slug)
if not service or not service.token_endpoint:
raise HTTPException(status_code=404, detail=f"No OAuth service {slug!r}.")
raise OauthPageError(f"No OAuth service {slug!r}.", status_code=404)
return service


Expand All @@ -82,23 +83,21 @@ async def connect_oauth_service(
if connection:
row = OauthConnection.get(connection)
if not row or row.provider != slug:
raise HTTPException(
status_code=404, detail=f"No connection {connection!r} on {slug!r}."
)
raise OauthPageError(f"No connection {connection!r} on {slug!r}.", status_code=404)
if next and (not next.startswith("/") or next.startswith(("//", "/\\"))):
# A bare same-origin path only — anything host-shaped is an open redirect.
raise HTTPException(status_code=422, detail="next must be a path starting with '/'.")
raise OauthPageError("next must be a path starting with '/'.", status_code=422)
endpoint = request.app.state.settings.urls.endpoint
if not endpoint:
raise HTTPException(
status_code=409,
detail="The provider redirects the operator's browser back to druks. "
raise OauthPageError(
"The provider redirects the operator's browser back to druks. "
"Set urls.endpoint to the address druks has in that browser.",
status_code=409,
)
try:
client = service.get_oauth_client()
except ServiceNotConnectedError as error:
raise HTTPException(status_code=409, detail=str(error)) from error
raise OauthPageError(str(error), status_code=409) from error
url = await client.begin_connect(
redirect_uri=f"{endpoint.rstrip('/')}/api/oauth/callback",
scopes=service.required_scopes(),
Expand All @@ -110,20 +109,20 @@ async def connect_oauth_service(
@oauth_router.get("/callback", response_class=HTMLResponse)
async def oauth_callback(state: str = "", code: str = "", error: str = "") -> Response:
if error:
raise HTTPException(
status_code=400, detail=f"The authorization server denied the request: {error}"
raise OauthPageError(
f"The authorization server denied the request: {error}", status_code=400
)
if not state or not code:
raise HTTPException(status_code=400, detail="Missing state or code in the callback.")
raise OauthPageError("Missing state or code in the callback.", status_code=400)
try:
tokens, pending = await complete_connect(state=state, code=code)
except OauthExchangeError as exchange_error:
raise HTTPException(status_code=400, detail=str(exchange_error)) from exchange_error
raise OauthPageError(str(exchange_error), status_code=400) from exchange_error
provider = pending["provider"]
service = services.get(provider)
if not service:
# A state begun by another door (an MCP connect) finishes at its own callback.
raise HTTPException(status_code=400, detail=f"No OAuth service {provider!r}.")
raise OauthPageError(f"No OAuth service {provider!r}.", status_code=400)
granted = tokens.get("scope", "").split() or pending["scopes"]
identity = await service.get_identity(tokens["access_token"])
connection_id = pending["connection_id"]
Expand All @@ -133,8 +132,8 @@ async def oauth_callback(state: str = "", code: str = "", error: str = "") -> Re
if connection_id:
row = OauthConnection.get(connection_id)
if not row:
raise HTTPException(
status_code=400, detail="The connection was removed while consent was open."
raise OauthPageError(
"The connection was removed while consent was open.", status_code=400
)
elif service.identity_key and (value := identity.get(service.identity_key)):
row = OauthConnection.get_for_identity(
Expand Down
23 changes: 22 additions & 1 deletion backend/tests/test_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,28 @@ def test_oauth_connect_guards(tmp_path, acme, druks_db):
# The client credentials are not connected yet.
response = client.get("/api/oauth/acme/connect", follow_redirects=False)
assert response.status_code == 409
assert "not connected" in response.json()["detail"]
assert "not connected" in response.text


def test_a_failed_connect_renders_an_operator_page(tmp_path, acme, druks_db):
from druks.testing import configure_app_for_test

with TestClient(configure_app_for_test(settings=make_settings(tmp_path))) as client:
# The connect door is reached full-page, so a failure is a page that
# names the fix — the dashboard chrome, not the JSON envelope.
page = client.get("/api/oauth/acme/connect", follow_redirects=False)
assert page.status_code == 409
assert page.headers["content-type"].startswith("text/html")
assert "urls.endpoint" in page.text
assert '<div class="wordmark">druks</div>' in page.text

# The callback is a browser stop too — a denied consent renders a page.
denied = client.get(
"/api/oauth/callback", params={"state": "s", "code": "c", "error": "denied"}
)
assert denied.status_code == 400
assert denied.headers["content-type"].startswith("text/html")
assert "denied" in denied.text


async def test_oauth_callback_creates_and_reconnects_a_connection(
Expand Down