Merge pull request #741 from buster-so/better-error-messages-for-sql-protections

Enhance SQL permission validation error messages with actionable guidance. Update tests to reflect improved error handling and specific feedback for unauthorized queries and wildcard usage. Ensure consistency in error reporting across various SQL operations.

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

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: `Permission validation failed: ${error instanceof Error ? error.message : 'Unknown error'}`,
+        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: ${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.`
       );
     }
   }

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

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(
-      `Failed to parse SQL: ${error instanceof Error ? error.message : 'Unknown 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 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.`,
     };
   }
 }