fix(auth): address PR #758 round-3 review
- Flow 2 (oauth_authorize_nextcloud) now generates a nonce, stores it on the oauth_session row, forwards it to the IdP, and verifies it via expected_nonce in oauth_callback_nextcloud — closes the last replay- protection gap (round-3 finding 1). - _origin_matches_self fails closed when mcp_server_url is missing instead of allowing the logout, and the diagnostic log is promoted from warning to error so the misconfiguration is monitorable (round-3 finding 2). New regression test pins the new behaviour. - The five user_id-accepting helpers in oauth_tools.py (get_provisioning_status, provision_nextcloud_access, revoke_nextcloud_access, check_provisioning_status, check_logged_in) are renamed with leading underscores to make the trust boundary structural rather than documentary (round-3 finding 3). - create_browser_session and delete_browser_session now emit audit_log rows so session establishment / teardown match the pattern used by the rest of the security-relevant storage operations (round-3 nit 5). delete_browser_session selects user_id before delete so the audit row is attributable. - oauth_login_callback no longer reflects raw IdP-error text or exception strings into the HTML failure page; users see a generic "internal error occurred" message + a correlation ID, with the detail logged server-side keyed by the same ID (round-3 nit 6). The XSS regression test is updated to pin the stricter contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
c33d52ea91
commit
9d0e7dcebe
@@ -66,16 +66,16 @@ 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, 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 "
|
||||
# Fail closed (PR #758 round-3 finding 2): a future code path that
|
||||
# leaves ``mcp_server_url`` unset would otherwise silently disable
|
||||
# CSRF protection on /oauth/logout. Blocking the logout is
|
||||
# recoverable — the user just re-logs-in once the misconfiguration
|
||||
# is fixed — and the error log makes the cause monitorable.
|
||||
logger.error(
|
||||
"CSRF check failed on /oauth/logout: mcp_server_url not "
|
||||
"configured in oauth_context — set NEXTCLOUD_MCP_SERVER_URL"
|
||||
)
|
||||
return True
|
||||
return False
|
||||
|
||||
expected = _normalise_origin(mcp_server_url)
|
||||
raw = request.headers.get("origin") or request.headers.get("referer")
|
||||
@@ -434,14 +434,19 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
token_data = response.json()
|
||||
|
||||
except httpx.HTTPStatusError as e:
|
||||
# Correlation IDs let the user reference a specific failure in the
|
||||
# server logs without us having to reflect raw exception/IdP text
|
||||
# back into the HTML page (PR #758 round-3 nit 6).
|
||||
correlation_id = secrets.token_hex(8)
|
||||
error_body = (
|
||||
e.response.text if hasattr(e.response, "text") else str(e.response.content)
|
||||
)
|
||||
logger.error(
|
||||
"Token exchange failed: HTTP %s - %s", e.response.status_code, error_body
|
||||
"Token exchange failed (correlation_id=%s): HTTP %s - %s",
|
||||
correlation_id,
|
||||
e.response.status_code,
|
||||
error_body,
|
||||
)
|
||||
# html_escape: error_body originates from the IdP and could contain
|
||||
# markup that would be reflected into the failure page otherwise.
|
||||
return HTMLResponse(
|
||||
f"""
|
||||
<!DOCTYPE html>
|
||||
@@ -449,15 +454,17 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
<head><title>Login Failed</title></head>
|
||||
<body>
|
||||
<h1>Login Failed</h1>
|
||||
<p>Failed to exchange authorization code for tokens</p>
|
||||
<p>HTTP {e.response.status_code}: {html_escape(error_body)}</p>
|
||||
<p>An internal error occurred while exchanging the authorization code.</p>
|
||||
<p>Correlation ID: <code>{html_escape(correlation_id)}</code></p>
|
||||
<p>Please try again, or contact your administrator if the problem persists.</p>
|
||||
</body>
|
||||
</html>
|
||||
""",
|
||||
status_code=500,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.error("Token exchange failed: %s", e)
|
||||
correlation_id = secrets.token_hex(8)
|
||||
logger.error("Token exchange failed (correlation_id=%s): %s", correlation_id, e)
|
||||
return HTMLResponse(
|
||||
f"""
|
||||
<!DOCTYPE html>
|
||||
@@ -465,8 +472,9 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
<head><title>Login Failed</title></head>
|
||||
<body>
|
||||
<h1>Login Failed</h1>
|
||||
<p>Failed to exchange authorization code for tokens</p>
|
||||
<p>Error: {html_escape(str(e))}</p>
|
||||
<p>An internal error occurred while exchanging the authorization code.</p>
|
||||
<p>Correlation ID: <code>{html_escape(correlation_id)}</code></p>
|
||||
<p>Please try again, or contact your administrator if the problem persists.</p>
|
||||
</body>
|
||||
</html>
|
||||
""",
|
||||
@@ -508,13 +516,19 @@ async def oauth_login_callback(request: Request) -> RedirectResponse | HTMLRespo
|
||||
expected_nonce=nonce,
|
||||
)
|
||||
except IdTokenVerificationError as e:
|
||||
logger.error("ID token verification failed: %s", e)
|
||||
# html_escape: defense-in-depth. The exception text is currently
|
||||
# server-constructed, but escape on the success path too so any
|
||||
# future error wrapping that includes IdP response text can't
|
||||
# smuggle markup into the login-failure page.
|
||||
# Same correlation-ID pattern as token-exchange failures
|
||||
# (PR #758 round-3 nit 6) — log the detail server-side and only
|
||||
# show a generic message + correlation ID in the browser.
|
||||
correlation_id = secrets.token_hex(8)
|
||||
logger.error(
|
||||
"ID token verification failed (correlation_id=%s): %s",
|
||||
correlation_id,
|
||||
e,
|
||||
)
|
||||
return HTMLResponse(
|
||||
f"<h1>Login Failed</h1><p>ID token failed verification: {html_escape(str(e))}</p>",
|
||||
f"<h1>Login Failed</h1>"
|
||||
f"<p>The ID token failed verification.</p>"
|
||||
f"<p>Correlation ID: <code>{html_escape(correlation_id)}</code></p>",
|
||||
status_code=400,
|
||||
)
|
||||
|
||||
|
||||
@@ -465,7 +465,12 @@ async def oauth_authorize_nextcloud(
|
||||
digest = hashlib.sha256(code_verifier.encode()).digest()
|
||||
code_challenge = urlsafe_b64encode(digest).decode().rstrip("=")
|
||||
|
||||
# Store code_verifier in session for retrieval during callback
|
||||
# OIDC nonce binds the IdP-returned ID token to THIS auth request
|
||||
# (PR #758 round-3 finding 1). Browser flow + AS proxy already do
|
||||
# this; Flow 2 is the third path and was missing it.
|
||||
nonce = secrets.token_urlsafe(32)
|
||||
|
||||
# Store code_verifier + nonce in session for retrieval during callback
|
||||
storage = oauth_ctx["storage"]
|
||||
await storage.store_oauth_session(
|
||||
session_id=state,
|
||||
@@ -475,6 +480,7 @@ async def oauth_authorize_nextcloud(
|
||||
code_challenge=code_challenge,
|
||||
code_challenge_method="S256",
|
||||
mcp_authorization_code=code_verifier, # Store code_verifier here temporarily
|
||||
nonce=nonce,
|
||||
flow_type="flow2",
|
||||
ttl_seconds=600, # 10 minutes
|
||||
)
|
||||
@@ -512,6 +518,7 @@ async def oauth_authorize_nextcloud(
|
||||
"response_type": "code",
|
||||
"scope": scopes,
|
||||
"state": state,
|
||||
"nonce": nonce,
|
||||
"code_challenge": code_challenge,
|
||||
"code_challenge_method": "S256",
|
||||
"prompt": "consent", # Force consent to show resource access
|
||||
@@ -572,12 +579,15 @@ async def oauth_callback_nextcloud(request: Request):
|
||||
storage: RefreshTokenStorage = oauth_ctx["storage"]
|
||||
oauth_config = oauth_ctx["config"]
|
||||
|
||||
# Retrieve code_verifier from session storage (PKCE required by Nextcloud OIDC)
|
||||
# Retrieve code_verifier + nonce from session storage (PKCE + OIDC
|
||||
# nonce binding both required for Flow 2 — round-3 finding 1).
|
||||
code_verifier = ""
|
||||
nonce: str | None = None
|
||||
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 = oauth_session.get("nonce")
|
||||
logger.info(
|
||||
f"Retrieved code_verifier for Flow 2 callback (state={state[:16]}...)"
|
||||
)
|
||||
@@ -636,12 +646,16 @@ async def oauth_callback_nextcloud(request: Request):
|
||||
id_token = token_data.get("id_token")
|
||||
|
||||
# Verify ID token signature + claims (issue #626 finding 1).
|
||||
# ``expected_nonce`` is the per-request nonce stored on the
|
||||
# oauth_session row (PR #758 round-3 finding 1); falsy → skip nonce
|
||||
# check for sessions written before the column existed.
|
||||
logger.info("oauth_callback_nextcloud: Verifying ID token")
|
||||
try:
|
||||
userinfo = await verify_id_token(
|
||||
id_token,
|
||||
discovery_url=discovery_url,
|
||||
expected_audience=mcp_server_client_id,
|
||||
expected_nonce=nonce or None,
|
||||
)
|
||||
except IdTokenVerificationError as e:
|
||||
logger.error("ID token verification failed: %s", e)
|
||||
|
||||
@@ -1178,6 +1178,16 @@ class RefreshTokenStorage:
|
||||
ttl_seconds,
|
||||
)
|
||||
|
||||
# Audit log to match the pattern used by the other security-relevant
|
||||
# storage operations (PR #758 round-3 nit 5). Browser session
|
||||
# establishment is a security-relevant event.
|
||||
await self._audit_log(
|
||||
event="create_browser_session",
|
||||
user_id=user_id,
|
||||
resource_type="browser_session",
|
||||
resource_id=session_id[:8],
|
||||
)
|
||||
|
||||
async def get_browser_session_user(self, session_id: str) -> str | None:
|
||||
"""Look up the user_id bound to a browser session_id, or None.
|
||||
|
||||
@@ -1210,7 +1220,16 @@ 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).
|
||||
async with aiosqlite.connect(self.db_path) as db:
|
||||
async with db.execute(
|
||||
"SELECT user_id FROM browser_sessions WHERE session_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,)
|
||||
)
|
||||
@@ -1219,6 +1238,13 @@ class RefreshTokenStorage:
|
||||
|
||||
if deleted:
|
||||
logger.debug("Deleted browser session %s", session_id[:8])
|
||||
if user_id:
|
||||
await self._audit_log(
|
||||
event="delete_browser_session",
|
||||
user_id=user_id,
|
||||
resource_type="browser_session",
|
||||
resource_id=session_id[:8],
|
||||
)
|
||||
return deleted
|
||||
|
||||
async def cleanup_expired_browser_sessions(self) -> int:
|
||||
|
||||
Reference in New Issue
Block a user