Re: BUG #18782: Inconsistent behaviour with triggers and row level security - depends on prior number of inserts - Mailing list pgsql-bugs

PG Bug reporting form <noreply@postgresql.org> writes:
> We have been seeing behaviour of the interaction between triggers and row
> level security being inconsistent depending on the number of inserts that
> have previously been made to the table which the trigger is attached to. I
> will attach full a code reproduction at the end of the issue.

Thanks for the carefully-documented bug report!  Often we have to
ask a lot of questions before we can get to the bottom of a problem.

> Our expectation is that a trigger which calls RLS which throws an exception
> should always throw an exception.

I think this expectation is faulty, and there's not really any bug
here.  Poking at your example, I get

regression=# set role app_user;
SET
regression=> explain verbose
SELECT 1
            FROM rls_table
            WHERE rls_table.should_not_duplicate = gen_random_uuid();
ERROR:  invalid input syntax for type uuid: ""
regression=> set team.team_id = '6a43cea8-4a5c-4989-bae2-ef5a77d92620';
SET
regression=> explain verbose
SELECT 1
            FROM rls_table
            WHERE rls_table.should_not_duplicate = gen_random_uuid();
                                                                QUERY PLAN
                  

------------------------------------------------------------------------------------------------------------------------------------------
 Seq Scan on public.rls_table  (cost=0.00..40.00 rows=1 width=4)
   Output: 1
   Filter: ((rls_table.should_not_duplicate = gen_random_uuid()) AND (rls_table.team_id =
(current_setting('team.team_id'::text))::uuid))
(3 rows)

Now first off, notice that I couldn't even do an EXPLAIN as app_user
until I set team.team_id.  That indicates that the error is being
thrown during planning.  The planner is trying to estimate the
selectivity of the "rls_table.team_id = something" clause, and
since current_setting is not volatile it sees no reason not to
evaluate it to get a constant to check against the statistics for
team_id.  Then when it tries to evaluate the cast to uuid, kaboom.

Secondly, notice that the RLS-derived team_id clause is placed
after the one about should_not_duplicate.  This is perhaps
surprising, but it's legitimate because uuid_eq is marked
leakproof, meaning we trust it not to leak any information
about the table contents.  What that means is that at runtime,
the cast to uuid will blow up only if there's at least one match
in rls_table.should_not_duplicate to the proposed new value.
In your test case, rls_table remains empty so there will never
be a run-time failure, whether team.team_id is valid or not.

So the error is not coming from where you think.  It's coming from
planning, and the reason it stops appearing after a few successful
trigger executions is that the plancache switches over to a generic
plan and stops invoking the planner.

(I did verify that the test on rls_table.team_id is still present
in the generic plan, so there's not a bug of that sort.)

In general, I would be wary of depending on errors to be thrown in
specific contexts and not other contexts.  That's a hard property to
guarantee in the face of any optimization, and we don't really try.
In the situation at hand, that means that RLS conditions that could
throw errors are a bad idea.  This one, far from enforcing that the
app_user can't learn about what is in rls_table, actually facilitates
doing so :-(.  I think it'd be safer to form the RLS tests as

    team_id::text = current_setting('team.team_id', true)

(where "true" prevents failure if team.team_id is unset).

            regards, tom lane