midday-ai / midday

Run your business smarter 🪄
https://midday.ai
GNU Affero General Public License v3.0
5.94k stars 549 forks source link

[security concerns] getUser() from cached-queries with getSession #284

Closed lumpinif closed 3 weeks ago

lumpinif commented 1 month ago

This is what I have discovered how getUser() works in the cached-quesries from packages/supabase:

export const getUser = async () => {
  const {
    data: { session },
  } = await getSession();
  const userId = session?.user?.id;

  if (!userId) {
    return null;
  }

  const supabase = createClient();

  return unstable_cache(
    async () => {
      return getUserQuery(supabase, userId);
    },
    ["user", userId],
    {
      tags: [`user_${userId}`],
      revalidate: 180,
    },
  )(userId);
};

It is not wrong, however, it concerns me a little bit that it relies on getting userId from the getSession which is returning unencoded session data from the local which is not secured and can be tempered according to the docs can be found from Supabase here.

As far as I know, it should always use getUser() instead of getSession() for actions that require absolutely verified and secured user identities. For example, deleteUserAction() from actions in the app/dashboard is one of those using it right now.

BTW, Midday is amazing!

pontusab commented 4 weeks ago

Thank you for this, I know supabase is working on an update regarding this too, I will investigate a bit on this tomorrow and have a fix!

pontusab commented 3 weeks ago

Thanks again for this, I have made a PR and will test a bit before I merge, Im using getSession where it's not critical but I just need to check if your authorized or not (middelware etc)

https://github.com/midday-ai/midday/pull/287

lumpinif commented 3 weeks ago

You are very welcome! I have learned a lot and respect Midday from every perspective you value. Please keep doing the amazing work.