fix(security): address critical security issues from PR #401 code review

Implemented 6 critical security fixes identified during PR #401 review: 1. Token Rotation Race Condition (Issue 1) - Added in-progress marker pattern to prevent concurrent refresh - Prevents token invalidation when multiple requests refresh simultaneously - File: token_broker.py:324, 343-390 2. Hardcoded Localhost URL (Issue 2) - Added getNextcloudBaseUrl() with fallback chain - Supports overwrite.cli.url, trusted_domains, and localhost fallback - File: IdpTokenRefresher.php:38-61, 116 3. Error Information Leakage (Issue 3) - Replaced 13 instances of str(e) with sanitized errors - Prevents exposure of stack traces, paths, and tokens - File: management.py:368, 444, 492, 510, 546, 571, 625, 643, 695, 750, 919, 956, 1121 4. Input Validation Gaps (Issue 4) - Added validation helpers: _parse_int_param, _parse_float_param, _validate_query_string - Applied bounds checking to get_chunk_context and unified_search - File: management.py:119-164, 807-835, 1197-1212 5. PHP Refresh Token Validation (Issue 5) - Added explicit refresh_token presence check - Prevents silent token rotation failures - File: IdpTokenRefresher.php:122-132 6. Cookie Security Configuration (Issue 6) - Added _should_use_secure_cookies() with auto-detection - Supports explicit COOKIE_SECURE env var or auto-detect from NEXTCLOUD_HOST - Files: browser_oauth_routes.py:27-44, 470; env.sample:54-57 Testing: - Unit tests: 195 passed - Integration tests: 102 passed, 4 skipped - OAuth tests: 9 passed - All linting and type checks passed Follow-up work tracked in issues #408-#417 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
2025-12-19 13:57:33 +01:00
parent fe54733a39
commit daabd90359
5 changed files with 238 additions and 31 deletions
@@ -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
 # ============================================
@@ -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

@@ -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",
    )

@@ -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]:
@@ -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;