Repository: ClickHouse/clickhouse-go

Issue:
Input validation issue on bind method
## Observed

In the [format](https://github.com/ClickHouse/clickhouse-go/blob/c463789f1b7d2d1ccbc533efda0c6d79b483f623/bind.go#L264) function, the cases of `time.Time` and `*time.Time` lack the proper sanitization of the provided value which can result in unexpected behavior of the SQL query executed against the database.

The impact and chance of abuse are low since it requires for the user of the library to allow arbitrary input into the time parameter.  


## Expected behaviour
The expected behavior is for the bind function to sanitize the provided input of all types.

## Code example
Using the following snippet you can recreate the issue.

```go
	// Create a test table
	_, err = db.Exec(`
		CREATE TABLE IF NOT EXISTS test_users (
			id UInt32,
			username String,
			email String,
			created_at DateTime
		) ENGINE = Memory
	`)
	if err != nil {
		log.Fatal("Failed to create table:", err)
	}

	// Dummy data
	_, err = db.Exec(`
		INSERT INTO test_users (id, username, email, created_at) VALUES
		(1, 'alice', 'alice@example.com', now()),
		(2, 'bob', 'bob@example.com', now()),
		(3, 'admin', 'admin@example.com', now())
	`)
	if err != nil {
		log.Fatal("Failed to insert test data:", err)
	}

	// The union content would be user provided input.
	namedMaliciousLoc := time.FixedZone("UTC') UNION ALL SELECT id, username, email FROM test_users --", 0)
	namedMaliciousTime := time.Now().In(namedMaliciousLoc)	
	rows, err := db.Query(
		"SELECT id, username, email FROM test_users WHERE created_at > @timestamp",
		clickhouse.Named("timestamp", namedMaliciousTime),
	)
```

Hints from issue comments:
@kavirajk: Update: After discussion with other language clients maintainers and previous maintainer (@SpencerTorres ) of clickhouse-go, we decided to deprecate the client side bindings(also probably remove it after few versions) and do server side binding by default (similar to [postgres ](https://github.com/jackc/pgx/blob/v5.7.6/conn.go#L668-L672)and [mysql driver](https://github.com/go-sql-driver/mysql?tab=readme-ov-file#interpolateparams))

Expect a PR soon (realistically in a day or two).@kavirajk: @michael-anastasakis @SpencerTorres @mshustov 

It's more harder to implement only server side binding than I expected. Let me explain why.

### Problem

The problem is two folds
1. ClickHouse server accepts params via `SET param_value=10; SELECT * FROM test WHERE value={value:Int64}`

NOTE: Here, the client need to **send both the value and it's type to the server.**

2. Go client currently accepts three different ways to bind the parameters.

a. Positional binding.
```go
conn.Query("SELECT * FROM test WHERE name=? AND value=?", "some-name", some-value)
```
b. Numeric binding.
```go
conn.Query("SELECT * FROM test WHERE name=$2 AND value=$1", "some-name", some-value)
```
c. ClickHouse native binding
```go
	params := clickhouse.WithParameters(clickhouse.Parameters{
		"value": "10",
	})

	ctx := clickhouse.Context(context.Background(), params)

	rows, err := conn.QueryContext(ctx, "SELECT * FROM test WHERE value={value: Int64}")
```

(c) is server side binding. The problem is with (a) and (b) which are client side binded now. Notice that arguments passed for both positional binding and numeric binding are of type `any`. **Meaning we cannot infer exact type** (int vs int32 vs int64 vs float32 vs float64, etc). Basically in order to convert those into server side binding, we need to know their exact type to send it to ClickHouse server. Which we cannot.

(a) and (b) is the standard way Go's `sql.DB` works unfortunately.

(@mshustov probably this is the one you were saying that Go's standard is the blocker to migrate to server-side binding)

### Proposal
1. We can deprecate using both positional and numeric binding in our Go client and warn the user to switch to clickhouse native binding
2. Properly support both client-side and server-side binding.

Both has tradeoffs. Neither is is the single best way to solve the problem. I'm bit concerned with (1) because don't know how users (both internal and OSS) would consider this change, given this is the standard way how people using SQL in Go.

Open for any thoughts and ideas@nils-borrmann-tacto: Note that once you make server-side bindings the default, users will likely run into this problem: https://github.com/ClickHouse/ClickHouse/issues/91168

Which is also why python's clickhouse-driver [uses client-side bindings by default](https://github.com/mymarilyn/clickhouse-driver/issues/376).@kavirajk: After some internal discussions with other client maintainers. We have the following consensus.

1. We deprecate client-side binding on `clickhouse_api` interface.
2. Keep supporting both client-side and server-side binding on `std` interface.
3. Have a clear docs explaining the drawbacks of client side binding. 

@nils-borrmann-tacto thanks for the thoughts.@kavirajk: The original bug reported is fixed in this PR #1727 hence closing the issue.

For larger problem of dealing with client-side binding (and migrating to server-side binding) [we are tracking in this issue](https://github.com/ClickHouse/clickhouse-go/issues/1729)

Solution patch:
diff --git a/bind.go b/bind.go
index f0fe30f..fa90ade 100644
--- a/bind.go
+++ b/bind.go
@@ -2,6 +2,7 @@ package clickhouse
 
 import (
 	std_driver "database/sql/driver"
+	"errors"
 	"fmt"
 	"reflect"
 	"regexp"
@@ -12,6 +13,10 @@ import (
 	"github.com/ClickHouse/clickhouse-go/v2/lib/driver"
 )
 
+var (
+	ErrInvalidTimezone = errors.New("invalid timezone value")
+)
+
 func Named(name string, value any) driver.NamedValue {
 	return driver.NamedValue{
 		Name:  name,
@@ -228,7 +233,9 @@ func bindNamed(tz *time.Location, query string, args ...any) (_ string, err erro
 }
 
 func formatTime(tz *time.Location, scale TimeUnit, value time.Time) (string, error) {
-	switch value.Location().String() {
+	locVal := value.Location().String()
+
+	switch locVal {
 	case "Local", "":
 		// It's required to pass timestamp as string due to decimal overflow for higher precision,
 		// but zero-value string "toDateTime('0')" will be not parsed by ClickHouse.
@@ -252,10 +259,17 @@ func formatTime(tz *time.Location, scale TimeUnit, value time.Time) (string, err
 		}
 		return fmt.Sprintf("toDateTime64('%s', %d)", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3)), nil
 	}
+
+	// Escape the timezone string (timezone may contain malicious SQL query)
+	escapedTimezone := stringQuoteReplacer.Replace(locVal)
+	if locVal != escapedTimezone {
+		return "", fmt.Errorf("%w: %q", ErrInvalidTimezone, locVal)
+	}
+
 	if scale == Seconds {
-		return fmt.Sprintf("toDateTime('%s', '%s')", value.Format("2006-01-02 15:04:05"), value.Location().String()), nil
+		return fmt.Sprintf("toDateTime('%s', '%s')", value.Format("2006-01-02 15:04:05"), escapedTimezone), nil
 	}
-	return fmt.Sprintf("toDateTime64('%s', %d, '%s')", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3), value.Location().String()), nil
+	return fmt.Sprintf("toDateTime64('%s', %d, '%s')", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3), escapedTimezone), nil
 }
 
 var stringQuoteReplacer = strings.NewReplacer(`\`, `\\`, `'`, `\'`)



Test patch:
diff --git a/bind_test.go b/bind_test.go
index 72aceb7..55829ac 100644
--- a/bind_test.go
+++ b/bind_test.go
@@ -330,6 +330,54 @@ func TestFormatMap(t *testing.T) {
 	assert.Equal(t, "map('a', 1)", val)
 }
 
+func TestTimezoneSQLEscaping(t *testing.T) {
+	t.Run("prevent SQL injection via timezone name", func(t *testing.T) {
+		maliciousLoc := time.FixedZone("UTC') UNION ALL SELECT 1,2,3 --", 0)
+		time.LoadLocation(maliciousLoc.String())
+		maliciousTime := time.Now().In(maliciousLoc)
+
+		val, err := format(time.UTC, Seconds, maliciousTime)
+		require.Error(t, err)
+		assert.Equal(t, "", val)
+		assert.ErrorIs(t, err, ErrInvalidTimezone)
+	})
+
+	t.Run("prevent SQL injection via timezone name with milliseconds", func(t *testing.T) {
+		maliciousLoc := time.FixedZone("America/New_York'); DROP TABLE users; --", 0)
+		maliciousTime := time.Now().In(maliciousLoc)
+
+		val, err := format(time.UTC, MilliSeconds, maliciousTime)
+		require.Error(t, err)
+		assert.Equal(t, "", val)
+		assert.ErrorIs(t, err, ErrInvalidTimezone)
+	})
+
+	t.Run("prevent SQL injection via timezone with backslashes", func(t *testing.T) {
+		maliciousLoc := time.FixedZone(`UTC\' OR 1=1 --`, 0)
+		maliciousTime := time.Now().In(maliciousLoc)
+
+		val, err := format(time.UTC, Seconds, maliciousTime)
+		// require.NoError(t, err)
+		require.Error(t, err)
+		assert.Equal(t, "", val)
+		assert.ErrorIs(t, err, ErrInvalidTimezone)
+	})
+
+	t.Run("normal timezone names remain unaffected", func(t *testing.T) {
+		// Test that normal, safe timezone names still work correctly
+		normalLoc := time.FixedZone("America/New_York", -5*3600)
+		normalTime := time.Now().In(normalLoc)
+
+		val, err := format(time.UTC, Seconds, normalTime)
+		require.NoError(t, err)
+
+		// Should contain the timezone name without any escaping
+		assert.Contains(t, val, "'America/New_York'")
+		assert.NotContains(t, val, `\'`, "Normal timezone names should not have escaped quotes")
+		assert.Contains(t, val, "toDateTime(")
+	})
+}
+
 // a simple (non thread safe) ordered map, implementing the column.OrderedMap interface
 type OrderedMap struct {
 	keys   []any