Skip to content

fix: pass all OAuth params in consent redirect to prevent session race #630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cbcoutinho wants to merge 2 commits into from
Closed

fix: pass all OAuth params in consent redirect to prevent session race #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc0100c
fix: restore session fallback for all critical OAuth params after con…
cbcoutinho Mar 17, 2026
ca24f05
fix: pass all OAuth params in consent redirect and guard against null
cbcoutinho Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions lib/Controller/ConsentController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
43 changes: 32 additions & 11 deletions lib/Controller/LoginRedirectorController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,18 +213,39 @@ 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)) {
Copy link
Copy Markdown
Owner

@H2CK H2CK Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This again allows a partial fallback (per missing attribute) to the session which leads to mixed oidc parameters and can cause problems as described in #621. Session fallback should only happen if client id is missing and then all parameters should be read from session. Otherwise parameters will be mixed if multiple client perform authentication.

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');
}

// 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)) {
Copy link
Copy Markdown
Owner

@H2CK H2CK Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break implicit flow implementation of guacamole. See #626.
The state might be missing without any impact.

$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
Expand Down
Loading