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

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

Closed

Conversation

hf
Copy link
Contributor

@hf hf commented Apr 1, 2024

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

@j4w8n
Copy link
Contributor

j4w8n commented Apr 5, 2024

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
Copy link
Contributor

j4w8n commented Apr 6, 2024

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.

@kangmingtay
Copy link
Member

closing since this is outdated

@kangmingtay kangmingtay closed this May 2, 2024
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