-
Notifications
You must be signed in to change notification settings - Fork 13
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
Moved auth token from localStorage to cookie #103
Conversation
Since localStorage is vulnerable to XXS attacks, auth token should be stored in a cookie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. I left a few comments.
src/api/index.ts
Outdated
@@ -39,6 +39,23 @@ import * as converter from './converter'; | |||
|
|||
export * from './types'; | |||
|
|||
// setCookie sets new cookie key-value pair. | |||
export function setCookie(key: string, value: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The api
folder contains functions related to the Yorkie API.
(https://github.com/yorkie-team/dashboard/blob/main/design/file-structure.md)
How about moving the 'setCookie' and 'getCookie' functions to the utils
folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to set the domain
and path
for the cookie? We also use the cookie on the homepage.
https://github.com/yorkie-team/yorkie-team.github.io/blob/25af19213edecc9a23cb9d83a1dc0aa1842398c1/components/Layout/Header.tsx#L15
src/features/users/usersSlice.ts
Outdated
@@ -74,7 +74,7 @@ type JWTPayload = { | |||
}; | |||
|
|||
const initialState: UsersState = { | |||
token: localStorage.getItem('token') || '', | |||
token: api.getCookie('token'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be good that the getCookie
function returns undefined
or null
to distinguish between empty string values and unset values?
src/features/users/usersSlice.ts
Outdated
@@ -134,7 +134,7 @@ export const usersSlice = createSlice({ | |||
initialState, | |||
reducers: { | |||
logoutUser: (state) => { | |||
localStorage.removeItem('token'); | |||
api.setCookie('token', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be clearer to delete the cookie, instead of setting an empty string.
Thanks for your contribution. It looks like we need to create a cookie as a response header. I'll close this PR for now. |
What this PR does / why we need it?
Any background context you want to provide?
What are the relevant tickets?
Fixes #42
Checklist