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: suppress getSession warning whenever _saveSession is called #895

Merged
merged 1 commit into from
May 1, 2024

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented May 1, 2024

What kind of change does this PR introduce?

@kangmingtay kangmingtay merged commit 59ec9af into master May 1, 2024
4 checks passed
@kangmingtay kangmingtay deleted the km/suppress-get-session-warning branch May 1, 2024 12:04
@dagassa
Copy link

dagassa commented May 2, 2024

🙏

kangmingtay pushed a commit that referenced this pull request May 3, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.64.2](v2.64.1...v2.64.2)
(2024-05-03)


### Bug Fixes

* signOut should ignore 403s
([#894](#894))
([eeb77ce](eeb77ce))
* suppress getSession warning whenever _saveSession is called
([#895](#895))
([59ec9af](59ec9af))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@larbish
Copy link

larbish commented May 13, 2024

Hey @kangmingtay, maintainer of the nuxt/supabase module here.

We have a PR to migrate on the @supabase/ssr package and we're still experiencing this issue with the latest released version.

I've removed all occurrences of getSession() in the module and I still have the warning.

Any help on this would be appreciate 🙏 I can't merge and release this PR until I get rid of this warning.

@ekendra-nz
Copy link

Still get :

Using the user object as returned from supabase.auth.getSession() or from some supabase.auth.onAuthStateChange() events could be insecure! This value comes directly from the storage medium (usually cookies on the server) and many not be authentic. Use supabase.auth.getUser() instead which authenticates the data by contacting the Supabase Auth server.

with

		"@supabase/supabase-js": "^2.44.4",
		"@supabase/ssr": "^0.4.1"
		

@AdrienLF
Copy link

AdrienLF commented Aug 4, 2024

Same here with

		"@supabase/ssr": "^0.4.0",
		"@supabase/supabase-js": "^2.45.0"

@marcusklausen
Copy link

marcusklausen commented Sep 6, 2024

Any reason there's not just a prop we can pass to getSession() to suppress the warning manually? This safeguarding is very overboard I think. It is already perfectly clear that getSession is not safe for authorization on it's own.

I think it's a very common thing to use getSession in nextjs middleware. Having to do a roundtrip to supabase servers using getUser on each request on an edge network is just bonkers, so getSession makes sense for the routing. And using getSession in middleware causes an unbelievable amount of warnings in the log, to the point where debugging other stuff becomes really cumbersome.

@greendra8
Copy link

I've gone banner blind to this log I've seen it so much. Please fix!

@outranker
Copy link

annoying af

@jeromevvb
Copy link

Can you please explain in what sense this warming makes sense?
I am using Next JS, and on the middleware, I have a getUser (await supabase.auth.getUser()) check for my protected route (which is slow, by the way)

Since I have the user checked on my middleware, why would I want to get the user again? i would likely use getSession

@marcusklausen
Copy link

marcusklausen commented Oct 10, 2024

@jeromevvb you would likely do it the other way around, route access based on getSession but verify the user against supabase before fetching any data or running server actions etc. The latter would not be secure if you use getSession in those and calling getUser in the middleware causes excessive requests to third party making every request matched by the middleware super slow for no reason at all in most cases.

Nevertheless I've tried to make a PR for suppressing the warning but I'm not sure how I can get someone to look at it.
#953

@jeromevvb
Copy link

jeromevvb commented Oct 10, 2024

@marcusklausen Thank you for your answer!
However, I thought the reason Supabase is calling getUser in the middleware is to refresh the token if needed. How do you handle this?

@marcusklausen
Copy link

@jeromevvb well any time data is fetched in a server component or the user triggers a server action, if you use getUser in those cases, you're achieving the same. But having it in the middleware means it'll trigger on loads of requests, even images etc, if the matcher is not setup correctly. It makes 0 sense to me.

@jeromevvb
Copy link

Hey @marcusklausen
Thank you so much for your insights.
My application is mainly on the client side; therefore, if you use getUser in the client component, would this also refresh the token? Do I need to add "cookies" in the create client for the client-side?
Thank you!

@marcusklausen
Copy link

marcusklausen commented Oct 19, 2024

@jeromevvb if you want to authorise the user client side you have to use getUser as you have no other way for validating their credentials than to make the roundtrip to sb servers and verify. You dont need cookies for that.

The token is not refreshed though

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.

10 participants