Align with backchannel logout OIDC spec by Spitfireap · Pull Request #1432 · nextcloud/user_oidc

Spitfireap · 2026-04-28T18:28:30Z

Current behaviour

The response of BackChannel logout seems to be missing the Cache-Control header (spec).

The BC logout validation is also missing exp token validation (not expired) and iss validation (equals to issuer in discovery endpoint) per Backchannel logout token validation spec and ID Token validation spec

Changes

Removed an unused variable from getBackchannelLogoutErrorResponse ($throttleMetadata)
Increased the severity of the logging when a Backchannel logout fails
Added the Cache-Control header in the backchannel logout response
Added a verification for the exp and iss claim
Check that all required tokens by the spec are there

Spitfireap · 2026-04-28T18:43:51Z

@julien-nc not entirely sure about the change for the error logging.
I think the severity should be increased since it might be a security issue. The thing is : could these be triggered by a random idp and in that case we should perhaps be cautious about using error or it requires the idp to be verified so error would be well suited ?

julien-nc

Nice, thanks. Let's get #1431 in first, then you can rebase and adjust this one.

Spitfireap · 2026-04-29T15:42:04Z

Added some more alignment with the spec (exp + 1 iss validation step).

Was wondering if we should verify that jti and iat claims are there as well (since they are required) or am I overdoing it ?

julien-nc

Hey. #1431 is merged. Sorry there seems to be a small conflict with your branch. You can rebase on main or just merge main in your branch to resolve the conflict.

Was wondering if we should verify that jti and iat claims are there as well...

Yeah sure, let's keep it simple: It's enough to check if those token attributes are defined.

julien-nc · 2026-05-01T07:39:04Z

-				['iss_should_be_set' => true]
+				true
+			);
+		} elseif ($iss !== $discovery['issuer']) {


$iss is not defined at this point.

julien-nc · 2026-05-01T07:46:16Z

+			$this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description .
+			'. This is likely an IdP issue.');


Suggested change

$this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description .

'. This is likely an IdP issue.');

$this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description .

'. This is likely an IdP issue.', ['metadata' => $metadata]);

You could keep the metadata param (which can be renamed) and include it in the logs so the information is more precise than the $isLikelyIdpSide boolean. Wdyt?

julien-nc · 2026-05-01T07:46:49Z

-					['multiple_sessions_found' => $sid]
-				);
+				$this->logger->warning('[BackchannelLogout] Multiple OIDC sessions retrieved (sid+sub+iss). ' .
+				'This should not happen. Please check that you have created your DB indexes')


Missing ;

Spitfireap · 2026-05-01T09:38:31Z

Hey. #1431 is merged. Sorry there seems to be a small conflict with your branch. You can rebase on main or just merge main in your branch to resolve the conflict.

Was wondering if we should verify that jti and iat claims are there as well...

Yeah sure, let's keep it simple: It's enough to check if those token attributes are defined.

Working on May the 1st are you :p ?
I'll push the rebase and some rework, then test it a bit on my instance. I'll ping you when it's ready.

julien-nc · 2026-05-01T09:43:51Z

Working on May the 1st are you :p ?

Checking your github notifications on May 1st? 😁

Yeah for a weird reason I'm working today. I can tell you on a private channel if you're curious.

I'll push the rebase and some rework, then test it a bit on my instance. I'll ping you when it's ready.

Nice! Thank you for the enthusiasm. No rush though, feel free to take the public holiday 😉

Spitfireap · 2026-05-03T19:18:20Z

@julien-nc should be ready.
Funny thing : LemonLDAP implementation of OP is actually not aligned with the spec (exp token missing).
So although my initial issue is fixed, I still get the error message after this PR. I don't think we should let it pass anyway though. Wdyt ?

Spitfireap · 2026-05-03T19:34:28Z

Note : this means that LemonLDAP's users won't be logged out anymore in case of OP initiated logout...
This might be a security issue so idk if we should add a way of bypassing it or add a warning of some sort in the README for instance

julien-nc · 2026-05-04T10:01:36Z

 		string $description,
-		array $throttleMetadata = [],
+		array $metadata = [],
+		string $severity = 'warning',


Suggested change

string $severity = 'warning',

string $severity = \Psr\Log\LogLevel::WARNING,

julien-nc · 2026-05-04T10:02:51Z

+			return $this->getBackchannelLogoutErrorResponse(
+				'could not reach provider endpoint',
+				'URL: ' . $provider->getDiscoveryEndpoint() . 'was not reachable',
+				severity: 'error',


Suggested change

severity: 'error',

severity: \Psr\Log\LogLevel::ERROR,

julien-nc · 2026-05-04T10:07:25Z

 				'invalid audience',
 				'The audience of the logout token does not match the provider',
-				['invalid_audience' => $logoutTokenPayload->aud]
+				['extra_context' => 'Probably is an IdP side issue', 'aud' => $aud]


Suggested change

['extra_context' => 'Probably is an IdP side issue', 'aud' => $aud]

['extra_context' => 'Probably is an IdP side issue', 'aud' => $aud, 'client_id' => $clientId]

julien-nc · 2026-05-04T10:09:12Z

+		$iss = $logoutTokenPayload->iss;
+		if ($iss !== $discovery['issuer']) {


Suggested change

$iss = $logoutTokenPayload->iss;

if ($iss !== $discovery['issuer']) {

$iss = $logoutTokenPayload->iss;

$discoveryIssuer = $discovery['issuer'] ?? '';

if ($iss !== $discoveryIssuer) {

julien-nc · 2026-05-04T10:09:32Z

+				[
+					'extra_context' => 'Probably is an IdP side issue',
+					'iss' => $iss,
+					'issuer' => $discovery['issuer']


Suggested change

'issuer' => $discovery['issuer']

'issuer' => $discoveryIssuer,

julien-nc · 2026-05-04T10:09:59Z

+				[
+					'extra_context' => 'Probably is an IdP side issue',
+					'exp' => $logoutTokenPayload->exp,
+					'current_time' => $this->timeFactory->getTime()


Suggested change

'current_time' => $this->timeFactory->getTime()

'current_time' => $this->timeFactory->getTime(),

julien-nc · 2026-05-04T10:12:42Z

-					$sub === null ? 'Multiple sessions were found with this (sid,iss)' : 'Multiple sessions were found with this (sid,sub,iss)',
-					['multiple_sessions_found' => $sid]
+				$this->logger->warning('[BackchannelLogout] Multiple OIDC sessions retrieved (sid+sub+iss). '
+				. 'This should not happen. Please check that you have created your DB indexes'


Suggested change

. 'This should not happen. Please check that you have created your DB indexes'

. 'This should not happen.'

Missing indexes don't change anything to this. It cannot make the request return multiple results.

julien-nc · 2026-05-04T10:16:15Z

I don't think we should let it pass anyway though. Wdyt ?

I think it's fine to let this pass if the token expiration is not defined. So:

If the exp is defined, we validate it
If it's not defined, we just ignore

Sounds good to you?

refurbished $throttleMetadata not used since 9b5d6c6 Signed-off-by: Spitap <dev@asdrip.fr>

Signed-off-by: Spitap <dev@asdrip.fr>

Spitfireap · 2026-05-04T11:11:47Z

I don't think we should let it pass anyway though. Wdyt ?

I think it's fine to let this pass if the token expiration is not defined. So:
* If the exp is defined, we validate it

* If it's not defined, we just ignore
Sounds good to you?

If you say so. Although it is a clear deviation from the spec I think since it is REQUIRED, and we ought to verify it as we would do in the code function...

For instance, had LemonLDAP not forget to make the exp claim required in its RP implementation, the OP would get the 400 error pretty much as current fix.

So do I make it optional ?

deliberately removed. See nextcloud#1432 Signed-off-by: Spitap <dev@asdrip.fr>

julien-nc

Let's get this in while it implements the RFC. We can discuss the missing exp later.

julien-nc · 2026-05-04T12:59:18Z

Thanks a lot for the adjustments.

Spitfireap force-pushed the fix/1430-backchannel-oidc-spec branch from da506df to 149ded7 Compare April 28, 2026 18:33

julien-nc requested changes Apr 29, 2026

View reviewed changes

Comment thread lib/Controller/LoginController.php Outdated

Comment thread lib/Controller/LoginController.php

Spitfireap force-pushed the fix/1430-backchannel-oidc-spec branch 2 times, most recently from 0478e3e to 05b982d Compare April 29, 2026 12:07

julien-nc requested changes May 1, 2026

View reviewed changes

Spitfireap force-pushed the fix/1430-backchannel-oidc-spec branch from f0865f4 to f209fb6 Compare May 1, 2026 22:54

Spitfireap marked this pull request as draft May 1, 2026 23:24

Spitfireap force-pushed the fix/1430-backchannel-oidc-spec branch from f209fb6 to d9bf10b Compare May 3, 2026 19:13

Spitfireap marked this pull request as ready for review May 3, 2026 19:18

julien-nc requested changes May 4, 2026

View reviewed changes

Spitfireap added 5 commits May 4, 2026 12:59

Improved logging for failed BC logout

5c64d29

refurbished $throttleMetadata not used since 9b5d6c6 Signed-off-by: Spitap <dev@asdrip.fr>

Fixed DB exception

50936c4

Signed-off-by: Spitap <dev@asdrip.fr>

tell idp not to cache BC logout response

08efbae

Signed-off-by: Spitap <dev@asdrip.fr>

Check for the required claims

919950f

Signed-off-by: Spitap <dev@asdrip.fr>

added verification for exp and iss in BC-LO

5576b98

Signed-off-by: Spitap <dev@asdrip.fr>

Spitfireap force-pushed the fix/1430-backchannel-oidc-spec branch from d9bf10b to 5576b98 Compare May 4, 2026 11:11

Removed exp claim from required BC-LO

7fcb03d

deliberately removed. See nextcloud#1432 Signed-off-by: Spitap <dev@asdrip.fr>

julien-nc approved these changes May 4, 2026

View reviewed changes

julien-nc merged commit c9e1f86 into nextcloud:main May 4, 2026
45 checks passed

Spitfireap mentioned this pull request May 4, 2026

Backchannel logout missing exp claim check #1436

Open

		$this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description .
		'. This is likely an IdP issue.');

	string $severity = 'warning',
	string $severity = \Psr\Log\LogLevel::WARNING,

	['extra_context' => 'Probably is an IdP side issue', 'aud' => $aud]
	['extra_context' => 'Probably is an IdP side issue', 'aud' => $aud, 'client_id' => $clientId]

		$iss = $logoutTokenPayload->iss;
		if ($iss !== $discovery['issuer']) {

	'issuer' => $discovery['issuer']
	'issuer' => $discoveryIssuer,

	'current_time' => $this->timeFactory->getTime()
	'current_time' => $this->timeFactory->getTime(),

	. 'This should not happen. Please check that you have created your DB indexes'
	. 'This should not happen.'

Conversation

Spitfireap commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current behaviour

Changes

Uh oh!

Spitfireap commented Apr 28, 2026

Uh oh!

julien-nc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Spitfireap commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

julien-nc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spitfireap commented May 1, 2026

Uh oh!

julien-nc commented May 1, 2026

Uh oh!

Spitfireap commented May 3, 2026

Uh oh!

Spitfireap commented May 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julien-nc commented May 4, 2026

Uh oh!

Spitfireap commented May 4, 2026

Uh oh!

julien-nc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julien-nc commented May 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Spitfireap commented Apr 28, 2026 •

edited

Loading

Spitfireap commented Apr 29, 2026 •

edited

Loading