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

feat: add isServer property to server-side storage adapters #722

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

hf
Copy link
Contributor

@hf hf commented Jan 20, 2024

This property will be picked up by auth-js (formerly known as gotrue-js) to warn or adjust behavior given that the storage medium (in this case cookies) cannot be trusted.

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not too clear as to why we can trust the cookies if isServer=false - can you elaborate more on this?

@hf
Copy link
Contributor Author

hf commented Jan 22, 2024

i'm not too clear as to why we can trust the cookies if isServer=false - can you elaborate more on this?

We can't trust the cookies when isServer=true (sorry for the reverse logic but it's simpler) because, suppose you're an attacker:

  1. You find a vulnerable website that uses SSR, and uses getSession().user to render the user's bank account info.
  2. You craft a cookie header that includes the user you want, with an expires_at and expires_in that are in the future, so the client library doesn't try to refresh the session.
  3. Boom. Next.js SSR will render any user's banking information because getSession().user ultimately does not verify the authenticity of the data inside the cookie. Only the JWT can be authenticated, but that is only done by getUser().

It's very easy to miss; it's also easy to miss by just thinking that getSession() acts as an authentication barrier.

In the reverse case where isServer=false the cookies can be trusted, in the sense that they're somehow authenticated. Let's say that the developer builds a custom storage adapter that authenticates the data inside the cookie -- like a JWT containing the JWT.

@hf
Copy link
Contributor Author

hf commented Jan 22, 2024

isServer is just a hint to the gotrue-js/auth-js library to know what sort of storage is being used so it knows to warn / adjust behavior based on prioritizing security over performance.

@kangmingtay
Copy link
Member

@hf but the same thing holds true if getSession is used on the client right? Or is it much harder because the attacker will need access to the user's browser?

@hf
Copy link
Contributor Author

hf commented Jan 30, 2024

because the attacker will need access to the user's browser?

Yes, or to the underlying server -- which are both scenarios out of any security model on the web.

@hf hf merged commit 8a9de46 into main Jan 30, 2024
3 checks passed
@hf hf deleted the hf/add-is-server-storage-property branch January 30, 2024 13:34
kangmingtay added a commit that referenced this pull request Jan 31, 2024
kangmingtay added a commit that referenced this pull request Jan 31, 2024
* chore: add missing changeset for #726

* chore: add missing changeset for #722
hf added a commit to supabase/auth-js that referenced this pull request Feb 4, 2024
Warn about using `getSession()` when the storage has `isSever` to true
without previously having called `getUser()`.

Warn each time the `user` is accessed as returned by `getSession()`.

Relates to:
- supabase/auth-helpers#722
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.

2 participants