diff --git a/.agents/skills/safegres/SKILL.md b/.agents/skills/safegres/SKILL.md index fe1f9f796..271ced580 100644 --- a/.agents/skills/safegres/SKILL.md +++ b/.agents/skills/safegres/SKILL.md @@ -72,7 +72,7 @@ The score only improves by being **explicit** (declaring exposure and intent) or | R3 | medium | fail-open | RLS table has grants **TO PUBLIC** | | W1 | medium | meta | No exposure surface configured — DB assumed reachable, score capped | -Perf-dimension rules (only collected with `--perf`, scored on their own axis; `S*` additionally need `--stats`): **X1** FK with no covering index (medium), **X2** policy filters on a column that leads no index (medium), **X3** policy casts/wraps its own column with no matching expression index (medium), **X4** policy calls a non-LEAKPROOF function (low), **X5** redundant/duplicate index (low), **X6** no primary key and no usable replica identity (low), **X7** search column with no index the search can use — `tsvector` w/o GIN/GiST, `vector` w/o HNSW/IVFFlat (medium), **X8** sort-shaped `timestamptz`/`date` column leading no index (info, heuristic), plus P1/P1b and the runtime-statistics rules **S1**-**S4**. +Perf-dimension rules (only collected with `--perf`, scored on their own axis; `S*` additionally need `--stats`): **X1** FK with no covering index (medium), **X2** policy filters on a column that leads no index (medium), **X3** policy casts/wraps its own column with no matching expression index (medium), **X4** policy calls a non-LEAKPROOF function (low), **X5** redundant/duplicate index (low), **X6** no primary key and no usable replica identity (low), **X7** search column with no index the search can use — `tsvector` w/o GIN/GiST, `vector` w/o HNSW/IVFFlat (medium), **X8** sort-shaped `timestamptz`/`date` column leading no index (info, heuristic), **X9** policy calls a STABLE function per row because it is not wrapped in a scalar sub-select (medium), plus P1/P1b and the runtime-statistics rules **S1**-**S4**. **Direction is the key idea:** `fail-open` = real exposure (untrusted side reaches more than intended). `fail-closed` = denied at runtime (hygiene/availability, not a leak) — contributes **0** to the score by default. R1/R2 are no-ops until you configure a role list; `safegres:constructive` sets them for `anonymous`. @@ -103,11 +103,13 @@ Typed variant (`defineConfig` from `confstash`) works too — see the package RE ## Performance dimension (`--perf`, off by default) -`safegres perf` / `safegres audit --perf` adds `report.perf` — its own findings, summary, and 0-100 score over index hygiene (X1/X5/X6/X7/X8), policy-aware index rules (X2/X3/X4) and policy cost (P1/P1b). Security and perf scores are never mixed: `report.score` sees only security findings, `report.perf.score` only perf ones. All checks are catalog + policy-AST analysis, so they're deterministic against an empty CI database. +`safegres perf` / `safegres audit --perf` adds `report.perf` — its own findings, summary, and 0-100 score over index hygiene (X1/X5/X6/X7/X8), policy-aware index rules (X2/X3/X4/X9) and policy cost (P1/P1b). Security and perf scores are never mixed: `report.score` sees only security findings, `report.perf.score` only perf ones. All checks are catalog + policy-AST analysis, so they're deterministic against an empty CI database. X7 is Constructive-specific: `graphile-search` exposes a full-text filter for every `tsvector` column and similarity search for every `vector` column *from the codec alone*, so an unindexed one is a live API field served by a seq scan. BM25/pg_trgm are intentionally not checked — those adapters are discovered from their indexes, so no index means the feature isn't exposed. X8 is the one heuristic (any column is orderable; timestamps are what feeds are actually sorted by), so it is `info`, scores 0, and is meant to be read rather than gated on. -X2/X3/X4 read the policy predicate itself: RLS quals run before user quals on every candidate row, so an unindexed policy column (X2), a cast/function wrapping it (X3), or a non-LEAKPROOF call that blocks qual pushdown (X4) is a tax on every query against the table, not just one slow report. +X2/X3/X4/X9 read the policy predicate itself: RLS quals run before user quals on every candidate row, so an unindexed policy column (X2), a cast/function wrapping it (X3), or a non-LEAKPROOF call that blocks qual pushdown (X4) is a tax on every query against the table, not just one slow report. + +X9 is the InitPlan rule. `STABLE` does not mean "evaluated once": measured on 200k rows, `USING (other_id = current_principal_id())` executed the function 200,000 times (424 ms) while `USING (other_id = (SELECT current_principal_id()))` executed it once (22 ms). The penalty is plan-dependent — an unwrapped call the planner turns into an index condition is evaluated once per scan, and the same policy costs 200k calls the moment the qual lands in a Filter (unindexed column, join, OR branch). Wrapping removes the dependence: the sub-select references no column, so it is hoisted into an InitPlan whatever plan is chosen, and the result is a constant the index can probe with. Detection is structural (non-IMMUTABLE + no column-referencing argument + not already inside an uncorrelated scalar sub-select), so it needs no list of known identity functions and catches a bare `current_setting()` too. VOLATILE calls are excluded on purpose (per-row is their contract; P1 covers them), and an `EXISTS` sub-select does not count as hoisted — it is correlated, so it runs per row. ```jsonc { diff --git a/packages/safegres/README.md b/packages/safegres/README.md index c710c7941..866b31493 100644 --- a/packages/safegres/README.md +++ b/packages/safegres/README.md @@ -147,6 +147,7 @@ A slow database is a different problem from an unsafe one, so safegres scores th | X6 | low | index | **No primary key** and no usable replica identity — rows cannot be addressed by updates, deletes, or logical replication | | X7 | medium | index | **Search column with no index the search can use** — a `tsvector` without GIN/GiST, a `vector` without HNSW/IVFFlat | | X8 | info | index | **Sort-shaped column leads no index** (`timestamptz`/`date`) — ordering or cursor-paginating a connection by it sorts the whole table | +| X9 | medium | index | **Policy calls a STABLE function per row** — the call isn't wrapped in a scalar sub-select, so the planner can't hoist it into an InitPlan | | P1 | high | anti-pattern | Policy body calls a **VOLATILE function** — re-evaluated per row | | P1b | medium | anti-pattern | Policy body calls a **STABLE function** in a per-row position | @@ -154,7 +155,16 @@ Every check is pure catalog + AST analysis: deterministic, workload-free, and sa X7 exists because the column type *is* the API declaration: `graphile-search` exposes a full-text filter for every `tsvector` column and a similarity search for every `vector` column, purely from the codec — so an unindexed one is a first-class API field backed by a sequential scan plus a per-row match or distance computation. BM25 and pg_trgm are deliberately not checked: those adapters are discovered *from* their indexes, so a missing index means the feature was never exposed. X8 is the one heuristic in the set — any column is orderable over a connection, but timestamps are what feeds are actually sorted and keyset-paginated by — so it defaults to `info`, contributes 0 to the score, and is meant to be read, not gated on (`perf.rules: { "X8": "off" }` to silence it). Trailing-position and partial indexes don't count for either rule: neither can serve the sort or the search on its own. -X2–X4 are the checks a generic index linter can't make, because they read the policy predicate. RLS quals are evaluated *before* user quals, on every candidate row, for every caller — so an unindexed or cast-wrapped policy column is a whole-table tax rather than a slow query. X2 requires the policy column to be the *leading* column of some index (a trailing position can't serve the qual alone); X3 looks for an expression index matching the exact wrapped shape; X4 skips built-ins, whose leakproofness is a property of the server rather than a schema choice. +X2–X4 and X9 are the checks a generic index linter can't make, because they read the policy predicate. RLS quals are evaluated *before* user quals, on every candidate row, for every caller — so an unindexed or cast-wrapped policy column is a whole-table tax rather than a slow query. X2 requires the policy column to be the *leading* column of some index (a trailing position can't serve the qual alone); X3 looks for an expression index matching the exact wrapped shape; X4 skips built-ins, whose leakproofness is a property of the server rather than a schema choice. + +X9 is the one that costs the most and looks the most innocent. `STABLE` promises a function's result won't change within the statement; it does **not** make the planner evaluate it once. Measured on 200k rows with a policy function that counts its own invocations: + +| Policy qual | Calls | Time | +| --- | --- | --- | +| `other_id = current_principal_id()` (Filter) | 200,000 | 424 ms | +| `other_id = (SELECT current_principal_id())` (InitPlan) | 1 | 22 ms | + +The honest caveat: the penalty is **plan-dependent**. When the planner can turn the qual into an index condition it evaluates the function once per scan even unwrapped — so the same policy costs one call on an indexed column and 200,000 on a Filter (unindexed column, a join, an OR branch, a plan change after `ANALYZE`). Wrapping removes the dependence: `(SELECT f())` references no column, so it is hoisted into an **InitPlan** and evaluated once per query whatever plan is chosen, and the result is a constant the index can be probed with. X9 is structural, not a name list — it fires on any non-IMMUTABLE call whose arguments reference no column of the row and that isn't already inside an uncorrelated scalar sub-select, so a GUC-reading helper added next year is caught without configuration. `current_setting()` itself is STABLE and is flagged too: removing the wrapper function doesn't avoid the per-row call. VOLATILE calls are deliberately excluded — per-row evaluation is their defined behaviour, and hoisting one would change semantics (that's P1's job). Being inside an `EXISTS` sub-select is not a defence: that subquery is correlated with the outer row, so it runs per row and takes the call with it. ```bash safegres perf --database mydb diff --git a/packages/safegres/__tests__/fixtures/x9-initplan.sql b/packages/safegres/__tests__/fixtures/x9-initplan.sql new file mode 100644 index 000000000..d9e19a89f --- /dev/null +++ b/packages/safegres/__tests__/fixtures/x9-initplan.sql @@ -0,0 +1,78 @@ +-- X9: a STABLE, row-independent call in a policy qual that the planner will +-- re-evaluate per row because it is not wrapped in a scalar sub-select. +DROP SCHEMA IF EXISTS fx_x9 CASCADE; +CREATE SCHEMA fx_x9; + +CREATE FUNCTION fx_x9.current_tenant() RETURNS uuid AS $$ + SELECT nullif(current_setting('jwt.claims.tenant_id', true), '')::uuid; +$$ LANGUAGE sql STABLE; + +-- Row-dependent: the argument references a column, so there is nothing to hoist. +CREATE FUNCTION fx_x9.owns(row_tenant uuid) RETURNS boolean AS $$ + SELECT row_tenant IS NOT NULL; +$$ LANGUAGE sql STABLE; + +-- Volatile: per-row evaluation is its defined behaviour (P1's business, not X9's). +CREATE FUNCTION fx_x9.roll() RETURNS uuid AS $$ + SELECT gen_random_uuid(); +$$ LANGUAGE sql VOLATILE; + +-- Immutable: folded at plan time, never flagged. +CREATE FUNCTION fx_x9.zero() RETURNS uuid AS $$ + SELECT '00000000-0000-0000-0000-000000000000'::uuid; +$$ LANGUAGE sql IMMUTABLE; + +CREATE TABLE fx_x9.tenants ( + id uuid PRIMARY KEY, + tenant_id uuid NOT NULL +); +CREATE INDEX tenants_tenant_idx ON fx_x9.tenants (tenant_id); + +-- X9: bare call, evaluated per row. +CREATE TABLE fx_x9.bare (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.bare ENABLE ROW LEVEL SECURITY; +CREATE POLICY bare_tenant ON fx_x9.bare + USING (tenant_id = fx_x9.current_tenant()); + +-- No X9: already wrapped in an uncorrelated scalar sub-select (InitPlan). +CREATE TABLE fx_x9.hoisted (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.hoisted ENABLE ROW LEVEL SECURITY; +CREATE POLICY hoisted_tenant ON fx_x9.hoisted + USING (tenant_id = (SELECT fx_x9.current_tenant())); + +-- X9: inside an EXISTS sub-select. That subquery is correlated with the outer +-- row, so it runs per row and takes the unhoisted call with it. +CREATE TABLE fx_x9.correlated (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.correlated ENABLE ROW LEVEL SECURITY; +CREATE POLICY correlated_tenant ON fx_x9.correlated + USING (EXISTS ( + SELECT 1 FROM fx_x9.tenants t + WHERE t.id = correlated.id AND t.tenant_id = fx_x9.current_tenant() + )); + +-- No X9: the argument references a column, so the call cannot be hoisted. +CREATE TABLE fx_x9.row_dependent (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.row_dependent ENABLE ROW LEVEL SECURITY; +CREATE POLICY row_dependent_tenant ON fx_x9.row_dependent + USING (fx_x9.owns(tenant_id)); + +-- No X9: VOLATILE and IMMUTABLE calls are both out of scope. +CREATE TABLE fx_x9.other_volatility (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.other_volatility ENABLE ROW LEVEL SECURITY; +CREATE POLICY other_volatility_tenant ON fx_x9.other_volatility + USING (tenant_id = fx_x9.roll() OR tenant_id = fx_x9.zero()); + +-- X9: current_setting() is STABLE too — dropping the wrapper does not fix it. +CREATE TABLE fx_x9.raw_guc ( + id uuid PRIMARY KEY, + tenant_name text NOT NULL +); +ALTER TABLE fx_x9.raw_guc ENABLE ROW LEVEL SECURITY; +CREATE POLICY raw_guc_tenant ON fx_x9.raw_guc + USING (tenant_name = current_setting('jwt.claims.tenant', true)); + +-- X9 on WITH CHECK as well as USING. +CREATE TABLE fx_x9.writes (LIKE fx_x9.tenants INCLUDING ALL); +ALTER TABLE fx_x9.writes ENABLE ROW LEVEL SECURITY; +CREATE POLICY writes_tenant ON fx_x9.writes FOR INSERT + WITH CHECK (tenant_id = fx_x9.current_tenant()); diff --git a/packages/safegres/__tests__/perf.test.ts b/packages/safegres/__tests__/perf.test.ts index 856778ea9..936be16a8 100644 --- a/packages/safegres/__tests__/perf.test.ts +++ b/packages/safegres/__tests__/perf.test.ts @@ -151,6 +151,47 @@ describe('policy-aware perf rules', () => { }); }); +describe('X9 — per-row policy calls that the planner could hoist', () => { + it('flags unwrapped STABLE calls and spares wrapped ones', async () => { + await applyFixture('x9-initplan.sql'); + const report = await audit(pg.client as never, { schemas: ['fx_x9'], perf: true }); + const x9 = report.perf!.findings.filter((f) => f.code === 'X9'); + + expect(tablesFor(x9, 'X9')).toEqual([ + // Correlated subquery: runs per outer row, so the call inside it does too. + 'fx_x9.bare', + 'fx_x9.correlated', + // current_setting() is STABLE — dropping the wrapper doesn't fix anything. + 'fx_x9.raw_guc', + 'fx_x9.writes' + ]); + + const bare = x9.find((f) => f.table === 'bare'); + expect(bare?.severity).toBe('medium'); + expect(bare?.dimension).toBe('perf'); + expect(bare?.context).toMatchObject({ function: 'fx_x9.current_tenant', clause: 'USING' }); + expect(bare?.hint).toContain('(SELECT fx_x9.current_tenant())'); + + expect(x9.find((f) => f.table === 'writes')?.context).toMatchObject({ clause: 'WITH CHECK' }); + }); + + it('ignores row-dependent, volatile and immutable calls', async () => { + const report = await audit(pg.client as never, { schemas: ['fx_x9'], perf: true }); + const tables = tablesFor(report.perf!.findings, 'X9'); + // Argument references a column — nothing to hoist. + expect(tables).not.toContain('fx_x9.row_dependent'); + // VOLATILE is per-row by definition; IMMUTABLE is folded at plan time. + expect(tables).not.toContain('fx_x9.other_volatility'); + // Already an InitPlan. + expect(tables).not.toContain('fx_x9.hoisted'); + }); + + it('stays off when perf is disabled', async () => { + const report = await audit(pg.client as never, { schemas: ['fx_x9'] }); + expect(report.findings.filter((f) => f.code === 'X9')).toHaveLength(0); + }); +}); + describe('P1/P1b re-homed to the perf dimension', () => { it('keeps P1 out of the security score', async () => { await applyFixture('p1-volatile-func.sql'); diff --git a/packages/safegres/src/checks/policy-index.ts b/packages/safegres/src/checks/policy-index.ts index 211541c79..38a6fd318 100644 --- a/packages/safegres/src/checks/policy-index.ts +++ b/packages/safegres/src/checks/policy-index.ts @@ -214,6 +214,110 @@ export function checkNonLeakproofPolicyFunctions( return out; } +/** + * System functions worth flagging for X9 despite being built in: reading a GUC + * per row is the exact pattern the rule exists to catch, whether it goes + * through a wrapper or not. + */ +const HOISTABLE_SYSTEM_FUNCTIONS = new Set(['current_setting', 'pg_catalog.current_setting']); + +/** + * X9: the policy calls a STABLE function whose arguments don't depend on the + * row, but the call isn't wrapped in a scalar sub-select. + * + * `STABLE` promises the result won't change within the statement; it does *not* + * make the planner evaluate the call once. A bare `current_principal_id()` in a + * qual is executed for every row the scan considers. Wrapped as + * `(SELECT current_principal_id())` the expression references no column, so the + * planner hoists it into an InitPlan: one call per query, and the result is a + * constant the index can be probed with. + * + * VOLATILE calls are deliberately excluded — per-row evaluation is their + * defined behaviour, and hoisting one would change semantics (P1 covers those). + * IMMUTABLE calls with constant arguments are folded at plan time already. + */ +export function checkUnhoistedPolicyFunctions( + table: TableSnapshot, + expr: PgAstNode, + volatility: Map, + policyName: string, + clause: PolicyClause +): Finding[] { + const out: Finding[] = []; + const seen = new Set(); + + walk(expr as object, (path: NodePath) => { + if (path.tag !== 'FuncCall') return; + + const node = path.node as Record; + const qualified = funcNameQualified(node); + const bare = qualified.split('.').pop() ?? qualified; + const info = volatility.get(qualified) ?? volatility.get(bare); + if (!info) return; + if (info.volatility !== 's') return; + if (info.isSystem && !HOISTABLE_SYSTEM_FUNCTIONS.has(info.name)) return; + if (referencesColumn(node)) return; + if (isHoisted(path)) return; + if (seen.has(info.name)) return; + seen.add(info.name); + + out.push({ + code: 'X9', + severity: 'medium', + category: 'index', + schema: table.schema, + table: table.name, + policy: policyName, + message: + `Policy "${policyName}" on ${table.schema}.${table.name} calls ${info.name}() per row — the call is not wrapped in a scalar sub-select`, + hint: + `STABLE does not mean "evaluated once". Whenever this qual lands in a Filter rather than an index ` + + `condition — an unindexed policy column, a join, an OR branch — ${info.name}() is executed for every ` + + `row the scan considers. Wrap it as (SELECT ${info.name}()) so the expression references no column and ` + + 'the planner hoists it into an InitPlan: one call per query, whatever plan it picks.', + context: { function: info.name, clause } + }); + }); + + return out; +} + +/** True when any argument of the call references a column (so it can't be hoisted). */ +function referencesColumn(funcCall: Record): boolean { + const args = funcCall.args; + if (!Array.isArray(args) || args.length === 0) return false; + + let found = false; + for (const arg of args) { + walk(arg as object, (path: NodePath) => { + if (path.tag === 'ColumnRef') found = true; + }); + if (found) return true; + } + return false; +} + +/** + * True when the call already sits inside an uncorrelated scalar sub-select — + * `(SELECT fn())` — which is what makes the planner hoist it. + * + * Being inside an `EXISTS` sub-select is *not* enough: that subquery is + * correlated with the outer row, so it runs per row and takes the function + * call with it. + */ +function isHoisted(path: NodePath): boolean { + for (let cursor = path.parent; cursor; cursor = cursor.parent) { + if (cursor.tag !== 'SubLink') continue; + const sublink = cursor.node as Record; + if (sublink.subLinkType !== 'EXPR_SUBLINK') return false; + const subselect = sublink.subselect as Record | undefined; + const stmt = (subselect?.SelectStmt ?? subselect) as Record | undefined; + const from = stmt?.fromClause; + return !Array.isArray(from) || from.length === 0; + } + return false; +} + /** Columns that sit in the leading position of at least one index. */ function leadingColumns(indexes: TableIndexSnapshot): Set { const out = new Set(); diff --git a/packages/safegres/src/commands/audit.ts b/packages/safegres/src/commands/audit.ts index 9d9b9e698..ed7cda552 100644 --- a/packages/safegres/src/commands/audit.ts +++ b/packages/safegres/src/commands/audit.ts @@ -26,6 +26,7 @@ import { import { checkNonLeakproofPolicyFunctions, checkPolicyColumnCasts, + checkUnhoistedPolicyFunctions, checkUnindexedPolicyColumns, collectPredicateColumns, type PredicateColumn @@ -417,15 +418,19 @@ async function auditTableAst( if (!indexes) continue; - // --- Policy-aware perf rules (X2/X3/X4) --- + // --- Policy-aware perf rules (X2/X3/X4/X9) --- const cols: PredicateColumn[] = []; if (using) { cols.push(...collectPredicateColumns(using, 'USING', table.name)); findings.push(...checkNonLeakproofPolicyFunctions(table, using, volatility, policy.name)); + findings.push(...checkUnhoistedPolicyFunctions(table, using, volatility, policy.name, 'USING')); } if (withCheck) { cols.push(...collectPredicateColumns(withCheck, 'WITH CHECK', table.name)); findings.push(...checkNonLeakproofPolicyFunctions(table, withCheck, volatility, policy.name)); + findings.push( + ...checkUnhoistedPolicyFunctions(table, withCheck, volatility, policy.name, 'WITH CHECK') + ); } if (cols.length > 0) predicateColumns.set(policy.name, cols); } diff --git a/packages/safegres/src/index.ts b/packages/safegres/src/index.ts index 87d99adf8..8d0c50c8e 100644 --- a/packages/safegres/src/index.ts +++ b/packages/safegres/src/index.ts @@ -46,6 +46,7 @@ export { export { checkNonLeakproofPolicyFunctions, checkPolicyColumnCasts, + checkUnhoistedPolicyFunctions, checkUnindexedPolicyColumns, collectPredicateColumns } from './checks/policy-index'; diff --git a/packages/safegres/src/rules/registry.ts b/packages/safegres/src/rules/registry.ts index b811a56a9..fa8ef1555 100644 --- a/packages/safegres/src/rules/registry.ts +++ b/packages/safegres/src/rules/registry.ts @@ -230,6 +230,15 @@ export const RULES: RuleMeta[] = [ title: 'Sort-shaped column (timestamp/date) leads no index — heuristic advisory', scope: 'index' }, + { + code: 'X9', + category: 'index', + defaultSeverity: 'medium', + direction: 'neutral', + dimension: 'perf', + title: 'RLS policy calls a STABLE function per row — not wrapped in a scalar sub-select (no InitPlan)', + scope: 'policy-ast' + }, { code: 'S1', category: 'index',