diff --git a/packages/ai/src/utils/sql-permissions/permission-validator.test.ts b/packages/ai/src/utils/sql-permissions/permission-validator.test.ts index 5a199e5d4..a2f4391a7 100644 --- a/packages/ai/src/utils/sql-permissions/permission-validator.test.ts +++ b/packages/ai/src/utils/sql-permissions/permission-validator.test.ts @@ -255,7 +255,8 @@ describe('Permission Validator', () => { expect(result).toEqual({ isAuthorized: false, unauthorizedTables: [], - error: 'Permission validation failed: Database connection failed', + error: + 'Permission validation failed: Database connection failed. Please verify your SQL query syntax and ensure you have access to the requested resources.', }); }); @@ -270,7 +271,7 @@ describe('Permission Validator', () => { const result = await validateSqlPermissions('INVALID SQL SYNTAX HERE', 'user123'); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain('Failed to parse SQL'); + expect(result.error).toContain('Failed to parse SQL query'); }); it('should reject INSERT statements', async () => { @@ -295,7 +296,8 @@ describe('Permission Validator', () => { ); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain("Query type 'insert' is not allowed"); + expect(result.error).toContain("Query type 'INSERT' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of INSERT'); expect(result.unauthorizedTables).toHaveLength(0); }); @@ -321,7 +323,8 @@ describe('Permission Validator', () => { ); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain("Query type 'update' is not allowed"); + expect(result.error).toContain("Query type 'UPDATE' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of UPDATE'); }); it('should reject DELETE statements', async () => { @@ -346,21 +349,24 @@ describe('Permission Validator', () => { ); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain("Query type 'delete' is not allowed"); + expect(result.error).toContain("Query type 'DELETE' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of DELETE'); }); it('should reject CREATE TABLE statements', async () => { const result = await validateSqlPermissions('CREATE TABLE new_table (id INT)', 'user123'); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain("Query type 'create' is not allowed"); + expect(result.error).toContain("Query type 'CREATE' is not allowed"); + expect(result.error).toContain('DDL operations like CREATE are not permitted'); }); it('should reject DROP TABLE statements', async () => { const result = await validateSqlPermissions('DROP TABLE users', 'user123'); expect(result.isAuthorized).toBe(false); - expect(result.error).toContain("Query type 'drop' is not allowed"); + expect(result.error).toContain("Query type 'DROP' is not allowed"); + expect(result.error).toContain('DDL operations like DROP are not permitted'); }); it('should handle multiple datasets with overlapping tables', async () => { @@ -1219,13 +1225,13 @@ describe('Permission Validator', () => { it('should handle single table', () => { expect(createPermissionErrorMessage(['public.users'])).toBe( - 'Insufficient permissions: You do not have access to table: public.users' + 'Insufficient permissions: You do not have access to table: public.users. Please request access to this table or use a different table that you have permissions for.' ); }); it('should handle multiple tables', () => { expect(createPermissionErrorMessage(['public.users', 'sales.orders'])).toBe( - 'Insufficient permissions: You do not have access to the following tables: public.users, sales.orders' + 'Insufficient permissions: You do not have access to the following tables: public.users, sales.orders. Please request access to these tables or modify your query to use only authorized tables.' ); }); @@ -1247,7 +1253,9 @@ describe('Permission Validator', () => { ['private.secrets'], [{ table: 'users', column: 'password' }] ); - expect(message).toContain('You do not have access to table: private.secrets'); + expect(message).toContain( + 'You do not have access to table: private.secrets. Please request access' + ); expect(message).toContain('Unauthorized column access'); expect(message).toContain('password'); }); diff --git a/packages/ai/src/utils/sql-permissions/permission-validator.ts b/packages/ai/src/utils/sql-permissions/permission-validator.ts index 5334add56..15fd4e34a 100644 --- a/packages/ai/src/utils/sql-permissions/permission-validator.ts +++ b/packages/ai/src/utils/sql-permissions/permission-validator.ts @@ -39,7 +39,9 @@ export async function validateSqlPermissions( return { isAuthorized: false, unauthorizedTables: [], - error: readOnlyCheck.error || 'Only read-only queries are allowed', + error: + readOnlyCheck.error || + 'Only SELECT statements are allowed for read-only access. Please modify your query to use SELECT instead of write operations like INSERT, UPDATE, DELETE, or DDL statements.', }; } @@ -47,7 +49,9 @@ export async function validateSqlPermissions( // Store the wildcard error but continue to validate columns to provide comprehensive feedback let wildcardError: string | undefined; if (!wildcardCheck.isValid) { - wildcardError = wildcardCheck.error || 'Wildcard usage on physical tables is not allowed'; + wildcardError = + wildcardCheck.error || + 'SELECT * is not allowed on physical tables. Please explicitly list the column names you need (e.g., SELECT id, name, email FROM users). This helps prevent unintended data exposure and improves query performance.'; } // Extract physical tables from SQL @@ -180,10 +184,29 @@ export async function validateSqlPermissions( return result; } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + + // Provide more specific guidance based on the error + if (errorMessage.includes('parse')) { + return { + isAuthorized: false, + unauthorizedTables: [], + error: `Failed to validate SQL permissions due to parsing error: ${errorMessage}. Please check your SQL syntax and ensure it's valid.`, + }; + } + + if (errorMessage.includes('permission')) { + return { + isAuthorized: false, + unauthorizedTables: [], + error: `Permission check failed: ${errorMessage}. Please ensure you have the necessary access rights for the requested tables and columns.`, + }; + } + return { isAuthorized: false, unauthorizedTables: [], - error: `Permission validation failed: ${error instanceof Error ? error.message : 'Unknown error'}`, + error: `Permission validation failed: ${errorMessage}. Please verify your SQL query syntax and ensure you have access to the requested resources.`, }; } } @@ -197,13 +220,17 @@ export function createPermissionErrorMessage( ): string { const messages: string[] = []; - // Handle unauthorized tables + // Handle unauthorized tables with actionable guidance if (unauthorizedTables.length > 0) { const tableList = unauthorizedTables.join(', '); if (unauthorizedTables.length === 1) { - messages.push(`You do not have access to table: ${tableList}`); + messages.push( + `You do not have access to table: ${tableList}. Please request access to this table or use a different table that you have permissions for.` + ); } else { - messages.push(`You do not have access to the following tables: ${tableList}`); + messages.push( + `You do not have access to the following tables: ${tableList}. Please request access to these tables or modify your query to use only authorized tables.` + ); } } @@ -225,14 +252,18 @@ export function createPermissionErrorMessage( const columnMessages: string[] = []; for (const [table, columns] of columnsByTable) { const columnList = columns.join(', '); - columnMessages.push(`Table '${table}': columns [${columnList}] are not available`); + columnMessages.push( + `Table '${table}': columns [${columnList}] are not available in your permitted dataset` + ); } if (columnMessages.length === 1) { - messages.push(`Unauthorized column access - ${columnMessages[0]}`); + messages.push( + `Unauthorized column access - ${columnMessages[0]}. Please use only the columns that are available in your permitted datasets, or request access to additional columns.` + ); } else { messages.push( - `Unauthorized column access:\n${columnMessages.map((m) => ` - ${m}`).join('\n')}` + `Unauthorized column access:\n${columnMessages.map((m) => ` - ${m}`).join('\n')}\n\nPlease modify your query to use only the columns available in your permitted datasets, or request access to the additional columns you need.` ); } } diff --git a/packages/ai/src/utils/sql-permissions/sql-parser-helpers.test.ts b/packages/ai/src/utils/sql-permissions/sql-parser-helpers.test.ts index 34afe2581..c48c023c4 100644 --- a/packages/ai/src/utils/sql-permissions/sql-parser-helpers.test.ts +++ b/packages/ai/src/utils/sql-permissions/sql-parser-helpers.test.ts @@ -428,7 +428,7 @@ models: const sql = 'SELECT * FROM users'; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); - expect(result.error).toContain('SELECT * is not allowed on physical tables'); + expect(result.error).toContain('SELECT * is not allowed on physical table: users'); expect(result.blockedTables).toContain('users'); }); @@ -436,7 +436,7 @@ models: const sql = 'SELECT u.* FROM users u'; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); - expect(result.error).toContain('SELECT * is not allowed on physical tables'); + expect(result.error).toContain('SELECT * is not allowed on physical table: users'); expect(result.blockedTables).toContain('users'); }); @@ -472,7 +472,7 @@ models: `; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); - expect(result.error).toContain('SELECT * is not allowed on physical tables'); + expect(result.error).toContain('SELECT * is not allowed on physical table: users'); expect(result.blockedTables).toContain('users'); }); @@ -509,6 +509,7 @@ models: const sql = 'SELECT u.*, o.* FROM users u JOIN orders o ON u.id = o.user_id'; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); + expect(result.error).toContain('SELECT * is not allowed on physical tables: users, orders'); expect(result.blockedTables).toEqual(expect.arrayContaining(['users', 'orders'])); }); @@ -516,14 +517,14 @@ models: const sql = 'SELECT * FROM public.users'; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); - expect(result.error).toContain('SELECT * is not allowed on physical tables'); + expect(result.error).toContain('SELECT * is not allowed on physical table: users'); }); it('should handle invalid SQL gracefully', () => { const sql = 'NOT VALID SQL'; const result = validateWildcardUsage(sql); expect(result.isValid).toBe(false); - expect(result.error).toContain('Failed to validate wildcard usage'); + expect(result.error).toContain('Failed to validate wildcard usage in SQL query'); }); }); @@ -1195,42 +1196,48 @@ models: ); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('insert'); - expect(result.error).toContain("Query type 'insert' is not allowed"); + expect(result.error).toContain("Query type 'INSERT' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of INSERT'); }); it('should reject UPDATE statements', () => { const result = checkQueryIsReadOnly('UPDATE users SET name = "Jane" WHERE id = 1'); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('update'); - expect(result.error).toContain("Query type 'update' is not allowed"); + expect(result.error).toContain("Query type 'UPDATE' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of UPDATE'); }); it('should reject DELETE statements', () => { const result = checkQueryIsReadOnly('DELETE FROM users WHERE id = 1'); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('delete'); - expect(result.error).toContain("Query type 'delete' is not allowed"); + expect(result.error).toContain("Query type 'DELETE' is not allowed"); + expect(result.error).toContain('To read data, use SELECT statements instead of DELETE'); }); it('should reject CREATE statements', () => { const result = checkQueryIsReadOnly('CREATE TABLE new_users (id INT, name VARCHAR(100))'); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('create'); - expect(result.error).toContain("Query type 'create' is not allowed"); + expect(result.error).toContain("Query type 'CREATE' is not allowed"); + expect(result.error).toContain('DDL operations like CREATE are not permitted'); }); it('should reject DROP statements', () => { const result = checkQueryIsReadOnly('DROP TABLE users'); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('drop'); - expect(result.error).toContain("Query type 'drop' is not allowed"); + expect(result.error).toContain("Query type 'DROP' is not allowed"); + expect(result.error).toContain('DDL operations like DROP are not permitted'); }); it('should reject ALTER statements', () => { const result = checkQueryIsReadOnly('ALTER TABLE users ADD COLUMN age INT'); expect(result.isReadOnly).toBe(false); expect(result.queryType).toBe('alter'); - expect(result.error).toContain("Query type 'alter' is not allowed"); + expect(result.error).toContain("Query type 'ALTER' is not allowed"); + expect(result.error).toContain('DDL operations like ALTER are not permitted'); }); it('should handle PostgreSQL dialect', () => { @@ -1241,7 +1248,7 @@ models: it('should handle invalid SQL gracefully', () => { const result = checkQueryIsReadOnly('NOT VALID SQL'); expect(result.isReadOnly).toBe(false); - expect(result.error).toContain('Failed to parse SQL'); + expect(result.error).toContain('Failed to parse SQL query for validation'); }); }); }); diff --git a/packages/ai/src/utils/sql-permissions/sql-parser-helpers.ts b/packages/ai/src/utils/sql-permissions/sql-parser-helpers.ts index 9de04e8f7..d2c793cfc 100644 --- a/packages/ai/src/utils/sql-permissions/sql-parser-helpers.ts +++ b/packages/ai/src/utils/sql-permissions/sql-parser-helpers.ts @@ -129,8 +129,20 @@ export function extractPhysicalTables(sql: string, dataSourceSyntax?: string): P return physicalTables; } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + // Provide more specific guidance based on common parsing errors + if (errorMessage.includes('Expected')) { + throw new Error( + `SQL syntax error: ${errorMessage}. Please check your SQL syntax and ensure it's valid for the ${dialect} dialect.` + ); + } + if (errorMessage.includes('Unexpected token')) { + throw new Error( + `SQL parsing error: ${errorMessage}. This may be due to unsupported SQL features or incorrect syntax.` + ); + } throw new Error( - `Failed to parse SQL: ${error instanceof Error ? error.message : 'Unknown error'}` + `Failed to parse SQL query: ${errorMessage}. Please ensure your SQL is valid and uses standard ${dialect} syntax.` ); } } @@ -353,8 +365,11 @@ export function extractTablesFromYml(ymlContent: string): ParsedTable[] { } } } - } catch (_error) { - // If YML parsing fails, return empty array + } catch (error) { + // Log the error for debugging but don't throw - return empty array + // This is expected behavior when YML content is invalid or not a dataset + const errorMessage = error instanceof Error ? error.message : String(error); + console.warn(`Failed to parse YML content for table extraction: ${errorMessage}`); } return tables; @@ -473,8 +488,11 @@ export function extractDatasetsFromYml(ymlContent: string): ParsedDataset[] { } } } - } catch (_error) { - // If YML parsing fails, return empty array + } catch (error) { + // Log the error for debugging but don't throw - return empty array + // This is expected behavior when YML content is invalid or not a dataset + const errorMessage = error instanceof Error ? error.message : String(error); + console.warn(`Failed to parse YML content for dataset extraction: ${errorMessage}`); } return datasets; @@ -546,18 +564,25 @@ export function validateWildcardUsage( } if (blockedTables.length > 0) { + // Create a more helpful error message with specific tables and guidance + const tableList = + blockedTables.length > 1 + ? `tables: ${blockedTables.join(', ')}` + : `table: ${blockedTables[0]}`; + return { isValid: false, - error: `SELECT * is not allowed on physical tables. Please specify the columns you need.`, + error: `SELECT * is not allowed on physical ${tableList}. Please explicitly specify the column names you need instead of using wildcards. For example, use 'SELECT column1, column2 FROM table' instead of 'SELECT * FROM table'. This restriction helps ensure data security and prevents unintended data exposure.`, blockedTables, }; } return { isValid: true }; } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); return { isValid: false, - error: `Failed to validate wildcard usage: ${error instanceof Error ? error.message : 'Unknown error'}`, + error: `Failed to validate wildcard usage in SQL query: ${errorMessage}. Please ensure your SQL syntax is correct and try specifying explicit column names instead of using SELECT *.`, }; } } @@ -836,8 +861,12 @@ export function extractColumnReferences( } return tableColumnMap; - } catch (_error) { - // Return empty map if parsing fails + } catch (error) { + // Log the error for debugging but return empty map to allow validation to continue + const errorMessage = error instanceof Error ? error.message : String(error); + console.warn( + `Failed to extract column references from SQL: ${errorMessage}. Column-level permissions cannot be validated.` + ); return new Map(); } } @@ -1633,10 +1662,38 @@ export function checkQueryIsReadOnly(sql: string, dataSourceSyntax?: string): Qu // Only allow SELECT statements if (queryType !== 'select') { + // Provide specific guidance based on the query type + let guidance = ''; + switch (queryType) { + case 'insert': + guidance = ' To read data, use SELECT statements instead of INSERT.'; + break; + case 'update': + guidance = ' To read data, use SELECT statements instead of UPDATE.'; + break; + case 'delete': + guidance = ' To read data, use SELECT statements instead of DELETE.'; + break; + case 'create': + guidance = + ' DDL operations like CREATE are not permitted. Use SELECT to query existing data.'; + break; + case 'drop': + guidance = + ' DDL operations like DROP are not permitted. Use SELECT to query existing data.'; + break; + case 'alter': + guidance = + ' DDL operations like ALTER are not permitted. Use SELECT to query existing data.'; + break; + default: + guidance = ' Please use SELECT statements to query data.'; + } + return { isReadOnly: false, queryType: statement.type, - error: `Query type '${statement.type}' is not allowed. Only SELECT statements are permitted for read-only access.`, + error: `Query type '${statement.type.toUpperCase()}' is not allowed. Only SELECT statements are permitted for read-only access.${guidance}`, }; } } @@ -1647,9 +1704,10 @@ export function checkQueryIsReadOnly(sql: string, dataSourceSyntax?: string): Qu queryType: 'select', }; } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); return { isReadOnly: false, - error: `Failed to parse SQL for query type check: ${error instanceof Error ? error.message : 'Unknown error'}`, + error: `Failed to parse SQL query for validation: ${errorMessage}. Please ensure your SQL syntax is valid. Only SELECT statements are allowed for read-only access.`, }; } }