From ce80a36877f0224298afb466bcc2d61c2c101a77 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 2 May 2026 17:24:50 +0200 Subject: [PATCH] fix(auth): address PR #757 round-3 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review items from the third-round review on PR #757: - scope_authorization: split the combined logger.warning(error_msg) in the require_scopes decorator's missing-app-password branch into two lazy %-style logger calls (one per branch), keeping the f-string error_msg for the exception only. The else branch also logs the elicit_result for diagnostics. Bypassing lazy %-interpolation in security-sensitive code formatted the message regardless of log level and matched the repo-wide lazy-logging preference; the new code now conforms. - config + browser_oauth_routes: wire COOKIE_SECURE through Settings (cookie_secure: bool | None = None) so _should_use_secure_cookies() reads it via get_settings() rather than os.getenv. Completes the consolidation pass that touched this file in commit 7464340 and removes the last raw os.getenv from browser_oauth_routes.py (import os dropped). Dynaconf auto-coerces "true"/"false" → bool; "1"/"0" arrive as int and are normalised by an explicit bool() at the consumer. - elicitation: clarify the _astrolabe_settings_url docstring to call out that the empty-string case is also a None-return path (matches the existing `if not base:` guard). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../auth/browser_oauth_routes.py | 23 ++++++++----------- nextcloud_mcp_server/auth/elicitation.py | 7 +++--- .../auth/scope_authorization.py | 13 ++++++++++- nextcloud_mcp_server/config.py | 7 ++++++ 4 files changed, 33 insertions(+), 17 deletions(-) diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index f579d9f9..8b33b0fb 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -6,7 +6,6 @@ for accessing admin UI endpoints like /app. import hashlib import logging -import os import secrets import time from base64 import urlsafe_b64encode @@ -30,23 +29,21 @@ logger = logging.getLogger(__name__) def _should_use_secure_cookies() -> bool: - """Determine if cookies should have secure flag. + """Determine if cookies should have the Secure flag. - Checks COOKIE_SECURE env var first, then auto-detects from NEXTCLOUD_HOST. + Reads ``settings.cookie_secure`` first (set via the ``COOKIE_SECURE`` + env var). Falls back to auto-detect from the ``nextcloud_host`` scheme + when unset. Returns: True if cookies should be secure (HTTPS), False otherwise """ - # Explicit configuration takes precedence - explicit = os.getenv("COOKIE_SECURE", "").lower() - if explicit == "true": - return True - if explicit == "false": - return False - - # Auto-detect from NEXTCLOUD_HOST protocol (read via Settings for - # consistency with the rest of this file). - nextcloud_host = get_settings().nextcloud_host or "" + settings = get_settings() + if settings.cookie_secure is not None: + # Dynaconf auto-coerces "true"/"false" → bool but "1"/"0" → int; + # bool() normalises both. + return bool(settings.cookie_secure) + nextcloud_host = settings.nextcloud_host or "" return nextcloud_host.startswith("https://") diff --git a/nextcloud_mcp_server/auth/elicitation.py b/nextcloud_mcp_server/auth/elicitation.py index 014c9b34..57b59d4c 100644 --- a/nextcloud_mcp_server/auth/elicitation.py +++ b/nextcloud_mcp_server/auth/elicitation.py @@ -46,9 +46,10 @@ def _astrolabe_settings_url() -> str | None: Prefers ``nextcloud_public_issuer_url`` (the browser-reachable public URL) over ``nextcloud_host`` (which may be an internal hostname in Docker - deployments). Returns None if neither is set, or if the configured base - URL is missing an http:// or https:// scheme — in the latter case the - caller renders the tool-only fallback message instead of a broken link. + deployments). Returns None if neither is set (or set to the empty + string), or if the configured base URL is missing an http:// or + https:// scheme — in the latter case the caller renders the tool-only + fallback message instead of a broken link. """ settings = get_settings() base = ( diff --git a/nextcloud_mcp_server/auth/scope_authorization.py b/nextcloud_mcp_server/auth/scope_authorization.py index 09eef207..6e6efd66 100644 --- a/nextcloud_mcp_server/auth/scope_authorization.py +++ b/nextcloud_mcp_server/auth/scope_authorization.py @@ -179,6 +179,11 @@ def require_scopes(*required_scopes: str): # time the next retry can still hit a not-yet- # populated entry — hence the "wait a moment" # qualifier below. + logger.warning( + "Access denied to %s: app password missing " + "after user accepted elicitation; advising retry", + func_name, + ) error_msg = ( f"Access denied to {func_name}: Nextcloud " f"access was not provisioned at the time of " @@ -188,12 +193,18 @@ def require_scopes(*required_scopes: str): f"completing; wait a moment and try again." ) else: + logger.warning( + "Access denied to %s: app password missing; " + "advising nc_auth_provision_access " + "(elicit_result=%s)", + func_name, + elicit_result, + ) error_msg = ( f"Access denied to {func_name}: " f"Nextcloud access not provisioned. " f"Please call 'nc_auth_provision_access' first." ) - logger.warning(error_msg) raise ProvisioningRequiredError(error_msg) if stored_scopes == "all": diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 95652f80..eb2cb59f 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -32,6 +32,7 @@ _DEFAULTS: dict[str, Any] = { "nextcloud_mcp_server_url": None, "nextcloud_resource_uri": None, "nextcloud_public_issuer_url": None, + "cookie_secure": None, # OAuth/OIDC "oidc_discovery_url": None, "nextcloud_oidc_client_id": None, @@ -412,6 +413,11 @@ class Settings: # nextcloud_host when unset. nextcloud_public_issuer_url: str | None = None + # Browser cookie Secure flag. None = auto-detect from nextcloud_host + # scheme (https → True, else False). Set COOKIE_SECURE=true/false to + # override. + cookie_secure: bool | None = None + # Nextcloud SSL/TLS settings nextcloud_verify_ssl: bool = True nextcloud_ca_bundle: str | None = None @@ -784,6 +790,7 @@ def get_settings() -> Settings: "nextcloud_password": "NEXTCLOUD_PASSWORD", "nextcloud_app_password": "NEXTCLOUD_APP_PASSWORD", "nextcloud_public_issuer_url": "NEXTCLOUD_PUBLIC_ISSUER_URL", + "cookie_secure": "COOKIE_SECURE", # Nextcloud SSL/TLS settings "nextcloud_verify_ssl": "NEXTCLOUD_VERIFY_SSL", "nextcloud_ca_bundle": "NEXTCLOUD_CA_BUNDLE",