Merge pull request #1622 from constructive-io/feat/safegres-with-check-option

feat(safegres): L18 — a writable view's filter is a read filter without WITH CHECK OPTION

Author: pyramation

.agents/skills/safegres/SKILL.md

@@ -88,7 +88,7 @@ The score only improves by being **explicit** (declaring exposure and intent) or
 | L15 | info | neutral | **Unreadable body** — an untrusted role reaches through a definer view *or function* whose body could not be followed |
 | L16 | info | fail-open | **Sequence privilege** — `nextval`/`setval`/`last_value` reach, which no policy filters |
 | L17 | info | fail-open | **Foreign-table grant** — a relation Postgres will not let you protect with RLS at all |
-| L18 | — | — | *(claimed by a concurrent PR: `WITH CHECK OPTION` on writable views)* |
+| L18 | info | fail-open | **Writable filtering view without `WITH CHECK OPTION`** — writes escape the view's own row filter |
 | L19 | info | fail-open | **Definer-function reach** — a SECURITY DEFINER body touches a relation as its owner, and EXECUTE is the only grant the caller needed |
 | L20 | info | fail-open | **`INSTEAD OF` trigger write** — the write becomes a trigger function's body, and a definer one lands it as the function owner |
 | W1 | medium | meta | No exposure surface configured — DB assumed reachable, score capped |
@@ -102,7 +102,7 @@ The score only improves by being **explicit** (declaring exposure and intent) or
 
 Reach is modelled as cells in `checks/role-reach.ts`, each carrying the role the access **executes as** (`effectiveRole`), the **path** of edges it arrived by (`grant` / `setrole` / `view` / `matview` / `rule` / `function` / `trigger`), and a **proof** bit: `catalog` (an ACL row or `pg_auth_members`) or `ast` (read out of a SQL body). `opaque-tainted` marks a proven read whose downstream effects are unknown, and L15 has two producers: a definer view, or a definer function, whose body executes SQL the analysis cannot see through yields a cell naming the *view or function itself*, since what lies past it cannot be named.
 
-**L8–L12 are the AST half: a view is not what its definition looks like.** A view without `security_invoker` runs as its owner, so a caller's SELECT on the view reads base relations under the *owner's* privileges (L8), and if the view is auto-updatable, writes land the same way (L9). Rewrite rules are worse: their actions are **not** governed by `security_invoker`, so L10 fires on invoker views where L9 does not. A materialized view stores rows computed at REFRESH time, so the bases are never consulted and their policies never run (L11). And a filtering view that is not `security_barrier` lets a leaky caller predicate be pushed below the filter (L12).
+**L8–L12 are the AST half: a view is not what its definition looks like.** A view without `security_invoker` runs as its owner, so a caller's SELECT on the view reads base relations under the *owner's* privileges (L8), and if the view is auto-updatable, writes land the same way (L9). Rewrite rules are worse: their actions are **not** governed by `security_invoker`, so L10 fires on invoker views where L9 does not. A materialized view stores rows computed at REFRESH time, so the bases are never consulted and their policies never run (L11). And a filtering view that is not `security_barrier` lets a leaky caller predicate be pushed below the filter (L12) — or, on the write side, lets a writer store rows outside it, because `WITH CHECK OPTION` is not the default (L18).
 
 **Which columns escape is a catalog fact, not a parsing problem.** `ViewSnapshot.columnDeps` reads the `pg_depend` rows the rewriter wrote for the view's `_RETURN` rule: `SELECT *` arrives expanded, a column used only in a `WHERE` counts as read, and a nested view depends on the *inner view's* columns. L8 puts that set in the message and in `context.columns`, and suppresses itself when every escaping column is one the role already holds by column grant **and** the base has no RLS — with RLS on, the owner reads rows the caller's policies hide, so the projection is beside the point. An absent column set is unknown, never narrow.
 
@@ -120,7 +120,7 @@ New L-rules ship `info` and **score-neutral** on purpose: the honest severity of
 
 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 and the untrusted-role L-rules are no-ops until you configure a role list: `"L8": ["info", { "roles": ["anonymous"] }]`, or `{ "rolesFrom": "anon" }` to take them from `exposure.anonRoles`, which is what `safegres:recommended` does for L5 and L7–L17. `safegres:constructive` sets R1/R2 for `anonymous`.
+**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 and the untrusted-role L-rules are no-ops until you configure a role list: `"L8": ["info", { "roles": ["anonymous"] }]`, or `{ "rolesFrom": "anon" }` to take them from `exposure.anonRoles`, which is what `safegres:recommended` does for L5 and L7–L20. `safegres:constructive` sets R1/R2 for `anonymous`.
 
 ## Configuration (confstash)
 

packages/safegres/README.md

@@ -153,6 +153,7 @@ family, **not** the dimension: `P1`/`P1b` are performance, `P5` is security.
 | L15 | info | neutral | **Unreadable body** — an untrusted role reaches through a definer view or function whose body the analysis could not follow † |
 | L16 | info | fail-open | **Sequence privilege** — an untrusted role can advance or read a sequence, which no policy filters † |
 | L17 | info | fail-open | **Foreign-table grant** — an untrusted role reaches a relation that cannot carry RLS at all † |
+| L18 | info | fail-open | **Writable filtering view without `WITH CHECK OPTION`** — an untrusted role writes rows the view's own filter excludes † |
 | L19 | info | fail-open | **Definer-function reach** — an untrusted role touches a relation by executing a `SECURITY DEFINER` function, which runs as its owner † |
 | L20 | info | fail-open | **`INSTEAD OF` trigger write** — a write against a view becomes a trigger function's body, and a definer one lands it as the function's owner † |
 | W1 | medium | — | **No exposure surface configured** — whole database assumed reachable, score capped |

packages/safegres/__tests__/definer-function.test.ts

@@ -72,6 +72,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [grant('anon', 'INSERT')],
     definition: 'SELECT id, body FROM app.secrets',
     writable: ['INSERT'],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/definer-view.test.ts

@@ -33,6 +33,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [grant('anon', 'SELECT')],
     definition: 'SELECT id, total FROM app.orders',
     writable: [],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/unaudited-reach.test.ts

@@ -35,6 +35,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [{ role: 'anon', privilege: 'SELECT', grantable: false, bypassRls: false }],
     definition: 'SELECT rolname FROM private.secrets',
     writable: [],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/unreadable-view.test.ts

@@ -33,6 +33,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [{ role: 'anon', privilege: 'SELECT', grantable: false, bypassRls: false }],
     definition: 'SELECT id FROM app.orders',
     writable: [],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/view-columns.test.ts

@@ -33,6 +33,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     definition: 'SELECT id, email FROM app.people',
     columnDeps: [{ schema: 'app', table: 'people', columns: ['email', 'id'] }],
     writable: [],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/view-exposure.test.ts

@@ -36,6 +36,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [grant('anon', 'SELECT')],
     definition: 'SELECT id, body FROM app.secrets',
     writable: [],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],

packages/safegres/__tests__/view-writes.test.ts

@@ -3,6 +3,7 @@ import { computeViewWriteReach } from '../src/checks/role-reach';
 import {
   analyzeViewWrites,
   checkDefinerViewWrite,
+  checkUncheckedViewWrite,
   checkViewRuleBypass
 } from '../src/checks/view-writes';
 import type { RoleAttributes } from '../src/pg/acl';
@@ -37,6 +38,7 @@ function view(partial: Partial<ViewSnapshot> = {}): ViewSnapshot {
     grants: [grant('anon', 'INSERT')],
     definition: 'SELECT id, body FROM app.submissions',
     writable: ['INSERT', 'UPDATE', 'DELETE'],
+    checkOption: 'none',
     insteadOfTriggers: false,
     insteadOf: [],
     rules: [],
@@ -303,6 +305,50 @@ describe('checkDefinerViewWrite (L9)', () => {
   });
 });
 
+describe('checkUncheckedViewWrite (L18)', () => {
+  const FILTERED = "SELECT id, body FROM app.submissions WHERE tenant_id = current_setting('app.tenant')";
+
+  async function check(views: ViewSnapshot[], tables: TableSnapshot[], roles: string[]) {
+    const { unchecked } = await analyzeViewWrites(views, tables);
+    return checkUncheckedViewWrite(unchecked, tables, GRAPH, { roles });
+  }
+
+  it('flags a filtering view a role writes through without WITH CHECK OPTION', async () => {
+    const writable = view({
+      definition: FILTERED,
+      grants: [grant('anon', 'INSERT'), grant('anon', 'UPDATE')]
+    });
+    const findings = await check([writable], [table()], ['anon']);
+    expect(findings.map((f) => f.privilege).sort()).toEqual(['INSERT', 'UPDATE']);
+    expect(findings[0]).toMatchObject({ code: 'L18', severity: 'info', table: 'submissions' });
+    expect(findings[0].message).toContain('no WITH CHECK OPTION');
+  });
+
+  it('stays silent once the view carries a check option', async () => {
+    const checked = view({ definition: FILTERED, checkOption: 'cascaded' });
+    expect(await check([checked], [table()], ['anon'])).toEqual([]);
+  });
+
+  it('stays silent for a writable view with no row filter to escape', async () => {
+    expect(await check([view()], [table()], ['anon'])).toEqual([]);
+  });
+
+  it('suppresses a view whose body cannot be read rather than clearing it', async () => {
+    const { unchecked, suppressed } = await analyzeViewWrites(
+      [view({ definition: 'SELECT ((( FROM app.submissions' })],
+      [table()]
+    );
+    expect(unchecked).toEqual([]);
+    expect(suppressed).not.toEqual([]);
+  });
+
+  it('never recommends revoking a grant', async () => {
+    const [finding] = await check([view({ definition: FILTERED })], [table()], ['anon']);
+    expect(finding.hint).toContain('CHECK OPTION');
+    expect(finding.hint).toContain('Do not revoke');
+  });
+});
+
 describe('checkViewRuleBypass (L10)', () => {
   async function check(views: ViewSnapshot[], tables: TableSnapshot[], roles: string[]) {
     const { ruleDriven } = await analyzeViewWrites(views, tables);

packages/safegres/corpus/cases/46-unchecked-writable-view/case.json

@@ -0,0 +1,41 @@
+{
+  "title": "A writable view's row filter is a read filter: no WITH CHECK OPTION, so writes escape it",
+  "dimension": "security",
+  "exposure": {
+    "schemas": [
+      "c_unchecked_view_write"
+    ],
+    "roles": [
+      "corpus_anon",
+      "corpus_user"
+    ],
+    "anonRoles": [
+      "corpus_anon"
+    ]
+  },
+  "expect": [
+    {
+      "code": "L18",
+      "relation": "c_unchecked_view_write.notes",
+      "note": "the view filters on `tenant` and carries no WITH CHECK OPTION, so corpus_anon's INSERT/UPDATE through it stores rows for any tenant \u2014 rows the view will not then show it"
+    },
+    {
+      "code": "L9",
+      "relation": "c_unchecked_view_write.notes",
+      "note": "the same view is auto-updatable and not security_invoker, so the write reaches `notes` as c_unchecked_write_owner at all \u2014 L9 is *whether* the write lands, L18 is whether the view's own condition constrains it"
+    },
+    {
+      "code": "A3",
+      "relation": "c_unchecked_view_write.notes",
+      "note": "RLS is not FORCEd, which is why the owner's rewritten write is not filtered by the table's own policy either"
+    }
+  ],
+  "forbid": [
+    "L8",
+    "L10",
+    "L11",
+    "L12"
+  ],
+  "worstSeverity": "low",
+  "fix": "Recreate c_unchecked_view_write.tenant_notes WITH LOCAL CHECK OPTION so a row written through it must satisfy the condition it is served under, or put the tenant predicate in an RLS policy's WITH CHECK clause on `notes`, which applies however the row arrives. Do not revoke corpus_anon's INSERT on the view \u2014 that grant is what the API serves."
+}