feat: enforce tenant scoping on sqlc queries with sqlclint - #5061
Draft
disintegrator wants to merge 1 commit into
Draft
feat: enforce tenant scoping on sqlc queries with sqlclint#5061disintegrator wants to merge 1 commit into
disintegrator wants to merge 1 commit into
Conversation
The postgresql skill has required every query to be scoped to a tenant for a long time, but nothing checked it. The invariant survived on reviewer attention and hand-written prose, which is a weak control for the failure it prevents: a query addressable by row id alone lets any caller who learns or guesses an id read or write another tenant's data. Reviewing this by eye does not scale to 1,306 queries, and the cases most likely to be missed are the ones least likely to look wrong. A query that binds organization_id on a project-scoped table reads as scoped and crosses every project in the org. One that binds a nullable project_id looks equivalent to a non-nullable bind at the Go call site and silently drops rows. Neither is visible without resolving the query against the schema, which is what sqlclint does. The check parses with libpg_query, the same grammar sqlc uses, rather than matching text. That is not a purity argument: a tenancy bound is equally real in a WHERE clause, a JOIN condition, an EXISTS subquery or an UPDATE ... FROM, and a text matcher either rejects the correct SQL that scopes a child through its parent or misses the target table of every mutation, which Postgres holds in a typed field rather than a node union. Existing violations are grandfathered rather than fixed here. Each entry is pinned to a hash of the query body, so the file is a ratchet and not a suppression list: editing a grandfathered query re-raises it, and there is no way to widen the debt without the diff showing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Contributor
|
|
||||||||||||||||
Contributor
|
|
||||||||||||||||
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.
Adds
sqlclint, a linter that checks every production sqlc query is bounded to a tenant, and gates it in CI. Draft: the tooling is complete and verified; the audit of existing violations is barely started.Why
.agents/skills/postgresql/SKILL.mdhas required every query to be scoped to a tenant for a long time, but nothing checked it. The invariant survived on reviewer attention and hand-written prose, which is a weak control for what it prevents: a query addressable by row id alone lets any caller who learns or guesses an id read or write another tenant's data.Reviewing 1,306 queries by eye does not scale, and the cases most likely to be missed are the ones least likely to look wrong:
organization_idon a table with a non-nullableproject_idreads as scoped in review and crosses every project in the org.project_idlooks identical to a non-nullable bind at the Go call site (both are*uuid.UUID) and silently drops rows whose project isNULL.Neither is visible without resolving the query against the schema.
How it decides
Per table, in order:
project_idisNOT NULL→ requireproject_id.organization_idisNOT NULL→ requireorganization_id.A table with neither column inherits its parents' requirement through its foreign keys. A table that reaches no tenancy-bearing table is global and requires nothing. Across
schema.sql: 171 tables — 83 requireproject_id, 43organization_id, 4 either, 41 inherit or are global.sqlc.nargnever satisfies a requirement. A nullable tenancy parameter either matches nothing or, in the(@x IS NULL OR col = @x)idiom, removes the boundary entirely whenNULLis passed.Parsing, not matching
Queries are parsed with
wasilibs/go-pgquery, a pure-Go (wazero/WASM, no cgo) build of libpg_query — the same parser sqlc itself uses, at the same version. sqlc's own wrapper is behind Go'sinternal/rule, but the grammar underneath it is importable, sosqlclintsees exactly what sqlc sees:@nameas a unary@expression,sqlc.arg/sqlc.nargas schema-qualified function calls,$1as a parameter reference. No preprocessing.This is not a purity argument. Three things a text matcher gets wrong, all found by walking the real AST:
UPDATE/DELETE/INSERThold their target table in a typedrelationfield, not aNodeunion — a walk keyed on the node wrapper misses the table every mutation writes to.FOR UPDATE OF trepresents the aliastas aRangeVar; 9 queries looked like unknown tables until the walk stopped descending into locking clauses.INSERT ... SELECTsupplies columns positionally from the target list; 6 queries bindingproject_idthere read as unscoped until that was handled.A bound also counts wherever it legitimately appears —
WHERE,JOIN ... ON,EXISTS,UPDATE ... FROM, a CTE, anINSERTcolumn list. Requiring a particular position would reject the correct SQL that scopes a child table through its parent.Rule catalog
18 embedded markdown documents, 10 diagnostics and 8 exemption categories, with uniform frontmatter and a fixed heading sequence per kind:
Diagnostics print
see: sqlclint rule <id>. A conformance test checks both directions, so a diagnostic cannot exist without a document and a document cannot be orphaned. The exemption vocabulary is exactly the set of exemption documents, so a category cannot be invented at a call site.Queries that genuinely cannot be bounded carry an annotation naming a category and a reason:
Side effect worth knowing: sqlc carries these into the generated Go doc comment, so the justification is visible at every call site.
The ratchet
.sqlclintignoregrandfathers the 304 violations that predate the check. It is generated debt, not approval. Each entry is pinned to a hash of the query body, so editing a grandfathered query re-raises it — an exemption cannot follow a name while the SQL underneath it changes. Stale entries also fail, so the file can only shrink. Structural problems (bad category, missing reason, unparseable query, unresolvable table) are never grandfathered.What is NOT done
The audit: 2 of 116. Only
SetSyncScheduleDisabledandRetrySyncScheduleare annotated, after tracing their callers.I stopped deliberately rather than bulk-annotating. Both sit in a package full of background pollers and look like
background-sweep, but the first is documented as recording "a user's explicit pause" — it is request-reachable. It is safe only because the handler resolves the config id through anorganization_id-scoped lookup, making itparent-authorized. Labelling that clusterbackground-sweepwould have documented a request-reachable unscoped write as a worker job. Each of the remaining 114 needs the same per-query caller trace;.sqlclintignoreis the worklist.Findings worth a look now
CloneDeploymentToolFunctions(server/internal/deployments/queries.sql) clones tool definitions keyed only ondeployment_id, copyingproject_idfrom the source row. A caller passing another project's deployment id clones into that project. This reads as a real IDOR, not an exemption.project_id(ai_integration_configsand similar), where the natural key genuinely looks like(organization_id, provider). That is a schema-versus-rule question — either the queries should bindproject_idor the column should be reconsidered — and I did not want to settle it by inventing a ninth exemption category.Known limitation
The check proves a tenancy predicate exists and is parameterized. It cannot prove the parameter carries the authenticated tenant rather than an attacker-supplied one from the request payload. Closing that needs Go-side taint analysis in
glint/trackingcontextvalues.AuthContextinto repo call arguments. Out of scope here, and stated in the skill so the lint is not read as a complete IDOR guarantee.Verification
mise run lint:queries→ clean;--write-ignore-fileregenerates.modified-ignored-query; an invalid category lists the valid ids; stale entries fail; annotate → regenerate drops the count.JOIN ... ON-only and ~102EXISTS/subquery-scoped queries all pass.sqlclint.yaml-only run produce identical output.mise run test:ci(73 tests, now covering./sqlclint/...),go build ./...,go mod tidyclean.mise run gen:sqlc-serverregenerates with only the two annotation comments changing.CI runs
lint-querieson thequeriespath filter and always in the merge queue, mirroringlint-migrations.🤖 Generated with Claude Code
Summary by cubic
Add
sqlclint, an AST-based linter that enforces tenant scoping on all productionsqlcqueries and gates it in CI. Existing violations are tracked in a ratcheting.sqlclintignore, and genuine exceptions require explicit annotations with reasons.New Features
project_idif NOT NULL, elseorganization_id, else either; inherits via FKs; global tables allowed.WHERE,JOIN ... ON,EXISTS, CTEs,UPDATE ... FROM, andINSERT ... SELECT.sqlc.nargfor tenancy binds and wrong-tenant columns.-- sqlclint:ignore <category> -- <reason>..sqlclintignorepinned to query-body hash; modified or stale entries fail; regenerate withmise run lint:queries -- --write-ignore-file.sqlclint run(usessqlclint.yaml) andsqlclint rules; taskmise run lint:queries; CI joblint-querieswithqueriesfilter and always in merge queue.SKILL.md; adds twoparent-authorizedannotations inaiintegrations.Migration
-- sqlclint:ignore <category> -- <reason>; choose from documented categories..sqlclintignore; runmise run lint:queries -- --write-ignore-fileafter fixes or annotations.mise run lint:querieslocally before pushing; CI blocks merges on failures.Written for commit 9deb2a1. Summary will update on new commits.