From 775bee83e305a9e8508eb420737ae009020cdc72 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 30 Mar 2026 11:37:42 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20info=20disclosure,=20conditional=20routes,=20cleanu?= =?UTF-8?q?p?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace exception details in user-facing error page with generic message - Only register /app/provision routes when enable_login_flow is true - Piggyback expired provision session cleanup on hourly cleanup loop - Add multi-process limitation comment on in-memory session store - Add comment explaining login_url vs poll_endpoint rewrite asymmetry - Document curl dependency in Dockerfile (healthcheck probes) Co-Authored-By: Claude Opus 4.6 (1M context) --- Dockerfile | 5 +++-- nextcloud_mcp_server/app.py | 19 ++++++++++++++----- nextcloud_mcp_server/auth/provision_routes.py | 13 ++++++++++--- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/Dockerfile b/Dockerfile index 7d47a2ba..39b16066 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,8 +3,9 @@ FROM docker.io/library/python:3.12-slim-trixie@sha256:f3fa41d74a768c2fce8016b98c COPY --from=ghcr.io/astral-sh/uv:0.10.12@sha256:72ab0aeb448090480ccabb99fb5f52b0dc3c71923bffb5e2e26517a1c27b7fec /uv /uvx /bin/ # Install dependencies -# 1. git (required for caldav dependency from git) -# 2. sqlite for development with token db +# 1. curl (required for container healthcheck probes) +# 2. git (required for caldav dependency from git) +# 3. sqlite for development with token db RUN apt update && apt install --no-install-recommends --no-install-suggests -y \ curl \ git \ diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index e0d60554..d3835f1f 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1398,6 +1398,9 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = from nextcloud_mcp_server.auth.oauth_routes import ( # noqa: PLC0415 _cleanup_expired_proxy_codes, ) + from nextcloud_mcp_server.auth.provision_routes import ( # noqa: PLC0415 + _cleanup_expired_sessions as _cleanup_expired_provision_sessions, + ) while True: try: @@ -1407,6 +1410,8 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = logger.info(f"Cleaned up {count} expired login flow sessions") # Also clean up expired AS proxy codes/sessions _cleanup_expired_proxy_codes() + # Clean up expired web provision sessions + _cleanup_expired_provision_sessions() except Exception as e: logger.warning(f"Login flow cleanup error: {e}") await anyio.sleep(3600) # Every hour @@ -2337,11 +2342,6 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = chunk_context_endpoint, methods=["GET"], ), # /app/chunk-context - # Login Flow v2 web provisioning (used by Astrolabe) - Route("/provision", provision_page, methods=["GET"]), # /app/provision - Route( - "/provision/status", provision_status, methods=["GET"] - ), # /app/provision/status # Webhook management routes (admin-only) Route("/webhooks", webhook_management_pane, methods=["GET"]), # /app/webhooks Route( @@ -2356,6 +2356,15 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = ), ] + # Login Flow v2 web provisioning (only when Login Flow is enabled) + if settings.enable_login_flow: + browser_routes += [ + Route("/provision", provision_page, methods=["GET"]), # /app/provision + Route( + "/provision/status", provision_status, methods=["GET"] + ), # /app/provision/status + ] + # Add static files mount if directory exists static_dir = os.path.join(os.path.dirname(__file__), "auth", "static") if os.path.isdir(static_dir): diff --git a/nextcloud_mcp_server/auth/provision_routes.py b/nextcloud_mcp_server/auth/provision_routes.py index 530057c0..6842b223 100644 --- a/nextcloud_mcp_server/auth/provision_routes.py +++ b/nextcloud_mcp_server/auth/provision_routes.py @@ -30,8 +30,10 @@ from nextcloud_mcp_server.config import get_nextcloud_ssl_verify, get_settings logger = logging.getLogger(__name__) -# In-memory store for web provision sessions (short-lived, no persistence needed) -# Maps provision_id → session data +# In-memory store for web provision sessions (short-lived, no persistence needed). +# Maps provision_id → session data. +# NOTE: This does not work with multi-process deployments (e.g. uvicorn --workers N). +# Login Flow v2 mode assumes a single worker process. _provision_sessions: dict[str, dict] = {} # Session TTL: 20 minutes (matches Nextcloud's Login Flow v2 timeout) @@ -192,7 +194,9 @@ async def provision_page(request: Request) -> RedirectResponse | HTMLResponse: except Exception as e: logger.error(f"Failed to initiate Login Flow v2 for web provision: {e}") return HTMLResponse( - content=_render_error(f"Failed to start login flow: {e}"), + content=_render_error( + "Failed to start login flow. Please try again later." + ), status_code=502, ) @@ -230,6 +234,9 @@ async def provision_page(request: Request) -> RedirectResponse | HTMLResponse: # Redirect to Nextcloud's Login Flow v2 login page. # The login_url may use the internal Docker hostname (http://app/...). # Replace with the public Nextcloud URL for the browser. + # Note: poll_endpoint is rewritten to NEXTCLOUD_HOST (server-side, in + # LoginFlowV2Client) while login_url is rewritten to the public issuer + # URL here because the browser needs a publicly-reachable address. login_url = init_response.login_url public_issuer = os.getenv("NEXTCLOUD_PUBLIC_ISSUER_URL", "") if public_issuer and nextcloud_host: