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: Backchannel Logout: Fix error log when RP responds with status code 204 #3731

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

SiebelsTim
Copy link
Contributor

@SiebelsTim SiebelsTim commented Mar 6, 2024

According to the specification at https://openid.net/specs/openid-connect-backchannel-1_0.html#BCResponse the Relying Party must respond with a status code 200. However, it also notes that the OpenID Provider should be prepared to handle status code 204 (No Content) as a successful response as well.

Related issue(s)

This is a previously unknown bug.

Reproduction:

  1. Configure backchannel logout
  2. The relying party responds with a 204 status code on the backchannel logout request
  3. Hydra logs an error

Note that this does not change any behaviour. The logged error does not have an influence whether the user is logged out or not.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works. -- Since this is only related to logging. Adding a test is difficult for me.
  • I have added or changed the documentation. -- I don't think this is necessary

Further Comments

@SiebelsTim SiebelsTim requested review from aeneasr and hperl as code owners March 6, 2024 10:59
@SiebelsTim SiebelsTim force-pushed the fix-backchannel-status-204 branch from 7eb5f01 to cb95f3e Compare March 6, 2024 11:00
@SiebelsTim SiebelsTim changed the title Backchannel Logout: Fix error log when RP responds with status code 204 fix: Backchannel Logout: Fix error log when RP responds with status code 204 Mar 6, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 76.29%. Comparing base (67a85cc) to head (69cd817).
Report is 2 commits behind head on master.

Files Patch % Lines
consent/strategy_default.go 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3731   +/-   ##
=======================================
  Coverage   76.29%   76.29%           
=======================================
  Files         134      134           
  Lines       10214    10214           
=======================================
  Hits         7793     7793           
  Misses       1904     1904           
  Partials      517      517           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ode 204

According to the specification at https://openid.net/specs/openid-connect-backchannel-1_0.html#BCResponse
the Relying Party must respond with a status code 200.
However, it also notes that the OpenID Provider should be prepared to
handle status code 204 (No Content) as a successful response as well.
@SiebelsTim SiebelsTim force-pushed the fix-backchannel-status-204 branch from cb95f3e to 69cd817 Compare March 6, 2024 11:58
@SiebelsTim
Copy link
Contributor Author

I am happy that the changes were approved so quickly. Do I need to do anything else?

@aeneasr
Copy link
Member

aeneasr commented Apr 4, 2024

Me clicking merge, sorry :)

@aeneasr aeneasr merged commit 153e4b5 into ory:master Apr 4, 2024
29 of 30 checks passed
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.

3 participants