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

<tgl(at)sss(dot)pgh(dot)pa(dot)us> writes
>> 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.

> In general, I would be wary of depending on errors to be thrown in
specific contexts and not other contexts.

Hi Tom, thank you very much for looking into this and that explanation
makes sense, I'll take a look at changing our RLS policies to use
`ok_missing = true` as that sounds like a sensible route forwards.

Many thanks,
Julian Wreford

On Sat, 25 Jan 2025 at 19:56, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> PG Bug reporting form <noreply(at)postgresql(dot)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
>