Skip to content

fix: set pgrst.db_schemas in 0028/0029 tests#162

Merged
samrose merged 1 commit into
mainfrom
fix-0028-0029-pgrst-db-schemas
Apr 27, 2026
Merged

fix: set pgrst.db_schemas in 0028/0029 tests#162
samrose merged 1 commit into
mainfrom
fix-0028-0029-pgrst-db-schemas

Conversation

@samrose
Copy link
Copy Markdown
Contributor

@samrose samrose commented Apr 27, 2026

Summary

PR #160 added a pgrst.db_schemas filter to lints 0028 and 0029 to scope function findings to schemas PostgREST actually exposes via /rest/v1/rpc. The test SQL files for those lints were not updated to set the GUC, so every positive case in the test stopped firing — the lint correctly returns 0 rows when pgrst.db_schemas is unset, which makes "DEFINER function with EXECUTE to anon" look like a non-finding to the test harness.

This PR sets set local pgrst.db_schemas = 'public'; at the top of both test files, mirroring the established pattern from test/sql/0023_sensitive_columns_exposed.sql (which uses the same filter for the same reason).

Why this slipped past CI

#160's CI run actually reported 2 of 28 tests failed in its log, but the workflow turned green. The cause is in dockerfiles/docker-compose.yml:

command:
  - bash
  - -c
  - "./bin/installcheck; cp /home/splinter/regression.diffs ... 2>/dev/null || true"

bash -c "A; B || true" returns the exit code of the last command — cp ... || true, which is always 0 — so the container exits 0 even when installcheck exits non-zero. The actions run conclusion is therefore success regardless of pg_regress results.

Two tactical options for fixing that, neither in scope for this PR:

  1. Replace the ;-and-|| true chain with ./bin/installcheck; rc=$?; cp ... 2>/dev/null || true; exit $rc so the cp still runs (for diagnostics) but the installcheck exit code is preserved.
  2. Move the cp into a separate compose service or a post_command hook so the test container can fail cleanly.

Worth a follow-up so future test breakage cannot hide behind a green check.

Verification

SUPABASE_VERSION=15.1.1.13 docker-compose -f dockerfiles/docker-compose.yml run --build --rm test
# All 28 tests passed.

Confirmed locally on supabase/postgres:15.1.1.13. Each positive case in 0028 and 0029 now produces the expected row counts:

  • case_definer_default_grant: 1 row (SECURITY DEFINER function created with default PUBLIC EXECUTE).
  • case_overloads: 2 rows with distinct cache_key values driven by pg_get_function_identity_arguments.
  • All negative cases (INVOKER, EXECUTE-revoked, system-schema) continue to return 0 rows.

Files

  • test/sql/0028_anon_security_definer_function_executable.sql — adds set local pgrst.db_schemas = 'public';
  • test/sql/0029_authenticated_security_definer_function_executable.sql — same.
  • test/expected/0028_anon_security_definer_function_executable.out — re-promoted to include the new echo line.
  • test/expected/0029_authenticated_security_definer_function_executable.out — same.

  #160 added a pgrst.db_schemas filter to the function lints to match
  PostgREST's API exposure scope, but the test SQL didn't set the
  guc, so the positive cases stopped firing.

  Mirrors the pattern from test/sql/0023_sensitive_columns_exposed.sql.
@samrose samrose merged commit 929e095 into main Apr 27, 2026
2 checks passed
joshenlim pushed a commit to supabase/supabase that referenced this pull request Apr 27, 2026
…R functions (#45260)

## I have read the
[CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md)
file.

YES

## What kind of change does this PR introduce?

Feature — wires up three new advisor lints landed in splinter, and
updates the self-hosted SQL bundle for the existing
`pg_graphql_anon_table_exposed` lint to track splinter's correctness
fixes. Companion to `supabase/splinter` #160 (already merged) and #162
(test fix in flight).

## What is the current behavior?

Splinter's `main` now exposes four lints in the pg_graphql / SECURITY
DEFINER family:

- `pg_graphql_anon_table_exposed` (0026, existing) — wired into Studio
in #45253; SQL in `packages/pg-meta` is the original version that uses
`has_table_privilege` and the relkind set `('r','p','v','m')`.
- `pg_graphql_authenticated_table_exposed` (0027, new) — paired check
against the `authenticated` role. Studio renders any new finding without
a `lintInfoMap` entry as a row with no icon, no title mapping, and no
"Fix" CTA. Self-hosted users do not see the lint at all because
`packages/pg-meta` does not include it.
- `anon_security_definer_function_executable` (0028, new) — `SECURITY
DEFINER` function executable by `anon`. Same Studio + self-hosted gaps
as 0027.
- `authenticated_security_definer_function_executable` (0029, new) —
same against `authenticated`.

Splinter has also updated 0026 itself (PR #160) in two ways that need to
flow into the self-hosted SQL bundle:
1. **`relkind` filter:** `('r','p','v','m')` → `('r','v','m','f')`.
Drops partitioned table roots (pg_graphql does not expose them; their
leaf partitions are still covered as `'r'`) and adds foreign tables,
which pg_graphql does expose.
2. **Privilege predicate:** `has_table_privilege(role, oid, 'SELECT')` →
`EXISTS` over `pg_attribute` calling `has_column_privilege`. Catches
column-level grants such as `GRANT SELECT (col) ON t TO anon`, which
pg_graphql's introspection exposes but `has_table_privilege` missed.

Cloud projects auto-fetch `splinter.sql` via the platform mgmt-api's
`getLintSql` (1-hour cache TTL), so they pick up #160's lint and SQL
changes independently of this PR. This PR is about the Studio display
mapping and the self-hosted SQL bundle.

## What is the new behavior?

Two minimal additions, mirroring the integration shape of #45253.

### `apps/studio/components/interfaces/Linter/Linter.utils.tsx`

Three new entries appended to `lintInfoMap`:

- `pg_graphql_authenticated_table_exposed` — `Eye` icon (paired with the
existing `pg_graphql_anon_table_exposed` entry); link points to the
Table Editor scoped to `metadata.schema` + `metadata.name`; `linkText:
'View object'`; `category: 'security'`.
- `anon_security_definer_function_executable` — `Unlock` icon (signals
"this thing is callable when it shouldn't be"); link points to the
Database Functions browser scoped to `metadata.schema` +
`metadata.name`; `linkText: 'View function'`; `category: 'security'`.
- `authenticated_security_definer_function_executable` — same as 0028
against `authenticated`.

Each entry's `docsLink` points at the splinter-hosted lint doc.

### `packages/pg-meta/src/sql/studio/advisor/lints.ts`

The existing `pg_graphql_anon_table_exposed` SQL block is updated in
place to match the new splinter version: new `relkind` set, `case`
statement for `'f'`, and the `EXISTS` over `pg_attribute` privilege
check. Three new `union all` blocks are appended for 0027/0028/0029. The
function lints (0028/0029) include the `pgrst.db_schemas` filter
(mirroring lint `0023_sensitive_columns_exposed`) so findings are scoped
to schemas PostgREST actually exposes; the self-hosted query wrapper
already sets the GUC when `exposedSchemas` is passed
(`enrichLintsQuery`).

## Coverage of the four exposure paths

| Role | Tables/views/MVs/foreign tables | SECURITY DEFINER functions |
|------|---------|----------|
| `anon` | 0026 (existing, updated) | 0028 (new) |
| `authenticated` | 0027 (new) | 0029 (new) |

The 0026/0027 pair covers `pg_graphql` introspection visibility; the
0028/0029 pair covers RLS bypass via privileged function execution
through `/rest/v1/rpc` (and `/graphql/v1` for compatible return types).
Each lint's doc cross-references its sibling so an operator hitting one
is steered toward the others.

## Verification

- `cd packages/pg-meta && npx tsc --noEmit` — clean.
- `cd apps/studio && npx tsc --noEmit` — clean for the changed file.
(Other unrelated TS errors exist in the working tree but are
pre-existing and not introduced by this PR.)
- `cd apps/studio && npx eslint
components/interfaces/Linter/Linter.utils.tsx` — clean.

## Files

- `apps/studio/components/interfaces/Linter/Linter.utils.tsx` — adds
three `lintInfoMap` entries (0027, 0028, 0029).
- `packages/pg-meta/src/sql/studio/advisor/lints.ts` — updates the 0026
SQL block to match splinter's correctness fixes, appends 0027/0028/0029
SQL blocks.

## Related

- supabase/splinter#160 — adds 0027/0028/0029 and rewrites 0026
(merged).
- supabase/splinter#162 — fixes test setup for 0028/0029 (in flight;
does not affect the SQL shipped here).
- #45253 — original 0026 Studio integration.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added security linting to detect authenticated-table exposure and
executable SECURITY DEFINER functions.
  * Added signed-in visibility checks alongside anonymous checks.

* **Bug Fixes / Improvements**
* Improved relation type handling for accurate table/foreign/partition
classification.
  * Switched to column-level privilege analysis for visibility.
* Improved entity naming shown in lints (includes function argument
display).
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Danny White <3104761+dnywh@users.noreply.github.com>
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.

2 participants