fix(auth): address PR #758 auto-review (id-token verify, nonce, CI key)

Blocking:
- AS proxy callback now calls verify_id_token before caching the proxy
  code so a tampered IdP response can't smuggle identity claims.

Important:
- Browser OAuth flow generates and verifies an OIDC nonce; new alembic
  migration 006 adds the nonce column to oauth_sessions.
- _origin_matches_self logs a warning when CSRF check is bypassed.
- oauth_tools.py uses get_shared_storage instead of fresh handles.

Nits:
- New token_utils.get_oidc_discovery shares the 5-minute cache with
  verify_id_token; oauth_login (integrated) and _revoke_refresh_token_at_idp
  now use it instead of issuing fresh discovery fetches.
- Drop typing.Optional from oauth_tools.py in favour of X | None.

CI:
- test.yml generates an ephemeral Fernet TOKEN_ENCRYPTION_KEY per run
  with openssl, removing the dependency on a missing repo secret.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-02 20:48:25 +02:00
co-authored by Claude Opus 4.7
parent 2ef4bfc4af
commit 4c84d82984
9 changed files with 277 additions and 54 deletions
+14 -5
View File
@@ -129,6 +129,18 @@ jobs:
# npm ci
# npm run build
# Generate an ephemeral Fernet key per CI run. docker-compose.yml
# requires TOKEN_ENCRYPTION_KEY (PR #758 finding 5 removed the
# hardcoded default), but the CI tokens.db is destroyed at the end of
# the job so there is no value in persisting the key as a repo secret.
# ``openssl rand -base64 32`` produces 32 bytes encoded as 44 base64
# chars; ``tr '+/' '-_'`` converts to URL-safe base64, which is
# exactly what Fernet expects.
- name: Generate ephemeral TOKEN_ENCRYPTION_KEY
run: |
KEY=$(openssl rand -base64 32 | tr '+/' '-_')
echo "TOKEN_ENCRYPTION_KEY=${KEY}" >> "$GITHUB_ENV"
# Start services with the appropriate profile
- name: Run docker compose
uses: hoverkraft-tech/compose-action@4894d2492015c1774ee5a13a95b1072093087ec3 # v2.5.0
@@ -139,11 +151,8 @@ jobs:
env:
MCP_SERVER_URL: ${{ matrix.mcp-internal-url }}
NEXTCLOUD_IMAGE: ${{ matrix.nextcloud_image }}
# Required by docker-compose.yml since PR #758 finding 5 (no more
# hardcoded Fernet keys). Generated once and stored as a repo
# secret; the CI tokens.db is ephemeral so a single shared key
# across services is acceptable.
TOKEN_ENCRYPTION_KEY: ${{ secrets.TOKEN_ENCRYPTION_KEY }}
# Inherited from $GITHUB_ENV via the previous step.
TOKEN_ENCRYPTION_KEY: ${{ env.TOKEN_ENCRYPTION_KEY }}
- name: Install the latest version of uv
uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0
@@ -0,0 +1,29 @@
"""Add nonce column to oauth_sessions for OIDC ID-token binding.
PR #758 finding 2: the browser OAuth flow generated PKCE + state but no
``nonce``. Without a nonce, an attacker who obtains a valid ID token for
another user (e.g. from a parallel auth request) could replay it inside
this flow because the token isn't cryptographically tied to the
authorization request. The nonce is generated in ``oauth_login``,
forwarded to the IdP in the auth URL, and verified on the way back.
Revision ID: 006
Revises: 005
Create Date: 2026-05-02 16:00:00.000000
"""
from alembic import op
revision = "006"
down_revision = "005"
branch_labels = None
depends_on = None
def upgrade() -> None:
op.execute("ALTER TABLE oauth_sessions ADD COLUMN nonce TEXT")
def downgrade() -> None:
# SQLite < 3.35 cannot DROP COLUMN; leave the column on downgrade.
pass
@@ -19,6 +19,7 @@ from starlette.responses import HTMLResponse, JSONResponse, RedirectResponse
from nextcloud_mcp_server.auth.token_utils import (
IdTokenVerificationError,
get_oidc_discovery,
verify_id_token,
)
from nextcloud_mcp_server.auth.userinfo_routes import (
@@ -65,7 +66,15 @@ def _origin_matches_self(request: Request, oauth_ctx: dict) -> bool:
cfg = oauth_ctx.get("config") or oauth_ctx
mcp_server_url = cfg.get("mcp_server_url")
if not mcp_server_url:
# Mis-configured deployment — fail open rather than break logout.
# Mis-configured deployment — fail open rather than break logout, but
# log loudly so the operator can see this is happening (PR #758
# finding 3). Other OAuth code paths require ``mcp_server_url`` and
# KeyError if it's absent, so this branch should never fire in a
# correctly configured deployment.
logger.warning(
"CSRF check bypassed on /oauth/logout: mcp_server_url not "
"configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL"
)
return True
expected = _normalise_origin(mcp_server_url)
@@ -149,6 +158,12 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
# Generate state for CSRF protection
state = secrets.token_urlsafe(32)
# Generate OIDC nonce so the ID token returned on callback can be bound
# to THIS auth request (PR #758 finding 2). Without a nonce, an attacker
# who acquired a separate valid ID token could replay it inside this
# flow.
nonce = secrets.token_urlsafe(32)
# Build OAuth authorization URL
mcp_server_url = oauth_config["mcp_server_url"]
callback_uri = f"{mcp_server_url}/oauth/callback"
@@ -164,7 +179,8 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
digest = hashlib.sha256(code_verifier.encode()).digest()
code_challenge = urlsafe_b64encode(digest).decode().rstrip("=")
# Store code_verifier in session for retrieval during callback (using state as key)
# Store code_verifier + nonce in session for retrieval during callback
# (using state as key)
await storage.store_oauth_session(
session_id=state, # Use state as session ID
client_id="browser-ui",
@@ -173,6 +189,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
code_challenge=code_challenge,
code_challenge_method="S256",
mcp_authorization_code=code_verifier, # Store code_verifier here temporarily
nonce=nonce,
flow_type="browser",
ttl_seconds=600, # 10 minutes
)
@@ -199,6 +216,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
"response_type": "code",
"scope": scopes,
"state": state,
"nonce": nonce,
"code_challenge": code_challenge,
"code_challenge_method": "S256",
"prompt": "consent", # Ensure refresh token
@@ -219,12 +237,11 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
status_code=500,
)
# Fetch authorization endpoint
async with nextcloud_httpx_client() as http_client:
response = await http_client.get(discovery_url)
response.raise_for_status()
discovery = response.json()
authorization_endpoint = discovery["authorization_endpoint"]
# Fetch authorization endpoint via the shared 5-minute discovery
# cache (PR #758 nit 5) so each browser login doesn't hit the IdP's
# discovery endpoint.
discovery = await get_oidc_discovery(discovery_url)
authorization_endpoint = discovery["authorization_endpoint"]
# Include offline_access only if the IdP advertises it (or if
# scopes_supported is absent from the discovery document).
@@ -257,6 +274,7 @@ async def oauth_login(request: Request) -> RedirectResponse | JSONResponse:
"response_type": "code",
"scope": scopes,
"state": state,
"nonce": nonce,
"code_challenge": code_challenge,
"code_challenge_method": "S256",
"prompt": "consent", # Ensure refresh token
@@ -336,13 +354,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
oauth_client = oauth_ctx["oauth_client"]
oauth_config = oauth_ctx["config"]
# Retrieve code_verifier and redirect URL from session storage
# Retrieve code_verifier, nonce, and redirect URL from session storage
code_verifier = ""
nonce: str | None = None
next_url = "/app" # Default redirect
oauth_session = await storage.get_oauth_session(state)
if oauth_session:
# code_verifier was stored in mcp_authorization_code field
code_verifier = oauth_session.get("mcp_authorization_code", "")
# nonce bound to this auth request — verified against the ID token
# below (PR #758 finding 2).
nonce = oauth_session.get("nonce")
# next_url was stored in client_redirect_uri field — re-validate at
# read-time as defense-in-depth (issue #758 finding 3). The session
# row could have been written by an older code path or reused.
@@ -483,6 +505,7 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
id_token,
discovery_url=verification_discovery_url,
expected_audience=verification_audience,
expected_nonce=nonce,
)
except IdTokenVerificationError as e:
logger.error("ID token verification failed: %s", e)
@@ -673,21 +696,21 @@ async def _revoke_refresh_token_at_idp(oauth_ctx: dict, refresh_token: str) -> N
if not discovery_url:
return
# Re-use the shared 5-minute discovery cache (PR #758 nit 6) so a
# burst of logouts doesn't hammer the IdP's discovery endpoint.
discovery = await get_oidc_discovery(discovery_url)
revocation_endpoint = discovery.get("revocation_endpoint")
if not revocation_endpoint:
logger.debug("IdP advertises no revocation_endpoint; skipping")
return
client_id = cfg.get("client_id") or settings.oidc_client_id
client_secret = cfg.get("client_secret") or settings.oidc_client_secret
if not (client_id and client_secret):
logger.debug("No OIDC client credentials available for revocation")
return
async with nextcloud_httpx_client() as http_client:
discovery_response = await http_client.get(discovery_url)
discovery_response.raise_for_status()
discovery = discovery_response.json()
revocation_endpoint = discovery.get("revocation_endpoint")
if not revocation_endpoint:
logger.debug("IdP advertises no revocation_endpoint; skipping")
return
client_id = cfg.get("client_id") or settings.oidc_client_id
client_secret = cfg.get("client_secret") or settings.oidc_client_secret
if not (client_id and client_secret):
logger.debug("No OIDC client credentials available for revocation")
return
response = await http_client.post(
revocation_endpoint,
data={
+22
View File
@@ -954,6 +954,28 @@ async def _oauth_callback_as_proxy(
f"(token_type={nc_token_response.get('token_type')})"
)
# Verify the ID token signature + claims before caching the response
# (PR #758 finding 1). Without this, a compromised IdP or tampered
# transport could plant arbitrary identity claims into the proxy code
# entry that gets handed back to the MCP client. Mirrors the
# verification done in oauth_callback_nextcloud.
id_token = nc_token_response.get("id_token")
try:
await verify_id_token(
id_token,
discovery_url=discovery_url,
expected_audience=mcp_server_client_id,
)
except IdTokenVerificationError as e:
logger.error("AS proxy: ID token verification failed: %s", e)
return JSONResponse(
{
"error": "invalid_token",
"error_description": "ID token failed verification",
},
status_code=400,
)
# Generate a proxy authorization code for the client
proxy_code = secrets.token_urlsafe(32)
_proxy_codes[proxy_code] = ProxyCodeEntry(
+6 -2
View File
@@ -917,6 +917,7 @@ class RefreshTokenStorage:
flow_type: str = "hybrid",
is_provisioning: bool = False,
requested_scopes: str | None = None,
nonce: str | None = None,
ttl_seconds: int = 600, # 10 minutes
) -> None:
"""
@@ -933,6 +934,8 @@ class RefreshTokenStorage:
flow_type: Type of flow ('hybrid', 'flow1', 'flow2')
is_provisioning: Whether this is a Flow 2 provisioning session
requested_scopes: Requested OAuth scopes
nonce: OIDC ``nonce`` value bound to this auth request, returned
in the ID token and verified on callback (PR #758 finding 2).
ttl_seconds: Session TTL in seconds
"""
if not self._initialized:
@@ -947,8 +950,8 @@ class RefreshTokenStorage:
INSERT INTO oauth_sessions
(session_id, client_id, client_redirect_uri, state, code_challenge,
code_challenge_method, mcp_authorization_code, flow_type,
is_provisioning, requested_scopes, created_at, expires_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
is_provisioning, requested_scopes, nonce, created_at, expires_at)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
""",
(
session_id,
@@ -961,6 +964,7 @@ class RefreshTokenStorage:
flow_type,
is_provisioning,
requested_scopes,
nonce,
now,
expires_at,
),
+11
View File
@@ -52,6 +52,17 @@ async def _get_cached(
return data
async def get_oidc_discovery(discovery_url: str) -> dict[str, Any]:
"""Return the cached OIDC discovery document for *discovery_url*.
Shares the 5-minute discovery cache used by `verify_id_token`, so a
callback that does discovery → token-exchange → ID-token verification
reuses one HTTP round-trip instead of three. Public alias for `_get_cached`
against `_discovery_cache` (PR #758 nits 5 & 6).
"""
return await _get_cached(_discovery_cache, discovery_url)
async def verify_id_token(
id_token: str,
*,
+10 -14
View File
@@ -9,7 +9,6 @@ import logging
import os
import secrets
from datetime import datetime, timezone
from typing import Optional
from urllib.parse import urlencode
from mcp.server.fastmcp import Context
@@ -18,7 +17,7 @@ from pydantic import BaseModel, Field
from nextcloud_mcp_server.auth import require_scopes
from nextcloud_mcp_server.auth.astrolabe_client import AstrolabeClient
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
from nextcloud_mcp_server.auth.storage import get_shared_storage
from nextcloud_mcp_server.auth.token_broker import TokenBrokerService
# Re-export for backward compatibility — canonical location is auth.token_utils
@@ -34,17 +33,17 @@ class ProvisioningStatus(BaseModel):
"""Status of Nextcloud provisioning for a user."""
is_provisioned: bool = Field(description="Whether Nextcloud access is provisioned")
provisioned_at: Optional[str] = Field(
provisioned_at: str | None = Field(
None, description="ISO timestamp when provisioned"
)
credential_type: Optional[str] = Field(
credential_type: str | None = Field(
None, description="Type of credential ('refresh_token' or 'app_password')"
)
client_id: Optional[str] = Field(
client_id: str | None = Field(
None, description="Client ID that initiated the original Flow 1"
)
scopes: Optional[list[str]] = Field(None, description="Granted scopes")
flow_type: Optional[str] = Field(
scopes: list[str] | None = Field(None, description="Granted scopes")
flow_type: str | None = Field(
None, description="Type of flow used ('hybrid', 'flow1', 'flow2')"
)
@@ -53,7 +52,7 @@ class ProvisioningResult(BaseModel):
"""Result of provisioning attempt."""
success: bool = Field(description="Whether provisioning was initiated")
provisioning_url: Optional[str] = Field(
provisioning_url: str | None = Field(
None, description="URL to Astrolabe settings for provisioning background sync"
)
message: str = Field(description="Status message for the user")
@@ -122,8 +121,7 @@ async def get_provisioning_status(ctx: Context, user_id: str) -> ProvisioningSta
logger.info(
f" get_provisioning_status: Looking up refresh token for user_id={user_id}"
)
storage = RefreshTokenStorage.from_env()
await storage.initialize()
storage = await get_shared_storage()
token_data = await storage.get_refresh_token(user_id)
@@ -291,8 +289,7 @@ async def revoke_nextcloud_access(ctx: Context, user_id: str) -> RevocationResul
)
# Initialize Token Broker to handle revocation
storage = RefreshTokenStorage.from_env()
await storage.initialize()
storage = await get_shared_storage()
# Get OAuth client credentials from storage
client_creds = await storage.get_oauth_client()
@@ -420,8 +417,7 @@ async def check_logged_in(ctx: Context, user_id: str) -> str:
state = secrets.token_urlsafe(32)
# Store state in session for validation on callback
storage = RefreshTokenStorage.from_env()
await storage.initialize()
storage = await get_shared_storage()
# Create OAuth session for Flow 2
session_id = f"flow2_{user_id}_{secrets.token_hex(8)}"
@@ -9,6 +9,11 @@ storage layer to confirm the row is gone after the callback runs.
We mock everything *after* the deletion (discovery + token exchange +
ID token verification) so the test focuses on the cleanup contract,
not the OAuth wire protocol.
Also pins the AS-proxy callback's ID-token verification rejection path
introduced in PR #758 finding 1 (auto-review): a forged or unsigned
id_token must surface as a 400 ``invalid_token`` JSONResponse and must
not register a proxy code.
"""
import tempfile
@@ -19,8 +24,15 @@ import httpx
import pytest
from cryptography.fernet import Fernet
from nextcloud_mcp_server.auth.oauth_routes import oauth_callback_nextcloud
from nextcloud_mcp_server.auth.oauth_routes import (
ASProxySession,
_as_proxy_sessions,
_oauth_callback_as_proxy,
_proxy_codes,
oauth_callback_nextcloud,
)
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
from nextcloud_mcp_server.auth.token_utils import IdTokenVerificationError
pytestmark = pytest.mark.unit
@@ -170,3 +182,85 @@ async def test_callback_no_session_row_does_not_crash(storage):
# No crash, no row, no surprises.
assert await storage.get_oauth_session(state) is None
# ---------------------------------------------------------------------------
# AS proxy callback (PR #758 finding 1): ID-token verification rejection
# ---------------------------------------------------------------------------
def _build_as_proxy_request(*, code: str, state: str):
request = MagicMock()
request.query_params = {"code": code, "state": state}
request.app.state.oauth_context = {
"config": {
"discovery_url": "https://idp.example.com/.well-known/openid-configuration",
"mcp_server_url": "https://mcp.example.com",
"client_id": "mcp-server",
"client_secret": "mcp-secret",
}
}
return request
async def test_as_proxy_rejects_invalid_id_token():
"""Forged/unsigned id_token in the IdP token response → 400 invalid_token.
Pins PR #758 finding 1. Without verification a compromised IdP or
tampered transport could plant arbitrary identity claims into the
cached ProxyCodeEntry that downstream clients pick up.
"""
server_state = "as-proxy-state-rejected"
_as_proxy_sessions[server_state] = ASProxySession(
client_id="mcp-client",
client_redirect_uri="http://127.0.0.1:9999/callback",
client_state="client-state-xyz",
code_challenge="challenge",
code_challenge_method="S256",
requested_scopes="openid",
)
_proxy_codes.clear()
request = _build_as_proxy_request(code="auth-code", state=server_state)
fake_discovery = {
"token_endpoint": "https://idp.example.com/token",
"issuer": "https://idp.example.com",
}
fake_token_response = MagicMock(status_code=200)
fake_token_response.json.return_value = {
"access_token": "ac-tok",
"refresh_token": "rf-tok",
"id_token": "forged.id.token",
"token_type": "Bearer",
}
fake_http = MagicMock()
fake_http.post = AsyncMock(return_value=fake_token_response)
fake_http.__aenter__ = AsyncMock(return_value=fake_http)
fake_http.__aexit__ = AsyncMock(return_value=None)
with (
patch(
"nextcloud_mcp_server.auth.oauth_routes._get_cached_discovery",
new=AsyncMock(return_value=fake_discovery),
),
patch(
"nextcloud_mcp_server.auth.oauth_routes.nextcloud_httpx_client",
return_value=fake_http,
),
patch(
"nextcloud_mcp_server.auth.oauth_routes.verify_id_token",
new=AsyncMock(side_effect=IdTokenVerificationError("bad signature")),
),
):
response = await _oauth_callback_as_proxy(request, server_state)
assert response.status_code == 400
body = bytes(response.body).decode()
assert "invalid_token" in body
# Critical: the proxy code store must not have grown — a rejected
# callback must not be turned into a redeemable proxy code.
assert _proxy_codes == {}
# And the session has been popped (one-time use).
assert server_state not in _as_proxy_sessions
+44 -9
View File
@@ -20,6 +20,7 @@ import pytest
from cryptography.fernet import Fernet
from starlette.requests import HTTPConnection
from nextcloud_mcp_server.auth import token_utils
from nextcloud_mcp_server.auth.browser_oauth_routes import (
_revoke_refresh_token_at_idp,
oauth_logout,
@@ -30,6 +31,20 @@ from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
pytestmark = pytest.mark.unit
@pytest.fixture(autouse=True)
def _clear_oidc_discovery_cache():
"""Reset the shared discovery cache so tests don't see each other's fetches.
``_revoke_refresh_token_at_idp`` was changed (PR #758 nit 6) to use
``token_utils.get_oidc_discovery`` which caches for 5 minutes — without
this clear, the second test in the file would see the first test's
discovery doc and skip the MockTransport call.
"""
token_utils._discovery_cache.clear()
yield
token_utils._discovery_cache.clear()
# ---------------------------------------------------------------------------
# storage fixture (real SQLite backend; lighter than mocking every call)
# ---------------------------------------------------------------------------
@@ -337,9 +352,17 @@ async def test_revoke_helper_posts_to_revocation_endpoint():
kwargs["transport"] = transport
return httpx.AsyncClient(**kwargs)
with patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
# Discovery now goes through token_utils.get_oidc_discovery (PR #758 nit
# 6); revocation POST still uses browser_oauth_routes' httpx client.
with (
patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
),
patch(
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
side_effect=fake_client,
),
):
await _revoke_refresh_token_at_idp(
{
@@ -373,9 +396,15 @@ async def test_revoke_helper_skips_when_no_revocation_endpoint():
kwargs["transport"] = transport
return httpx.AsyncClient(**kwargs)
with patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
with (
patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
),
patch(
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
side_effect=fake_client,
),
):
# Returns None and does not raise
result = await _revoke_refresh_token_at_idp(
@@ -403,9 +432,15 @@ async def test_revoke_helper_silent_on_idp_error():
kwargs["transport"] = transport
return httpx.AsyncClient(**kwargs)
with patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
with (
patch(
"nextcloud_mcp_server.auth.browser_oauth_routes.nextcloud_httpx_client",
side_effect=fake_client,
),
patch(
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
side_effect=fake_client,
),
):
result = await _revoke_refresh_token_at_idp(
{