From fe54733a39f76bd2ed9ca3891b33f0072fe2e2fa Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 18 Dec 2025 01:55:04 +0100 Subject: [PATCH] fix(oauth): enable PKCE for all clients and add token_broker to oauth_context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes two OAuth issues in the Astrolabe app: 1. **Always use PKCE (RFC 9207)**: - PKCE is now used for all OAuth flows (public and confidential clients) - Previous code only used PKCE for public clients, causing failures - Confidential clients now use both PKCE + client_secret (defense in depth) - Nextcloud OIDC provider requires PKCE, so token exchange was failing 2. **Add token_broker to oauth_context**: - Token broker is now stored in oauth_context for management API access - Fixes "Token broker not configured" error when revoking access - Revoke endpoint needs token_broker to delete refresh tokens and invalidate cache Changes: - OAuthController.php: Always generate PKCE verifier/challenge for all clients - OAuthController.php: Always include code_verifier in token exchange - app.py: Store token_broker in oauth_context after creation Fixes: - Astrolabe OAuth flow now works with Nextcloud OIDC - Revoke/disconnect functionality now works in Astrolabe settings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 --- nextcloud_mcp_server/app.py | 5 ++ .../lib/Controller/OAuthController.php | 86 ++++++++----------- 2 files changed, 43 insertions(+), 48 deletions(-) diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index ade967b..728aea8 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1445,6 +1445,11 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = client_secret=sync_client_secret, ) + # Store token broker in oauth_context for management API (revoke endpoint) + if hasattr(app.state, "oauth_context"): + app.state.oauth_context["token_broker"] = token_broker + logger.info("Token broker added to oauth_context for management API") + # Initialize Qdrant collection before starting background tasks logger.info("Initializing Qdrant collection...") from nextcloud_mcp_server.vector.qdrant_client import get_qdrant_client diff --git a/third_party/astrolabe/lib/Controller/OAuthController.php b/third_party/astrolabe/lib/Controller/OAuthController.php index dcfd3f8..23709d5 100644 --- a/third_party/astrolabe/lib/Controller/OAuthController.php +++ b/third_party/astrolabe/lib/Controller/OAuthController.php @@ -24,9 +24,12 @@ use Psr\Log\LoggerInterface; /** * OAuth controller for MCP Server UI. * - * Implements OAuth 2.0 Authorization Code flow with support for both: - * - Confidential clients (with client_secret): Direct token refresh, no PKCE - * - Public clients (without client_secret): PKCE-based flow for fallback + * Implements OAuth 2.0 Authorization Code flow with PKCE (RFC 9207). + * PKCE is always used for all clients (public and confidential) as recommended + * by modern OAuth 2.0 best practices. + * + * - Public clients: PKCE only + * - Confidential clients: PKCE + client_secret (defense in depth) */ class OAuthController extends Controller { private $config; @@ -67,10 +70,8 @@ class OAuthController extends Controller { /** * Initiate OAuth authorization flow. * - * For confidential clients (with client_secret): Standard OAuth flow, no PKCE. - * For public clients (without client_secret): Generates PKCE code verifier and challenge. - * - * Stores state in session, then redirects user to IdP authorization endpoint. + * Always generates PKCE code verifier and challenge (RFC 9207). + * Stores state and code verifier in session, then redirects user to IdP authorization endpoint. * * @return RedirectResponse|TemplateResponse */ @@ -98,31 +99,25 @@ class OAuthController extends Controller { throw new \Exception('MCP server URL not configured'); } - // Check if confidential client secret is configured + // Always generate PKCE values (RFC 9207: PKCE recommended for all clients) + $codeVerifier = bin2hex(random_bytes(32)); + $codeChallenge = $this->base64UrlEncode(hash('sha256', $codeVerifier, true)); + + // Check if confidential client secret is also configured $clientSecret = $this->config->getSystemValue('astrolabe_client_secret', ''); $isConfidentialClient = !empty($clientSecret); - // Generate PKCE values only for public clients - $codeVerifier = null; - $codeChallenge = null; - - if (!$isConfidentialClient) { - // Public client: use PKCE - $codeVerifier = bin2hex(random_bytes(32)); - $codeChallenge = $this->base64UrlEncode(hash('sha256', $codeVerifier, true)); - - $this->logger->info('Using public client mode with PKCE'); + if ($isConfidentialClient) { + $this->logger->info('Using confidential client mode with PKCE and client secret'); } else { - $this->logger->info('Using confidential client mode with client secret'); + $this->logger->info('Using public client mode with PKCE only'); } // Generate state for CSRF protection $state = bin2hex(random_bytes(16)); // Store values in session - if ($codeVerifier) { - $this->session->set('mcp_oauth_code_verifier', $codeVerifier); - } + $this->session->set('mcp_oauth_code_verifier', $codeVerifier); $this->session->set('mcp_oauth_state', $state); $this->session->set('mcp_oauth_user_id', $user->getUID()); @@ -181,13 +176,10 @@ class OAuthController extends Controller { throw new \Exception('Invalid state parameter (CSRF protection)'); } - // Get stored PKCE verifier (may be null for confidential clients) + // Get stored PKCE verifier (always required) $codeVerifier = $this->session->get('mcp_oauth_code_verifier'); - - // Check if we have either client_secret or code_verifier - $clientSecret = $this->config->getSystemValue('astrolabe_client_secret', ''); - if (empty($clientSecret) && empty($codeVerifier)) { - throw new \Exception('Neither client secret nor code verifier available for authentication'); + if (empty($codeVerifier)) { + throw new \Exception('PKCE code verifier not found in session'); } // Get user ID from session @@ -287,16 +279,18 @@ class OAuthController extends Controller { * to find the authorization endpoint. Supports both Nextcloud OIDC and * external IdPs like Keycloak. * + * Always uses PKCE (RFC 9207 recommends PKCE for all clients). + * * @param string $mcpServerUrl Base URL of MCP server * @param string $state CSRF state parameter - * @param string|null $codeChallenge PKCE code challenge (null for confidential clients) + * @param string $codeChallenge PKCE code challenge * @return string Authorization URL * @throws \Exception if OIDC discovery fails */ private function buildAuthorizationUrl( string $mcpServerUrl, string $state, - ?string $codeChallenge, + string $codeChallenge, ): string { // First, query MCP server to discover which IdP it's configured to use $this->logger->info('buildAuthorizationUrl: Starting', [ @@ -407,11 +401,9 @@ class OAuthController extends Controller { 'resource' => $mcpServerPublicUrl, // RFC 8707 Resource Indicator - request token with MCP server audience ]; - // Add PKCE parameters only for public clients - if ($codeChallenge !== null) { - $params['code_challenge'] = $codeChallenge; - $params['code_challenge_method'] = 'S256'; - } + // Add PKCE parameters (always required per RFC 9207) + $params['code_challenge'] = $codeChallenge; + $params['code_challenge_method'] = 'S256'; return $authEndpoint . '?' . http_build_query($params); } @@ -419,22 +411,23 @@ class OAuthController extends Controller { /** * Exchange authorization code for access token. * - * For confidential clients: Uses client_secret for authentication. - * For public clients: Uses PKCE code_verifier for authentication. + * Always uses PKCE code_verifier (RFC 9207). + * For confidential clients: Also includes client_secret for additional security. + * For public clients: Uses PKCE code_verifier only. * * Queries MCP server for IdP configuration, then performs OIDC discovery * to find the token endpoint. Supports both Nextcloud OIDC and external IdPs. * * @param string $mcpServerUrl Base URL of MCP server * @param string $code Authorization code - * @param string|null $codeVerifier PKCE code verifier (null for confidential clients) + * @param string $codeVerifier PKCE code verifier * @return array Token data containing access_token, refresh_token, expires_in * @throws \Exception on HTTP or token error */ private function exchangeCodeForToken( string $mcpServerUrl, string $code, - ?string $codeVerifier, + string $codeVerifier, ): array { // Query MCP server to discover which IdP it's configured to use try { @@ -494,19 +487,16 @@ class OAuthController extends Controller { 'client_id' => $this->client->getClientId(), ]; - // Add client authentication based on client type - $clientSecret = $this->config->getSystemValue('astrolabe_client_secret', ''); + // Always include PKCE code verifier (RFC 9207) + $postData['code_verifier'] = $codeVerifier; + // Also include client secret if configured (defense in depth for confidential clients) + $clientSecret = $this->config->getSystemValue('astrolabe_client_secret', ''); if (!empty($clientSecret)) { - // Confidential client: use client secret for authentication $postData['client_secret'] = $clientSecret; - $this->logger->info('Using client secret for token exchange'); - } elseif ($codeVerifier !== null) { - // Public client: use PKCE proof for authentication - $postData['code_verifier'] = $codeVerifier; - $this->logger->info('Using PKCE code verifier for token exchange'); + $this->logger->info('Using PKCE with client secret for token exchange'); } else { - throw new \Exception('Neither client_secret nor code_verifier available for token exchange'); + $this->logger->info('Using PKCE only for token exchange'); } // Use Nextcloud's HTTP client for token request