From be360edab0b7809448726960eacdc430a759283a Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 1 Jun 2026 15:15:48 +0200 Subject: [PATCH] fix(api): distinguish Nextcloud 5xx from auth failure; tighten body parse (#824) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #825 review round 2: - _validate_nextcloud_credentials now only maps OCS HTTP 401/403 to a 401 "invalid credential"; any other non-200 (5xx, 503 maintenance mode) surfaces as 502 "Nextcloud returned a server error" so ops don't chase a phantom bad password when Nextcloud is actually down. - The client-facing 401 message is now a parameter, so delete_app_password keeps its "Invalid credentials" wording without unwrapping/rebuilding the helper's JSONResponse. - Body parsing catches (ValueError, UnicodeDecodeError) instead of bare Exception, and guards body.get behind isinstance(body, dict) — no longer swallows RuntimeError/AttributeError or a non-object JSON body. - Add a unit test asserting 500/503 -> 502. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/api/passwords.py | 56 ++++++++++++------- .../test_management_app_password_endpoints.py | 27 +++++++++ 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/nextcloud_mcp_server/api/passwords.py b/nextcloud_mcp_server/api/passwords.py index 70ca0e2d..2cf365a0 100644 --- a/nextcloud_mcp_server/api/passwords.py +++ b/nextcloud_mcp_server/api/passwords.py @@ -188,7 +188,11 @@ async def _get_app_password_storage(request: Request) -> RefreshTokenStorage: async def _validate_nextcloud_credentials( - nextcloud_host: str, login_name: str, password: str + nextcloud_host: str, + login_name: str, + password: str, + *, + invalid_credential_error: str = "Invalid app password", ) -> tuple[str | None, JSONResponse | None]: """Validate a credential against Nextcloud and return the account UID. @@ -206,11 +210,15 @@ async def _validate_nextcloud_credentials( crash behind issue #824 (``AttributeError: 'list' object has no attribute 'get'`` escaping as an unhandled 500). + Args: + invalid_credential_error: client-facing error string for the 401 path, + so callers can keep their own wording. + Returns: ``(ocs_user_id, None)`` on success, otherwise ``(None, error_response)`` - with a ready-to-return :class:`JSONResponse`: 401 for an invalid - credential, 502 when Nextcloud is unreachable or returns something we - cannot parse. + with a ready-to-return :class:`JSONResponse`: **401** for an invalid + credential, **502** when Nextcloud is unreachable, errors out (5xx / + maintenance mode), or returns something we cannot parse. """ try: async with nextcloud_httpx_client( @@ -229,13 +237,22 @@ async def _validate_nextcloud_credentials( status_code=502, ) - # v2.php maps an OCS auth failure onto a real HTTP status (e.g. 401). - if response.status_code != 200: + # v2.php maps an OCS auth failure onto a real HTTP status. Only 401/403 mean + # "bad credential" — anything else non-200 (5xx, 503 maintenance mode) is a + # Nextcloud-side problem and must surface as 502, not a misleading "invalid + # password" that sends ops chasing the wrong cause. + if response.status_code in (401, 403): logger.warning("Credential validation failed: HTTP %s", response.status_code) return None, JSONResponse( - {"success": False, "error": "Invalid app password"}, + {"success": False, "error": invalid_credential_error}, status_code=401, ) + if response.status_code != 200: + logger.error("Nextcloud OCS returned HTTP %s", response.status_code) + return None, JSONResponse( + {"success": False, "error": "Nextcloud returned a server error"}, + status_code=502, + ) # Parse defensively: even on HTTP 200 the body may be malformed or carry a # non-dict ``ocs.data`` (the v1.php auth-failure shape, kept as a guard @@ -268,7 +285,7 @@ async def _validate_nextcloud_credentials( ocs_user_id is not None, ) return None, JSONResponse( - {"success": False, "error": "Invalid app password"}, + {"success": False, "error": invalid_credential_error}, status_code=401, ) @@ -342,10 +359,11 @@ async def provision_app_password(request: Request) -> JSONResponse: nc_username = None try: body = await request.json() + except (ValueError, UnicodeDecodeError): + body = None # No / malformed JSON body = legacy call without extras + if isinstance(body, dict): scopes = body.get("scopes") # list[str] | None nc_username = body.get("username") # Nextcloud loginName - except Exception: - pass # No JSON body = legacy call without scopes / loginName login_name = nc_username or username @@ -476,9 +494,10 @@ async def delete_app_password(request: Request) -> JSONResponse: nc_username = None try: body = await request.json() + except (ValueError, UnicodeDecodeError): + body = None # No / malformed JSON body = legacy call without a loginName + if isinstance(body, dict): nc_username = body.get("username") - except Exception: - pass # No JSON body = legacy call without an explicit loginName login_name = nc_username or username # Validate credentials against Nextcloud. Shares the OCS v2 + defensive @@ -495,16 +514,15 @@ async def delete_app_password(request: Request) -> JSONResponse: status_code=500, ) + # Keep this route's historical "Invalid credentials" wording via the helper + # rather than unwrapping and rebuilding its response. ocs_user_id, error_response = await _validate_nextcloud_credentials( - nextcloud_host, login_name, password + nextcloud_host, + login_name, + password, + invalid_credential_error="Invalid credentials", ) if error_response is not None: - # Preserve the historical "Invalid credentials" wording for this route. - if error_response.status_code == 401: - return JSONResponse( - {"success": False, "error": "Invalid credentials"}, - status_code=401, - ) return error_response # The authenticated account must be the UID whose password is being deleted. diff --git a/tests/unit/test_management_app_password_endpoints.py b/tests/unit/test_management_app_password_endpoints.py index 7e191b16..7ad924b8 100644 --- a/tests/unit/test_management_app_password_endpoints.py +++ b/tests/unit/test_management_app_password_endpoints.py @@ -429,6 +429,33 @@ async def test_provision_app_password_ocs_v1_failure_payload_returns_401(mocker) assert "Invalid app password" in response.json()["error"] +async def test_provision_app_password_nextcloud_5xx_returns_502(mocker): + """A Nextcloud server error / maintenance mode (5xx) surfaces as 502, not a + misleading 401 that blames the user's password.""" + mocker.patch( + "nextcloud_mcp_server.api.passwords.get_settings", + return_value=MagicMock( + nextcloud_host="http://localhost:8080", + nextcloud_verify_ssl=True, + nextcloud_ca_bundle=None, + ), + ) + for status_code in (500, 503): + passwords._rate_limit_attempts.clear() + _mock_ocs_client(mocker, status_code=status_code, json_payload={}) + client = TestClient(_provision_only_app()) + response = client.post( + "/api/v1/users/testuser/app-password", + headers={ + "Authorization": create_basic_auth_header( + "testuser", "aaaaa-bbbbb-ccccc-ddddd-eeeee" + ) + }, + ) + assert response.status_code == 502, f"HTTP {status_code} should map to 502" + assert "server error" in response.json()["error"].lower() + + async def test_provision_app_password_nondict_data_does_not_500(mocker): """Regression for #824: a non-dict ``ocs.data`` under HTTP 200 (list, or ``null``) is handled gracefully (401), not as an unhandled 500/