fix: contain a raising RLS policy to the subscription it came from - #2096
Open
hamodywe wants to merge 1 commit into
Open
fix: contain a raising RLS policy to the subscription it came from#2096hamodywe wants to merge 1 commit into
hamodywe wants to merge 1 commit into
Conversation
realtime.apply_rls evaluates each subscription's policy with an unguarded `execute walrus_rls_stmt`. A policy that raises - a malformed claim cast to uuid is enough - propagates out of apply_rls and takes down the whole list_changes call, so every subscriber on the tenant loses the batch, not only the one whose claims are bad. The slot's confirmed_flush advances during decoding, so those changes are dropped rather than replayed on the next poll. Wrap the per-subscription check in its own block. A subscription whose policy raises is disqualified on its own and reported back with `Error 500`, carrying only schema, table and action - the same shape as the existing 400/401 rows - so a policy that could not be evaluated never discloses the row it was meant to gate. Closes supabase#2093
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2093.
The problem
realtime.apply_rlschecks each subscription against the table's policies with a bareexecute 'execute walrus_rls_stmt' into subscription_has_access;Nothing catches an error there. A policy that raises on one subscription's stored claims — a malformed claim cast to
uuidis enough — propagates out ofapply_rlsand takes down the entirelist_changescall, so every subscriber on the tenant loses the batch, including subscriptions on other roles and other tables whose own policies cannot fail.It is silent from every angle an operator would look at: sockets connect,
realtime.subscriptionlooks correct, the slot stays active. And the changes are not replayed —confirmed_flush_lsnadvances during decoding even though the statement errored, so each affected batch is dropped for good. That is what turned this into a ~2.5 hour project-wide outage in #2093.The change
The per-subscription check goes into its own block. A subscription whose policy raises is disqualified on its own and collected into
errored_role_sub_ids; every other subscription is evaluated and delivered normally.Those subscriptions then get a row of their own carrying
Error 500: Internal Server Error, RLS policy evaluation failed, with onlyschema,tableandtypein the payload — the same shape the existing 400 and 401 rows use — so a policy that could not be evaluated never ends up disclosing the row it was meant to gate. Silently dropping the subscription instead would have turned a loud tenant-wide outage into a silent per-subscriber one, which is the part of #2093 that made it expensive. Happy to cut that half if you would rather keep the change minimal.The warning follows the pattern
realtime.sendalready uses (WarnSendingBroadcastMessage), with the subscription id in it so the poisoned row can be found. Note thatlist_changesruns withSET log_min_messages TO 'fatal', so in the polling path this reaches the client rather than the server log; theerrorsarray is the reliable signal there.Everything else in the function is byte-identical to
20260709120000_fix_apply_rls_filter_role_leak.Verification
Elixir side, in
subscriptions_test.exs: a poisoned subscription alongside a healthy one on the same role, and alongside a subscription on a different role. Asserts the healthy subscriptions still receive the record, the poisoned one receives the error row without arecordkey, and the batch is consumed rather than left behind.I could not run
mix testlocally — this is a Windows box with no Elixir toolchain and the suite provisions tenant databases through its own Docker backend. So I verified the SQL directly instead, onsupabase/postgres:17.6.1.127with the tenant migrations applied, and the tests are written to run on CI:Reproduced first. Three subscriptions on one table (two
anon, oneauthenticatedwithusing (true)), oneanoncarryingapp_metadata.app_idas the string"null". Pre-fix:ERROR: invalid input syntax for type uuid: "null"atexecute walrus_rls_stmt, and all three subscriptions get nothing.After the change, same scenario:
No behaviour change when nothing raises. Ran a scenario covering INSERT/UPDATE/DELETE, a
selected_columnssubscription, two roles, and a table with no primary key (the 400 path) against both versions of the function and diffed the output: identical apart from the generated ids.Cost of the extra subtransaction, interleaved A/B on the same box, 100
apply_rlscalls per run, best of 5, 100 subscriptions on one record:About 3%, roughly 1 µs per subscription evaluation. If that is too much for the hot path, the alternative is to keep the loop unguarded and re-run it with per-subscription isolation only after something raises — zero cost normally, at the price of duplicating the loop. I went with the simpler version because this is a correctness fix in a function that is already hard to read; say the word and I will switch it.