Add methods to choose the correct session cookie

[aeneasr]

Sometimes it can happen that a browser has two cookies for with the same name. This might be the case where developers forget to enforce a path variable or the domain attribute or accidentally call Set-Cookie twice. In those cases, this library would previously return whatever cookie r.Cookie(name) would return.

This patch introduces a new function called GetExact which allows one to pass a filter function which receives the Session attribute. That way, there is a way to search for a specific session if multiple session have been found.

This functional change is fully backwards compatible by introducing a new interface StoreExact which defines what a store needs to implement to conform to this new logic. Stores which do not have these new methods will fallback to the previous behavior, were the first cookie found will be used.

All default stores in this library already implement the StoreExact method with this patch.

In particular, this issue has affected our open source projects at Ory quite heavily. The changes in this patch will contribute towards making these type of errors easier to deal with!

[aeneasr]

Thank you for the fast review!

This is an interesting approach. Forgive me if this is dense, but I'm having a hard time imagining a situation where someone who goes about implementing Matcher cannot also resolve bugs in their codebase (or otherwise) causing two cookies to share a name. Maybe a bit more context could help me understand. Can you elaborate on how the reliance on *http.Request.Cookie returning the first cookie affected your projects at Ory? That is, do you have an implementation of Matcher besides the default that would be used here?

That is a good question! There are multiple stages where you might end up with several cookies and you want to select one based on whatever criteria you match. I think the opening use case does not explain this well. The problem is, that everytime a mistake happens, an upgrade to the session mechanism is rolled out, or another change affecting session cookies is used, it can happen that you end up with more than one session cookie.

While fixing the problem in the codebase is one of the mitigation steps developers can do, asking millions of users to clear their cookies, is almost never possible (as most won't follow your advice anyways). Therfore it is important to have a methodology in place which allows us to choose which value we want to use and for what reason. Basically we need something to handle this situation gracefully and without disruption.

Another option I considered (but did not implement) was to look up a cookie without passing the request, but by passing a *Cookie parameter. That way the filtering part could be move outside of gorilla/sessions and into the application code. So something like this:

for _, c := range r.Cookies() {
  sess, err := sessions.Parse(ctx, c)
  if !sess.Something {
    // Delete cookie, try next one
  }
}

In retrospect I think that would have been the smarter choice from an API design perspective.

[DavidLarsKetch]

@aeneasr thanks for clarifying the use case. Indeed, it is incumbent on the server to handle potentially invalid cookies rather than a user and sessions ought to enable developers where possible.

Curious if you have more thoughts on this now that it's sat a moment. One thing I appreciate about the current way a cookie and subsequent session is retrieved is how it mirrors the *http.Request.Cookie method insofar as the first cookie that matches the name is returned, nothing fussy. Your most recent code snippet (in contrast to the patches) follows the same strategy as the std lib. That is, if Cookie doesn't do it for you, Cookies is available for looping through all the cookies and to which you can apply your own logic to select the one needed.

As well, returning all sessions that match name gives application code an opportunity to make decisions about the sessions and cookies not needed. Parse or GetAll, a change in such a direction would still expand the API surface as your GetExact does, but my impression is the library code would be simpler and potentially clearer to the user.

[aeneasr]

Thank you for the constructive feedback :)

Yes, I think that proposal makes more sense! I have to revisit why I did not go that path right away. I believe one issue was that the cache is pretty difficult to expand to multiple cookies as it keeps reference of the values set (so when setting a value and we have multiple sessions which one is it applied to?

Do you have an idea how the cache issue should be addressed?

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