diff --git a/env.sample b/env.sample index 5cd983e..d9dc0cf 100644 --- a/env.sample +++ b/env.sample @@ -51,6 +51,11 @@ NEXTCLOUD_MCP_SERVER_URL=http://localhost:8000 NEXTCLOUD_USERNAME= NEXTCLOUD_PASSWORD= +# Cookie security (browser UI) +# Auto-detects from NEXTCLOUD_HOST protocol if not set +# Set explicitly for non-standard setups +#COOKIE_SECURE=true + # ============================================ # Document Processing Configuration # ============================================ diff --git a/nextcloud_mcp_server/api/management.py b/nextcloud_mcp_server/api/management.py index 00ba141..99a5fd4 100644 --- a/nextcloud_mcp_server/api/management.py +++ b/nextcloud_mcp_server/api/management.py @@ -116,6 +116,54 @@ def _sanitize_error_for_client(error: Exception, context: str = "") -> str: return "An internal error occurred. Please contact your administrator." +def _parse_int_param( + value: str | None, + default: int, + min_val: int, + max_val: int, + param_name: str, +) -> int: + """Parse and validate integer parameter.""" + if value is None: + return default + try: + parsed = int(value) + except ValueError: + raise ValueError(f"Invalid {param_name}: must be an integer") + if parsed < min_val or parsed > max_val: + raise ValueError( + f"Invalid {param_name}: must be between {min_val} and {max_val}" + ) + return parsed + + +def _parse_float_param( + value: Any, + default: float, + min_val: float, + max_val: float, + param_name: str, +) -> float: + """Parse and validate float parameter.""" + if value is None: + return default + try: + parsed = float(value) + except (ValueError, TypeError): + raise ValueError(f"Invalid {param_name}: must be a number") + if parsed < min_val or parsed > max_val: + raise ValueError( + f"Invalid {param_name}: must be between {min_val} and {max_val}" + ) + return parsed + + +def _validate_query_string(query: str, max_length: int = 10000) -> None: + """Validate query string length.""" + if len(query) > max_length: + raise ValueError(f"Query too long: maximum {max_length} characters") + + async def get_server_status(request: Request) -> JSONResponse: """GET /api/v1/status - Server status and version. @@ -365,7 +413,10 @@ async def revoke_user_access(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/users/{{user_id}}/revoke: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "revoke_user_access"), + }, status_code=401, ) @@ -441,7 +492,10 @@ async def get_installed_apps(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/apps: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "get_installed_apps"), + }, status_code=401, ) @@ -489,7 +543,10 @@ async def get_installed_apps(request: Request) -> JSONResponse: except Exception as e: logger.error(f"Error getting installed apps for user {user_id}: {e}") return JSONResponse( - {"error": "Internal error", "message": str(e)}, + { + "error": "Internal error", + "message": _sanitize_error_for_client(e, "get_installed_apps"), + }, status_code=500, ) @@ -507,7 +564,10 @@ async def list_webhooks(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "list_webhooks"), + }, status_code=401, ) @@ -543,7 +603,10 @@ async def list_webhooks(request: Request) -> JSONResponse: except Exception as e: logger.error(f"Error listing webhooks for user {user_id}: {e}") return JSONResponse( - {"error": "Internal error", "message": str(e)}, + { + "error": "Internal error", + "message": _sanitize_error_for_client(e, "list_webhooks"), + }, status_code=500, ) @@ -568,7 +631,10 @@ async def create_webhook(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "create_webhook"), + }, status_code=401, ) @@ -622,7 +688,10 @@ async def create_webhook(request: Request) -> JSONResponse: except Exception as e: logger.error(f"Error creating webhook for user {user_id}: {e}") return JSONResponse( - {"error": "Internal error", "message": str(e)}, + { + "error": "Internal error", + "message": _sanitize_error_for_client(e, "create_webhook"), + }, status_code=500, ) @@ -640,7 +709,10 @@ async def delete_webhook(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/webhooks: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "delete_webhook"), + }, status_code=401, ) @@ -692,7 +764,10 @@ async def delete_webhook(request: Request) -> JSONResponse: except Exception as e: logger.error(f"Error deleting webhook for user {user_id}: {e}") return JSONResponse( - {"error": "Internal error", "message": str(e)}, + { + "error": "Internal error", + "message": _sanitize_error_for_client(e, "delete_webhook"), + }, status_code=500, ) @@ -747,19 +822,50 @@ async def unified_search(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/search: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "unified_search"), + }, status_code=401, ) try: # Parse request body body = await request.json() - query = body.get("query", "") + + # Validate and parse parameters + try: + query = body.get("query", "") + _validate_query_string(query, max_length=10000) + + limit = _parse_int_param( + str(body.get("limit")) if body.get("limit") is not None else None, + 20, + 1, + 100, + "limit", + ) + + offset = _parse_int_param( + str(body.get("offset")) if body.get("offset") is not None else None, + 0, + 0, + 1000000, + "offset", + ) + + score_threshold = _parse_float_param( + body.get("score_threshold"), + 0.0, + 0.0, + 1.0, + "score_threshold", + ) + except ValueError as e: + return JSONResponse({"error": str(e)}, status_code=400) + algorithm = body.get("algorithm", "hybrid") fusion = body.get("fusion", "rrf") - score_threshold = body.get("score_threshold", 0.0) - limit = min(body.get("limit", 20), 100) # Enforce max limit - offset = body.get("offset", 0) include_pca = body.get("include_pca", False) include_chunks = body.get("include_chunks", True) doc_types = body.get("doc_types") # Optional filter @@ -777,9 +883,6 @@ async def unified_search(request: Request) -> JSONResponse: if fusion not in valid_fusions: fusion = "rrf" - # Validate score threshold - score_threshold = max(0.0, min(1.0, float(score_threshold))) - # Execute search using the appropriate algorithm from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, @@ -916,7 +1019,10 @@ async def unified_search(request: Request) -> JSONResponse: except Exception as e: logger.error(f"Error in unified search: {e}") return JSONResponse( - {"error": "Internal error", "message": str(e)}, + { + "error": "Internal error", + "message": _sanitize_error_for_client(e, "unified_search"), + }, status_code=500, ) @@ -953,7 +1059,10 @@ async def vector_search(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/vector-viz/search: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "vector_search"), + }, status_code=401, ) @@ -1118,7 +1227,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: except Exception as e: logger.warning(f"Unauthorized access to /api/v1/chunk-context: {e}") return JSONResponse( - {"error": "Unauthorized", "message": str(e)}, + { + "error": "Unauthorized", + "message": _sanitize_error_for_client(e, "get_chunk_context"), + }, status_code=401, ) @@ -1128,7 +1240,6 @@ async def get_chunk_context(request: Request) -> JSONResponse: doc_id = request.query_params.get("doc_id") start_str = request.query_params.get("start") end_str = request.query_params.get("end") - context_chars = int(request.query_params.get("context", "500")) # Validate required parameters if not all([doc_type, doc_id, start_str, end_str]): @@ -1146,8 +1257,21 @@ async def get_chunk_context(request: Request) -> JSONResponse: assert doc_id is not None assert doc_type is not None - start = int(start_str) - end = int(end_str) + # Parse and validate integer parameters with bounds checking + try: + context_chars = _parse_int_param( + request.query_params.get("context"), + 500, + 0, + 10000, + "context_chars", + ) + start = _parse_int_param(start_str, 0, 0, 10000000, "start") + end = _parse_int_param(end_str, 0, 0, 10000000, "end") + if end <= start: + raise ValueError("end must be greater than start") + except ValueError as e: + return JSONResponse({"success": False, "error": str(e)}, status_code=400) # Convert doc_id to int if possible (most IDs are int) doc_id_val: str | int = int(doc_id) if doc_id.isdigit() else doc_id diff --git a/nextcloud_mcp_server/auth/browser_oauth_routes.py b/nextcloud_mcp_server/auth/browser_oauth_routes.py index 8f939be..ffeefdd 100644 --- a/nextcloud_mcp_server/auth/browser_oauth_routes.py +++ b/nextcloud_mcp_server/auth/browser_oauth_routes.py @@ -24,6 +24,26 @@ from nextcloud_mcp_server.auth.userinfo_routes import ( logger = logging.getLogger(__name__) +def _should_use_secure_cookies() -> bool: + """Determine if cookies should have secure flag. + + Checks COOKIE_SECURE env var first, then auto-detects from NEXTCLOUD_HOST. + + 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 + nextcloud_host = os.getenv("NEXTCLOUD_HOST", "") + return nextcloud_host.startswith("https://") + + async def oauth_login(request: Request) -> RedirectResponse | JSONResponse: """Browser OAuth login endpoint - redirects to IdP for authentication. @@ -447,7 +467,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo value=user_id, max_age=86400 * 30, # 30 days httponly=True, - secure=False, # Set to True in production with HTTPS + secure=_should_use_secure_cookies(), samesite="lax", ) diff --git a/nextcloud_mcp_server/auth/token_broker.py b/nextcloud_mcp_server/auth/token_broker.py index 36ab339..c02c64f 100644 --- a/nextcloud_mcp_server/auth/token_broker.py +++ b/nextcloud_mcp_server/auth/token_broker.py @@ -321,6 +321,8 @@ class TokenBrokerService: """ # Check cache first (background tokens can be cached) cache_key = f"{user_id}:background:{','.join(sorted(required_scopes))}" + refresh_in_progress_key = f"{user_id}:refresh_in_progress" + cached_token = await self.cache.get(cache_key) if cached_token: return cached_token @@ -337,13 +339,28 @@ class TokenBrokerService: ) return cached_token - # Get stored refresh token - refresh_data = await self.storage.get_refresh_token(user_id) - if not refresh_data: - logger.info(f"No refresh token found for user {user_id}") - return None + # Check if another thread is currently refreshing + if await self.cache.get(refresh_in_progress_key): + logger.debug(f"Refresh in progress for user {user_id}, waiting briefly") + await anyio.sleep(0.1) # Brief wait for in-progress refresh + # Check cache one more time after wait + cached_token = await self.cache.get(cache_key) + if cached_token: + logger.debug( + f"Token refreshed by another thread for user {user_id}" + ) + return cached_token + + # Mark refresh as in-progress + await self.cache.set(refresh_in_progress_key, "true", expires_in=5) try: + # Get stored refresh token + refresh_data = await self.storage.get_refresh_token(user_id) + if not refresh_data: + logger.info(f"No refresh token found for user {user_id}") + return None + # storage.get_refresh_token() returns already-decrypted token refresh_token = refresh_data["refresh_token"] @@ -370,6 +387,10 @@ class TokenBrokerService: await self.cache.invalidate(cache_key) return None + finally: + # Always clear the in-progress marker + await self.cache.invalidate(refresh_in_progress_key) + async def _refresh_access_token( self, refresh_token: str, user_id: str | None = None ) -> Tuple[str, int]: diff --git a/third_party/astrolabe/lib/Service/IdpTokenRefresher.php b/third_party/astrolabe/lib/Service/IdpTokenRefresher.php index 8d73cfa..d57e051 100644 --- a/third_party/astrolabe/lib/Service/IdpTokenRefresher.php +++ b/third_party/astrolabe/lib/Service/IdpTokenRefresher.php @@ -35,6 +35,31 @@ class IdpTokenRefresher { $this->mcpServerClient = $mcpServerClient; } + /** + * Get Nextcloud base URL for constructing internal OIDC endpoint URLs. + * + * @return string Base URL (e.g., "https://nextcloud.example.com") + */ + private function getNextcloudBaseUrl(): string { + // Prefer explicit CLI URL override + $baseUrl = $this->config->getSystemValue('overwrite.cli.url', ''); + + if (!empty($baseUrl)) { + return rtrim($baseUrl, '/'); + } + + // Fallback to first trusted domain with protocol + $trustedDomains = $this->config->getSystemValue('trusted_domains', []); + if (!empty($trustedDomains)) { + $protocol = $this->config->getSystemValue('overwriteprotocol', 'https'); + return $protocol . '://' . $trustedDomains[0]; + } + + // Last resort: localhost (log warning) + $this->logger->warning('IdpTokenRefresher: No Nextcloud URL configured, using localhost fallback'); + return 'http://localhost'; + } + /** * Refresh access token using refresh token. * @@ -87,8 +112,8 @@ class IdpTokenRefresher { $tokenEndpoint = $discovery['token_endpoint']; } else { - // Nextcloud's OIDC app - use internal URL directly - $tokenEndpoint = 'http://localhost/apps/oidc/token'; + // Nextcloud's OIDC app - use internal URL + $tokenEndpoint = $this->getNextcloudBaseUrl() . '/apps/oidc/token'; $this->logger->info('IdpTokenRefresher: Using Nextcloud OIDC app', [ 'token_endpoint' => $tokenEndpoint, @@ -119,6 +144,18 @@ class IdpTokenRefresher { throw new \RuntimeException('Invalid token response from IdP'); } + // Validate refresh_token is present (required for token rotation) + if (!isset($tokenData['refresh_token'])) { + $this->logger->error( + 'IdpTokenRefresher: No refresh token in response - token rotation will fail', + [ + 'has_access_token' => isset($tokenData['access_token']), + 'response_keys' => array_keys($tokenData), + ] + ); + return null; + } + $this->logger->info('IdpTokenRefresher: Token refresh successful'); return $tokenData;