supabase / splinter

Supabase Postgres Linter: Performance and Security Advisors
https://supabase.github.io/splinter/
83 stars 7 forks source link

Database linter recommendation don't work after update and resfresh #47

Closed gianniginom closed 5 months ago

gianniginom commented 5 months ago

Hi,

I'm encountering an issue with a database linter alert related to a row-level security policy in my Supabase project. Here's the specific problem:

I have a row-level security policy (SELECT policy) on the table test_profil in the public schema. The policy originally used auth.uid() directly, but upon recommendation from the linter, I updated it to (SELECT auth.uid()) to optimize performance. However, the linter alert persists even after making this change.

create policy "SELECT policy" on "public"."test_profil" as permissive for select to public using ((( SELECT auth.uid() AS uid) IS NOT NULL));

I've followed the recommendations provided by the linter and ensured that the policy is correctly updated. Despite this, the alert continues to show up. Any insights or guidance on how to resolve this issue would be greatly appreciated.

Thanks in advance for your help!

olirice commented 5 months ago

I was not able to reproduce the issue with the SQL you provided

You can see the reproduction attempt here here

Or the relevant part is below:

    create table public.test_profil( id int primary key);
    create policy "SELECT policy" on "public"."test_profil" 
      as permissive
      for select
          to public
          using ((( SELECT auth.uid() AS uid) IS NOT NULL));
    select * from lint."0003_auth_rls_initplan";
 name | level | facing | description | detail | remediation | metadata | cache_key 
------+-------+--------+-------------+--------+-------------+----------+-----------
(0 rows)

Did you click the Rerun linter with the green button (screenshot below) after solving updating your policy?

Screenshot 2024-04-12 at 2 55 08 PM
gianniginom commented 5 months ago

Thanks for reply !

Yes i have click on green button but warning stay :

Capture d'écran 2024-04-12 à 21 57 22

gianniginom commented 5 months ago

I used your code :

create table public.test_profil2( id int primary key); create policy "SELECT policy" on "public"."test_profil2" as permissive for select to public using ((( SELECT auth.uid() AS uid) IS NOT NULL));

The error appears again in the interface:

Capture d'écran 2024-04-12 à 22 00 05

GaryAustin1 commented 5 months ago

I can confirm this seems to be the case on my instance that has been around since mid last year.

I had this warning: image Edited the code to: image And still get the message after refresh and green button.

Note also that is not the policy I entered. Supabase added the as uid and extra parenthesis that are not needed. Don't like that part.

olirice commented 5 months ago

Very strange, I just tried on a Supabase instance it works as expected.

Could you please provide a project ref and write "allow support access" in the message body so I can take a look directly?

gianniginom commented 5 months ago

pjfxdtmqrdooydkqtudn -> allow support access

GaryAustin1 commented 5 months ago

It is not really the change it is missing. It seems to just not work for the correct policy. I just went to a month old instance. Duplicated the above and then on a table with no policies wrote the correct policy and the linter does not like it with the same error. image

I'm upgrading the project from 15.1.1.37 to see if that matters.

olirice commented 5 months ago

pjfxdtmqrdooydkqtudn -> allow support access

thank you

the exact SQL to see what should show up for that lint is:

set local search_path='';

with policies as (
    select
        nsp.nspname as schema_,
        polrelid::regclass table_,
        pc.relrowsecurity is_rls_active,
        polname as policy_name,
        polpermissive as is_permissive, -- if not, then restrictive
        (select array_agg(r::regrole) from unnest(polroles) as x(r)) as roles,
        case polcmd
            when 'r' then 'SELECT'
            when 'a' then 'INSERT'
            when 'w' then 'UPDATE'
            when 'd' then 'DELETE'
            when '*' then 'ALL'
        end as command,
        qual,
        with_check
    from
        pg_catalog.pg_policy pa
        join pg_catalog.pg_class pc
            on pa.polrelid = pc.oid
        join pg_catalog.pg_namespace nsp
            on pc.relnamespace = nsp.oid
        join pg_catalog.pg_policies pb
            on pc.relname = pb.tablename
            and nsp.nspname = pb.schemaname
            and pa.polname = pb.policyname
)
select
    'auth_rls_initplan' as name,
    'WARN' as level,
    'EXTERNAL' as facing,
    'Detects if calls to \`auth.<function>()\` in RLS policies are being unnecessarily re-evaluated for each row' as description,
    format(
        'Table \`%s\` has a row level security policy \`%s\` that re-evaluates an auth.<function>() for each row. This produces suboptimal query performance at scale. Resolve the issue by replacing \`auth.<function>()\` with \`(select auth.<function>())\`. See https://supabase.com/docs/guides/database/postgres/row-level-security#call-functions-with-select for more.',
        table_,
        policy_name
    ) as detail,
    'https://supabase.com/docs/guides/database/database-linter?lint=0003_auth_rls_initplan' as remediation,
    jsonb_build_object(
        'schema', schema_,
        'name', table_,
        'type', 'table'
    ) as metadata,
    format('auth_rls_init_plan_%s_%s_%s', schema_, table_, policy_name) as cache_key,
    table_::text
from
    policies
where
    is_rls_active
    and schema_::text not in (
        '_timescaledb_internal', 'auth', 'cron', 'extensions', 'graphql', 'graphql_public', 'information_schema', 'net', 'pgroonga', 'pgsodium', 'pgsodium_masks', 'pgtle', 'pgbouncer', 'pg_catalog', 'pgtle', 'realtime', 'repack', 'storage', 'supabase_functions', 'supabase_migrations', 'tiger', 'topology', 'vault'
    )
    and (
        (
            -- Example: auth.uid()
            qual  ~ '(auth)\.(uid|jwt|role|email)\(\)'
            -- Example: select auth.uid()
            and lower(qual) !~ 'select\s+(auth)\.(uid|jwt|role|email)\(\)'
        )
        or
        (
            -- Example: auth.uid()
            with_check  ~ '(auth)\.(uid|jwt|role|email)\(\)'
            -- Example: select auth.uid()
            and lower(with_check) !~ 'select\s+(auth)\.(uid|jwt|role|email)\(\)'
        )
    );

I'n getting hits on your other tables but not on public.test_profil.

@saltcod might this be a frontend thing? or is there a local cache that needs to be cleared etc?

GaryAustin1 commented 5 months ago

Upgrading made no difference.

I had two tables, dropping one and the linter dropped that in the dashboard.

If I change the policy to true it drops the error. So not seeming like caching.

GaryAustin1 commented 5 months ago

Also have two policies generates 4 warnings? image

GaryAustin1 commented 5 months ago

@olirice So I ran your code:

    create table public.test_profil( id int primary key);
    create policy "SELECT policy" on "public"."test_profil" 
      as permissive
      for select
          to public
          using ((( SELECT auth.uid() AS uid) IS NOT NULL));
      And get this LINT error

image

Seems like the linter is broken except for you (LOL) and just does not understand a good format.

image

GaryAustin1 commented 5 months ago

One more observation. If I run your linter code example above it does get a row for a real auth.uid() = user_id and not for the other table.

But the dashboard shows an error for the other table with it correct AND it did so without it being wrong first, so don't think a caching thing.

It is like a different piece of linter code is being run than you snipped and provided...

olirice commented 5 months ago

Also have two policies generates 4 warnings?

That's expected behavior but I can see how it's confusing. Multiple permissive policies that apply to different roles are fine, but multiple permissive policies that apply to the same role are a problem. For that reason, we currently display a warning for each role/table combination that has multiple permissive policies.

If 2 policies apply to multiple roles, that can generate multiple warnings.

I'll have to think about how to collapse that down in a reasonable way

olirice commented 5 months ago

But the dashboard shows an error for the other table with it correct AND it did so without it being wrong first, so don't think a caching thing.

It is like a different piece of linter code is being run than you snipped and provided...

Thanks! that's very helpful context I suspect the linter code is out of date on Studio or something similar. I'll dig into it more on Monday

olirice commented 5 months ago

@gianniginom created a support issue for this. We're working through it and I'll report the solution back here once its resolved

gianniginom commented 5 months ago

Hello, I just tested with the SQL command, and after updating all the RLS rules, I no longer have any errors (and the response times for my two most complex functions have been divided by 10!).

However, at the graphical interface level, some errors still persist. Maybe there's a bug in this part?

Thanks.

GaryAustin1 commented 5 months ago

@gianniginom Make sure you check out https://supabase.com/docs/guides/database/postgres/row-level-security#rls-performance-recommendations

The linter is only catching low hanging fruit on somewhat clear and simple cases. That same syntax works for many other functions you might use, and if you do joins with other tables there are even larger improvements possible.

gianniginom commented 5 months ago

Thanks, so I wrapped all my functions with selects. I'm learning to use Supabase, optimizing as I learn ;)

olirice commented 5 months ago

response times for my two most complex functions have been divided by 10

nice!

However, at the graphical interface level, some errors still persist. Maybe there's a bug in this part?

could you please screenshot what you're seeing for me?

muraisheeding commented 5 months ago

hello,

I'm having the same issue where I'm getting an alert from a RLS policy in my public.ingredients table, and i already solve it (In the second image is my actual RLS policy), but when i rerun the linter, the same alert is still there.

image image

olirice commented 5 months ago

please see #63

closing this in favor of that issue which is more narrowly scoped & active