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

PR to move auth details from local storage to cookies #973

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

obafemitayor
Copy link

Related Issue

What Changed

  • Refactored the authentication logic here to store the auth details in the cookies instead of local storage

Testing Strategy

https://www.loom.com/share/f2d07c6c31b94c978900cb01ad8b1fdc

Copy link

vercel bot commented Dec 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
find-a-mentor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 15, 2024 2:57pm
find-a-mentor-jfb1 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 15, 2024 2:57pm

@togakangaroo
Copy link

togakangaroo commented Dec 15, 2024

I haven't been involved in the development (just saw a comment asking for a review on the Slack) but isn't the point of moving auth from localStorage to cookies that you want to move to http-only cookies? Otherwise it seems like just a bunch of overhead and no improvement to security?

Typically for a cookie you are going to be dealing with it only on the server since that's where you would set and read it.

@obafemitayor
Copy link
Author

I haven't been involved in the development (just saw a comment asking for a review on the Slack) but isn't the point of moving auth from localStorage to cookies that you want to move to http-only cookies? Otherwise it seems like just a bunch of overhead and no improvement to security?

Typically for a cookie you are going to be dealing with it only on the server since that's where you would set and read it.

@togakangaroo I just tried to follow the request of the issue while working on the PR. So if I understand you correctly, you mean the task is actually not to move to a normal cookie but an HTTP cookie?

@togakangaroo
Copy link

I haven't been involved in the development (just saw a comment asking for a review on the Slack) but isn't the point of moving auth from localStorage to cookies that you want to move to http-only cookies? Otherwise it seems like just a bunch of overhead and no improvement to security?
Typically for a cookie you are going to be dealing with it only on the server since that's where you would set and read it.

@togakangaroo I just tried to follow the request of the issue while working on the PR. So if I understand you correctly, you mean the task is actually not to move to a normal cookie but an HTTP cookie?

Yeah, I'm guessing that it's not just "put the token in a cookie instead of localStorage" because why would that be useful? The idea would be to actually use it as a cookie. That means it is both set and read only on the server, the client doesn't have to do anything special at all (except do fetch withCredentials).

The benefit to doing that is that when you set the cookie you can put the http-only attribute on it which makes it so that something like a cross-site-scripting attack cannot steal your credentials

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.

3 participants