admin/mcp-nextcloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(auth): address PR #758 round-3 final review

Browse Source 
Seven findings from the latest review on #758, plus a regression test
catching the substance of the cache-stampede fix:

- verify_id_token: widen id_token annotation to str | None to match
  callers passing nc_token_response.get("id_token")
- extract_user_id_from_token: use JSON-RPC reserved error code -32001
  instead of -1
- _get_cached: per-URL anyio.Lock dict + meta-lock coalesces concurrent
  cache misses into a single IdP fetch (mirrors token_broker.py idiom)
- delete_browser_session: collapse SELECT+DELETE into atomic
  DELETE ... RETURNING user_id (SQLite >= 3.35)
- new test_origin_normalise.py: parametrized port/scheme/host equivalence
  cases for the CSRF Origin guard
- browser_oauth_routes: correct misleading "PR #758 finding 5" cross-
  references (finding 5 was Fernet-key hardening, not CSRF)
- ASProxySession.nonce: make required, drop spurious "legacy session"
  default; reword the in-flight `or None` comment to reflect that
  ASProxySession is purely in-memory
- new test_get_cached_coalesces_concurrent_misses: pins the
  cache-stampede protection — fires 10 concurrent _get_cached calls and
  asserts exactly one HTTP fetch

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-02 23:14:07 +02:00
co-authored by Claude Opus 4.7
parent 9d0e7dcebe
commit 3a4fa8adc8
7 changed files with 152 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/auth/browser_oauth_routes.py
+5 -4
View File
@@ -54,7 +54,8 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool:
"""Return True when Origin/Referer is missing or matches our own host.
Used to gate POST /oauth/logout against cross-origin form submissions
(PR #758 finding 5). Per OWASP CSRF cheat sheet, the policy is:
(PR #758 round-3 review hardening). Per OWASP CSRF cheat sheet, the
policy is:
- If neither Origin nor Referer is set, allow (same-origin POST in
privacy-conscious browsers may strip both).
- Otherwise, the (scheme, hostname, port) tuple of the first present
@@ -640,9 +641,9 @@ async def oauth_logout(request: Request) -> RedirectResponse | JSONResponse:
5. Clears the cookie on the response.
Method is POST-only at the route layer to defeat passive CSRF (PR #758
finding 5). Origin / Referer headers are also validated against the
configured ``mcp_server_url`` when present, blocking same-method-but-
cross-origin form submissions.
round-3 review hardening). Origin / Referer headers are also validated
against the configured ``mcp_server_url`` when present, blocking
same-method-but-cross-origin form submissions.
Query parameters:
next: Optional URL to redirect to after logout (default: /oauth/login)
nextcloud_mcp_server/auth/oauth_routes.py
+5 -4
View File
@@ -92,7 +92,7 @@ class ASProxySession:
code_challenge: str
code_challenge_method: str
requested_scopes: str
nonce: str = ""
nonce: str
created_at: float = field(default_factory=time.time)
expires_at: float = field(default_factory=lambda: time.time() + 600)
@@ -959,9 +959,10 @@ async def _oauth_callback_as_proxy(
# verification done in oauth_callback_nextcloud.
#
# ``expected_nonce`` is the per-request nonce we forwarded to the IdP
# in oauth_authorize (PR #758 round-2 finding 2); falsy → skip nonce
# check for backward compatibility with sessions stored before the
# nonce field was added.
# in oauth_authorize (PR #758 round-2 finding 2). ASProxySession is
# in-memory only and ``nonce`` is now a required field, so for any
# session created via the current code path this is always set; the
# ``or None`` is defence-in-depth and a no-op in practice.
id_token = nc_token_response.get("id_token")
try:
await verify_id_token(
nextcloud_mcp_server/auth/storage.py
+8 -9
View File
@@ -1220,23 +1220,22 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# SELECT the row before DELETE so we can attribute the audit log
# entry to the right user (PR #758 round-3 nit 5).
# DELETE ... RETURNING (SQLite ≥ 3.35) reads ``user_id`` atomically
# with the delete itself, so the audit log can't race against a
# concurrent delete that empties the row between SELECT and DELETE
# (PR #758 round-3 review).
user_id: str | None = None
async with aiosqlite.connect(self.db_path) as db:
async with db.execute(
"SELECT user_id FROM browser_sessions WHERE session_id = ?",
"DELETE FROM browser_sessions WHERE session_id = ? RETURNING user_id",
(session_id,),
) as cursor:
row = await cursor.fetchone()
user_id = row[0] if row else None
cursor = await db.execute(
"DELETE FROM browser_sessions WHERE session_id = ?", (session_id,)
)
await db.commit()
deleted = cursor.rowcount > 0
deleted = row is not None
if deleted:
user_id = row[0]
logger.debug("Deleted browser session %s", session_id[:8])
if user_id:
await self._audit_log(
nextcloud_mcp_server/auth/token_utils.py
+42 -13
View File
@@ -8,6 +8,7 @@ import logging
import time
from typing import Any
import anyio
import jwt
from jwt import PyJWKSet
from mcp.server.auth.middleware.auth_context import get_access_token
@@ -30,6 +31,23 @@ _discovery_cache: dict[str, tuple[float, dict[str, Any]]] = {}
_jwks_cache: dict[str, tuple[float, dict[str, Any]]] = {}
_OIDC_CACHE_TTL = 300
# Per-URL fetch locks coalesce concurrent cache misses into a single HTTP
# request, preventing thundering-herd against the IdP at cache expiry
# (PR #758 round-3 review). Mirrors the lock-dict + meta-lock idiom from
# token_broker.py.
_fetch_locks: dict[str, anyio.Lock] = {}
_fetch_locks_lock = anyio.Lock()
async def _get_fetch_lock(url: str) -> anyio.Lock:
"""Return the per-URL lock used to serialise cache-miss fetches."""
async with _fetch_locks_lock:
lock = _fetch_locks.get(url)
if lock is None:
lock = anyio.Lock()
_fetch_locks[url] = lock
return lock
class IdTokenVerificationError(Exception):
"""Raised when an OIDC ID token fails signature or claim verification."""
@@ -48,19 +66,29 @@ async def _get_cached(
``/.well-known/openid-configuration`` path issues a 301), but JWKS
fetches deliberately stay strict — the URL came from the discovery
document we already trust, so a redirect there would be suspicious.
Concurrent callers seeing the same cache miss are coalesced via a
per-URL ``anyio.Lock``: only one fetch runs, the rest wait and read the
populated cache.
"""
now = time.time()
entry = cache.get(url)
if entry is not None:
expires_at, data = entry
if now < expires_at:
return data
async with nextcloud_httpx_client(follow_redirects=follow_redirects) as http_client:
response = await http_client.get(url)
response.raise_for_status()
data = response.json()
cache[url] = (now + _OIDC_CACHE_TTL, data)
return data
if entry is not None and time.time() < entry[0]:
return entry[1]
lock = await _get_fetch_lock(url)
async with lock:
# Re-check inside the lock — a concurrent waiter may have already
# populated the cache before we acquired it.
entry = cache.get(url)
if entry is not None and time.time() < entry[0]:
return entry[1]
async with nextcloud_httpx_client(
follow_redirects=follow_redirects
) as http_client:
response = await http_client.get(url)
response.raise_for_status()
data = response.json()
cache[url] = (time.time() + _OIDC_CACHE_TTL, data)
return data
async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]:
@@ -78,7 +106,7 @@ async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]:
async def verify_id_token(
id_token: str,
id_token: str | None,
*,
discovery_url: str,
expected_audience: str,
@@ -240,7 +268,8 @@ async def extract_user_id_from_token(_ctx: Context) -> str:
)
raise McpError(
ErrorData(
code=-1,
# JSON-RPC 2.0 reserves -32000..-32099 for application errors.
code=-32001,
message="Cannot determine user identity from access token",
)
)
tests/unit/test_id_token_verification.py
+50
View File
@@ -12,6 +12,7 @@ import time
from base64 import urlsafe_b64encode
from unittest.mock import patch
import anyio
import httpx
import jwt
import pytest
@@ -32,9 +33,11 @@ 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()
token_utils._fetch_locks.clear()
yield
token_utils._discovery_cache.clear()
token_utils._jwks_cache.clear()
token_utils._fetch_locks.clear()
# Generated once per process — RSA keypair generation is slow.
@@ -478,3 +481,50 @@ async def test_verify_id_token_caches_discovery_and_jwks():
assert fetches.get(DISCOVERY_URL) == 1, "discovery fetched more than once"
assert fetches.get(JWKS_URI) == 1, "JWKS fetched more than once"
async def test_get_cached_coalesces_concurrent_misses():
"""Concurrent cache misses must collapse into a single HTTP fetch.
PR #758 round-3 review: without the per-URL lock in ``_get_cached``,
N simultaneous callers at cache expiry would each fire their own
request to the IdP, potentially tripping rate limits. The async
handler yields with ``anyio.sleep(0.01)`` so all 10 callers reach
the cache-miss branch concurrently — without coalescing the count
would be 10.
"""
fetch_count = {"n": 0}
async def slow_handler(request: httpx.Request) -> httpx.Response:
fetch_count["n"] += 1
# Yield so concurrent waiters all reach the lock acquisition
# while the first holder is still mid-fetch.
await anyio.sleep(0.01)
return _idp_handler(request)
transport = httpx.MockTransport(slow_handler)
def fake_client(**kwargs):
kwargs["transport"] = transport
return httpx.AsyncClient(**kwargs)
results: list[dict] = []
async def fetch_once():
results.append(await token_utils._get_cached(token_utils._jwks_cache, JWKS_URI))
with patch(
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
side_effect=fake_client,
):
async with anyio.create_task_group() as tg:
for _ in range(10):
tg.start_soon(fetch_once)
assert fetch_count["n"] == 1, (
f"expected exactly one fetch via lock coalescing, got {fetch_count['n']}"
)
assert len(results) == 10
assert all(r == results[0] for r in results), (
"concurrent callers received divergent cached data"
)
tests/unit/test_oauth_callback_session_cleanup.py
+1
View File
@@ -218,6 +218,7 @@ async def test_as_proxy_rejects_invalid_id_token():
code_challenge="challenge",
code_challenge_method="S256",
requested_scopes="openid",
nonce="nonce-rejected",
)
_proxy_codes.clear()
tests/unit/test_origin_normalise.py
+41
View File
@@ -0,0 +1,41 @@
"""Tests for ``_normalise_origin`` port + scheme + host normalisation.
The CSRF guard on POST /oauth/logout (PR #758 round-3 review hardening)
compares ``Origin`` / ``Referer`` against the configured ``mcp_server_url``
via ``_normalise_origin``. RFC 6454 §6.2 says browsers omit default ports
(80 for http, 443 for https) from Origin headers, so the function strips
those before comparison. These tests pin that behaviour so it can't
silently regress.
"""
import pytest
from nextcloud_mcp_server.auth.browser_oauth_routes import _normalise_origin
pytestmark = pytest.mark.unit
@pytest.mark.parametrize(
"left, right, equal",
[
# Default ports are stripped — these MUST compare equal.
("https://example.com", "https://example.com:443", True),
("https://example.com:443", "https://example.com", True),
("http://example.com", "http://example.com:80", True),
("http://example.com:80", "http://example.com", True),
# Non-default ports are preserved.
("https://example.com:8443", "https://example.com", False),
("http://example.com:8080", "http://example.com", False),
("https://example.com:8443", "https://example.com:443", False),
# Cross-scheme defaults don't collapse (https:443 != http:80 even
# though both ports get stripped, because the scheme differs).
("https://example.com", "http://example.com", False),
("https://example.com:443", "http://example.com:80", False),
# Hostname matters and is case-insensitive.
("https://example.com", "https://other.com", False),
("https://example.com", "https://EXAMPLE.COM", True),
("https://Example.Com:443", "https://example.com", True),
],
)
def test_normalise_origin_equivalence(left: str, right: str, equal: bool):
assert (_normalise_origin(left) == _normalise_origin(right)) is equal