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

Cookie session store fails silently, if the cookie data gets too large #226

Open
hsalokor opened this issue Sep 10, 2015 · 8 comments
Open

Comments

@hsalokor
Copy link

When I tried to take cookie-store into use, I noticed that it silently failed, not creating any cookie and without warning (either on the ring side, or on the Chrome side).

After some debugging and adding traces to ring code I found out that the cookie data size grew quite large, and that most likely caused the silent failure. In our case the root cause was debug data that got leaked into session, and issue got fixed by removing it.

It would be probably a good idea to log a warning or fail if the size of the encrypted cookie exceeds the maximum size, which is around 4kB I believe?

@weavejester
Copy link
Member

The maximum size depends on the browser, and it also sometimes depends on what other cookies are set on the same domain. Around 4KB tends to be a reasonable limit, but I don't know whether it's Ring's responsibility to warn about arbitrary browser limitations.

Perhaps some middleware might be a good idea that adds a warning if the cookie is too long?

@GetContented
Copy link

@weavejester that sounds sensible. Most web developers know not to put anything over a few KB in the cookie.

@hsalokor
Copy link
Author

Adding the warning to cookie middleware is probably a good idea. I reported the issue mainly because I was briefly confused with the issue, and a warning or error would have helped a lot.

Another aspect is the production use. We use mailed errors for errors in production, and being able to log error on exceeding a cookie size limit would be definitely helpful. Real-world data being a bit hairy and all.

@GetContented
Copy link

@hsalokor not to be rude, but what are you storing in your cookie? At most, it should be a single session identifier, shouldn't it? The rest should be loaded from a separate session storage.

@hsalokor
Copy link
Author

No offense taken :) We do actually store only minimal set of data to session (token and username). It's the human error aspect and the ease of detecting it that I'm referring to. Session is a common place to store things either by intent or by ignorance to and someone is prone to slip up.

Therefore the lack of warning is my major concern. The 4k limits seems quite established. Obviously we could plug our own middleware there, but that would only help us.

@timorantalaiho
Copy link

Most web developers know not to put anything over a few KB in the cookie.

However, in the JVM world it's sometimes too easy to put an entire serialized representation of an object to where you were just going to put a single attribute of it. Or in Clojure, plucking an entire branch of a tree shaped data structure instead of a leaf. When such errors happen, detecting what went wrong would be easier with a warning or failure because of an exceptionally large cookie size.

I think it would be good to have a configurable max size for the cookie with a default of 4 kilobytes (following the current de facto standard in browsers) and that the cookie store would fail if that gets exceeded.

Sure it would be great if browsers would make more noise about that too, but that's not so easy to change.

@GetContented
Copy link

What shape does failing take? this is perhaps why middleware is the right spot for this.

Personally I've always been incredibly careful about only storing either an integer or a small string in my cookies, and I've never had any issues with it.

@hsalokor
Copy link
Author

What shape does failing take? this is perhaps why middleware is the right spot for this.

The failure is silent. Cookie does not get created or updated anymore on the browser side.

I think this issue is more likely in teams. Consider following scenario:

Development:

  • Developer A introduces cookie-storage session, adds SQL query for it and is careful about size
  • Developer B comes along, and needs to do access control. He doesn't know about this restriction (yet) and modifies the query to return LDAP groups. His test account has only 1 LDAP group, so size limit is not exceeded and everything works

Issue:

  • Update gets deployed. User C tries to log in, but login fails. User D notes that his service works fine (having logged in before the update). Developer B starts to investigate.

Resolution:

  • After debugging the issue, the cookie size issue is eventually figured out by developer A, and the root cause of the issue is tracked to real users having over 40 different LDAP groups.

Proposal:

  • If cookie encryption would have written warning to log when 4k size is exceeded, the issue would most likely have been found out by developer B or maybe even never put into production

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants