fix(tools/clickhouse,tools/bigquery): validate identifier parameters to prevent injection (#3219)

## Summary

This PR fixes identifier-injection vulnerabilities in three built-in
tools that interpolate caller-supplied parameters directly into SQL
without sanitisation.

---

### 1. `clickhouse-list-tables` — database identifier injection
(original fix)

`Tool.Invoke` interpolates the caller-supplied `database` parameter
directly into a `SHOW TABLES FROM %s` statement:

```go
query := fmt.Sprintf("SHOW TABLES FROM %s", database)
```

The existing comment acknowledges the risk. The fix adds a
`validIdentifier` regex (`^[A-Za-z_][A-Za-z0-9_]*$`) that must match
before interpolation.

Concrete payloads on a default ClickHouse deployment:

| `database` parameter | rendered query | effect |
|---|---|---|
| `default LIKE '%secret%'` | `SHOW TABLES FROM default LIKE '%secret%'`
| filters listing by attacker-chosen pattern |
| `system FORMAT JSONEachRow` | `SHOW TABLES FROM system FORMAT
JSONEachRow` | enumerates `system.*` (system.users, etc.) |
| `default INTO OUTFILE '/tmp/x.tsv'` | `SHOW TABLES FROM default INTO
OUTFILE '/tmp/x.tsv'` | writes to disk on ClickHouse host |

---

### 2. `bigquery-forecast` — backtick + column-name injection

Two injection classes:

**a. Backtick injection in `history_data` (table identifier path)**

```go
historyDataSource = fmt.Sprintf("TABLE `%s`", historyData)
```

When `allowed_datasets` is not configured (the default), no validation
runs before this line. An attacker-supplied `history_data` containing a
backtick closes the opening delimiter and appends arbitrary SQL:

```
history_data = "ds.tbl` UNION ALL SELECT secret FROM private.pii --"
→  TABLE `ds.tbl` UNION ALL SELECT secret FROM private.pii --`
```

**b. Column-name injection in `data_col`, `timestamp_col`, `id_cols`**

```go
sql := fmt.Sprintf(`... data_col => '%s', timestamp_col => '%s' ...`, dataCol, timestampCol, ...)
```

Column names are interpolated as single-quoted strings with no
validation. A value containing `'` breaks out of the string literal
context.

**Fix:** `ValidTableID` (restricts `history_data` to `[a-zA-Z0-9_]+`
components with 1–2 dots) applied in the table-ID else-branch;
`ValidColumnName` (`^[a-zA-Z_][a-zA-Z0-9_]*$`) applied to `data_col`,
`timestamp_col`, and each element of `id_cols`.

---

### 3. `bigquery-analyze-contribution` — backtick + OPTIONS injection

**a. Backtick injection in `input_data` (table identifier path)**

```go
inputDataSource = fmt.Sprintf("SELECT * FROM `%s`", inputData)
```

Same root cause as bigquery-forecast: when `allowed_datasets` is absent,
no validation before interpolation.

**b. Column-name and OPTIONS injection**

```go
options = append(options, fmt.Sprintf("IS_TEST_COL = '%s'", paramsMap["is_test_col"]))
// and each element of dimension_id_cols:
strCols = append(strCols, fmt.Sprintf("'%s'", c))
```

`is_test_col` and `dimension_id_cols` are column names interpolated into
BigQuery ML `OPTIONS()` string literals without validation.

`contribution_metric` (e.g. `SUM(col)/COUNT(DISTINCT col2)`) goes into
`OPTIONS(CONTRIBUTION_METRIC = '%s')`. A single-quote in this value
breaks the OPTIONS string literal.

**Fix:** `ValidTableID` for `input_data` in the table-ID else-branch;
`ValidColumnName` for `is_test_col` and each `dimension_id_cols`
element; single-quote check for `contribution_metric`.

---

## Shared validators in `bigquerycommon`

`ValidTableID` and `ValidColumnName` are exported from the
`bigquerycommon` package so both tools share the same rules. Tests are
in `validators_test.go`.

## Test plan

- [x] `go build ./...` clean
- [x] `go test ./internal/tools/bigquery/... -v` passes (all existing
tests + new `TestValidTableID`, `TestValidColumnName`)
- [x] `go test ./internal/tools/clickhouse/clickhouselisttables/... -v`
passes

## Not a dupe of #2811 / #779

Those cover the `templateParameter` flow
(`internal/util/parameters/parameters.go:applyEscape`). The three tools
fixed here use hard-coded `fmt.Sprintf` interpolation in their own
`Invoke` methods — none of the templateParameter mitigations apply.

---------

Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>

Author: evilgensec

internal/tools/bigquery/bigqueryanalyzecontribution/bigqueryanalyzecontribution.go

@@ -191,16 +191,39 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
 
 	modelID := fmt.Sprintf("contribution_analysis_model_%s", strings.ReplaceAll(uuid.New().String(), "-", ""))
 
+	contributionMetric, ok := paramsMap["contribution_metric"].(string)
+	if !ok {
+		return nil, util.NewAgentError(fmt.Sprintf("unable to cast contribution_metric parameter %v", paramsMap["contribution_metric"]), nil)
+	}
+	if strings.ContainsRune(contributionMetric, '\'') {
+		return nil, util.NewAgentError("invalid 'contribution_metric': must not contain single quotes", nil)
+	}
+
+	isTestCol, ok := paramsMap["is_test_col"].(string)
+	if !ok {
+		return nil, util.NewAgentError(fmt.Sprintf("unable to cast is_test_col parameter %v", paramsMap["is_test_col"]), nil)
+	}
+	if !bqutil.ValidColumnName(isTestCol) {
+		return nil, util.NewAgentError(fmt.Sprintf("invalid column name for 'is_test_col': %q; must match [a-zA-Z_][a-zA-Z0-9_]*", isTestCol), nil)
+	}
+
 	var options []string
 	options = append(options, "MODEL_TYPE = 'CONTRIBUTION_ANALYSIS'")
-	options = append(options, fmt.Sprintf("CONTRIBUTION_METRIC = '%s'", paramsMap["contribution_metric"]))
-	options = append(options, fmt.Sprintf("IS_TEST_COL = '%s'", paramsMap["is_test_col"]))
+	options = append(options, fmt.Sprintf("CONTRIBUTION_METRIC = '%s'", contributionMetric))
+	options = append(options, fmt.Sprintf("IS_TEST_COL = '%s'", isTestCol))
 
 	if val, ok := paramsMap["dimension_id_cols"]; ok {
 		if cols, ok := val.([]any); ok {
 			var strCols []string
 			for _, c := range cols {
-				strCols = append(strCols, fmt.Sprintf("'%s'", c))
+				colStr, ok := c.(string)
+				if !ok {
+					return nil, util.NewAgentError(fmt.Sprintf("dimension_id_cols contains non-string value: %v", c), nil)
+				}
+				if !bqutil.ValidColumnName(colStr) {
+					return nil, util.NewAgentError(fmt.Sprintf("invalid column name in 'dimension_id_cols': %q; must match [a-zA-Z_][a-zA-Z0-9_]*", colStr), nil)
+				}
+				strCols = append(strCols, fmt.Sprintf("'%s'", colStr))
 			}
 			options = append(options, fmt.Sprintf("DIMENSION_ID_COLS = [%s]", strings.Join(strCols, ", ")))
 		} else {
@@ -254,6 +277,9 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
 		}
 		inputDataSource = fmt.Sprintf("(%s)", inputData)
 	} else {
+		if !bqutil.ValidTableID(inputData) {
+			return nil, util.NewAgentError(fmt.Sprintf("invalid table identifier for 'input_data': %q; expected 'dataset.table' or 'project.dataset.table'", inputData), nil)
+		}
 		if len(source.BigQueryAllowedDatasets()) > 0 {
 			parts := strings.Split(inputData, ".")
 			var projectID, datasetID string

internal/tools/bigquery/bigquerycommon/util.go

@@ -17,6 +17,7 @@ package bigquerycommon
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"sort"
 	"strings"
 
@@ -25,6 +26,31 @@ import (
 	bigqueryrestapi "google.golang.org/api/bigquery/v2"
 )
 
+// validBQTableID matches BigQuery table identifiers in 'dataset.table' or
+// 'project.dataset.table' form. Components are restricted to letters, digits,
+// and underscores — the character set that BigQuery allows for dataset and
+// table IDs and that is safe to interpolate inside a backtick-quoted SQL
+// identifier.
+var validBQTableID = regexp.MustCompile(`^[a-zA-Z0-9_-]+(\.([a-zA-Z0-9_]+)){1,2}$`)
+
+// validBQColumnName matches BigQuery column names: a letter or underscore
+// followed by letters, digits, or underscores.
+var validBQColumnName = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`)
+
+// ValidTableID returns true if s is a safe BigQuery table identifier of the
+// form 'dataset.table' or 'project.dataset.table'. Values that fail this check
+// must not be interpolated into backtick-quoted SQL.
+func ValidTableID(s string) bool {
+	return validBQTableID.MatchString(s)
+}
+
+// ValidColumnName returns true if s is a safe BigQuery column name.
+// Values that fail this check must not be interpolated as SQL identifiers
+// or into single-quoted SQL string arguments that represent column references.
+func ValidColumnName(s string) bool {
+	return validBQColumnName.MatchString(s)
+}
+
 // DryRunQuery performs a dry run of the SQL query to validate it and get metadata.
 func DryRunQuery(ctx context.Context, restService *bigqueryrestapi.Service, projectID string, location string, sql string, params []*bigqueryrestapi.QueryParameter, connProps []*bigqueryapi.ConnectionProperty, maximumBytesBilled int64) (*bigqueryrestapi.Job, error) {
 	useLegacySql := false

internal/tools/bigquery/bigquerycommon/validators_test.go

@@ -0,0 +1,118 @@
+// Copyright 2026 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package bigquerycommon_test
+
+import (
+	"testing"
+
+	"github.com/googleapis/mcp-toolbox/internal/tools/bigquery/bigquerycommon"
+)
+
+func TestValidTableID(t *testing.T) {
+	tcs := []struct {
+		in    string
+		valid bool
+	}{
+		// Allowed: dataset.table
+		{"my_dataset.my_table", true},
+		{"ds.t", true},
+		{"ds_1.tbl_2", true},
+
+		// Allowed: project.dataset.table
+		{"proj.ds.tbl", true},
+		{"PROJ.DS.TBL", true},
+		{"my_project.my_dataset.my_table", true},
+
+		// Rejected: hyphens (not valid in dataset/table IDs).
+		{"my-project.my-dataset.my_table", false},
+		{"my-project.my_dataset.my-table", false},
+		{"my-project-123.dataset.table", true},
+
+		// Rejected: only one component (no dot)
+		{"my_dataset", false},
+		{"", false},
+
+		// Rejected: too many dots (4+ parts)
+		{"a.b.c.d", false},
+
+		// Rejected: injection characters
+		{"dataset.table`", false},
+		{"dataset.table` UNION ALL SELECT 1 --", false},
+		{"dataset.table'; DROP TABLE x --", false},
+		{"dataset.table\n", false},
+		{"dataset.table ", false},
+		{" dataset.table", false},
+		{"dataset.table\t", false},
+
+		// Rejected: backtick (closes identifier in SQL)
+		{"ds.`table`", false},
+
+		// Rejected: SQL metacharacters
+		{"ds.table--", false},
+		{"ds.table/*", false},
+		{"ds.table;", false},
+	}
+	for _, tc := range tcs {
+		if got := bigquerycommon.ValidTableID(tc.in); got != tc.valid {
+			t.Errorf("ValidTableID(%q) = %v, want %v", tc.in, got, tc.valid)
+		}
+	}
+}
+
+func TestValidColumnName(t *testing.T) {
+	tcs := []struct {
+		in    string
+		valid bool
+	}{
+		// Allowed: simple identifiers.
+		{"sales", true},
+		{"sales_col", true},
+		{"_internal", true},
+		{"Col1", true},
+		{"A", true},
+		{"is_test", true},
+		{"timestamp_col", true},
+
+		// Rejected: empty string.
+		{"", false},
+
+		// Rejected: leading digit.
+		{"1col", false},
+
+		// Rejected: SQL injection characters.
+		{"col'", false},
+		{"col`", false},
+		{"col; DROP TABLE x", false},
+		{"col UNION SELECT", false},
+		{"col--", false},
+		{"col/*", false},
+		{"col(", false},
+		{"col)", false},
+		{"col/col2", false},
+
+		// Rejected: whitespace.
+		{"col name", false},
+		{" col", false},
+		{"col ", false},
+
+		// Rejected: dots (not valid in unquoted column names).
+		{"ds.col", false},
+	}
+	for _, tc := range tcs {
+		if got := bigquerycommon.ValidColumnName(tc.in); got != tc.valid {
+			t.Errorf("ValidColumnName(%q) = %v, want %v", tc.in, got, tc.valid)
+		}
+	}
+}

internal/tools/bigquery/bigqueryforecast/bigqueryforecast.go

@@ -191,6 +191,18 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
 		}
 	}
 
+	if !bqutil.ValidColumnName(dataCol) {
+		return nil, util.NewAgentError(fmt.Sprintf("invalid column name for 'data_col': %q; must match [a-zA-Z_][a-zA-Z0-9_]*", dataCol), nil)
+	}
+	if !bqutil.ValidColumnName(timestampCol) {
+		return nil, util.NewAgentError(fmt.Sprintf("invalid column name for 'timestamp_col': %q; must match [a-zA-Z_][a-zA-Z0-9_]*", timestampCol), nil)
+	}
+	for _, col := range idCols {
+		if !bqutil.ValidColumnName(col) {
+			return nil, util.NewAgentError(fmt.Sprintf("invalid column name in 'id_cols': %q; must match [a-zA-Z_][a-zA-Z0-9_]*", col), nil)
+		}
+	}
+
 	bqClient, restService, err := source.RetrieveClientAndService(accessToken)
 	if err != nil {
 		return nil, util.NewClientServerError("failed to retrieve BigQuery client", http.StatusInternalServerError, err)
@@ -232,6 +244,9 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
 		}
 		historyDataSource = fmt.Sprintf("(%s)", historyData)
 	} else {
+		if !bqutil.ValidTableID(historyData) {
+			return nil, util.NewAgentError(fmt.Sprintf("invalid table identifier for 'history_data': %q; expected 'dataset.table' or 'project.dataset.table'", historyData), nil)
+		}
 		if len(source.BigQueryAllowedDatasets()) > 0 {
 			parts := strings.Split(historyData, ".")
 			var projectID, datasetID string

internal/tools/clickhouse/clickhouselisttables/clickhouselisttables.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"regexp"
 
 	yaml "github.com/goccy/go-yaml"
 	"github.com/googleapis/mcp-toolbox/internal/embeddingmodels"
@@ -30,6 +31,14 @@ import (
 const listTablesType string = "clickhouse-list-tables"
 const databaseKey string = "database"
 
+// validIdentifier matches the ClickHouse unquoted identifier grammar: a letter
+// or underscore followed by letters, digits, or underscores. The `database`
+// parameter is interpolated directly into a `SHOW TABLES FROM` statement and
+// cannot be bound as a positional value, so restricting it to this character
+// set is the only safe option short of refactoring to use ClickHouse's
+// `system.tables` view.
+var validIdentifier = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
+
 func init() {
 	if !tools.Register(listTablesType, newListTablesConfig) {
 		panic(fmt.Sprintf("tool type %q already registered", listTablesType))
@@ -120,8 +129,17 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
 		return nil, util.NewAgentError(fmt.Sprintf("invalid or missing '%s' parameter; expected a string", databaseKey), nil)
 	}
 
-	// Query to list all tables in the specified database
-	// Note: formatting identifier directly is risky if input is untrusted, but standard for this tool structure.
+	// The database name is interpolated directly into the `SHOW TABLES FROM`
+	// statement. Reject anything that is not a plain identifier so that a
+	// caller cannot smuggle additional clauses (LIKE, LIMIT, FORMAT,
+	// INTO OUTFILE, ...), quoted identifiers, or other expressions through
+	// this parameter. Without this check, an MCP client (or a prompt-injected
+	// LLM) could escape the intended scope of the tool and read arbitrary
+	// system tables, switch the output format to one the calling layer cannot
+	// parse safely, or chain a SHOW with an `INTO OUTFILE` write.
+	if !validIdentifier.MatchString(database) {
+		return nil, util.NewAgentError(fmt.Sprintf("invalid '%s' parameter %q: must be a plain identifier matching %s", databaseKey, database, validIdentifier.String()), nil)
+	}
 	query := fmt.Sprintf("SHOW TABLES FROM %s", database)
 
 	out, err := source.RunSQL(ctx, query, nil)

internal/tools/clickhouse/clickhouselisttables/clickhouselisttables_test.go

@@ -97,3 +97,51 @@ func TestListTablesToolParseParams(t *testing.T) {
 		t.Errorf("expected database parameter to be 'test_db', got %v", mapParams["database"])
 	}
 }
+
+func TestValidIdentifierRegexp(t *testing.T) {
+	tcs := []struct {
+		in    string
+		valid bool
+	}{
+		// Allowed: plain identifiers.
+		{"default", true},
+		{"my_db", true},
+		{"DB1", true},
+		{"_internal", true},
+		{"a", true},
+
+		// Rejected: empty, whitespace, control characters.
+		{"", false},
+		{" ", false},
+		{"\t", false},
+		{"\n", false},
+
+		// Rejected: leading digit (ClickHouse identifier rule).
+		{"1db", false},
+
+		// Rejected: identifier-quoting characters that would let the value
+		// re-open the identifier and append clauses after it.
+		{"`default`", false},
+		{`"default"`, false},
+
+		// Rejected: separators / statement terminators / metacharacters.
+		{"default;DROP TABLE x", false},
+		{"default LIKE '%'", false},
+		{"default LIMIT 0 FORMAT JSON", false},
+		{"default INTO OUTFILE '/tmp/x'", false},
+		{"system.tables", false},
+		{"default--", false},
+		{"default/*", false},
+
+		// Rejected: unicode and surrounding spaces.
+		{"𝐝𝐞𝐟𝐚𝐮𝐥𝐭", false},
+		{"  default  ", false},
+		{"default ", false},
+		{" default", false},
+	}
+	for _, tc := range tcs {
+		if got := validIdentifier.MatchString(tc.in); got != tc.valid {
+			t.Errorf("validIdentifier.MatchString(%q) = %v, want %v", tc.in, got, tc.valid)
+		}
+	}
+}