Skip to content

fix(0029): cover custom PostgREST request roles - #191

Open
sidgaikwad wants to merge 1 commit into
supabase:mainfrom
sidgaikwad:fix/0029-custom-request-roles
Open

sidgaikwad wants to merge 1 commit into
supabase:mainfrom
sidgaikwad:fix/0029-custom-request-roles

Conversation

@sidgaikwad

Copy link
Copy Markdown

Addresses #185 for 0029. 0028 is deliberately left alone — see the last section.

The problem

0029 decides exposure with a hardcoded role literal:

and pg_catalog.has_function_privilege('authenticated', p.oid, 'EXECUTE')

A project that runs signed-in requests as its own PostgREST role moves EXECUTE off authenticated onto 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.136 with a SECURITY DEFINER function in public whose only EXECUTE grant is held by a custom role that is a member of authenticated and granted to authenticator:

before after
function granted only to the custom request role 0 findings 1 finding
function granted only to service_role 0 findings 0 findings
function with default grants (via PUBLIC) 1 finding 1 finding

The fix

Widen the check to any role authenticator can SET ROLE into — that being the set of roles PostgREST can actually serve a request as — excluding anon and service_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 only anon, so service_role is included. That role holds EXECUTE on 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 JOIN multiplies findings. One row per (function, role) pair. Running the suggested query against three probe functions returned six rows, each with its own cache_key:

proname            | flagged_via_role
-------------------+------------------
claude_sd_backend  | service_role        <- false positive
claude_sd_custom   | claude_app_user
claude_sd_custom   | service_role        <- duplicate
claude_sd_normal   | authenticated
claude_sd_normal   | claude_app_user     <- duplicate
claude_sd_normal   | service_role        <- duplicate

This PR uses EXISTS instead, 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.sql creates no authenticator role, so a replacement would have made 0029 report 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/installcheck via dockerfiles/, Postgres 17.6): all 29 tests pass, including queries_are_unionable and 0028. 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_role to test/sql/0029…, asserting the positive case returns exactly one row (guarding the duplicate regression) and that moving the grant to service_role returns none.

splinter.sql is regenerated with bin/compile.py, not hand-edited. splinter.json is gitignored, so it is not in the diff.

On 0028

I 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 returned null, as did pgrst.db_schemas. I would rather not guess at that, so I left 0028 for 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 authenticator can assume, so functions it can execute now surface under 0029 rather than 0028. Reported under a slightly wrong heading beats not reported, but if you would prefer a different split, say so and I will rework it.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant