fix(security): authorize the invoked operation against the authenticated principal

[cb1kenobi]

Fixes operation authorization in chooseOperation: authorize the invoked operation against the authenticated principal, and stop a caller-supplied search_operation from standing in as the permission subject.

What this fixes

verifyPerms reads both halves of the permission question off the object it's handed — the principal from hdb_user and the tables from schema/database/table/records. chooseOperation used to hand it json.search_operation, a caller-supplied field, making both halves body-controlled. Four issues, each with regression cover in integrationTests/security/choose-operation-authz.test.ts:

1. Cross-privilege redirect — verifyPerms ran against json.search_operation ?? json for every operation, but the handler runs against top-level json and only dataLayer/export.ts consumes search_operation. A non-super user could send a privileged top-level operation with a benign search_operation and get authorized against the benign tables. Now search_operation is the permission subject only for export_local/export_to_s3; every other op is checked against top-level json.
2. Principal smuggling — the nested principal was backfilled (if (!hdb_user)), honoring a body-supplied one. Now hdb_user is overwritten from the authenticated top-level principal unconditionally.
3. parsed_sql_object smuggling — the export worker re-reads parsed_sql_object (carrying permissions_checked) off the nested object, so a body-supplied one would execute an AST no check ever saw. It's now deleted at dispatch, forcing a re-parse + re-check in the worker.
4. Silent SQL denial — processAST's guard tested permissionsCheck.length > 0, but a denial is a PermissionResponseObject with no .length, so undefined > 0 was always false and denials executed. Fixed to if (permissionsCheck).

Non-object search_operation on an export op is now a 400 (was a wrapped 500), and the SQL AST check runs additively after verifyPerms rather than as an exclusive branch.

For the reviewer

• server/serverHelpers/serverUtilities.ts (chooseOperation) is the core. The load-bearing invariant: the object passed to verifyPerms must be the one the handler actually operates on. DESIGN.md (new chooseOperation section) documents the three rules and why each is required.
• export_local table check is currently unreachable — both export ops are requires_su and every path returns before the nested table check (super_user early-returns; a role granted the op via operations returns at gate 2; anything else is refused earlier). So the nested-table substitution is inert today. This is intentional for this patch.

Behavior change (release note)

Non-super users can no longer invoke export_local / export_to_s3 via a SQL search_operation. These operations are requires_su, and NoSQL export already enforced that ("Operation 'export_local' is restricted to 'super_user' roles"). SQL-based export previously took the SQL-only branch and skipped verifyPerms, so a non-super user could invoke the privileged export by wrapping it in a SQL search — governed only by table read perms. Routing export through verifyPerms closes that bypass and brings SQL export to parity with NoSQL export. Any non-super role that relied on SQL-export-without-export_local will now receive a 403. Three northwind export tests that encoded the old bypass were updated to assert the requires_su denial (they now mirror the existing NoSQL export case; they could be consolidated).

Deferred gaps (tracked, not fixed here)

Two pre-existing authorization gaps are left open deliberately — closing either changes authorization outcomes for existing role configurations, so each needs its own change + release note. Both are documented in DESIGN.md and pinned by regression tests so there's a test to flip:

• export_local/export_to_s3 via operations allowlist bypasses table-level grants (non-SQL nested search) #2215 — a role granted export_local/export_to_s3 via operations can export tables it holds no grant on (non-SQL nested search reaches the handler with no table check).
• Direct sql operation bypasses the operations allowlist (gate 1) #2216 — a direct sql operation bypasses the operations allowlist (gate 1); authorized only by the AST table check.

Verification

• integrationTests/security/choose-operation-authz.test.ts — pins the denial shapes and the current boundaries (principal override, nested-shape 400, forged-AST rejection, and the two deferred-gap boundaries NESTED-NOSQL / gate-1).
• Deep-review, multipass (2 independent runs × auth + SQL/AST lenses). Confirmed the fix closes the cross-privilege redirect, principal smuggling, AST smuggling, and the processAST denial-drop, with no other .length-style denial-drop anywhere in the tree. The multipass independently rediscovered the two deferred gaps above (now ticketed). One low cleanup nit surfaced: serverUtilities.ts:298 sets a top-level parsed_sql_object that's inert for the export path.

Review coverage

• Deep-review, multipass (Claude — Harper-domain + SQL/AST lenses, 2 independent runs): confirmed the fix closes the cross-privilege redirect, principal smuggling, AST smuggling, and the processAST denial-drop; no other .length-style denial-drop in the tree.
• Cross-model, Codex (graded, opposite-family): verdict COMMENTS, no production blockers. Two findings, both in the test file (below).
• Degraded legs (noted for honesty): Gemini agy failed (stochastic print-mode hang; CLI retries + 2 manual attempts all timed out) — no perf/maintainability lens this run; cursor-composer not authenticated locally; the CLI's Claude domain-adjudicator leg couldn't run (claude not on PATH) — covered instead by the separate multipass deep-review above.

Unresolved review findings (test-only, non-blocking)

• Codex, minor — integrationTests/security/choose-operation-authz.test.ts: no end-to-end case proves the export worker's nested-SQL recheck can deny an unauthorized query. The allowed super-user path reaching COMPLETE is covered, and unitTests/sqlTranslator/processAST.test.js proves the object-shaped denial stops routing, but the full dispatch → principal-propagation → reparse → denial path isn't exercised together. Worth adding a non-super-user unauthorized-export-SQL case that asserts denial.
• Codex, nit — same file: trim added comments that narrate tests / restate identifiers / address the reviewer (the suite catalog + run-command block at the top, and identifier restatements).

[gemini-code-assist]

Code Review

This pull request strengthens the security and authorization model of the operations API, specifically within chooseOperation. It ensures that the authenticated principal is always enforced and prevents body-supplied parameters (like nested hdb_user or parsed_sql_object) from bypassing authorization checks. Additionally, it fixes a critical bug in processAST where permission denials (which are objects lacking a length property) were not correctly blocked. The review feedback suggests a minor optimization in chooseOperation to avoid polluting the top-level request object with parsed_sql_object when executing nested SQL searches, as it is only consumed by direct SQL operations.

[claude]

Reviewed; no blockers found.

[dawsontoth]

We'll have some overlap with #2202 on your point 4 -- more details to come shortly!

[kriszyp]

Nice, glad to be reviewing the auth on SQL, looks like some good improvements. It looks like there are a few things to address (and maybe needs a rebase).

_{🤖 Reviewed with Codex}

[cb1kenobi]

Rebased onto main and worked through the review. Since this branch was opened, main independently landed several overlapping pieces (#2202 SQL denial fix, #2176/#2260 scoped tokens + operations allowlist), so the reconciliation keeps main's structure and layers this PR's hardening on top rather than re-introducing the older shape.

What changed in the reconciliation

• Principal from authentication — a nested hdb_user is overwritten, never backfilled.
• search_operation as subject only for exports — gated to export_local/export_to_s3, must be an object, and must name a supported export operation (search_by_value/search_by_hash/search_by_conditions/sql); anything else (including {}) is a request-time 400.
• Outer + nested authorized separately — the export invocation is authorized first, then the nested search is authorized additively against its real handler and the authenticated principal, so a role granted export_local but not the underlying read is denied. NESTED-NOSQL is flipped to a denial; POSITIVE-NOSQL covers the permitted case.
• SQL export routed through verifyPerms — so its requires_su gate applies exactly as on the non-SQL path (main's SQL branch checked only allowlist + AST, so a non-super user could export via a SQL search_operation). The northwind SQL-export cases now assert the requires_su denial.
• Gemini's suggestion (only assign json.parsed_sql_object on the direct-SQL path) is also implemented.

Testing — npm run build clean; the authz/SQL unit suites pass (operation_authorization, tokenOperationScope, processAST, serverUtilities = 200+ specs green). I could not run the integration suites locally (this machine lacks the loopback alias pool), so choose-operation-authz.test.ts and the northwind changes will get their first real run in CI — I did validate the two load-bearing behaviors (SQL export → requires_su denial for a non-super user; export_local-granted role without read → nested denial) directly against the built verifyPerms.

One open question flagged inline on the nested-read thread: the nested check re-applies gate 1, making non-SQL export slightly stricter than SQL export for roles with a restrictive operations allowlist. Fail-closed, but happy to converge them if you'd prefer.

_{🤖 Generated by Barber AI (Opus 4.8)}