From dc0100cf808ec6616b38b7a9d3e1e11d57433000 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 17 Mar 2026 07:21:31 +0100 Subject: [PATCH 1/2] fix: restore session fallback for all critical OAuth params after consent The consent flow redirects back to /apps/oidc/authorize with only client_id and scope in the URL. Version 1.16.2 changed the session fallback to only trigger when client_id is empty, which means state, response_type, and redirect_uri are lost after consent, causing a 500. Restore the 1.16.1 behavior of falling back to session when ANY critical parameter is missing, but with per-parameter guards so URL-provided values are preserved. This fixes both: - Consent redirect (has client_id but missing state/response_type/redirect_uri) - Implicit flow (missing client_id, as in Guacamole case from #626) Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/Controller/LoginRedirectorController.php | 30 +++++++++++++------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/Controller/LoginRedirectorController.php b/lib/Controller/LoginRedirectorController.php index 453df55d..a29f0ca4 100644 --- a/lib/Controller/LoginRedirectorController.php +++ b/lib/Controller/LoginRedirectorController.php @@ -213,18 +213,26 @@ public function authorize( $scopeFromParam = $scope ?? 'null'; $this->logger->debug('[SCOPE DEBUG] Scope from URL parameter: ' . $scopeFromParam); - if (empty($client_id)) { - $client_id = $this->session->get('oidc_client_id'); - $this->logger->debug('[CLIENT DEBUG] Client ID from session fallback: ' . ($client_id ?? 'null')); - $state = $this->session->get('oidc_state'); - $response_type = $this->session->get('oidc_response_type'); - $redirect_uri = $this->session->get('oidc_redirect_uri'); - $scope = $this->session->get('oidc_scope'); + if (empty($client_id) || empty($state) || empty($response_type) || empty($redirect_uri)) { + if (empty($client_id)) { + $client_id = $this->session->get('oidc_client_id'); + $this->logger->debug('[CLIENT DEBUG] Client ID from session fallback: ' . ($client_id ?? 'null')); + } + if (empty($state)) { + $state = $this->session->get('oidc_state'); + } + if (empty($response_type)) { + $response_type = $this->session->get('oidc_response_type'); + } + if (empty($redirect_uri)) { + $redirect_uri = $this->session->get('oidc_redirect_uri'); + } + $scope = $scope ?? $this->session->get('oidc_scope'); $this->logger->debug('[SCOPE DEBUG] Scope from session fallback: ' . ($scope ?? 'null')); - $nonce = $this->session->get('oidc_nonce'); - $resource = $this->session->get('oidc_resource'); - $code_challenge = $this->session->get('oidc_code_challenge'); - $code_challenge_method = $this->session->get('oidc_code_challenge_method'); + $nonce = $nonce ?? $this->session->get('oidc_nonce'); + $resource = $resource ?? $this->session->get('oidc_resource'); + $code_challenge = $code_challenge ?? $this->session->get('oidc_code_challenge'); + $code_challenge_method = $code_challenge_method ?? $this->session->get('oidc_code_challenge_method'); } // Set default scope if scope is not set at all From ca24f055b19ef05ea91dc26c92da871e6d265d8d Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 18 Mar 2026 16:36:17 +0100 Subject: [PATCH 2/2] fix: pass all OAuth params in consent redirect and guard against null MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consent grant redirect only passed client_id and scope, relying on session fallback in the authorize endpoint. This caused intermittent 500 errors on NC 32 (PHP 8.4) when session values were lost between session->close() and the subsequent GET request — a race condition. Now ConsentController::grant() includes all OAuth params (state, response_type, redirect_uri, nonce, resource, code_challenge, code_challenge_method) in the redirect URL, eliminating the session dependency entirely. Also adds a guard in LoginRedirectorController::authorize() that returns a 400 error if critical params are still missing after session fallback, preventing unhandled 500s from trim(null) or matchRedirectUri(null). Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/Controller/ConsentController.php | 17 ++++++++++++++--- lib/Controller/LoginRedirectorController.php | 13 +++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/Controller/ConsentController.php b/lib/Controller/ConsentController.php index 8f79e168..67c2c98d 100644 --- a/lib/Controller/ConsentController.php +++ b/lib/Controller/ConsentController.php @@ -201,11 +201,22 @@ public function grant(): RedirectResponse { // Update the scope in session to use granted scopes instead of requested $this->session->set('oidc_scope', $grantedScopes); - // Build authorize URL BEFORE closing session (need to read oidc_client_id) - $authorizeUrl = $this->urlGenerator->linkToRoute('oidc.LoginRedirector.authorize', [ + // Build authorize URL with ALL OAuth params to avoid session dependency. + // Previously only client_id and scope were passed, relying on session + // fallback in LoginRedirectorController. This caused intermittent 500 + // errors when session values were lost between the redirect and the + // subsequent GET request (race condition with session->close()). + $authorizeUrl = $this->urlGenerator->linkToRoute('oidc.LoginRedirector.authorize', array_filter([ 'client_id' => $this->session->get('oidc_client_id'), 'scope' => $grantedScopes, - ]); + 'state' => $this->session->get('oidc_state'), + 'response_type' => $this->session->get('oidc_response_type'), + 'redirect_uri' => $this->session->get('oidc_redirect_uri'), + 'nonce' => $this->session->get('oidc_nonce'), + 'resource' => $this->session->get('oidc_resource'), + 'code_challenge' => $this->session->get('oidc_code_challenge'), + 'code_challenge_method' => $this->session->get('oidc_code_challenge_method'), + ])); // IMPORTANT: Close the session to commit changes before redirecting // Without this, the authorize endpoint won't see the updated session values diff --git a/lib/Controller/LoginRedirectorController.php b/lib/Controller/LoginRedirectorController.php index a29f0ca4..adf4a395 100644 --- a/lib/Controller/LoginRedirectorController.php +++ b/lib/Controller/LoginRedirectorController.php @@ -235,6 +235,19 @@ public function authorize( $code_challenge_method = $code_challenge_method ?? $this->session->get('oidc_code_challenge_method'); } + // Guard: if critical OAuth params are still missing after session fallback, + // return a meaningful error instead of letting downstream code crash with a 500 + // (e.g. matchRedirectUri(null, ...) or trim(null) in PHP 8.4). + if (empty($state) || empty($response_type) || empty($redirect_uri)) { + $this->logger->error('Missing critical OAuth params after session fallback: ' + . 'state=' . var_export($state, true) . ', ' + . 'response_type=' . var_export($response_type, true) . ', ' + . 'redirect_uri=' . var_export($redirect_uri, true)); + return new TemplateResponse('core', '400', [ + 'message' => $this->l->t('Authorization session expired. Please try again.'), + ], 'error'); + } + // Set default scope if scope is not set at all if (!isset($scope)) { $scope = Application::DEFAULT_SCOPE;