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
Stopped outdated error from being returned #251
Conversation
@elithrar There we go, I committed the pr. |
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.
I suggest not to merge this. It prevents new sessions (which have been created because e.g. keys had been rotated) from being able to be saved using .Save()
because they'll never make it to the session store, thus never will be called in for name, info := range s.sessions {
.
IMO a test should be added to ensure this type regression can't happen:
- Client sends cookie with incorrect signature (e.g. wrong secret, malformed string)
- Server wants to save cookie
- Client receives updated cookie, which now has the correct value
@aeneasr |
In the context of this library, an error will be returned when the signature of an existing cookie is invalid. In that case, the error should be ignored, and the cookie should still be set, and then saved, with a signature that is valid. This means that we gracefully handle non-existent cookies, or cookies which have an incorrect signature. Removing this code would mean that those cookies would NEVER be saved because they can NEVER be retrieved in the first place, as they will always return an error. The underlying problem this PR attempts to resolve is #249 . However, #249 is an issue because the storage implementation of the mongo adapter does not respect the interface (i.e. it returns a nil session when an error is given which clearly violates the contract witch expects a session always). Therefore, this is a pretty serious breaking change that would cause cookies with invalid signatures to no longer be saved, unless the developers make some big hoops to catch this particular case (in most cases they will not). |
Nice, thank you for reproducing the issue on your end!
There's a particular use case where the error is needed. Currently
return the same result. This is useful if you want to figure out whether the cookie was invalid and recreated, or whether it existed in the first place. You can do this with:
with your suggested change, this would not work any more once you call |
That makes sense, thanks for pointing it out!
…On Sat, 8 Jan 2022 at 14:00, hackerman ***@***.***> wrote:
Nice, thank you for reproducing the issue on your end!
Not saving the error is a good change because first of all the error being
saved is only used in Registry.Get and nowhere else so it doesn't have any
impact if it's saved or not.
There's a particular use case where the error is needed. Currently
cookie, err := session.Get(...)
cookie, err = session.Get(...)
return the same result. This is useful if you want to figure out whether
the cookie was invalid and recreated, or whether it existed in the first
place. You can do this with:
cookie, err := session.Get(...)
if err == nil && cookie.IsNew {
/ It's a new cookie, not a cookie recovered from an error
}
with your suggested change, this would not work any more once you call
session.Get twice. However, this change is important because we might
want to delete a cookie if it is invalid, but we do not want to delete a
cookie if no cookie existed in the first place.
—
Reply to this email directly, view it on GitHub
<#251 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKDQO4TYKZMAQZACMEGUUILUVAYN3ANCNFSM5KEDTVZQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days. |
Fixes #249
Summary of Changes
This didn't need any test changes.