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: ignore CSRF middleware on Apple OIDC callback #3643

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

sidartha
Copy link
Contributor

@sidartha sidartha commented Nov 26, 2023

This change ensures that CSRF middleware is ignored on /self-service/oidc/callback/apple (previously it was only exempted).

After a user authenticates on Apple, Apple redirects the user back to Kratos using a form POST. Since this is a POST, the CSRF cookie is not passed (since the cookie's SameSite attribute is set to Lax). This causes Kratos to add a different CSRF cookie.

While this doesn't break the registration flow itself, it causes a problem if we want to send the user back to the original login flow. In some cases, we may want to do this to cancel the registration and allow the user to log in with different credentials. It may be important to maintain the existing login flow in case the login originated from an OAuth2 client via Hydra instead of simply creating a new login flow.

Related issue(s)

Not reported separately. Steps to reproduce:

  1. Configure Kratos with Apple OIDC provider.
  2. Attempt to login with an Apple account that hasn't been registered in Kratos previously.
  3. After Apple redirects back to Kratos, you will reach the registration page.
  4. On the registration page, redirect the user back to original login flow.
  5. The login flow cannot be accessed since the CSRF cookie has been overridden.

This issue does not impact other OIDC providers such as Google or Facebook since they do not rely on form POST when redirecting to the Kratos callback endpoint.

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
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Copy link
Member

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

This LGTM. Thank you.

Test failures seem unrelated. I re-ran. Let's see.

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (180828e) 78.39% compared to head (7c0e02e) 78.40%.
Report is 1 commits behind head on master.

❗ Current head 7c0e02e differs from pull request most recent head 5d0a425. Consider uploading reports for the commit 5d0a425 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3643      +/-   ##
==========================================
+ Coverage   78.39%   78.40%   +0.01%     
==========================================
  Files         345      344       -1     
  Lines       23560    23496      -64     
==========================================
- Hits        18469    18422      -47     
+ Misses       3697     3690       -7     
+ Partials     1394     1384      -10     

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

Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@hperl hperl enabled auto-merge (rebase) December 5, 2023 06:37
@hperl hperl merged commit 309c506 into ory:master Dec 5, 2023
25 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