Fix bug making anyone claims admin

[aslakhellesoy]

First off - thanks for this library!

What kind of change does this PR introduce?

Security fix.

What is the current behavior?

The is_claims_admin function always returns true. This allows any authenticated user to set claims on their own account. This completely defeats the purpose of this library.

What is the new behavior?

The is_claims_admin function returns false when it should.

Additional context

The session_user is always "postgres" (the user that was used to establish the connection). I assume current_user is what was intended here.

Furthermore, the current_user will never have the value "authenticator". However, for authenticated users it will have the value "authenticated".

Since this library is about security, I suggest it comes with a disclaimer saying that this library does not have any automated tests, and therefore might not work as intended.

FWIW, I discovered the bug through automated tests in my own application using this library.

[burggraf]

This is incorrect. session_user should return authenticator if the call is being made from the javascript client (where the role can be either anon or authenticated. As it says in the source code, if the session_user is not authenticator then we return true because we're assuming this function is being called from a trigger or some other process.

You are free to change this function if you want this security check to go beyond the scope of calls made from the javascript client. Otherwise, I think we should keep it like this.

[burggraf]

However, this block seems backward (and is indeed a bug!) Can you verify this and make sure I'm correct here and that this is indeed wrong?

      IF coalesce((current_setting('request.jwt.claims', true)::jsonb)->'app_metadata'->'claims_admin', 'false')::bool THEN
        return true; -- user has claims_admin set to true
      ELSE
        return false; -- user does NOT have claims_admin set to true
      END IF;

[GaryAustin1]

I think it comes from this "bug" in the cli....
The CLI is/was incorrectly using postgres instead of authenticator for the PostgREST role.
supabase/supabase#10470

[burggraf]

@GaryAustin1 Ah, so what does session_user return now?
Maybe we should only return true in this case if session_user is a specific value? That would be more secure, no?

[GaryAustin1]

I don't use the CLI, was just helping someone debug your function and found root cause looking at the CLI config.

[aslakhellesoy]

When I run a local supabase instance, it seems the PostgREST server connects to Postgres with the postgres user. I'll have to double check though...

[GaryAustin1]

It was/is definitely a bug in the local version setup (Steve confirmed using postgres user was incorrect) but to me looked like they had fixed it. But the issue is still marked open.

[burggraf]

Here's a proposal. Instead of checking session_user what if we instead check to see if current_setting('request.jwt', true)::jsonb has a value? In other words, "if there is a jwt, we know this is a PostgREST user, if not, we assume it's a logged-in user or a trigger or something"? Do you see any problems with that?

[burggraf]

I'm going to test this:

CREATE OR REPLACE FUNCTION has_jwt() RETURNS "bool"
  LANGUAGE "plpgsql" 
  AS $$
  BEGIN
    return (select current_setting('request.jwt', true)::jsonb <> null);
  END;
  $$;

[burggraf]

That's not working for me. Back to the original code, can you create this function and call it from your app via .rpc() to see what it returns (in self-hosted mode?)

CREATE OR REPLACE FUNCTION get_session_user() RETURNS text
  LANGUAGE plpgsql 
  AS $$
  BEGIN
    return session_user;
  END;
  $$;

In my Supabase-hosted apps, it always returns authenticator if I'm using the javascript client (whether authenticated or anonymous).

[burggraf]

In looking at this again, it does look to be correct (the false part is just a default value if it's not set, so this look correct now):

      IF coalesce((current_setting('request.jwt.claims', true)::jsonb)->'app_metadata'->'claims_admin', 'false')::bool THEN
        return true; -- user has claims_admin set to true
      ELSE
        return false; -- user does NOT have claims_admin set to true
      END IF;

[GaryAustin1]

@burggraf The hosted app has always been correct as far as I know. Just the docker/cli image used in local has been incorrect for a long time. I traced back the change history quite a ways back when the issue came up and could never find a point it was correct.