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: regenerate csrf if flow has expired #2455

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

splaunov
Copy link
Contributor

When user clicks the verification link sent over email and the link has expired, then Kratos creates new verification flow and browser is redirected to verification ui with the new flow id set as a URL parameter.
The problem is that the verification UI cannot fetch the new flow as it has no csrf token set. So, browser gets 403 "Forbidden" error.

This PR eliminates csrf checking if verification flow has no csrf token.

Related issue(s)

Checklist

  • [ x] I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • [ x] I am following the
    contributing code guidelines.
  • [ x] I have read the security policy.
  • [ x] 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 green light (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

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2455 (4b2e69a) into master (1ed6839) will increase coverage by 0.67%.
The diff coverage is 100.00%.

❗ Current head 4b2e69a differs from pull request most recent head 3aae25a. Consider uploading reports for the commit 3aae25a to get more accurate results

@@            Coverage Diff             @@
##           master    #2455      +/-   ##
==========================================
+ Coverage   75.89%   76.57%   +0.67%     
==========================================
  Files         311      317       +6     
  Lines       19185    17636    -1549     
==========================================
- Hits        14560    13504    -1056     
+ Misses       3486     3193     -293     
+ Partials     1139      939     -200     
Impacted Files Coverage Δ
selfservice/flow/verification/handler.go 65.09% <100.00%> (+2.06%) ⬆️
selfservice/strategy/oidc/provider_facebook.go 0.00% <0.00%> (-83.10%) ⬇️
x/provider.go 0.00% <0.00%> (-75.00%) ⬇️
cmd/identities/definitions.go 35.93% <0.00%> (-35.50%) ⬇️
selfservice/flow/verification/strategy.go 40.00% <0.00%> (-35.00%) ⬇️
x/nocache.go 40.00% <0.00%> (-33.34%) ⬇️
identity/credentials.go 69.23% <0.00%> (-10.77%) ⬇️
cmd/identities/list.go 71.42% <0.00%> (-10.39%) ⬇️
identity/identity_recovery.go 50.00% <0.00%> (-6.25%) ⬇️
session/session.go 71.11% <0.00%> (-6.24%) ⬇️
... and 332 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

selfservice/flow/verification/handler.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented May 27, 2022

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

@aeneasr aeneasr marked this pull request as draft May 27, 2022 19:10
@splaunov splaunov changed the title fix: do not check csrf if it's not set in verification flow fix: regenerate csrf if flow has expired Sep 2, 2022
@splaunov splaunov marked this pull request as ready for review September 2, 2022 14:59
@splaunov splaunov requested review from aeneasr and removed request for zepatrik September 2, 2022 14:59
aeneasr
aeneasr previously approved these changes Nov 1, 2022
@aeneasr
Copy link
Member

aeneasr commented Nov 1, 2022

@jonas-jonas do we need to do this in the code strategy too?

@aeneasr
Copy link
Member

aeneasr commented Nov 1, 2022

@splaunov sorry for the late review, I am just catching ip on PRs! If the merge conflicts are addressed I will merge this immediately!

@jonas-jonas
Copy link
Member

Yes, recovery should need that too: https://github.com/ory/kratos/blob/master/selfservice/flow/recovery/error.go#L82

I can take a look, or if you want to do it, you can do that in here. @splaunov :)

@splaunov
Copy link
Contributor Author

The fix has been merged form some other branch.
Only the test change has been left not merged.

@aeneasr aeneasr merged commit 7025081 into ory:master Dec 29, 2022
@splaunov splaunov deleted the fix-verification-csrf branch December 29, 2022 08:38
CNLHC pushed a commit to seekthought/kratos that referenced this pull request May 16, 2023
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
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