Supabase Linter reports warnings with default Ash functions

[alexslade]

Describe the bug The functions installed by Ash generate these warnings when used with a Supabase DB. (I naively assume these warnings have some merit if included with a large project like Supabase)

To Reproduce

• Install Ash + Ash Postgres on a new phoenix app
• Connect to a supabase database
• Run migrations
• Visit your supabase "Advisors" page

Expected behavior No errors or warnings are reported.

** Runtime

• Elixir version: I generated the migrations on Elixir 1.17.2-otp-27
• OS: generated on MacOS, I assume running on Linux in Supabase
• Ash version: 3.4.1, AshPostgres 2.2.0

Additional context These warnings are gone if you modify functions like so:

    execute("""
    CREATE OR REPLACE FUNCTION scope_test_1(left BOOLEAN, in right ANYCOMPATIBLE, out f1 ANYCOMPATIBLE)
    AS $$ SELECT COALESCE(NULLIF($1, FALSE), $2) $$
    LANGUAGE SQL
    IMMUTABLE;
    """)

    execute("""
    CREATE OR REPLACE FUNCTION scope_test_2(left BOOLEAN, in right ANYCOMPATIBLE, out f1 ANYCOMPATIBLE)
    AS $$ SELECT COALESCE(NULLIF($1, FALSE), $2) $$
    LANGUAGE SQL
    SET search_path = ''
    IMMUTABLE;
    """)

scope_test_1 gives a warning, _2 does not.

This is a very naive test though, I don't know enough yet about search paths to know how this might interfere with Ash functions, especially if schema multitenancy is used.

I'm reporting this now, and would be happy to pursue a PR later in the week if you think it's worth it.

[zachdaniel]

Hmmm...I'm not familiar with setting search paths for functions, but if we can confirm it has no problematic behavior I see no reason not to adjust our migration code to do so. Only for future users, though. So we'd modify the existing code, not add a new version that forces everyone to do another migration.

[alexslade]

Cool, I'll work on a PR when I can.

What would give you confidence that this works? All tests passing in this lib? (I'm not aware yet if there are any further tests to run that cover Ash + AshPostgres together)

[zachdaniel]

Honestly just a snippet explaining why the lint error exists and what modifying search path for the function does, and why the function triggered it would be sufficient 😆. I can do my own research on that front if necessary.

It seems like something to do with the fact that the function does a SELECT and something could then set the search path causing it to see different functions/relations than what it expects. But it also doesn't call any functions or use any relations where I imagine that could be a problem, so it's a bit surprising. Maybe an overzealous warning potentially even. But if it has no ill effects then I have no problem setting a search path for the functions we generate.

[alexslade]

It appears to be a very minor hardening step. I just found this rationale in the linter: https://supabase.github.io/splinter/0011_function_search_path_mutable/

When a function does not have its search_path explicitly set, it inherits the search_path of the current session when it is invoked. This behavior can lead to several problems:

Inconsistency: The function may behave differently depending on the user's search_path settings.

Security Risks: Malicious users could potentially exploit the search_path to direct the function to use unexpected objects, such as tables or other functions, that the malicious user controls.

And here's a related CVE from older versions of Postgres: https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path

I also can't see anything that Ash is calling that would be a problem: all the functions are pg_catalog functions which are usually selected first. (Barring some exotic situations).

I won't push this any further and happy for you to delete the issue if you think it's not worth adding the hardening for.

But I can also confirm that the modified functions (e.g. see below) work for me.

    CREATE OR REPLACE FUNCTION uuid_generate_v7()
    RETURNS UUID
    AS $$
    DECLARE
      timestamp    TIMESTAMPTZ;
      microseconds INT;
    BEGIN
      timestamp    = clock_timestamp();
      microseconds = (cast(extract(microseconds FROM timestamp)::INT - (floor(extract(milliseconds FROM timestamp))::INT * 1000) AS DOUBLE PRECISION) * 4.096)::INT;
      RETURN encode(
        set_byte(
          set_byte(
            overlay(uuid_send(gen_random_uuid()) placing substring(int8send(floor(extract(epoch FROM timestamp) * 1000)::BIGINT) FROM 3) FROM 1 FOR 6
          ),
          6, (b'0111' || (microseconds >> 8)::bit(4))::bit(8)::int
        ),
        7, microseconds::bit(8)::int
      ),
      'hex')::UUID;
    END
    $$
    LANGUAGE PLPGSQL
    SET search_path = ''
    VOLATILE;

[zachdaniel]

Based on all the information here, it makes sense that we should set a search path for our functions in that case. 👍 PRs welcome!

[alexslade]

I'll issue one this week. The change is trivial but I want to do some testing, spin up new apps etc to see it all working.