Proposal: detect read policies whose USING is unconditionally true for unauthenticated sessions (inverted auth.* IS NULL disjunct)

Abierto
  • ¿Algo no está bien?
#165 3 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
5/5
Tiempo estimado
Más de una semana
Aptitud para principiantes
45/100
Tipo de issue
Nueva funcionalidad
Claridad
Bastante claro
Estado de actividad
Tranquilo
Stack tecnológico
postgres
Área
databases, security

Línea de trabajo

Empieza por el skill /new-lint y el lint existente 0024_rls_policy_always_true; después, revisa el contrato estándar de lint y la estructura de los tests de pg_regress. Confirma el número de lint nuevo y la elección entre WARN/INFO antes de continuar. Se considera terminado cuando se detecte el verdadero positivo propuesto y los negativos enumerados—current_user, IS NULL condicionado mediante AND, un predicado con ámbito restringido y current_setting(name, false)—sigan sin producir resultados en los tests de regresión.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

Hi! A suggestion for a new Security lint, related to but framed differently from PR #28 ("feat: add lint to detect RLS policies that allow access to anonymous users").

The gap

0024_rls_policy_always_true deliberately scopes its always-true USING check to command in ('UPDATE','DELETE','ALL') and matches only the literal forms ('true','(true)','1=1','(1=1)') — with the comment that SELECT … USING (true) is often intentional. That's the right call for literal true. But a very common real-world shape slips through entirely:

create policy "read own"
  on public.documents for select to authenticated
  using ( auth.uid() is null or owner_id = auth.uid() );

For any request where auth.uid() is NULL (an anonymous session, or any token-less call), auth.uid() is null is true, so the whole USING is true and every row is readable. It isn't literal-true, and it's a SELECT, so 0024 can't see it — yet it exposes the table.

Why this isn't the PR #28 discussion

This lint does not flag "the policy is reachable by anonymous users" — that's a deliberate design choice and not inherently a bug (which, as I read it, was the substance of the pushback on PR #28). It flags a predicate whose top-level OR short-circuits to true for any session where the auth function returns NULL — a logic error that is wrong whether or not anonymous sign-ins are enabled. (Which sessions those are depends on the policy's roles: for a public/anon policy it's any token-less request; for a TO authenticated policy it's the narrower case of an authenticated token with a NULL sub.) An … IS NULL OR … disjunct on an auth function almost always means the author intended the negation (IS NOT NULL AND …) or a scoped check, and accidentally turned the policy into a no-op. Framing it as a correctness defect (rather than an anonymous-access judgement) keeps it squarely in advisor territory.

Proposed detection

A pure SQL view over pg_policies, standard lint contract. It fires on a predicate shape, not on any project setting, so it needs no conditional execution:

  • permissive policy, cmd in ('SELECT','ALL'), granted to anon / authenticated / public;
  • normalized qual contains a top-level OR disjunct of the form <auth fn>() is null, where <auth fn> is genuinely nullable — auth.uid() / auth.jwt() / auth.role(), or current_setting('…', true).
  • Exclusions, to stay false-positive-clean: never-NULL identity functions (current_user / session_user / current_role / user) — those disjuncts are constant-false dead branches, not leaks; and current_setting(name) / current_setting(name, false), which raise rather than return NULL.
  • Severity: WARN/INFO; remediation: "remove the IS NULL disjunct, or replace it with an explicit deny."

A couple of questions before a PR

  1. Would you prefer a fresh lint number, or to revive/rework PR #28?
  2. Severity preference — WARN or INFO?

Happy to implement it with the /new-lint skill + pg_regress tests covering the true positive above plus negatives (current_user is null disjunct, AND-gated is null, a plain scoped predicate, current_setting(name, false) is null).

Lenguaje dominante
PLpgSQL
Estrellas
284
Forks
110
Sin PR fusionados en 30 d

Preparar el entorno

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de supabase/splinter

Todos los issues de supabase/splinter

Issues similares

Más issues de Databases

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.