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

Fixes nuxt-community/auth-module#133 #160

Merged
merged 1 commit into from
Apr 29, 2018
Merged

Fixes nuxt-community/auth-module#133 #160

merged 1 commit into from
Apr 29, 2018

Conversation

nysos3
Copy link
Contributor

@nysos3 nysos3 commented Apr 28, 2018

No description provided.

@pi0
Copy link
Member

pi0 commented Apr 29, 2018

Thanks, @nysos3 for this PR.

For making SSR logout working we may need to clear token cookie using setCookie which is currently implemented for client-side only.

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

@pi0, This actually fixes SSR logout without needing to implement setCookie on the server, as nuxt transfers the store to the client, and since the logout method now sets the user/tokens to false, they don't register as null or undefined, and are then properly propagated to the cookies/localStorage by the client, since the store is the first point of truth for getUniversal and setUniversal.

@pi0
Copy link
Member

pi0 commented Apr 29, 2018

But token is not being stored in the Vuex (https://github.com/nysos3/auth-module/blob/ccc0ea7257880174c5118f0cb198e4a11fd9cd17/lib/core/storage.js#L99) how can it be propagated? :D

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

Ah, I'm using token. as the prefix instead of _token.. What's the point of keeping it out of the store? Immutability outside of the auth module? Or simply to keep it 'hidden'?

@pi0
Copy link
Member

pi0 commented Apr 29, 2018

@nysos3 Keeping secure things as hidden as possible indeed. If it is being stored in vuex it will be in every SSR response and also window.$nuxt.$store. Cookies are a secure mechanism for transferring such things.

I totally agree with your current changes in this PR but it would be much much better if we also use set-cookie header for set/unset tokens from the server side.

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

@pi0, I still don't understand what good keeping it out of vuex does.

So you're protecting from javascript that has access to the window object? In all modern browsers, cross domain iframes are isolated and don't have access to the same window object, and therefore none of this auth data.

I can still hit:
window.$nuxt.$options.$auth.$storage._state['_token.local']
or:
window.$nuxt.$options.$axios.defaults.headers.common.Authorization

or simply:

oldHeader = XMLHttpRequest.prototype.setRequestHeader;
XMLHttpRequest.prototype.setRequestHeader = function(header,value){
   if(header === 'Authorization')
       alert(value);
   oldHeader.apply(this, arguments);
}

or even more simply: window.document.cookie

Cookies are just as insecure.

Edit: also, window.localStorage

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

@pi0 Shall I make another PR to set the default token prefix to just token.? 😃

@pi0
Copy link
Member

pi0 commented Apr 29, 2018

I'm not sure and still thinking using cookies as the only source of trust for token would be a better option.

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

Then why implement the store or localStorage at all?

Also, what about those that have disabled cookies, and rely on vuex-persisted-state instead, since that's a package built for multi-storage, where as this is an Auth module, allowing for the separation of responsibilities?

@nysos3
Copy link
Contributor Author

nysos3 commented Apr 29, 2018

For the record, I agree that we need to be able to modify cookies server side as well, but we also need support for false user/token. This pull request achieves that much. Maybe I can work out another for handling cookies in SSR after this one.

@pi0 pi0 merged commit 0450b57 into nuxt-community:dev Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants