madmurphy / cookies.js

Simple cookie framework with full Unicode support
GNU General Public License v3.0
265 stars 54 forks source link

Attempting to save large cookies fails silently #26

Closed chaorace closed 4 years ago

chaorace commented 4 years ago

There's a 4 kilobyte size limit on any given cookie, anything larger will not be saved by Chomium (and probably most other standards compliant browsers).

Attempting to set a cookie larger than the 4kB limit via docCookies.setItem method will fail, despite the method returning true. I believe false should be returned in this scenario.

I believe docCookies.setItem should be modified to actually validate that the new value exists in the cookiejar before returning true. Hopefully that should also help cover any other odd edge cases where the browser silently rejects the cookie assignment.

madmurphy commented 4 years ago

@chaorace

There's a 4 kilobyte size limit on any given cookie, anything larger will not be saved by Chomium (and probably most other standards compliant browsers).

The size limit depends on both the browser and the server and can have many forms. The non-compliant browser in this case is Chromium. Quoting from MDN:

RFC 2965 (Section 5.3, "Implementation Limits") specifies that there should be no maximum length of a cookie's key or value size, and encourages implementations to support arbitrarily large cookies. Each browser's implementation maximum will necessarily be different, so consult individual browser documentation.

Going back to your point…

I believe docCookies.setItem should be modified to actually validate that the new value exists in the cookiejar before returning true. Hopefully that should also help cover any other odd edge cases where the browser silently rejects the cookie assignment.

The biggest problem is that the size limit in most cases is server-side – Chromium is more the exception than the rule. And when the limit is server-side there is nothing to do until it's too late.

I believe that this is one of these cases where programmers should just pay attention to their code.

chaorace commented 4 years ago

Thanks for the enlightening response. I might argue that a specification that stipulates no maximum means there is no such thing as a compliant browser. I did some quick skimming to try and see if there was such a browser, but it seems that all major implementations limit cookies to ~5 KB or less.

Still, it's your work and I respect your decision. I suppose I shouldn't have assumed the return value of setItem would be some kind of endorsement of success. Now that I double-check, it's not documented in the repo readme and the Web Storage API this mirrors doesn't return anything for setItem. Go figure!

madmurphy commented 4 years ago

There is a return value for setItem(), which is always true unless you use invalid cookie names, in which case it is false.

I would implement a further check in setItem(), but there are two problems against that:

  1. If the browser fixes the limit to 5 KB but the server fixes it to 4 KB, my function will still return true, but the navigation will be broken until you clean the cache (try yourself to go beyond the server's limit and see what happens). There are literally no checks possible to prevent that, just your knowledge of the server you are dealing with.
  2. Last time I checked a cookie could be set to a different path inaccessible from the current location (for example /foo while you are in /bar), and in that case you would not have means to check if the cookie has been stored or not, since you cannot access what you have sent – actually it would not be a bad idea to check again if this policy has changed, it was long time ago last time I tried it.