-
Notifications
You must be signed in to change notification settings - Fork 26
fix: decrypt session before setting cookies in auth flows #300
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
base: main
Are you sure you want to change the base?
Conversation
|
this PR still makes me nervous. anything auth related does. please run through it once again. |
|
Fix merge conflicts so I can try it out properly please :) I can also recommend splitting the Session problem and API key as they are separate. And yes, this HAS to work with old sessions AND hashed ones. Same for old apikeys AND hashed apikeys. Etc. |
|
@yashsinghcodes ping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we still handle this? In case somehow the Encryption fails we will have a log on it for debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't i see what line you have commented on. can you point out the code block for me?
a15e403 to
be2365f
Compare
Done @frikky |
@yashsinghcodes I want your go-ahead before I spend time on this again. Review both apikeys and sessions, for new users AND existing ones, as to see if absolutely anything breaks. |
this was stupid on my part. i never tested login itself, i was too concerned about existing users not getting locked out. testing properly this time (the attached session is an invalid one)
Checklist to test for myself: