Example with cookie auth #5821
Example with cookie auth #5821
Conversation
|
This PR looks pretty good @j0lv3r4! I'm going to invite you to the example repo I made but didn't finish so that you can compare notes. |
|
Could you also send me a DM on Spectrum: https://spectrum.chat/users/timneutkens |
|
thanks, @timneutkens! DM sent. |
|
|
||
| async handleSubmit (event) { | ||
| event.preventDefault() | ||
| const username = this.state.username |
There was a problem hiding this comment.
Not sure what do you mean with inline, could you show me an example or elaborate what are you suggesting?
There was a problem hiding this comment.
I mean... I usually bring out variables either for better readability or reusability. In the current implementation, it doesn't fulfill any of these traits - this.state.username already says clearly, that this is a username, so you could just do login({ username: this.state.username }).
But this is just nitpicking and maybe just my personal taste so you can just ignore it 😄
| }) | ||
| if (response.ok) { | ||
| const { token } = await response.json() | ||
| cookie.set('token', token, { expires: 1 }) |
There was a problem hiding this comment.
Is this a good example? Storing access tokens without the secure and httpOnly attributes is vulnerable to XSS attacks. If we want to omit httpOnly, I'd suggest at least adding some info to the readme. What do you think?
There was a problem hiding this comment.
You're right, but I tried to do the example as simple as possible. The only thing I'm trying to demo here is the minimal implementation you can do for cookie auth, the security and other concerns are out of the scope just for this demo.
I totally have to state this in the readme.md.
There was a problem hiding this comment.
Yeah, it would be awesome to state that in there. People often look at the examples as "best practices" - this is my only concern here 😄
There was a problem hiding this comment.
httpOnly doesn't work with SSR because the server and the client both need to be able to use the token
There was a problem hiding this comment.
@dav-is Of course it works, you just have to do it differently. As simple as an endpoint which returns user data if you have a valid token (cookie) does the job here. But of course we don't want to do anything like this in this example - that's why I suggested adding some info in the README.
There was a problem hiding this comment.
@sarneeh User logs in from client device, API adds httpOnly cookie to it's response. Fetch results from API, the browser automatically keeps the cookie in the request. The user does a hard refresh and SSR kicks in. The SSR fetches data from the API (except now it doesn't have a cookie because the other is stored on the browser of the other domain).
I guess what you're saying could be possible if the API was in a subdirectory, but I like separating SSR and the API by a subdomain. I personally don't like using cookies for auth in an API. If you're using an Auth header with a token appended on each request makes CSRF nearly impossible. Also something about using a cookie seems stateful and doesn't fit with a stateless API.
There was a problem hiding this comment.
@dav-is I don't understand. Why do you assume that the cookie is for a different domain? This doesn't make sense to me. When you have an API endpoint for logging in, I think it's logical that it sets a cookie for the domain where the frontend is on. You can have your Next.js server on domain.com and the auth API on api.domain.com and it just works.
According to CSRF - yes, authenticating requests with an Auth header protects you from CSRF, but in this case you probably store tokens in a non-httpOnly cookie or localStorage which exposes you for XSS.
There was a problem hiding this comment.
Would www.domain.tld to api.domain.tld work? When I store cookies I also store them with the current domain written out so a vulnerability on another subdomain couldn't leak a users cookie.
And the difference it makes in XSS is minimal. If you had an XSS vulnerability and you were using httpOnly the malicious script might not be able to steal your token, but it could just make requests on your behalf. An XSS vulnerability could just get your username and password plaintext when a user logs in—at that point who cares about the token...
Being able to just not worry about CSRF is great and it instantly makes your API more easily made public. If you had a client that allowed you to log into multiple accounts at once and switch accounts and just provide a different Auth header with each request. This way you could maintain each session just by storing the token. Cookies are weird and stateful I kind of hate the idea of them with an API lol
There was a problem hiding this comment.
@dav-is Huh, you're right! You kinda persuaded me against cookies now 😄 Even using httpOnly, when I'm vulnerable to XSS then I'm... busted. I don't know why I didn't come into this realization earlier, thanks! 😄
There was a problem hiding this comment.
Theoretically yes, if you are vulnerable to XSS then anything can happen. But the difficulty is different. I would prefer if the attacker (for example an npm library going rogue) had to do something a bit harder than just harvesting document.cookie. So I like having auth tokens in httpOnly cookies. CSRF token (not doing auth by itself) can be non-httpOnly, that’s ok.
| export const logout = () => { | ||
| cookie.remove('token') | ||
| // to support logging out from all windows | ||
| window.localStorage.setItem('logout', Date.now()) |
There was a problem hiding this comment.
That's an interesting trick! I need to remember it 😄
There was a problem hiding this comment.
When a user logs out, they assume all of their data has been removed. Evidence that they have visited the site before isn't removing all data. Feel like this could have GDPR/Privacy Policy implications.
There was a problem hiding this comment.
Yeah, the use of Date.now keeps the date they logged out. Probably an empty object could do the trick, I need to put something there so it can trigger the event.
Other solution could be a global event emitter, but I don't think is a good idea.
I'll try with an empty object or a "random" string.
There was a problem hiding this comment.
Might want to check this out: https://scotthelme.co.uk/a-new-security-header-clear-site-data/
There was a problem hiding this comment.
I totally forgot about this. When withAuthSync umounts, it deletes the localStorage item.
There was a problem hiding this comment.
Oh ok then why not call it "isLoggingOut" and set it to true or 1.
Seems borderline hacky but I like the innovation
- Migrate to Micro - Add backend and make everything a monorepo - Separate each endpoint into its own file - Edit `now.json` to deploy as monorepo
- Remove Microroutes from dependencies - Remove unneeded comments - Edit README.md file according to new changes on the backend server
- Add `run` from Micro to make it work with Now - Fix bugs in the backend not returning the right data - Fix bug in `profile.js` to allow redirection in client and server - Pass API URL in headers on server and window.location.host in client
|
I think see some of my stuff, glad this made it in!! 😁 |
|
🙌 |
|
There is missing index.js file in api folder. |
Fixes #153
This is my attempt at #153
Following @rauchg instructions:
I deployed a passwordless backend onThe backend is included in the repository and you can deploy everything together by runningnow.sh(https://with-cookie-api.now.sh, src)nowAlso, from reviewing other PRs, I made sure to:
Here's a little demo: