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

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);
 			}
 		}

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');
+	});
+});