auth-js icon indicating copy to clipboard operation
auth-js copied to clipboard

fix: check for no error, instead of data, on insecure ssr getSession() warning

Open hf opened this issue 1 year ago • 2 comments

Checks for no error instead of the presence of data when showing the warning about potential insecure use of getSession().

hf avatar Apr 01 '24 19:04 hf

I'd like to point out that even though this change is how it should work, it's actually going to cause more console warnings when there's no logged-in user. Because result.error is going to be true in that case - returning an invalid claim: missing sub claim error and will not trigger a suppression.

Even worse, people will call getUser() before getSession(), as they've been told to do, but there will be this edge case where the warning will still log. This is going to cause more frustration.

UPDATE: I'm using SvelteKit and just realized that the new SSR docs code in hooks.server.ts might resolve the edge case, as long as you're not calling getSession() explicitly anywhere else.

j4w8n avatar Apr 05 '24 21:04 j4w8n

After doing a lot of testing, I have two suggestions and one observation. Hopefully they're not ridiculous and a waste of your time.

  1. Also change the corresponding check in getSession() here, because result.data will always be true, even if there is no user logged-in; as it will return { session: null }. This change will prevent the warning in situations where there's no logged-in user. But perhaps that's your intention - to run the resulting code no matter what, but if that were the case, you could just do the server storage check.
- if (result.data && this.storage.isServer) {
+ if (result.data.session && this.storage.isServer) {
  1. Don't suppress the warning after calling getSession() here - unless we're just trying to avoid being a logging nuisance. I'm unclear why we're doing this if getSession() can't be trusted on the server-side anyway. Doesn't it allow someone to bypass calling getUser() first to avoid further warnings?
- this.insecureGetSessionWarningShown = true;

Aside from that, I don't understand the code and comment here. It's causing the warning to log three times, even after calling getUser() with a logged-in user - at least in my base SvelteKit + SSR implementation, but maybe I'm doing something wrong.

j4w8n avatar Apr 06 '24 00:04 j4w8n

closing since this is outdated

kangmingtay avatar May 02 '24 06:05 kangmingtay