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

[REQUEST] - Consider if it is truly necessary to put user_id in the session - it would be more secure to only put the token #69

Open
peaceful-james opened this issue Jun 27, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request triage Needs to be prioritized

Comments

@peaceful-james
Copy link
Contributor

The session can be modified client-side and what is generated here makes it too easy to trust that the user_id in the session is trustworthy.

@peaceful-james peaceful-james added enhancement New feature or request triage Needs to be prioritized labels Jun 27, 2024
@peaceful-james
Copy link
Contributor Author

I am starting to think that :fetch_current_user should be removed from the browser pipeline.
Firstly, all it does is assign the user_id to the session, which is probably not ideal.
Secondly, it clears the session when there is no token. This removes anything in the session from other plugs, e.g. locale, user_return_to, etc.

I think the best thing would be to modify RequireUser functions to use the token in the session.

Feel free to ignore this issue and my musings on it. I will open a PR that encapsulates.

@peaceful-james
Copy link
Contributor Author

Note to self: don't forget to test with Strict cookies. These would be broken for common flows, e.g. "confirm email".

Simple solution - like the remember_me token in phx.gen.auth, have a 2nd cookie, Lax, with just the token as value. Encrypt it.

@type1fool
Copy link
Collaborator

This week, I learned that Safari misbehaves when accessing localhost with secure: true.

https://codedamn.com/news/web-development/safari-cookie-is-not-being-set

The workaround I found was to set secure: Application.fetch_env(:my_app, :env), where :env is set to the Mix compile environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage Needs to be prioritized
Projects
None yet
Development

No branches or pull requests

2 participants