From 7cc924971d25d4c2ce4ab4a73c48e189b5edb4d0 Mon Sep 17 00:00:00 2001 From: Dawson Toth Date: Tue, 18 Aug 2026 12:55:40 -0400 Subject: [PATCH] fix(security): honor the SQL permission denial processAST computes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `processAST` guarded its permission check with `permissionsCheck && permissionsCheck.length > 0`, but `checkASTPermissions` returns either null or a `PermissionResponseObject` — a class with no `length`. So the test evaluated `undefined > 0`, was always false, and every denial reaching that branch was computed correctly and then discarded. This survived because the branch is normally the second check rather than the first: a direct `sql` call arrives with `permissions_checked` already true from `chooseOperation`, whose own guard (`if (astPermCheck)`) is correct. The branch only executes when something re-parses a statement — a job dispatching its nested `search_operation` — which is exactly the path with no outer gate behind it. Now a bare truthiness test, matching the identical consumer in serverUtilities.ts. Tests drive processAST directly, including the already-checked and allowed paths so this cannot start denying statements that were always permitted; the two negative cases were confirmed to fail against the old guard. Found while reviewing #2173, which does not depend on this: its own gate refuses the export/write combination at the front door. Split out because the defect is pre-existing and affects all SQL authorization, not just that feature. Co-Authored-By: Claude Opus 4.8 --- sqlTranslator/index.ts | 12 ++- .../processASTPermissions.test.js | 86 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 unitTests/sqlTranslator/processASTPermissions.test.js diff --git a/sqlTranslator/index.ts b/sqlTranslator/index.ts index d1799a9fc..a299c0dcb 100644 --- a/sqlTranslator/index.ts +++ b/sqlTranslator/index.ts @@ -136,7 +136,17 @@ export function processAST(jsonMessage: any, parsedSqlObject: any, callback: any // server/serverHelpers/serverHandlers.js and components/mcp/tools/operations.ts). if (!isOperationAuthorizationBypassed() && !parsedSqlObject.permissions_checked) { let permissionsCheck = checkASTPermissions(jsonMessage, parsedSqlObject); - if (permissionsCheck && permissionsCheck.length > 0) { + // Bare truthiness, matching the identical consumer in serverUtilities.ts. + // `checkASTPermissions` returns either null or a PermissionResponseObject, and that class + // has no `length` — so `permissionsCheck.length > 0` evaluated `undefined > 0` and was + // ALWAYS false, discarding a denial that had been computed correctly. No denial reaching + // this branch has ever been honored. + // + // It goes unnoticed because this is normally the second check, not the first: a direct + // `sql` call arrives with permissions_checked already true from chooseOperation, whose own + // guard is correct. This branch only runs when something re-parses — a job dispatching its + // nested search_operation — which is exactly where there is no outer gate behind it. + if (permissionsCheck) { return callback(UNAUTHORIZED_RESPONSE, permissionsCheck); } } diff --git a/unitTests/sqlTranslator/processASTPermissions.test.js b/unitTests/sqlTranslator/processASTPermissions.test.js new file mode 100644 index 000000000..5803efe43 --- /dev/null +++ b/unitTests/sqlTranslator/processASTPermissions.test.js @@ -0,0 +1,86 @@ +'use strict'; + +// processAST is the SQL permission check of last resort. It normally runs as the *second* check — a +// direct `sql` call arrives with permissions_checked already true, set by chooseOperation — so its +// own guard being dead went unnoticed. The branch only executes when something re-parses a statement, +// which is where there is no outer gate behind it. +// +// The guard was `permissionsCheck && permissionsCheck.length > 0`, but checkASTPermissions returns +// null or a PermissionResponseObject, which has no `length`. `undefined > 0` is false, so every +// denial reaching this branch was computed and then discarded. + +const assert = require('node:assert'); +const testUtils = require('../testUtils.js'); +testUtils.preTestPrep(); + +const sql = require('#src/sqlTranslator/index'); + +/** Runs processAST and resolves with what it handed the callback. */ +function runProcessAST(jsonMessage) { + const parsed = sql.convertSQLToAST(jsonMessage.sql); + // Resolved rather than asserted inside: processAST wraps its body in try/catch, so an assertion + // thrown in the callback would be swallowed and re-reported as a second invocation. + return new Promise((resolve) => { + sql.processAST(jsonMessage, parsed, (error, results) => resolve({ error, results })); + }); +} + +function userWithRole(permission) { + return { username: 'restricted', role: { role: 'r', permission } }; +} + +describe('processAST acts on the permission denial it computes', () => { + // A role with no table permissions at all cannot read data.dog, so verifyPermsAST returns a + // PermissionResponseObject. Before this fix it was discarded and the statement ran. + it('refuses a statement the permission check denied', async () => { + const { error, results } = await runProcessAST({ + operation: 'sql', + sql: 'SELECT * FROM data.dog', + hdb_user: userWithRole({ super_user: false }), + }); + + assert.strictEqual(error, 403, 'a denied statement must come back unauthorized'); + assert.ok(results?.unauthorized_access, 'expected the permission response, not a result set'); + }); + + // The denial object is what the caller renders, so it has to survive intact rather than being + // coerced into a bare status. + it('passes the permission response through to the caller', async () => { + const { results } = await runProcessAST({ + operation: 'sql', + sql: 'DELETE FROM data.dog', + hdb_user: userWithRole({ super_user: false }), + }); + + assert.ok(results.error, 'expected the response object to carry its error message'); + }); + + // Guards the other direction: this must not start denying statements that were always allowed. + // A super_user returns null from verifyPermsAST, so the branch falls through as before. + it('does not interfere when the permission check allows the statement', async () => { + const { error } = await runProcessAST({ + operation: 'sql', + sql: 'SELECT * FROM data.dog', + hdb_user: userWithRole({ super_user: true }), + }); + + assert.notStrictEqual(error, 403, 'an authorized statement must not be refused'); + }); + + // Already-checked statements skip the branch entirely — chooseOperation has done the work and + // set the flag, and re-checking here would double the cost of every SQL call. + it('skips the check when permissions were already verified', async () => { + const parsed = sql.convertSQLToAST('SELECT * FROM data.dog'); + parsed.permissions_checked = true; + + const { error } = await new Promise((resolve) => { + sql.processAST( + { operation: 'sql', sql: 'SELECT * FROM data.dog', hdb_user: userWithRole({ super_user: false }) }, + parsed, + (err, results) => resolve({ error: err, results }) + ); + }); + + assert.notStrictEqual(error, 403, 'a pre-checked statement must not be re-denied here'); + }); +});