Skip to content
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.

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

Fix statuscode of backchannel logout response #1692

Closed

Conversation

fkammer
Copy link

@fkammer fkammer commented Mar 6, 2024

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

📋 Changes

The handler for backchannel logout requests previously responded with status code 204 although the specification requires status code 200.

The internal function res.send204() was used. Since the handler for backchannel logout requests was the only place in the code that used this function, it was removed and replaced by sendEmpty200.

📎 References

"If the logout succeeded, the RP MUST respond with HTTP 200 OK."

OpenID Connect Back-Channel Logout 1.0

🎯 Testing

Test library is changed accordingly.

@frederikprijck
Copy link
Member

frederikprijck commented Mar 6, 2024

Thank you for the PR and trying to improve the SDK.

I think this is a breaking change, and 204 is actually not entirely against the spec (even though 200 might be more correct).

From the spec:

If the logout succeeded, the RP MUST respond with HTTP 200 OK. However, note that some Web frameworks will substitute an HTTP 204 No Content response for an HTTP 200 OK when the HTTP body is empty. Therefore, OPs should be prepared to also process an HTTP 204 No Content response as a successful response.

Personally, I'd argue 204 makes more sense (as 204 exists exactly for the no content purpose), but 200 might be more in-line with the exact specification.

Based on that I would vote against merging this now, but consider if this is something we would like to change or not in a future major version.

@fkammer
Copy link
Author

fkammer commented Mar 6, 2024

I see your point. And yes, 204 would be the correct status code in terms of http.

The OIDC specification on the other hand uses the key word "MUST" and not "SHOULD". Therefore status code 200 should be the one we return.

To make the change non-breaking we could introduce BackchannelLogoutOptions to configure the used status code with 204 as a default for now. The next major update could change the default to 200.

Do you consider the removal of the send204 function a breaking change too or is this internal?

@frederikprijck
Copy link
Member

frederikprijck commented Mar 6, 2024

Given send204 is a publicly documented method, I consider it a breaking change as well.

The OIDC specification on the other hand uses the key word "MUST" and not "SHOULD". Therefore status code 200 should be the one we return.

I do not disagree, but this is an Auth0-specific SDK. Not an OIDC compliant SDK (but yes, we do try to follow the spec as much as possible).

To make the change non-breaking we could introduce BackchannelLogoutOptions to configure the used status code with 204 as a default for now. The next major update could change the default to 200.

In this case, I see no direct issue with using 204, as the spec clearly says that everyone should expect the status code to be 200 or 204. Therefore I think we should be fine to stick to 204 until we get to a new major and there is no need to add additional complexity to make the status code configurable.

Unless there is some actual problem with our current version that needs to be addresses, that would change the conversation. Is there?

@fkammer
Copy link
Author

fkammer commented Mar 6, 2024

I do not disagree, but this is an Auth0-specific SDK. Not an OIDC compliant SDK (but yes, we do try to follow the spec as much as possible).

We actually use the SDK with a different provider, which (contrary to the specification) expects the status code 200.
This is definitely an error with the provider and we have created an issue there. We just noticed the SDK isn't fully OIDC compliant on this and wanted to help.

By the way, we have already fixed the problem on our side with a workaround:

// src/app/api/auth/logout/backchannel/route.ts

import { AppRouteHandlerFnContext } from '@auth0/nextjs-auth0';
import { NextRequest } from 'next/server';

import { getAuth0Instance } from '../../../../utils/auth0/node';

export async function POST(
  request: NextRequest,
  context: AppRouteHandlerFnContext,
): Promise<Response> {
  const response = await getAuth0Instance().handleBackchannelLogout(request, context);
  // Workaround to return 200 as status code
  if (response.status === 204) {
    return new Response(response.body, {
      status: 200,
      statusText: response.statusText,
      headers: response.headers,
    });
  }
  return response;
}

@frederikprijck
Copy link
Member

frederikprijck commented Mar 6, 2024

We actually use the SDK with a different provider,

This is not a use-case we support. This SDK is specifically for Auth0 and is not intended to be used with other providers nor is it intended to be OIDC compliant to work with other IDPs. It may or may not work, there is no guarentee for that. But we do have internals specifically for Auth0.

which (contrary to the specification) expects the status code 200.

Yeah they should allow 204.

We just noticed the SDK isn't fully OIDC compliant on this and wanted to help.

That is appreciated a lot, thank you for that 🙏 . But as said, this SDK isn't intended to be used with other providers in its current state, and we do not want to add additional complexity to support another provider (who arguably is less in line with the spec than this SDK is, as it should allow 204).

Closing this, as I believe this isn't a change we can merge anyway due to the breaking change, nor do we think this is something we want to enable in an opt-in manner for the time being. But we may look into improving this in a next major version, as we do try and follow the specification where possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants