fix(security): honor the SQL permission denial processAST computes

[dawsontoth]

Split out of #2173 at a reviewer's request — the defect is pre-existing and affects all SQL authorization, not just that feature.

The bug

sqlTranslator/index.ts guarded its permission check with:

let permissionsCheck = checkASTPermissions(jsonMessage, parsedSqlObject);
if (permissionsCheck && permissionsCheck.length > 0) {

checkASTPermissions returns either null or a PermissionResponseObject. That class has error, unauthorized_access, and invalid_schema_items — no length. So the condition evaluated undefined > 0, was always false, and the denial was computed correctly and then dropped on the floor.

Why nobody noticed

This branch is normally the second permission check, not the first. A direct sql call reaches processAST with permissions_checked already true, set by chooseOperation — whose own consumer of the same function uses a correct bare truthiness test (server/serverHelpers/serverUtilities.ts). So in the common path the branch never runs.

It runs when something re-parses a statement: a job dispatches its nested search_operation, which carries no parsed_sql_object, so evaluateSQL re-parses with permissions_checked === false. That is precisely the path with no outer gate behind it.

The fix

A bare truthiness test, matching the identical consumer in serverUtilities.ts.

Tests

unitTests/sqlTranslator/processASTPermissions.test.js drives processAST directly rather than asserting that a denial is merely computed — which is how a dead consumer went unnoticed in the first place. Four cases:

• a denied statement comes back 403 with the permission response
• the response object survives intact for the caller to render
• an allowed statement is still allowed (this must not start denying what was always permitted)
• an already-checked statement still skips the branch

The two negative cases were confirmed to fail against the old guard before being kept (2 passing / 2 failing when reverted, 4 passing with the fix).

Scope

No behavior change for any statement that was correctly authorized. The change is that a denial computed on the re-parse path is now acted upon instead of ignored.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request fixes a critical authorization bypass bug in sqlTranslator/index.ts where permission denials were being discarded because the code checked for a .length property on a PermissionResponseObject (which does not exist, causing the check to always evaluate to false). The fix replaces this check with a simple truthiness check on permissionsCheck. Additionally, a comprehensive suite of unit tests has been added in unitTests/sqlTranslator/processASTPermissions.test.js to verify the correct behavior of processAST under various permission scenarios. There are no review comments, and we have no additional feedback to provide as the changes are correct and well-tested.

[claude]

Reviewed; no blockers found.

[dawsontoth]

Moving this to draft. It is a real pre-existing defect and the fix stands, but it touches shared SQL authorization for every caller, not just the OIDC feature, so it needs the full harper-engineering-guidelines treatment on its own — cross-model reviews with the coverage reported in the description — rather than inheriting #2173's. That will be picked up separately.

Nothing in #2173 depends on this landing: its own gate refuses the export/write combination at the front door, and it no longer attempts to carry a job's operation into the branch this fixes.

One thing for whoever picks this up: making the processAST branch live means a job's SQL will be authorized as sql rather than as its own operation (export_local), so a token scoped only to the job operation would be denied by its own job. Carrying the real operation on the request body was tried in #2173 and reverted — on the direct-SQL path that body is client-supplied and this check is the only gate, so any property it consults is forgeable. That carrier needs to be something a client cannot set.

🤖 Generated with Claude Code

[dawsontoth]

Ordering note for whoever picks this up — #2173 now carries a test that will go red when this lands, deliberately.

Making the processAST branch live means a job's SQL is authorized as sql rather than as its own operation, because checkASTPermissions falls back to jsonMessage.operation and at that call site jsonMessage is the nested search_operation. So a token scoped to export_local would be denied by its own export job. Verified by applying this PR's diff on top of #2173 and rebuilding.

The failing test is admits an in-scope export job through the path export.ts actually dispatches in unitTests/security/tokenOperationScope.test.js. The fix is not to relax the scope check or delete the test — it is to give the job's real operation a carrier a client cannot set. A request-body property is not one: that was tried in #2173 and reverted, because on the direct-SQL path the body is client-supplied and that check is the only gate, so any property it consults is forgeable.

The two PRs can merge in either order; whichever is second goes red rather than silently shipping an export-scoped token that 403s on its own export.

🤖 Generated with Claude Code