feat(safegres): L9/L10 — write reachability through definer views and rewrite rules - #1612
Merged
Conversation
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
L8 models what a view lets an untrusted role read. This adds the two write paths that escape it, both verified against PostgreSQL 18 in a scratch schema rather than inferred from the docs:
INSERT/UPDATE/DELETEon the view onto the base relation; on a non-security_invokerview that rewritten command is permission-checked against the owner.anonholding nothing butINSERTon the view writes a table it has no ACL row on. The view body says which relation; onlypg_relation_is_updatable(oid, true)says the write lands there at all._RETURNrule is invisible topg_get_viewdef, soON INSERT TO v DO INSTEAD INSERT INTO auditreaches a relation the definition never names. The probe result that matters:security_invokerdoes not govern rule actions — an invoker view with such a rule still writesauditas the view owner. L10 therefore fires on invoker views too, which L9 deliberately does not.Both are
info/ score-neutral, same posture as L7/L8. Considered severity once proven: L9 ishigh(an anonymous write to a table no grant names it on, under an owner that is frequently RLS-exempt — strictly worse than the L8 read it mirrors) and L10medium(same escalation, but rules are rare and overwhelminglyDO INSTEAD NOTHING). They shipinfobecause they are new and the write path has more ways to be wrong than the read one, not because the finding is informational.Catalog
introspectViewsnow also returns, per view:Model
One new edge on
RoleReachEdge, and the write projection alongsidecomputeViewReach:viaandprivilegecome apart for rules:ON DELETE ... DO INSTEAD UPDATE auditis reached withDELETEon the view and costsUPDATEonaudit.definer-view.tsexports its relation resolver (buildRelationIndex/resolveRelation) so the write analysis resolves references exactly as L8 does rather than growing a second, subtly different resolver.extractAccess()is new incallgraph/extract.ts: same parse asextractQuery, but tagging eachRangeVarwith the command that reaches it (InsertStmt/UpdateStmt/DeleteStmtrelation → that privilege, everything else →SELECT), which is what reading a rule action requires.What it refuses to say
Every one of these suppresses rather than guesses, and none of the remedies is a revoke — the grant on the view is what the API serves:
INSTEAD OFtriggers → the write lands wherever a function body puts it; not followed, so suppressed.DO INSTEAD NOTHING→ reaches nothing, reports nothing. This is the commonest rule in the wild (~120 of them in constructive-db) and the correct answer is silence.ON ... TO <view>relation in a ruledef is the trigger, not a target, and is dropped.Corpus
Four cases, two of them negatives (
27/29fire,28/30must not):INSERTto anonsecurity_invoker = trueDO INSTEAD INSERT INTO audit_logDO INSTEAD NOTHINGrulesVerification
pnpm build,pnpm lint,pnpm testgreen inpackages/safegres— 368 tests, 31 suites, including 26 new unit tests and the two new introspection assertions against a live PG. Score and fingerprint tests unchanged.Ran the full
bin/safegres-check.cjsagainst constructive-db with this build: zero L9 and zero L10 findings, score unchanged at 100 (A+) / 91.2 (A) — the ~120 rules there are allDO INSTEAD NOTHINGon read-only views and the managed views aresecurity_invoker, so the new rules stay silent on a real schema.Touched shared files (minimal, additive)
src/commands/audit.ts— one import, the L9/L10 role/enablement lines, one dispatch block.src/index.ts— one export block.src/config/presets.ts,src/rules/registry.ts— the two new rule entries.Link to Devin session: https://app.devin.ai/sessions/f340c08768814b278a179aea7994f924
Requested by: @pyramation