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

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

Browse Source 
Seven findings from the latest review on #758 (3 medium, 4 low/nit):

Medium:
- storage.py: replace 5 ``assert self.cipher is not None`` sites with
  explicit ``RuntimeError`` so missing TOKEN_ENCRYPTION_KEY can't silently
  become an AttributeError under ``python -O``
- session_backend.py: document the silent-invalidation invariant —
  refresh-token TTL expiry without explicit logout deliberately makes
  the browser session unusable; future readers must not relax it
- server/oauth_tools.py: drop user_id from the Flow 2 session_id
  identifier — use ``flow2_{secrets.token_hex(16)}`` so audit logs and
  DB rows don't carry user_id in the session_id field

Low / nit:
- token_utils.py: drop _fetch_locks dict entry in finally so a probed
  deployment can't grow the lock dict without bound; coalescing test
  now pins the invariant with len(_fetch_locks) == 0
- browser_oauth_routes.py: strip trailing slash from settings.nextcloud_host
  before constructing the well-known URL so a host configured as
  ``https://cloud.example.com/`` doesn't produce a double-slash
- browser_oauth_routes.py: add comment explaining the three-layer CSRF
  policy on the mcp_session cookie set (SameSite=Lax + POST-only logout
  + Origin/Referer check)
- oauth_routes.py: convert all 23 f-string log calls to lazy %-style
  per the CLAUDE.md / memory feedback_lazy_logging convention

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-03 00:57:12 +02:00
co-authored by Claude Opus 4.7
parent 3a4fa8adc8
commit b696541918
7 changed files with 122 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
nextcloud_mcp_server/auth/browser_oauth_routes.py
+11 -1
View File
@@ -614,6 +614,12 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
)
response = RedirectResponse(next_url, status_code=302)
# CSRF protection is layered: ``SameSite=Lax`` blocks cross-site POSTs
# in modern browsers; ``oauth_logout`` is POST-only with an Origin /
# Referer check (``_origin_matches_self``) to cover older browsers and
# non-browser clients. ``HttpOnly`` blocks JS exfiltration on XSS;
# ``Secure`` is gated to non-HTTP hosts in dev (PR #758 round-4 review
# nit 6).
response.set_cookie(
key="mcp_session",
value=session_id,
@@ -705,8 +711,12 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N
try:
discovery_url = cfg.get("discovery_url") or settings.oidc_discovery_url
if not discovery_url and settings.nextcloud_host:
# Strip trailing slash so a host configured as
# ``https://cloud.example.com/`` doesn't produce a double-slash
# in the well-known URL (PR #758 round-4 review nit 5).
discovery_url = (
f"{settings.nextcloud_host}/.well-known/openid-configuration"
f"{settings.nextcloud_host.rstrip('/')}"
"/.well-known/openid-configuration"
)
if not discovery_url:
return
nextcloud_mcp_server/auth/oauth_routes.py
+23 -23
View File
@@ -267,7 +267,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
)
if not is_valid:
logger.warning(f"Client validation failed: {error_msg}")
logger.warning("Client validation failed: %s", error_msg)
return JSONResponse(
{
"error": "unauthorized_client",
@@ -326,10 +326,10 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
callback_uri = f"{mcp_server_url}/oauth/callback"
logger.info("AS Proxy: Intermediary authorization flow")
logger.info(f" Client: {client_id}")
logger.info(f" MCP server client_id: {mcp_server_client_id}")
logger.info(f" Server callback: {callback_uri}")
logger.info(f" Scopes: {scopes}")
logger.info(" Client: %s", client_id)
logger.info(" MCP server client_id: %s", mcp_server_client_id)
logger.info(" Server callback: %s", callback_uri)
logger.info(" Scopes: %s", scopes)
# Discover Nextcloud authorization endpoint
discovery_url = oauth_config.get("discovery_url")
@@ -369,7 +369,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
)
idp_scope_str = _transform_scopes_for_idp(scopes, resource_server_id)
if resource_server_id:
logger.info(f" IdP scopes (prefixed): {idp_scope_str}")
logger.info(" IdP scopes (prefixed): %s", idp_scope_str)
# Redirect to Nextcloud with MCP server's own client_id (no PKCE — confidential client)
idp_params = {
@@ -384,7 +384,7 @@ async def oauth_authorize(request: Request) -> RedirectResponse | JSONResponse:
}
auth_url = f"{authorization_endpoint}?{urlencode(idp_params)}"
logger.info(f"Redirecting to Nextcloud OIDC: {auth_url.split('?')[0]}")
logger.info("Redirecting to Nextcloud OIDC: %s", auth_url.split("?")[0])
return RedirectResponse(auth_url, status_code=302)
@@ -553,7 +553,7 @@ async def oauth_callback_nextcloud(request: Request):
error_description = request.query_params.get(
"error_description", "Authorization failed"
)
logger.error(f"Flow 2 authorization error: {error} - {error_description}")
logger.error("Flow 2 authorization error: %s - %s", error, error_description)
return JSONResponse(
{
"error": error,
@@ -685,16 +685,16 @@ async def oauth_callback_nextcloud(request: Request):
refresh_expires_at = None
if refresh_expires_in:
refresh_expires_at = int(time.time()) + refresh_expires_in
logger.info(f" refresh_expires_in: {refresh_expires_in}s")
logger.info(f" refresh_expires_at: {refresh_expires_at}")
logger.info(" refresh_expires_in: %ss", refresh_expires_in)
logger.info(" refresh_expires_at: %s", refresh_expires_at)
logger.info("Storing refresh token:")
logger.info(f" user_id: {user_id}")
logger.info(" user_id: %s", user_id)
logger.info(" flow_type: flow2")
logger.info(" token_audience: nextcloud")
logger.info(f" provisioning_client_id: {state[:16]}...")
logger.info(f" scopes: {granted_scopes}")
logger.info(f" expires_at: {refresh_expires_at}")
logger.info(" provisioning_client_id: %s...", state[:16])
logger.info(" scopes: %s", granted_scopes)
logger.info(" expires_at: %s", refresh_expires_at)
await storage.store_refresh_token(
user_id=user_id,
@@ -705,7 +705,7 @@ async def oauth_callback_nextcloud(request: Request):
scopes=granted_scopes,
expires_at=refresh_expires_at,
)
logger.info(f"✓ Stored Flow 2 master refresh token for user {user_id}")
logger.info("✓ Stored Flow 2 master refresh token for user %s", user_id)
logger.info("=" * 60)
# Return success HTML page
@@ -787,7 +787,7 @@ async def oauth_callback(request: Request):
oauth_session.get("flow_type", "browser") if oauth_session else "browser"
)
logger.info(f"Unified callback: flow_type={flow_type} (from session lookup)")
logger.info("Unified callback: flow_type=%s (from session lookup)", flow_type)
if flow_type == "flow2":
# Flow 2: Resource Provisioning - MCP server gets delegated Nextcloud access
@@ -801,7 +801,7 @@ async def oauth_callback(request: Request):
else:
# Unknown flow type
logger.warning(f"Unknown flow_type in OAuth session: {flow_type}")
logger.warning("Unknown flow_type in OAuth session: %s", flow_type)
return JSONResponse(
{
"error": "invalid_request",
@@ -831,7 +831,7 @@ async def _oauth_callback_as_proxy(
error_description = request.query_params.get(
"error_description", "Authorization failed"
)
logger.error(f"AS proxy callback error: {error} - {error_description}")
logger.error("AS proxy callback error: %s - %s", error, error_description)
# Retrieve session to redirect back to client with error
session = _as_proxy_sessions.pop(server_state, None)
@@ -1186,7 +1186,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse:
)
if not _verify_pkce_s256(code_verifier, entry.code_challenge):
logger.warning(f"PKCE verification failed for client {entry.client_id}")
logger.warning("PKCE verification failed for client %s", entry.client_id)
return JSONResponse(
{
"error": "invalid_grant",
@@ -1196,7 +1196,7 @@ async def _token_authorization_code(request: Request, form) -> JSONResponse:
)
logger.info(
f"AS proxy token: Returning Nextcloud token for client {entry.client_id}"
"AS proxy token: Returning Nextcloud token for client %s", entry.client_id
)
# Return the stored Nextcloud token response directly
@@ -1329,7 +1329,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse:
# Remove timestamps outside the window
timestamps = [t for t in timestamps if now - t < _DCR_RATE_LIMIT_WINDOW]
if len(timestamps) >= _DCR_RATE_LIMIT_MAX:
logger.warning(f"DCR rate limit exceeded for {client_ip}")
logger.warning("DCR rate limit exceeded for %s", client_ip)
return JSONResponse(
{
"error": "too_many_requests",
@@ -1365,7 +1365,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse:
status_code=400,
)
logger.info(f"DCR proxy: Forwarding registration to {registration_endpoint}")
logger.info("DCR proxy: Forwarding registration to %s", registration_endpoint)
async with nextcloud_httpx_client() as http_client:
response = await http_client.post(
@@ -1401,7 +1401,7 @@ async def oauth_register_proxy(request: Request) -> JSONResponse:
redirect_uris=redirect_uris,
name=client_name,
)
logger.info(f"DCR proxy: Registered client {new_client_id} in local registry")
logger.info("DCR proxy: Registered client %s in local registry", new_client_id)
return JSONResponse(nc_response, status_code=response.status_code)
nextcloud_mcp_server/auth/session_backend.py
+12
View File
@@ -22,6 +22,18 @@ class SessionAuthBackend(AuthenticationBackend):
For BasicAuth mode: Always authenticates as the configured user.
For OAuth mode: Checks for valid session cookie with stored refresh token.
Behavior note — silent invalidation on refresh-token TTL expiry:
The OAuth path requires *both* a live ``browser_sessions`` row and a
live ``refresh_tokens`` row for the resolved user. Logout deletes
both atomically, so a logged-out user always fails closed here.
However, if the refresh token expires by TTL (without an explicit
logout) the row is removed by ``get_refresh_token`` and the browser
session becomes unusable — the user simply gets redirected to
``/oauth/login``. This is intentional defense-in-depth: the
refresh-token check is what makes a leaked or stale browser cookie
unusable after revocation. Do not relax this without first removing
the cleanup invariant on logout (PR #758 round-4 review medium 2).
"""
def __init__(self, oauth_enabled: bool = False):
nextcloud_mcp_server/auth/storage.py
+40 -10
View File
@@ -227,8 +227,14 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# Type narrowing: cipher is set after initialize()
assert self.cipher is not None
# ``assert`` is stripped under ``python -O``, which would silently
# turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on
# the next ``self.cipher.encrypt(...)``. Raise explicitly instead
# (PR #758 round-4 review medium 1).
if self.cipher is None:
raise RuntimeError(
"TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable"
)
encrypted_token = self.cipher.encrypt(refresh_token.encode())
now = int(time.time())
scopes_json = json.dumps(scopes) if scopes else None
@@ -374,8 +380,14 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# Type narrowing: cipher is set after initialize()
assert self.cipher is not None
# ``assert`` is stripped under ``python -O``, which would silently
# turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on
# the next ``self.cipher.encrypt(...)``. Raise explicitly instead
# (PR #758 round-4 review medium 1).
if self.cipher is None:
raise RuntimeError(
"TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable"
)
start_time = time.time()
try:
@@ -461,8 +473,14 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# Type narrowing: cipher is set after initialize()
assert self.cipher is not None
# ``assert`` is stripped under ``python -O``, which would silently
# turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on
# the next ``self.cipher.encrypt(...)``. Raise explicitly instead
# (PR #758 round-4 review medium 1).
if self.cipher is None:
raise RuntimeError(
"TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable"
)
async with aiosqlite.connect(self.db_path) as db:
async with db.execute(
@@ -635,8 +653,14 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# Type narrowing: cipher is set after initialize()
assert self.cipher is not None
# ``assert`` is stripped under ``python -O``, which would silently
# turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on
# the next ``self.cipher.encrypt(...)``. Raise explicitly instead
# (PR #758 round-4 review medium 1).
if self.cipher is None:
raise RuntimeError(
"TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable"
)
# Encrypt sensitive data
encrypted_secret = self.cipher.encrypt(client_secret.encode())
@@ -708,8 +732,14 @@ class RefreshTokenStorage:
if not self._initialized:
await self.initialize()
# Type narrowing: cipher is set after initialize()
assert self.cipher is not None
# ``assert`` is stripped under ``python -O``, which would silently
# turn a missing TOKEN_ENCRYPTION_KEY into an ``AttributeError`` on
# the next ``self.cipher.encrypt(...)``. Raise explicitly instead
# (PR #758 round-4 review medium 1).
if self.cipher is None:
raise RuntimeError(
"TOKEN_ENCRYPTION_KEY is not set — token storage operations unavailable"
)
async with aiosqlite.connect(self.db_path) as db:
async with db.execute(
nextcloud_mcp_server/auth/token_utils.py
+25 -14
View File
@@ -75,20 +75,31 @@ async def _get_cached(
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
try:
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
finally:
# Drop the dict entry so a misconfigured deployment hitting
# arbitrary URLs can't grow ``_fetch_locks`` without bound (PR #758
# round-4 review nit 4). Already-queued waiters share our local
# ``lock`` reference and remain coalesced; new arrivals lazily
# recreate a lock — by which time the cache is populated, so they
# short-circuit before reaching the lock anyway.
async with _fetch_locks_lock:
if _fetch_locks.get(url) is lock:
del _fetch_locks[url]
async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]:
nextcloud_mcp_server/server/oauth_tools.py
+4 -2
View File
@@ -432,8 +432,10 @@ async def _check_logged_in(ctx: Context, user_id: str) -> str:
# Store state in session for validation on callback
storage = await get_shared_storage()
# Create OAuth session for Flow 2
session_id = f"flow2_{user_id}_{secrets.token_hex(8)}"
# Create OAuth session for Flow 2. Identifier is purely random
# so audit-log entries / DB rows don't carry the user_id in the
# session_id field (PR #758 round-4 review medium 3).
session_id = f"flow2_{secrets.token_hex(16)}"
redirect_uri = f"{os.getenv('NEXTCLOUD_MCP_SERVER_URL', 'http://localhost:8000')}/oauth/callback"
await storage.store_oauth_session(
tests/unit/test_id_token_verification.py
+7
View File
@@ -528,3 +528,10 @@ async def test_get_cached_coalesces_concurrent_misses():
assert all(r == results[0] for r in results), (
"concurrent callers received divergent cached data"
)
# Pin the round-4 cleanup invariant: _fetch_locks must drain after the
# fetch completes so a probed deployment can't accumulate locks for
# arbitrary URLs.
assert len(token_utils._fetch_locks) == 0, (
"expected _fetch_locks to be empty after fetch, "
f"found {list(token_utils._fetch_locks)}"
)