Start using cookies instead of localStorage by default #817
Conversation
| } | ||
|
|
||
| SSODataStorage.prototype.set = function(connection, sub) { | ||
| var ssodata = { |
There was a problem hiding this comment.
Is let maybe better in this context? Also several below.
There was a problem hiding this comment.
auth0.js is not transpiling code yet, so no let or const are allowed.
src/helper/storage/handler.js
Outdated
| this.warn = new Warn({}); | ||
| this.storage = new CookieStorage(); | ||
| if (!options.__tryLocalStorageFirst) { |
There was a problem hiding this comment.
Do we want to be explicit about what options.__tryLocalStorageFirst can be? This is truth-y, should it be if (!options.__tryLocalStorageFirst === true) instead? Edge case and a new option so no existing behavior but "no" or "false" would trigger a positive case here. Just wondering how this check is done elsewhere.
test/helper/storage-handler.test.js
Outdated
| handler.removeItem(key); | ||
| expect(removeItemSpy.firstCall.args).to.be.eql([key]); | ||
| }); | ||
| }); | ||
| describe('should use cookie storage', function() { | ||
| it('when localstorage is not available', function() { | ||
| it('when __tryLocalStorageFirst is true but localSTorage is not available', function() { |
There was a problem hiding this comment.
CAN I USE ALL CAPS LIKE THIS LOCALSTORAGE WHAT DO YOU THINK
There was a problem hiding this comment.
Either all caps or camel, pick one! 😆
I have an issue with this. Before this change, auth details were stored in localstorage, and kept entirely on the client. After this change, the auth details are stored in cookies. If I understand correctly, all of the cookies on the client are sent to the server for each request. This bloats the request size, and could represent a credentials leak. Is my understanding above wrong? If not, this does not seem a change without behavioral consequences. I was extremely surprised to see it in a minor release of the library. |
|
There are no auth details stored in cookies. We only store: One cookie for (expires in 24 hours):
One cookie for (expires in 30 minutes):
One cookie for (expires in 15 minutes):
Yes, the cookies increase the overall request size and that's why we made the transactional cookies short lived. We considered that slightly increasing the payload size was a better trade off than possibly making our customers' apps crash due to localStorage being full. |
|
FYI, cookies are more prone to size and storage quantity limitations.
One notable difference is that if cookie storage is full, it won't cause any JavaScript errors (to my knowledge), whereas |
| this.warn = new Warn({}); | ||
| this.storage = new CookieStorage(); | ||
| if (options.__tryLocalStorageFirst !== true) { |
There was a problem hiding this comment.
Hello @luisrudge. How can I use LocalStorage instead of CookieStorage in a TypeScript application? It seems impossible to set __tryLocalStorageFirst to true because of AuthorizeOptions or AuthOptions type definition: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/auth0-js/index.d.ts#L512
There was a problem hiding this comment.
send a PR to the typing repo adding this flag?
There was a problem hiding this comment.
Also, why you want to use localStorage instead of cookies? that would be pretty helpful to understand, since we're considering removing localStorage in the future.
There was a problem hiding this comment.
Hello @luisrudge
"send a PR to the typing repo adding this flag?", Yes. PR done and merged by DefinitelyTyped team 👍
"Also, why you want to use localStorage instead of cookies?" Because I did not find how to make auth0 cookie work with Electron because of file:// protocol.
fix #402 #816
In some cases, there's no way to old local storage entries because we don't have enough information:
Because of that, cookies are a better fit for storage, since it has built-in expiration dates.
There's a new option
__tryLocalStorageFirstthat restores the previous behavior if you find any compatibility issues (please report them if you find it) or prefer to use localStorage for some reason.This is not considered a breaking change because there shouldn't be any behavioral changes between versions.