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

[cbcoutinho]

Summary

• Pass all OAuth parameters in consent redirect URL instead of only client_id and scope, eliminating the session dependency that causes intermittent 500 errors
• Add null safety guard in LoginRedirectorController::authorize() to return a 400 error when critical OAuth params are missing after session fallback

Problem

After a user grants consent, ConsentController::grant() redirects back to the authorize endpoint with only client_id and scope in the URL. The authorize endpoint then relies on PHP session fallback to recover state, response_type, redirect_uri, and other critical OAuth parameters.

This works most of the time, but fails intermittently because:

1. $this->session->close() is called before the redirect (line 212)
2. The browser follows the 303 redirect immediately
3. The GET request to /authorize opens the session again
4. If session values are lost (timing-dependent), downstream code crashes:
   • matchRedirectUri(null, ...) → TypeError
   • trim(null) → E_DEPRECATED in PHP 8.4 (potentially promoted to error)

This is especially prevalent on Nextcloud 32 with PHP 8.4, where trim(null) deprecation handling is stricter.

History

The consent feature was introduced in v1.11.0 (1262cfe, PR #586). The bug has been present since then — ConsentController::grant() has always only passed client_id and scope in the redirect URL.

Two previous fixes attempted to address symptoms of this bug:

• v1.16.1 (49fd2f1) — Widened the session fallback condition in authorize() from if (empty($client_id)) to if (empty($client_id) || empty($state) || empty($response_type) || empty($redirect_uri)), so the fallback would trigger when consent only passed 2 params. However, this broke the non-consent flow where state/response_type/redirect_uri are legitimately present as URL params.
• v1.16.2 (5c5d1e1) — Reverted v1.16.1's condition change back to if (empty($client_id)), which restored the non-consent flow but left the consent race condition unfixed.

Neither fix addressed the root cause: ConsentController::grant() not passing the full set of OAuth parameters. This PR fixes that directly.

Affected versions

v1.11.0 through v1.16.2 (current).

Solution

ConsentController::grant() — pass all params in URL

$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'),
]));

array_filter() omits null values so optional params (resource, code_challenge) don't appear as empty strings in the URL.

Since all critical params are now in the URL, the session fallback in authorize() (guarded by if (empty($client_id))) is no longer needed for the consent flow — but remains functional for the initial login redirect flow where it's still required.

LoginRedirectorController::authorize() — null safety guard (defense in depth)

After the session fallback block, a guard checks if critical params are still missing and returns a meaningful 400 error instead of letting downstream code crash with a 500:

if (empty($state) || empty($response_type) || empty($redirect_uri)) {
    $this->logger->error('Missing critical OAuth params after session fallback: ...');
    return new TemplateResponse('core', '400', [
        'message' => $this->l->t('Authorization session expired. Please try again.'),
    ], 'error');
}

Test plan

• Verified consent flow works with PKCE (code_challenge params passed correctly)
• Verified consent flow works without PKCE (optional params omitted via array_filter)
• Verified session fallback still works for the initial login redirect flow
• Ran full OAuth test suite (68 tests pass)
• Tested on Nextcloud 31 and 32

---

This PR was generated with the help of AI, and reviewed by a Human

[H2CK]

Hi. Parts of the changes will open again the recently closed bugs. Please have a look at the comments in LoginRedirectController. The changes in ConsentController are in general fine.

[H2CK]

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

[H2CK]

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.

[H2CK]

The relevant changes of this PR have now been introduced with #631. Please have a look at the master branch. The described bug should be solved and the introduced guard was adapted to still allows the use of the Guacamole oidc client. If anything is missing please update the PR accordingly.

[H2CK]

Changes to fix the bug are already contained in > 1.16.3