fix(clickhouse): validate identifiers to prevent SQL injection (#693)

* fix(clickhouse): validate identifiers to prevent SQL injection

* chore: address review feedback

* fix(clickhouse): enforce non-empty table name

* test: remove invalid empty identifier test case

Author: lilmingwa13

tools/clickhouse.go

@@ -31,6 +31,18 @@ const (
 	ClickHouseFormatTable = 1
 )
 
+var clickHouseIdentifierRe = regexp.MustCompile(`^[a-zA-Z0-9_]+$`)
+
+func validateClickHouseIdentifier(name, field string) error {
+	if name == "" {
+		return nil
+	}
+	if !clickHouseIdentifierRe.MatchString(name) {
+		return fmt.Errorf("invalid %s: must contain only letters, numbers, and underscores", field)
+	}
+	return nil
+}
+
 // ClickHouseQueryParams defines the parameters for querying ClickHouse
 type ClickHouseQueryParams struct {
 	DatasourceUID string            `json:"datasourceUid" jsonschema:"required,description=The UID of the ClickHouse datasource to query. Use list_datasources to find available UIDs."`
@@ -398,6 +410,10 @@ type ClickHouseTableInfo struct {
 
 // listClickHouseTables lists tables from a ClickHouse datasource
 func listClickHouseTables(ctx context.Context, args ListClickHouseTablesParams) ([]ClickHouseTableInfo, error) {
+	if err := validateClickHouseIdentifier(args.Database, "database"); err != nil {
+		return nil, err
+	}
+
 	// Build the query to list tables
 	query := `SELECT database, name, engine, total_rows, total_bytes
 FROM system.tables
@@ -475,6 +491,18 @@ func describeClickHouseTable(ctx context.Context, args DescribeClickHouseTablePa
 		database = "default"
 	}
 
+	if err := validateClickHouseIdentifier(database, "database"); err != nil {
+		return nil, err
+	}
+
+	if args.Table == "" {
+		return nil, fmt.Errorf("table is required")
+	}
+
+	if err := validateClickHouseIdentifier(args.Table, "table"); err != nil {
+		return nil, err
+	}
+
 	// Query system.columns instead of using DESCRIBE TABLE
 	// This avoids the LIMIT clause issue with DESCRIBE statements
 	query := fmt.Sprintf(`SELECT name, type, default_kind as default_type, default_expression, comment

tools/clickhouse_test.go

@@ -333,3 +333,28 @@ func TestGenerateClickHouseEmptyResultHints(t *testing.T) {
 	}
 	assert.True(t, found, "Hints should suggest using list_clickhouse_tables")
 }
+
+func TestValidateClickHouseIdentifier(t *testing.T) {
+	tests := []struct {
+		name    string
+		field   string
+		wantErr bool
+	}{
+		{name: "default", field: "database", wantErr: false},
+		{name: "table_1", field: "table", wantErr: false},
+
+		//attack payloads (must fail)
+		{name: "default' OR 1=1 --", field: "database", wantErr: true},
+		{name: "default' UNION SELECT name FROM system.databases --", field: "database", wantErr: true},
+		{name: "table-name", field: "table", wantErr: true},
+		{name: "table name", field: "table", wantErr: true},
+		{name: "system.tables", field: "table", wantErr: true},
+	}
+
+	for _, tt := range tests {
+		err := validateClickHouseIdentifier(tt.name, tt.field)
+		if (err != nil) != tt.wantErr {
+			t.Errorf("validateClickHouseIdentifier(%q) error = %v, wantErr %v", tt.name, err, tt.wantErr)
+		}
+	}
+}