fix(security): authorize schema-unqualified SQL against the table the engine resolves

[kriszyp]

Fixes the authorization bypass tracked in GHSA-5c29-q62v-jrwf.

The bug

The SQL authorization layer derived the affected schema/table set from the AST's databaseid. When a statement omitted the schema qualifier that field was empty, so addSchemaTableToMap recorded nothing and verifyPermsAST ran hasPermissions against an empty map. hasPermissions iterates that map — an empty map authorizes by vacuous truth. The v2 engine's binder then resolved the same bare name to a concrete database via pickDefaultDatabase and executed against it.

Two independent name resolutions, one of which silently resolved to "nothing".

Scope is wider than the advisory

The advisory describes this as confidentiality-only. Verified against a live instance, with a role holding no grant at all on the target table:

Statement (schema-unqualified)	Result before this PR
SELECT id, ssn FROM UnqualSecret WHERE id='…'	200 — [{"id":"secret-row-1","ssn":"111-22-3333"}]
DELETE FROM UnqualSecret WHERE id='…'	200 — row destroyed
UPDATE UnqualSecret SET ssn='tampered'	200 — row tampered
INSERT INTO UnqualSecret …	200 — row written

So it is integrity and availability too, not just confidentiality. Writes bypass because chooseOperation pre-populates parsed_sql_object, which skips evaluateSQL's "No schema specified" guard on the ops sql path. Bare JOIN targets and SELECT * are affected the same way.

Two advisory claims did not reproduce, and the advisory should be corrected:

• System tables are not reachable. Bare SELECT * FROM hdb_user returns 200 [] — the system database isn't in getDatabases(), so pickDefaultDatabase can't resolve it. No credential material is exposed.
• A bare full-scan SELECT was already refused, but only by accident: it raises EngineUnsupportedError and falls back to legacy, which rejects unqualified names. The indexed-predicate form is the live read vector. Both were unauthorized regardless — the denial came from an engine capability limit, not from authorization.

The fix

Rather than patching either side, remove the divergence.

• sqlEngine/binder/defaultDatabase.ts (new) is the single owner of the default-database rule. The binder's pickDefaultDatabase delegates to it.
• The statement bucket resolves bare FROM/JOIN/write targets through that same rule and writes the result back onto the AST, so authorization and execution share one resolution instead of performing two that can disagree. A name matching zero or several databases is left bare, never guessed.
• verifyPermsAST fails closed per reference. Every table the statement references must appear in the affected-attribute map; any that doesn't is denied. Checked per reference rather than on the map being empty overall — a statement mixing a resolvable table with an unresolvable one has a non-empty map and would otherwise slip through on the strength of the table that did resolve. An empty result still means a calc-only select (SELECT ABS(-12)).

I did not take the advisory's suggested "reject unqualified SELECT targets": bare names are a deliberate v2 feature (pickDefaultDatabase has an explicit ambiguity error), so banning them would paper over the resolution mismatch instead of fixing it.

Behavior changes beyond the bypass

• A derived table (FROM (SELECT …)) and a SELECT … INTO target are now refused for non-super-users. Neither executes today — Harper rejects derived tables with a 500 and ignores INTO without writing — so nothing that worked stops working, but they are no longer authorized-by-omission if either is ever implemented.
• An unqualified full-scan SELECT on a permitted table now succeeds instead of returning 500 schema not defined, because the resolved database reaches the legacy path too.
• A statement whose FROM resolved while a JOIN didn't previously crashed on an undefined schema entry in getSelectAttributes; it now returns a clean 403. Guarded at all three affectedAttributes.get(schema) dereferences.

Known limitation

References reachable only from a WHERE subquery are still not enumerated. Not exploitable today — those statements fail with Circular reference before executing — but the enumeration is FROM/JOIN/write-target only, so a future subquery implementation must extend getTableTargets. Recorded as an invariant in AGENTS.md.

Testing

• integrationTests/security/sql-unqualified-table-authz.test.ts — 14 cases: read (indexed and full-scan), system table, DELETE/UPDATE/INSERT each with a follow-up check that the table was not mutated, JOIN, SELECT *, mixed resolvable/unresolvable FROM, derived table, SELECT INTO, plus positive cases proving legitimate bare references still resolve and return rows.
• 9 of the first 11 cases fail on the unfixed tree — I stashed the fix and re-ran to confirm none are vacuous.
• unitTests/sqlTranslator/unqualified-table-resolution.test.js — 16 cases over resolution, ambiguity, Object.prototype names, and every branch of getUnauthorizedTableRefs.
• Full local runs: 444 unit passing, 81 integration passing (security + SQL engine + auth + ttl), lint clean.

Review

Cross-model review found the first cut's fail-closed guard was too coarse (global instead of per-reference), plus the Object.prototype table-name match, the per-reference registry load, and the duplicate require — all fixed in b8a665ea0. Every flagged construct was checked empirically before fixing; the "subquery bypass" reported as a blocker is a latent gap, not a live bypass, since those statements error out before executing.

Coverage caveat: the Codex leg timed out, and that reviewer fell back to its own analysis — same model family as the author, so it does not count as cross-model coverage. Gemini (agy) is the only genuine outside-model pass here, and it is the one that caught the per-reference gap. A second outside-model pass before merge would be worthwhile.

Generated with Claude Opus 5.

Docs

No companion documentation PR needed. reference/operations-api/sql.md already states that tables are referenced as database.table, and already lists UNION as unsupported (✗). The constructs this PR now refuses for non-super-users — UNION branches, WHERE/EXISTS subqueries, derived tables and derived JOINs, INSERT ... SELECT, and SELECT ... INTO — are either documented as unsupported or undocumented, and none of them execute successfully today, so no documented behavior changes. Bare table names remain undocumented; the docs continue to steer users to the qualified form.

Follow-up filed

#1965 — INSERT INTO ... SELECT never returns (request hangs). Found while validating this fix. Refusing the construct here removes the vector for non-super-users, but super-users return before the authorization check and still reach the hang, so it needs its own fix.

[gemini-code-assist]

Code Review

This pull request addresses a security vulnerability (GHSA-5c29-q62v-jrwf) where schema-unqualified SQL statements could bypass authorization checks. It centralizes default-database resolution in a new module, resolves unqualified table references in place during AST interpretation, and fails closed on any unresolvable or unauthorized table references. Additionally, it includes comprehensive integration and unit tests to verify the fix. The reviewer's feedback suggests guarding the "databases" object in "bindTableRef" before calling "Object.hasOwn" to prevent a potential "TypeError" if the object is null or undefined.

[claude]

Reviewed the commit pushed since the last review (9e9ca92 — ORDER BY schema guard matching sibling collectors); no blockers found.

[kriszyp]

Review round 2 — what changed, and one finding I could not reproduce

An earlier review pass raised three issues on threads that no longer exist (the pending review was replaced before I could reply). Recording the outcomes here so they are not lost, since two of them changed the code materially.

Nested and compound queries were invisible to permission checking — fixed in df39ac25c. The attribute collectors only walk a statement's top-level FROM, JOINs and write target, so a table reachable only from a WHERE subquery (queries), an EXISTS subquery (exists), a UNION branch (union/unionall) or an INSERT's source SELECT (select) was never recorded, and so never checked. All are now reported as opaque and refused.

I marked them opaque rather than recursing into them, because a nested reference would still never be recorded in the affected-attribute map — descending to name it would not make it checkable, only refusable, which opaque already achieves.

This turned up a hang. INSERT INTO t SELECT ... FROM src does not just get rejected downstream — it never responds. A bounded probe gets no response in 8s; an unbounded suite run hung past 900s and had to be killed. Authorization previously let it through, because the source table is invisible and only the target's insert grant is checked, so any user holding insert on a single table could wedge a request. Refusing the construct closes that for non-super-users. The underlying hang is a separate defect — it still affects super-users, who return before this check — and I am filing it on its own.

SELECT ... INTO could be authorized by a read — fixed in df39ac25c. The INTO target was treated as a normal named reference, so it satisfied the membership test against the entry the FROM collector had just created. SELECT * INTO data.UnqualPublic FROM data.UnqualPublic returns 200 today. INTO is now unconditionally opaque, which also makes the code match what its comment already claimed.

An attribute-permission bypass on unaliased cross-database joins — could not reproduce. I built the reported scenario exactly: two databases, no aliases anywhere, vault.UnqualVault granted table-level read with ssn.read = false. Both the join form and the direct form are denied with and without the proposed fix:

403 {"error":"...not authorized...","invalid_schema_items":["Attribute 'ssn' does not exist on 'vault.UnqualVault'"]}

The attribute check is reached by some other path and ssn never comes back. I applied the underlying mapping change anyway — resolving a column's database through tableToSchemaLookup, which is keyed by alias and base table name, instead of falling back to the FROM table's database — because that fallback is genuinely wrong once bare names resolve to different databases. But it lands as correctness hardening, not a proven bypass fix, and its test is labelled a characterization guard rather than a regression guard, since it passes either way. If anyone has a variant that does reproduce, I would like to see it.

On verification

Every guard added in this round was checked by reverting it and re-running, so none of the new tests are vacuous:

• WHERE-subquery, EXISTS and UNION return 500 instead of 403 without the nested-field detection.
• With the INTO and nested-source guards removed, the suite wedges entirely rather than failing.
• The derived-JOIN case returns 500 Cannot set properties of undefined (setting 'as') without a32674547.
• One test was vacuous and I fixed it: the INSERT-source case was being denied by a missing insert grant on the target rather than by its source table, so the role now holds that grant and the source is the only thing under test.

Local runs: 429 unit passing, 632 integration passing (security, SQL engine, northwind), lint clean.

Coverage caveat

The Codex leg of the local cross-model review timed out and that reviewer fell back to its own analysis — same model family as the author, so it did not count as outside coverage. The findings above came from the repo's own CI review bots and from Gemini. Worth a human pass on the authorization logic specifically.

Generated with Claude Opus 5.

[cb1kenobi]

Not sure I understand why an exported function begins with an underscore. It's fairly common to prefix local variables or unused variables with an underscore, but an exported function is public and should not have an underscore. This looks like legacy code, but while you're touching it, might as well fix it.