fix(auth): address PR #758 review — XSS, CSRF, open redirect, JWKS cache

Addresses all 9 findings from the review on PR #758: Blocking: - _revoke_refresh_token_at_idp now reads config from oauth_ctx["config"] (the production-shaped nested dict). Previously read flat keys, causing IdP revocation to silently no-op in production. Test fixtures rebuilt to the realistic nested shape so the bug can't regress unnoticed. - HTML error responses in oauth_login_callback now wrap IdP-controlled error_body, str(e), and the attacker-controlled error/error_description query params in html_escape. New test_browser_oauth_xss.py pins this. Important: - New _safe_next_url helper validates the ?next= query param at write time (oauth_login), in oauth_logout, and on read from the session row in oauth_login_callback. Blocks https://, // (protocol-relative), and CRLF/whitespace injection. - verify_id_token now caches discovery + JWKS (5-min TTL) using the same pattern as oauth_routes._get_cached_discovery. New caching regression test pins to one fetch per URL across multiple calls. - /oauth/logout is now POST-only at the route layer (defeats passive CSRF via <img src>). oauth_logout also validates Origin/Referer against the configured mcp_server_url. Logout UI in user_info.html converted from <a href> to <form method="post">. - New storage.cleanup_expired_browser_sessions() called from the hourly cleanup loop in app.py — previously these rows accumulated for users who never explicitly logged out. Nits: - Demoted INFO logs that leaked oauth_config.keys() / client_id / token-storage state to DEBUG. Operator-relevant outcome lines (login successful, refresh token stored, logged out) stay INFO. - verify_id_token algorithms widened to RS256, PS256, ES256 — covers Azure AD (PS256) and Cognito/some Keycloak realms (ES256). Symmetric and "none" remain off the allowlist. - Migrated all Optional[X] usages in auth/storage.py to X | None per CLAUDE.md. Breaking change: GET /oauth/logout now returns 405. The in-tree logout UI was migrated to a POST form; any external bookmark or curl-based caller that relied on GET will need to switch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-02 18:26:39 +02:00
parent 15dbb26349
commit 931ee602eb
10 changed files with 581 additions and 110 deletions
@@ -0,0 +1,121 @@
+"""Regression tests for HTML XSS in browser OAuth error responses.
+
+The reviewer on PR #758 flagged that ``oauth_login_callback`` interpolated
+IdP-controlled and query-parameter-controlled text into HTMLResponse bodies
+without escaping. These tests pin the html_escape behavior so the
+vulnerability cannot regress silently.
+"""
+
+import json
+import tempfile
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import httpx
+import pytest
+from cryptography.fernet import Fernet
+
+from nextcloud_mcp_server.auth.browser_oauth_routes import oauth_login_callback
+from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
+
+pytestmark = pytest.mark.unit
+
+
+XSS_PAYLOAD = "<script>alert(1)</script>"
+
+
+@pytest.fixture
+async def storage():
+    with tempfile.TemporaryDirectory() as tmpdir:
+        db_path = Path(tmpdir) / "xss.db"
+        s = RefreshTokenStorage(
+            db_path=str(db_path), encryption_key=Fernet.generate_key().decode()
+        )
+        await s.initialize()
+        yield s
+
+
+def _build_request(*, query_params: dict, oauth_context: dict | None = None):
+    request = MagicMock()
+    request.query_params = query_params
+    request.cookies = {}
+    request.app.state.oauth_context = oauth_context
+    request.url_for = MagicMock(return_value="/oauth/login")
+    return request
+
+
+async def test_callback_escapes_error_query_params(storage):
+    """`error` and `error_description` are attacker-controlled — must be escaped."""
+    request = _build_request(
+        query_params={
+            "error": XSS_PAYLOAD,
+            "error_description": XSS_PAYLOAD,
+        },
+        oauth_context={"storage": storage, "config": {}},
+    )
+
+    response = await oauth_login_callback(request)
+    body = response.body.decode()
+
+    assert XSS_PAYLOAD not in body
+    assert "&lt;script&gt;alert(1)&lt;/script&gt;" in body
+
+
+async def test_callback_escapes_idp_http_error_body(storage):
+    """IdP-returned HTTPError body must be HTML-escaped before reflection."""
+    discovery = {"token_endpoint": "http://idp.example/token"}
+
+    def handler(request: httpx.Request) -> httpx.Response:
+        if request.url.path.endswith("/.well-known/openid-configuration"):
+            return httpx.Response(
+                200,
+                content=json.dumps(discovery).encode(),
+                headers={"content-type": "application/json"},
+            )
+        if str(request.url) == "http://idp.example/token":
+            return httpx.Response(400, content=XSS_PAYLOAD.encode())
+        return httpx.Response(404)
+
+    transport = httpx.MockTransport(handler)
+
+    def fake_client(**kwargs):
+        kwargs["transport"] = transport
+        return httpx.AsyncClient(**kwargs)
+
+    # Pre-populate the oauth_session row that the callback expects
+    await storage.store_oauth_session(
+        session_id="state-xss",
+        client_id="browser-ui",
+        client_redirect_uri="/app",
+        state="state-xss",
+        code_challenge="cc",
+        code_challenge_method="S256",
+        mcp_authorization_code="cv",
+        flow_type="browser",
+        ttl_seconds=600,
+    )
+
+    request = _build_request(
+        query_params={"code": "abc", "state": "state-xss"},
+        oauth_context={
+            "storage": storage,
+            "oauth_client": None,
+            "config": {
+                "discovery_url": "http://idp.example/.well-known/openid-configuration",
+                "client_id": "test",
+                "client_secret": "secret",
+                "mcp_server_url": "http://localhost",
+            },
+        },
+    )
+
+    with patch(
+        "nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
+        side_effect=fake_client,
+    ):
+        response = await oauth_login_callback(request)
+
+    body = response.body.decode()
+    assert response.status_code == 500
+    assert XSS_PAYLOAD not in body
+    assert "&lt;script&gt;alert(1)&lt;/script&gt;" in body
@@ -73,3 +73,27 @@ async def test_replace_existing_session_id(storage):
    await storage.create_browser_session(session_id=sid, user_id="bob")

    assert await storage.get_browser_session_user(sid) == "bob"
+
+
+async def test_cleanup_expired_browser_sessions(storage):
+    """Periodic cleanup removes expired rows but leaves fresh ones (PR #758 finding 6)."""
+    fresh_sid = secrets.token_urlsafe(32)
+    expired_sid = secrets.token_urlsafe(32)
+
+    await storage.create_browser_session(
+        session_id=fresh_sid, user_id="alice", ttl_seconds=3600
+    )
+    # ttl_seconds=-2 → expires_at strictly in the past (cleanup uses < now,
+    # so it must be actually less, not equal).
+    await storage.create_browser_session(
+        session_id=expired_sid, user_id="bob", ttl_seconds=-2
+    )
+
+    deleted = await storage.cleanup_expired_browser_sessions()
+    assert deleted == 1
+
+    # Fresh row survives, expired row is gone
+    assert await storage.get_browser_session_user(fresh_sid) == "alice"
+    assert await storage.get_browser_session_user(expired_sid) is None
+    # Calling again should be a no-op
+    assert await storage.cleanup_expired_browser_sessions() == 0
@@ -18,6 +18,7 @@ import pytest
 from cryptography.hazmat.primitives import serialization
 from cryptography.hazmat.primitives.asymmetric import rsa

+from nextcloud_mcp_server.auth import token_utils
 from nextcloud_mcp_server.auth.token_utils import (
    IdTokenVerificationError,
    verify_id_token,
@@ -26,6 +27,16 @@ from nextcloud_mcp_server.auth.token_utils import (
 pytestmark = pytest.mark.unit


+@pytest.fixture(autouse=True)
+def _clear_oidc_caches():
+    """Reset the discovery+JWKS caches so tests don't share fetched data."""
+    token_utils._discovery_cache.clear()
+    token_utils._jwks_cache.clear()
+    yield
+    token_utils._discovery_cache.clear()
+    token_utils._jwks_cache.clear()
+
+
 # Generated once per process — RSA keypair generation is slow.
 _KEY = rsa.generate_private_key(public_exponent=65537, key_size=2048)
 _PRIVATE_PEM = _KEY.private_bytes(
@@ -231,3 +242,48 @@ async def test_verify_id_token_missing_token_rejected():
        await verify_id_token(
            "", discovery_url=DISCOVERY_URL, expected_audience="test-client"
        )
+
+
+async def test_verify_id_token_caches_discovery_and_jwks():
+    """Discovery + JWKS must be cached: two verifications, one fetch each.
+
+    Pins the fix for PR #758 finding 4 — every login previously made two
+    extra HTTP round-trips to the IdP for the same metadata.
+    """
+    fetches: dict[str, int] = {}
+
+    def counting_handler(request: httpx.Request) -> httpx.Response:
+        url = str(request.url)
+        fetches[url] = fetches.get(url, 0) + 1
+        return _idp_handler(request)
+
+    transport = httpx.MockTransport(counting_handler)
+
+    def fake_client(**kwargs):
+        kwargs["transport"] = transport
+        return httpx.AsyncClient(**kwargs)
+
+    now = int(time.time())
+    token = _sign(
+        {
+            "iss": ISSUER,
+            "aud": "test-client",
+            "sub": "alice",
+            "iat": now,
+            "exp": now + 60,
+        }
+    )
+
+    with patch(
+        "nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
+        side_effect=fake_client,
+    ):
+        await verify_id_token(
+            token, discovery_url=DISCOVERY_URL, expected_audience="test-client"
+        )
+        await verify_id_token(
+            token, discovery_url=DISCOVERY_URL, expected_audience="test-client"
+        )
+
+    assert fetches.get(DISCOVERY_URL) == 1, "discovery fetched more than once"
+    assert fetches.get(JWKS_URI) == 1, "JWKS fetched more than once"
@@ -46,12 +46,20 @@ async def storage():
        yield s


-def _build_request(*, cookie: str | None, oauth_context: dict | None):
+def _build_request(
+    *,
+    cookie: str | None,
+    oauth_context: dict | None,
+    headers: dict | None = None,
+):
    """Build a minimal Starlette-style request stub for oauth_logout."""
    request = MagicMock()
    request.query_params = {}
    request.cookies = {"mcp_session": cookie} if cookie else {}
    request.app.state.oauth_context = oauth_context
+    # Headers default to empty so the CSRF check sees neither Origin nor
+    # Referer (allowed by policy — see _origin_matches_self).
+    request.headers = headers or {}
    return request


@@ -69,7 +77,7 @@ async def test_logout_deletes_refresh_token_and_session(storage):

    request = _build_request(
        cookie="sid-1",
-        oauth_context={"storage": storage, "discovery_url": None},
+        oauth_context={"storage": storage, "config": {"discovery_url": None}},
    )

    with patch(
@@ -93,7 +101,10 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage):
    revoke = AsyncMock()
    request = _build_request(
        cookie="sid-2",
-        oauth_context={"storage": storage, "discovery_url": "http://idp/.well-known"},
+        oauth_context={
+            "storage": storage,
+            "config": {"discovery_url": "http://idp/.well-known"},
+        },
    )

    with patch(
@@ -111,7 +122,8 @@ async def test_logout_calls_revocation_when_refresh_token_present(storage):
 async def test_logout_no_session_cookie_returns_302(storage):
    """Without a cookie, logout still 302s and doesn't touch storage."""
    request = _build_request(
-        cookie=None, oauth_context={"storage": storage, "discovery_url": None}
+        cookie=None,
+        oauth_context={"storage": storage, "config": {"discovery_url": None}},
    )
    response = await oauth_logout(request)
    assert response.status_code == 302
@@ -128,12 +140,78 @@ async def test_logout_swallows_storage_errors(storage):

    request = _build_request(
        cookie="sid-3",
-        oauth_context={"storage": broken_storage, "discovery_url": None},
+        oauth_context={
+            "storage": broken_storage,
+            "config": {"discovery_url": None},
+        },
    )
    response = await oauth_logout(request)
    assert response.status_code == 302  # logout still succeeds


+async def test_logout_blocks_cross_origin_post(storage):
+    """POST from a foreign Origin must be rejected with 403 (PR #758 finding 5)."""
+    await storage.create_browser_session(session_id="sid-X", user_id="alice")
+
+    request = _build_request(
+        cookie="sid-X",
+        oauth_context={
+            "storage": storage,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "discovery_url": None,
+            },
+        },
+        headers={"origin": "https://evil.example.com"},
+    )
+
+    response = await oauth_logout(request)
+    assert response.status_code == 403
+    # Session row must NOT have been deleted.
+    assert await storage.get_browser_session_user("sid-X") == "alice"
+
+
+async def test_logout_allows_same_origin_post(storage):
+    """POST with matching Origin proceeds normally."""
+    await storage.create_browser_session(session_id="sid-Y", user_id="alice")
+
+    request = _build_request(
+        cookie="sid-Y",
+        oauth_context={
+            "storage": storage,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "discovery_url": None,
+            },
+        },
+        headers={"origin": "https://mcp.example.com"},
+    )
+
+    response = await oauth_logout(request)
+    assert response.status_code == 302
+    assert await storage.get_browser_session_user("sid-Y") is None
+
+
+async def test_logout_allows_referer_when_origin_missing(storage):
+    """Some browsers strip Origin on POST; Referer is the fallback signal."""
+    await storage.create_browser_session(session_id="sid-Z", user_id="alice")
+
+    request = _build_request(
+        cookie="sid-Z",
+        oauth_context={
+            "storage": storage,
+            "config": {
+                "mcp_server_url": "https://mcp.example.com",
+                "discovery_url": None,
+            },
+        },
+        headers={"referer": "https://mcp.example.com/app"},
+    )
+
+    response = await oauth_logout(request)
+    assert response.status_code == 302
+
+
 async def test_logout_handles_session_with_no_refresh_token(storage):
    """Cookie + session row exist but refresh token already gone — logout is idempotent."""
    await storage.create_browser_session(session_id="sid-4", user_id="dave")
@@ -141,7 +219,7 @@ async def test_logout_handles_session_with_no_refresh_token(storage):
    revoke = AsyncMock()
    request = _build_request(
        cookie="sid-4",
-        oauth_context={"storage": storage, "discovery_url": None},
+        oauth_context={"storage": storage, "config": {"discovery_url": None}},
    )
    with patch(
        "nextcloud_mcp_server.auth.browser_oauth_routes._revoke_refresh_token_at_idp",
@@ -197,9 +275,11 @@ async def test_revoke_helper_posts_to_revocation_endpoint():
    ):
        await _revoke_refresh_token_at_idp(
            {
-                "discovery_url": discovery_url,
-                "client_id": "test-client",
-                "client_secret": "test-secret",
+                "config": {
+                    "discovery_url": discovery_url,
+                    "client_id": "test-client",
+                    "client_secret": "test-secret",
+                }
            },
            "rt-secret",
        )
@@ -232,9 +312,11 @@ async def test_revoke_helper_skips_when_no_revocation_endpoint():
        # Returns None and does not raise
        result = await _revoke_refresh_token_at_idp(
            {
-                "discovery_url": discovery_url,
-                "client_id": "x",
-                "client_secret": "y",
+                "config": {
+                    "discovery_url": discovery_url,
+                    "client_id": "x",
+                    "client_secret": "y",
+                }
            },
            "rt",
        )
@@ -259,9 +341,11 @@ async def test_revoke_helper_silent_on_idp_error():
    ):
        result = await _revoke_refresh_token_at_idp(
            {
-                "discovery_url": "http://x/.well-known",
-                "client_id": "x",
-                "client_secret": "y",
+                "config": {
+                    "discovery_url": "http://x/.well-known",
+                    "client_id": "x",
+                    "client_secret": "y",
+                }
            },
            "rt",
        )
@@ -0,0 +1,44 @@
+"""Tests for _safe_next_url, the open-redirect guard for ``?next=`` params.
+
+Pins the contract that any non-path target falls back to the default,
+preventing the open-redirect issue flagged on PR #758.
+"""
+
+import pytest
+
+from nextcloud_mcp_server.auth.browser_oauth_routes import _safe_next_url
+
+pytestmark = pytest.mark.unit
+
+
+@pytest.mark.parametrize(
+    "raw, expected",
+    [
+        # Valid path-only targets pass through.
+        ("/app", "/app"),
+        ("/app/foo", "/app/foo"),
+        ("/oauth/login", "/oauth/login"),
+        ("/app?x=1&y=2", "/app?x=1&y=2"),
+        ("/app#frag", "/app#frag"),
+        # Empty / missing → default.
+        ("", "/default"),
+        (None, "/default"),
+        # Absolute URLs → default.
+        ("https://evil.example.com", "/default"),
+        ("http://evil.example.com/path", "/default"),
+        # Protocol-relative → default. Browser would treat as cross-origin.
+        ("//evil.example.com", "/default"),
+        ("//evil.example.com/path", "/default"),
+        # No leading slash → default.
+        ("relative/path", "/default"),
+        ("app", "/default"),
+        # Whitespace / control chars → default. Defends against tab/space
+        # injection that some browsers historically tolerated.
+        ("/app\nfoo", "/default"),
+        ("/app\tfoo", "/default"),
+        ("/app\x00foo", "/default"),
+        ("/app foo", "/default"),
+    ],
+)
+def test_safe_next_url(raw, expected):
+    assert _safe_next_url(raw, "/default") == expected