fix(0029): cover custom PostgREST request roles - #191
Open
sidgaikwad wants to merge 1 commit into
Open
sidgaikwad wants to merge 1 commit into
sidgaikwad wants to merge 1 commit into
Conversation
0029 decided exposure with a hardcoded `has_function_privilege('authenticated', ...)`.
A project that runs signed-in requests as its own PostgREST role — a supported
pattern via the `custom_access_token` hook rewriting the `role` claim — moves
EXECUTE off `authenticated` onto that role, at which point the lint reports zero
findings while the functions stay reachable through the API. A security category
reading zero because the project changed how it authenticates is indistinguishable
from one that has been remediated.
Widen the check to any role `authenticator` can SET ROLE into, since that is the
set of roles PostgREST can actually serve a request as. `anon` is lint 0028's job,
and `service_role` is the trusted backend role rather than a request role — it
holds EXECUTE on precisely the functions a project means to keep server-side, so
including it would flag correctly-secured functions.
Written as an OR against the original predicate rather than replacing it, so the
change is purely additive: every finding reported before is still reported,
including on databases with no `authenticator` role at all. Using EXISTS rather
than a join keeps one finding per function instead of one per matching role.
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.
Addresses #185 for
0029.0028is deliberately left alone — see the last section.The problem
0029decides exposure with a hardcoded role literal:A project that runs signed-in requests as its own PostgREST role moves
EXECUTEoffauthenticatedonto that role. The predicate goes false and the lint reports zero — which reads as "remediated", not "never evaluated".Reproduced on
supabase/postgres:17.6.1.136with aSECURITY DEFINERfunction inpublicwhose onlyEXECUTEgrant is held by a custom role that is a member ofauthenticatedand granted toauthenticator:service_rolePUBLIC)The fix
Widen the check to any role
authenticatorcanSET ROLEinto — that being the set of roles PostgREST can actually serve a request as — excludinganonandservice_role.Two deliberate differences from the SQL suggested in the issue, both because I ran it and it misbehaved:
It flags
service_role. The suggested filter excludes onlyanon, soservice_roleis included. That role holdsEXECUTEon precisely the functions a project means to keep server-side, so a function correctly locked down to it gets reported — the lint would advise revoking from the trusted backend role.CROSS JOINmultiplies findings. One row per (function, role) pair. Running the suggested query against three probe functions returned six rows, each with its owncache_key:This PR uses
EXISTSinstead, so a function yields at most one finding regardless of how many request roles can reach it.It is additive, not a replacement. The new condition is OR'd onto the original predicate rather than swapped in. That matters for correctness rather than tidiness:
test/fixtures.sqlcreates noauthenticatorrole, so a replacement would have made0029report zero across the entire existing suite — reintroducing the same fail-open this issue is about, in every database where that role doesn't exist. As written, every finding reported before is still reported.Verification
Ran the real harness (
bin/installcheckviadockerfiles/, Postgres 17.6): all 29 tests pass, includingqueries_are_unionableand0028. Before I regenerated the expected output, the only diff in the suite was my own new test case — every pre-existing case passed untouched, which is the evidence for the additive claim above.Added
case_custom_request_roletotest/sql/0029…, asserting the positive case returns exactly one row (guarding the duplicate regression) and that moving the grant toservice_rolereturns none.splinter.sqlis regenerated withbin/compile.py, not hand-edited.splinter.jsonis gitignored, so it is not in the diff.On
0028I did not touch it. The analogous fix would need to know the project's anonymous role, and the natural source —
pgrst.db_anon_role— is not reliably available: querying it through a live PostgREST request on a stock Supabase stack returnednull, as didpgrst.db_schemas. I would rather not guess at that, so I left0028for a maintainer who knows how that role is meant to be discovered.Worth noting the coverage gap still closes, just possibly under the other lint's name: a custom anonymous role is also a role
authenticatorcan assume, so functions it can execute now surface under0029rather than0028. Reported under a slightly wrong heading beats not reported, but if you would prefer a different split, say so and I will rework it.