fix: address PR review round 3 — info disclosure, conditional routes, cleanup
- 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
2508f36ebf
commit
775bee83e3
+3
-2
@@ -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/
|
COPY --from=ghcr.io/astral-sh/uv:0.10.12@sha256:72ab0aeb448090480ccabb99fb5f52b0dc3c71923bffb5e2e26517a1c27b7fec /uv /uvx /bin/
|
||||||
|
|
||||||
# Install dependencies
|
# Install dependencies
|
||||||
# 1. git (required for caldav dependency from git)
|
# 1. curl (required for container healthcheck probes)
|
||||||
# 2. sqlite for development with token db
|
# 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 \
|
RUN apt update && apt install --no-install-recommends --no-install-suggests -y \
|
||||||
curl \
|
curl \
|
||||||
git \
|
git \
|
||||||
|
|||||||
@@ -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
|
from nextcloud_mcp_server.auth.oauth_routes import ( # noqa: PLC0415
|
||||||
_cleanup_expired_proxy_codes,
|
_cleanup_expired_proxy_codes,
|
||||||
)
|
)
|
||||||
|
from nextcloud_mcp_server.auth.provision_routes import ( # noqa: PLC0415
|
||||||
|
_cleanup_expired_sessions as _cleanup_expired_provision_sessions,
|
||||||
|
)
|
||||||
|
|
||||||
while True:
|
while True:
|
||||||
try:
|
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")
|
logger.info(f"Cleaned up {count} expired login flow sessions")
|
||||||
# Also clean up expired AS proxy codes/sessions
|
# Also clean up expired AS proxy codes/sessions
|
||||||
_cleanup_expired_proxy_codes()
|
_cleanup_expired_proxy_codes()
|
||||||
|
# Clean up expired web provision sessions
|
||||||
|
_cleanup_expired_provision_sessions()
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.warning(f"Login flow cleanup error: {e}")
|
logger.warning(f"Login flow cleanup error: {e}")
|
||||||
await anyio.sleep(3600) # Every hour
|
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,
|
chunk_context_endpoint,
|
||||||
methods=["GET"],
|
methods=["GET"],
|
||||||
), # /app/chunk-context
|
), # /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)
|
# Webhook management routes (admin-only)
|
||||||
Route("/webhooks", webhook_management_pane, methods=["GET"]), # /app/webhooks
|
Route("/webhooks", webhook_management_pane, methods=["GET"]), # /app/webhooks
|
||||||
Route(
|
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
|
# Add static files mount if directory exists
|
||||||
static_dir = os.path.join(os.path.dirname(__file__), "auth", "static")
|
static_dir = os.path.join(os.path.dirname(__file__), "auth", "static")
|
||||||
if os.path.isdir(static_dir):
|
if os.path.isdir(static_dir):
|
||||||
|
|||||||
@@ -30,8 +30,10 @@ from nextcloud_mcp_server.config import get_nextcloud_ssl_verify, get_settings
|
|||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
# In-memory store for web provision sessions (short-lived, no persistence needed)
|
# In-memory store for web provision sessions (short-lived, no persistence needed).
|
||||||
# Maps provision_id → session data
|
# 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] = {}
|
_provision_sessions: dict[str, dict] = {}
|
||||||
|
|
||||||
# Session TTL: 20 minutes (matches Nextcloud's Login Flow v2 timeout)
|
# 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:
|
except Exception as e:
|
||||||
logger.error(f"Failed to initiate Login Flow v2 for web provision: {e}")
|
logger.error(f"Failed to initiate Login Flow v2 for web provision: {e}")
|
||||||
return HTMLResponse(
|
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,
|
status_code=502,
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -230,6 +234,9 @@ async def provision_page(request: Request) -> RedirectResponse | HTMLResponse:
|
|||||||
# Redirect to Nextcloud's Login Flow v2 login page.
|
# Redirect to Nextcloud's Login Flow v2 login page.
|
||||||
# The login_url may use the internal Docker hostname (http://app/...).
|
# The login_url may use the internal Docker hostname (http://app/...).
|
||||||
# Replace with the public Nextcloud URL for the browser.
|
# 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
|
login_url = init_response.login_url
|
||||||
public_issuer = os.getenv("NEXTCLOUD_PUBLIC_ISSUER_URL", "")
|
public_issuer = os.getenv("NEXTCLOUD_PUBLIC_ISSUER_URL", "")
|
||||||
if public_issuer and nextcloud_host:
|
if public_issuer and nextcloud_host:
|
||||||
|
|||||||
Reference in New Issue
Block a user