Stopped outdated error from being returned

[fednelpat]

Fixes #249

Summary of Changes

1. Internal change to make it so that if there is an error while creating a session the session won't be saved to the registry.
2. Fixed an issue which would cause a nil pointer dereference
3. Note: This does not change anything for the users utilizing the library apart from the fact that certain kinds of errors won't happen anymore

This didn't need any test changes.

[fednelpat]

@elithrar There we go, I committed the pr. Sadly I had to make a last commit in which I gofmt'ed everything (including the files I didn't need to modify) since since the latest commit a new go version was released which asked for changes to the //go:build and // +build lines.

[fednelpat]

@aeneasr The changes I made don't prevent new sessions from being able to be saved. They prevent sessions which returned errors during creation from being saved in the session cache. Errors returned should be handled by the developer when the function returns a value. Furthermore the issue with Save() is that it doesn't check for session errors anywhere which means that it will save invalid sessions, but once the session is retrieved using Store.Get it will be kept as valid because the error won't have been saved in the store. Also since the error is kept in the registry this means that when Registry.Get is called it will keep returning the same invalid session and the same error. After looking at the code more, it seems to me that there is no way to exit out of that loop (it will never stop sending the invalid session + error), but I may have missed something.

[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).

[fednelpat]

I tested my code and couldn't find any issues in case we only saved one session. You are right though, if multiple sessions are being saved using Registry.Save the session won't be saved. What do you think about, instead of returning the error without saving the session, saving the session even though there was an error? 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. Furthermore if we did instead save the error the issue would be that here it would still return an error even though the session was valid, which means that if someone were to actually handle the Registry.Get error (instead of ignoring it like it's done in most cases) they might get an outdated error which would no longer be valid.

[fednelpat]

The updated Registry.Get code looks like this. Now as to the nil pointer dereference issue that is an issue with the storage implementation and is out of the scope of this project, so I am going to rename this pr

[aeneasr]

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.

[fednelpat]

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 https://github.com/gorilla/sessions/pull/251#issuecomment-1007985624, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKDQO4TYKZMAQZACMEGUUILUVAYN3ANCNFSM5KEDTVZQ . You are receiving this because you authored the thread.Message ID: @.***>

[stale[bot]]

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.