supabase / splinter

Supabase Postgres Linter
https://supabase.github.io/splinter/
68 stars 6 forks source link

Third option to address 0002_auth_users_exposed #80

Closed tvogel closed 1 month ago

tvogel commented 1 month ago

https://github.com/supabase/splinter/blob/b8850303e7e92893023256cf0a925f5aed41d143/docs/0002_auth_users_exposed.md?plain=1#L18

I think, the linter should check and advise for a third option to prevent unintentionally exposing auth.users data: It is easily possible to include the intended RLS rules in the WHERE clause of the view definition which has the advantage that you can define the RLS rules independently of the previously defined rules.

In my example, I want to expose specific profile information such as contact information of admins of an organization to all members of the organization, even though those members of course shall not have full access to the admin's auth.users data.

I defined the view as:

CREATE VIEW "public"."admins" AS
 SELECT "person"."id",
    "organization"."id" AS "organization",
    "users"."email",
    "person"."card"
   FROM (("public"."organization"
     JOIN "auth"."users" ON ("public"."can"("users"."id", 'write'::"text", "organization"."id")))
     LEFT JOIN "public"."person" ON (("person"."id" = "users"."id")))
  WHERE ((CURRENT_ROLE = ANY (ARRAY['postgres'::"name", 'service_role'::"name"])) OR ("public"."can"('read'::"text") ? ("organization"."id")::"text"));

Here can() is a function that checks whether the authenticated user has write or read permissions to a specific resource (given by id). So, the first call to can() selects all admins of the given organization (modeled by write permission on the organization) and the second call to can() makes only those organizations visible to the user that they are a member of (modeled by having read permission on the organization).

I do not see a security flaw in this way of defining the view and duplicating the data into another table using the trigger approach seems like a workaround to me. It would also create complications because that shadow table would not only need to be updated when the auth.users data changes but also when the admin status of a user changes.

It would be cool if the linter could agree!

olirice commented 1 month ago

It is easily possible to include the intended RLS rules in the WHERE clause of the view definition which has the advantage that you can define the RLS rules independently of the previously defined rules.

I do not see a security flaw in this way of defining the view

supabase/splinter is a static analysis tool so introspecting function definitions to understand their security intent is outside what we can accomplish here. The current two methods we describe for resolving in the documentation have the benefit of being easily detectable through analysis of pg_catalog but they certainly aren't the only two solutions

What you're doing sounds perfectly valid if you're confident your where statement is covering the security case you care about. For our recommendations though, we'd like to stick with solutions that can be statically measured as correct/safe without understanding business logic as that will be the least error prone