fix(querier): drop the block comment guard now the parser handles it

The parser no longer loops on an unterminated block comment, so the hand-rolled
scan that worked around it is redundant, and it was the riskier of the two: it
duplicated the lexer's handling of string literals and could have rejected a
legitimate query. Leave the parser as the single source of truth.

The panic case likewise no longer panics, so its test can no longer cover the
recover and is removed. The recover stays as insurance, since this reaches
user-authored SQL and the parser has regressed this way before.

Cover INTERSECT and EXCEPT, which the parser only started accepting in this
version and which reach a second query through the same rules.

pkg/querier/clickhouse_query.go

@@ -100,9 +100,22 @@ func (q *chSQLQuery) renderVars(query string, vars map[string]qbtypes.VariableIt
 	return newQuery.String(), nil
 }
 
+func (q *chSQLQuery) render() (string, error) {
+	rendered, err := q.renderVars(q.query.Query, q.vars, q.fromMS, q.toMS)
+	if err != nil {
+		return "", err
+	}
+
+	if err := querybuilder.ValidateReadOnlySelect(rendered); err != nil {
+		return "", err
+	}
+
+	return rendered, nil
+}
+
 // Statement renders the SQL without executing it, for the preview path.
 func (q *chSQLQuery) Statement(_ context.Context) (*qbtypes.Statement, error) {
-	rendered, err := q.renderVars(q.query.Query, q.vars, q.fromMS, q.toMS)
+	rendered, err := q.render()
 	if err != nil {
 		return nil, err
 	}
@@ -124,11 +137,13 @@ func (q *chSQLQuery) Execute(ctx context.Context) (*qbtypes.Result, error) {
 		elapsed += p.Elapsed
 	}))
 
-	query, err := q.renderVars(q.query.Query, q.vars, q.fromMS, q.toMS)
+	query, err := q.render()
 	if err != nil {
 		return nil, err
 	}
 
+	ctx = ctxtypes.SetClickhouseReadOnly(ctx)
+
 	rows, err := q.telemetryStore.ClickhouseDB().Query(ctx, query, q.args...)
 	if err != nil {
 		return nil, err

pkg/query-service/app/http_handler.go

@@ -11,6 +11,7 @@ import (
 	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/flagger"
 	"github.com/SigNoz/signoz/pkg/modules/thirdpartyapi"
+	"github.com/SigNoz/signoz/pkg/querybuilder"
 	"github.com/SigNoz/signoz/pkg/queryparser"
 
 	"log/slog"
@@ -1021,21 +1022,6 @@ func prepareQuery(r *http.Request) (string, error) {
 		return "", fmt.Errorf("query is required")
 	}
 
-	notAllowedOps := []string{
-		"alter table",
-		"drop table",
-		"truncate table",
-		"drop database",
-		"drop view",
-		"drop function",
-	}
-
-	for _, op := range notAllowedOps {
-		if strings.Contains(strings.ToLower(query), op) {
-			return "", fmt.Errorf("operation %s is not allowed", op)
-		}
-	}
-
 	vars := make(map[string]string)
 	for k, v := range postData.Variables {
 		vars[k] = metrics.FormattedValue(v)
@@ -1051,30 +1037,34 @@ func prepareQuery(r *http.Request) (string, error) {
 		return "", tmplErr
 	}
 
-	if !constants.IsDotMetricsEnabled {
-		return queryBuf.String(), nil
-	}
-
 	query = queryBuf.String()
 
-	// Now handle $var replacements (simple string replace)
-	keys := make([]string, 0, len(vars))
-	for k := range vars {
-		keys = append(keys, k)
+	if constants.IsDotMetricsEnabled {
+		// Now handle $var replacements (simple string replace)
+		keys := make([]string, 0, len(vars))
+		for k := range vars {
+			keys = append(keys, k)
+		}
+
+		sort.Slice(keys, func(i, j int) bool {
+			return len(keys[i]) > len(keys[j])
+		})
+
+		newQuery := query
+		for _, k := range keys {
+			placeholder := "$" + k
+			v := vars[k]
+			newQuery = strings.ReplaceAll(newQuery, placeholder, v)
+		}
+
+		query = newQuery
 	}
 
-	sort.Slice(keys, func(i, j int) bool {
-		return len(keys[i]) > len(keys[j])
-	})
-
-	newQuery := query
-	for _, k := range keys {
-		placeholder := "$" + k
-		v := vars[k]
-		newQuery = strings.ReplaceAll(newQuery, placeholder, v)
+	if err := querybuilder.ValidateReadOnlySelect(query); err != nil {
+		return "", err
 	}
 
-	return newQuery, nil
+	return query, nil
 }
 
 func (aH *APIHandler) Get(rw http.ResponseWriter, r *http.Request) {
@@ -1092,7 +1082,7 @@ func (aH *APIHandler) queryDashboardVarsV2(w http.ResponseWriter, r *http.Reques
 		return
 	}
 
-	dashboardVars, err := aH.reader.QueryDashboardVars(r.Context(), query)
+	dashboardVars, err := aH.reader.QueryDashboardVars(ctxtypes.SetClickhouseReadOnly(r.Context()), query)
 	if err != nil {
 		RespondError(w, &model.ApiError{Typ: model.ErrorBadData, Err: err}, nil)
 		return

pkg/query-service/app/http_handler_test.go

@@ -67,7 +67,26 @@ func TestPrepareQuery(t *testing.T) {
 				Query: "ALTER TABLE signoz_table DELETE where true",
 			},
 			expectedErr: true,
-			errMsg:      "operation alter table is not allowed",
+			errMsg:      "only SELECT statements are allowed in ClickHouse SQL queries",
+		},
+		{
+			name: "query smuggles a statement through a variable",
+			postData: &model.DashboardVars{
+				Query: "select * from table where id = {{.id}}",
+				Variables: map[string]interface{}{
+					"id": "1'; DROP TABLE signoz_table; --",
+				},
+			},
+			expectedErr: true,
+			errMsg:      "ClickHouse SQL must contain exactly one statement",
+		},
+		{
+			name: "query reads a system table",
+			postData: &model.DashboardVars{
+				Query: "SELECT name FROM system.users",
+			},
+			expectedErr: true,
+			errMsg:      "the ClickHouse system database is not allowed in SQL queries",
 		},
 		{
 			name: "query text produces template exec error",

pkg/querybuilder/clickhouse_sql.go

@@ -0,0 +1,68 @@
+package querybuilder
+
+import (
+	"strings"
+
+	chparser "github.com/AfterShip/clickhouse-sql-parser/parser"
+	"github.com/SigNoz/signoz/pkg/errors"
+)
+
+// internalDatabases hold server metadata, credentials and grants rather than telemetry.
+var internalDatabases = map[string]struct{}{
+	"system":             {},
+	"information_schema": {},
+}
+
+// ValidateReadOnlySelect rejects a user-authored ClickHouse statement unless it is a
+// single SELECT that reads telemetry: no table function, no internal database and no
+// lowering of the readonly setting. It must run on the rendered statement, since the
+// substituted variable values are user input too.
+func ValidateReadOnlySelect(query string) (err error) {
+	// The parser has a history of panicking on malformed input rather than returning
+	// an error. No input is known to still do so, but this reaches user-authored SQL,
+	// so a regression must not take the process down.
+	defer func() {
+		if recovered := recover(); recovered != nil {
+			err = errors.NewInvalidInputf(errors.CodeInvalidInput, "invalid ClickHouse SQL: %v", recovered)
+		}
+	}()
+
+	stmts, parseErr := chparser.NewParser(query).ParseStmts()
+	if parseErr != nil {
+		// The cause is carried in the message because the renderers drop the wrapped error.
+		return errors.NewInvalidInputf(errors.CodeInvalidInput, "invalid ClickHouse SQL: %s", parseErr.Error())
+	}
+
+	if len(stmts) != 1 {
+		return errors.NewInvalidInputf(errors.CodeInvalidInput, "ClickHouse SQL must contain exactly one statement")
+	}
+
+	selectQuery, ok := stmts[0].(*chparser.SelectQuery)
+	if !ok {
+		return errors.NewInvalidInputf(errors.CodeInvalidInput, "only SELECT statements are allowed in ClickHouse SQL queries")
+	}
+
+	visitor := &chparser.DefaultASTVisitor{Visit: func(node chparser.Expr) error {
+		switch expr := node.(type) {
+		case *chparser.TableFunctionExpr:
+			// Source table functions remain usable in ClickHouse read-only mode.
+			return errors.NewInvalidInputf(errors.CodeInvalidInput, "ClickHouse table functions are not allowed in SQL queries: %s", chparser.Format(expr.Name))
+		case *chparser.TableIdentifier:
+			// Reading these is unaffected by ClickHouse read-only mode.
+			if expr.Database == nil {
+				return nil
+			}
+			if _, ok := internalDatabases[strings.ToLower(expr.Database.Name)]; ok {
+				return errors.NewInvalidInputf(errors.CodeInvalidInput, "the ClickHouse %s database is not allowed in SQL queries", expr.Database.Name)
+			}
+		case *chparser.SettingExpr:
+			// A query-level setting takes precedence over the context setting.
+			if strings.EqualFold(expr.Name.Name, "readonly") {
+				return errors.NewInvalidInputf(errors.CodeInvalidInput, "the ClickHouse readonly setting cannot be overridden")
+			}
+		}
+		return nil
+	}}
+
+	return selectQuery.Accept(visitor)
+}

pkg/querybuilder/clickhouse_sql_test.go

@@ -0,0 +1,326 @@
+package querybuilder
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateReadOnlySelect(t *testing.T) {
+	testCases := []struct {
+		name  string
+		query string
+		pass  bool
+	}{
+		{
+			name:  "Select_Valid",
+			query: "SELECT region AS r, zone FROM metrics WHERE metric_name = 'cpu' GROUP BY region, zone",
+			pass:  true,
+		},
+		{
+			name:  "SelectWithTrailingSemicolon_Valid",
+			query: "SELECT count() FROM signoz_logs.distributed_logs_v2;",
+			pass:  true,
+		},
+		{
+			name:  "CommonTableExpression_Valid",
+			query: "WITH t AS (SELECT fingerprint FROM signoz_metrics.time_series_v4 GROUP BY fingerprint) SELECT * FROM t",
+			pass:  true,
+		},
+		{
+			name:  "Join_Valid",
+			query: "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.b",
+			pass:  true,
+		},
+		{
+			name:  "GlobalIn_Valid",
+			query: "SELECT a FROM t WHERE a GLOBAL IN (SELECT b FROM t2)",
+			pass:  true,
+		},
+		{
+			name:  "Union_Valid",
+			query: "SELECT * FROM t UNION ALL SELECT * FROM t2",
+			pass:  true,
+		},
+		{
+			name:  "WindowFunction_Valid",
+			query: "SELECT sum(v) OVER (PARTITION BY a ORDER BY t) FROM t",
+			pass:  true,
+		},
+		{
+			name:  "UnrelatedSetting_Valid",
+			query: "SELECT * FROM t SETTINGS max_threads = 4",
+			pass:  true,
+		},
+		{
+			name:  "Empty_Invalid",
+			query: "",
+			pass:  false,
+		},
+		{
+			name:  "Unparseable_Invalid",
+			query: "SELECT FROM WHERE",
+			pass:  false,
+		},
+		{
+			name:  "TerminatedBlockComment_Valid",
+			query: "SELECT /* keep me */ count() FROM t",
+			pass:  true,
+		},
+		{
+			name:  "BlockCommentMarkerInsideStringLiteral_Valid",
+			query: "SELECT count() FROM t WHERE body = '/* not a comment'",
+			pass:  true,
+		},
+		{
+			// The parser used to loop forever on this. It now reads the comment to the
+			// end of the input, so this doubles as a canary for that regression.
+			name:  "TrailingUnterminatedBlockComment_Valid",
+			query: "SELECT count() FROM t /* unterminated",
+			pass:  true,
+		},
+		{
+			name:  "UnterminatedBlockCommentOnly_Invalid",
+			query: "/* x",
+			pass:  false,
+		},
+		{
+			name:  "Intersect_Valid",
+			query: "SELECT * FROM t INTERSECT SELECT * FROM t2",
+			pass:  true,
+		},
+		{
+			name:  "IntersectReadingInternalDatabase_Invalid",
+			query: "SELECT * FROM t INTERSECT SELECT * FROM system.users",
+			pass:  false,
+		},
+		{
+			name:  "ExceptReadingTableFunction_Invalid",
+			query: "SELECT * FROM t EXCEPT SELECT * FROM url('http://x', CSV, 'a String')",
+			pass:  false,
+		},
+		{
+			name:  "MultipleStatements_Invalid",
+			query: "SELECT 1; DROP TABLE signoz_logs.logs_v2",
+			pass:  false,
+		},
+		{
+			name:  "Drop_Invalid",
+			query: "DROP TABLE signoz_logs.logs_v2",
+			pass:  false,
+		},
+		{
+			name:  "Insert_Invalid",
+			query: "INSERT INTO signoz_logs.logs_v2 SELECT * FROM signoz_logs.logs_v2",
+			pass:  false,
+		},
+		{
+			name:  "AlterDelete_Invalid",
+			query: "ALTER TABLE signoz_logs.logs_v2 DELETE WHERE 1 = 1",
+			pass:  false,
+		},
+		{
+			name:  "Truncate_Invalid",
+			query: "TRUNCATE TABLE signoz_logs.logs_v2",
+			pass:  false,
+		},
+		{
+			name:  "CreateTable_Invalid",
+			query: "CREATE TABLE evil (a Int) ENGINE = Memory",
+			pass:  false,
+		},
+		{
+			name:  "AttachTable_Invalid",
+			query: "ATTACH TABLE x",
+			pass:  false,
+		},
+		{
+			name:  "Optimize_Invalid",
+			query: "OPTIMIZE TABLE signoz_logs.logs_v2 FINAL",
+			pass:  false,
+		},
+		{
+			name:  "Grant_Invalid",
+			query: "GRANT ALL ON *.* TO admin",
+			pass:  false,
+		},
+		{
+			name:  "Describe_Invalid",
+			query: "DESCRIBE TABLE signoz_logs.logs_v2",
+			pass:  false,
+		},
+		{
+			name:  "Set_Invalid",
+			query: "SET readonly = 0",
+			pass:  false,
+		},
+		{
+			name:  "ShowGrants_Invalid",
+			query: "SHOW GRANTS",
+			pass:  false,
+		},
+		{
+			name:  "System_Invalid",
+			query: "SYSTEM SHUTDOWN",
+			pass:  false,
+		},
+		{
+			name:  "Kill_Invalid",
+			query: "KILL QUERY WHERE 1",
+			pass:  false,
+		},
+		{
+			name:  "IntoOutfile_Invalid",
+			query: "SELECT * FROM t INTO OUTFILE '/tmp/x.csv'",
+			pass:  false,
+		},
+		{
+			name:  "UrlTableFunction_Invalid",
+			query: "SELECT * FROM url('http://attacker.example/x', CSV, 'a String')",
+			pass:  false,
+		},
+		{
+			name:  "FileTableFunction_Invalid",
+			query: "SELECT * FROM file('/etc/passwd', CSV, 'a String')",
+			pass:  false,
+		},
+		{
+			name:  "RemoteTableFunction_Invalid",
+			query: "SELECT * FROM remote('attacker:9000', system.users)",
+			pass:  false,
+		},
+		{
+			name:  "S3TableFunction_Invalid",
+			query: "SELECT * FROM s3('https://x/y.csv', 'CSV')",
+			pass:  false,
+		},
+		{
+			name:  "MysqlTableFunction_Invalid",
+			query: "SELECT * FROM mysql('host:3306', 'db', 'tbl', 'u', 'p')",
+			pass:  false,
+		},
+		{
+			name:  "ExecutableTableFunction_Invalid",
+			query: "SELECT * FROM executable('script.sh', CSV, 'a String')",
+			pass:  false,
+		},
+		{
+			name:  "ClusterTableFunction_Invalid",
+			query: "SELECT * FROM cluster('c', system, users)",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInJoin_Invalid",
+			query: "SELECT * FROM t1 JOIN url('http://x', CSV, 'a String') u ON 1 = 1",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInScalarSubquery_Invalid",
+			query: "SELECT (SELECT * FROM url('http://x', CSV, 'a String'))",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInCommonTableExpression_Invalid",
+			query: "WITH c AS (SELECT * FROM url('http://x', CSV, 'a String')) SELECT * FROM c",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInNestedSubquery_Invalid",
+			query: "SELECT * FROM (SELECT * FROM (SELECT * FROM file('/etc/passwd', CSV, 'a String')))",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInWhereSubquery_Invalid",
+			query: "SELECT * FROM t WHERE a IN (SELECT * FROM url('http://x', CSV, 'a String'))",
+			pass:  false,
+		},
+		{
+			name:  "TableFunctionInUnion_Invalid",
+			query: "SELECT * FROM t UNION ALL SELECT * FROM url('http://x', CSV, 'a String')",
+			pass:  false,
+		},
+		{
+			name:  "SystemUsers_Invalid",
+			query: "SELECT * FROM system.users",
+			pass:  false,
+		},
+		{
+			name:  "SystemGrants_Invalid",
+			query: "SELECT * FROM system.grants",
+			pass:  false,
+		},
+		{
+			name:  "SystemQuoted_Invalid",
+			query: "SELECT count() FROM `system`.`tables`",
+			pass:  false,
+		},
+		{
+			name:  "SystemUppercase_Invalid",
+			query: "SELECT * FROM SYSTEM.USERS",
+			pass:  false,
+		},
+		{
+			name:  "SystemInSubquery_Invalid",
+			query: "SELECT * FROM (SELECT name FROM system.parts)",
+			pass:  false,
+		},
+		{
+			name:  "SystemInCommonTableExpression_Invalid",
+			query: "WITH c AS (SELECT * FROM system.query_log) SELECT * FROM c",
+			pass:  false,
+		},
+		{
+			name:  "SystemInJoin_Invalid",
+			query: "SELECT * FROM signoz_logs.distributed_logs_v2 AS l JOIN system.users AS u ON 1 = 1",
+			pass:  false,
+		},
+		{
+			name:  "SystemInUnion_Invalid",
+			query: "SELECT name FROM t UNION ALL SELECT name FROM system.tables",
+			pass:  false,
+		},
+		{
+			name:  "InformationSchema_Invalid",
+			query: "SELECT * FROM information_schema.tables",
+			pass:  false,
+		},
+		{
+			name:  "InformationSchemaUppercase_Invalid",
+			query: "SELECT * FROM INFORMATION_SCHEMA.COLUMNS",
+			pass:  false,
+		},
+		{
+			name:  "TableNamedSystemInTelemetryDatabase_Valid",
+			query: "SELECT * FROM signoz_logs.system",
+			pass:  true,
+		},
+		{
+			name:  "ReadonlySettingOverride_Invalid",
+			query: "SELECT * FROM t SETTINGS readonly = 0",
+			pass:  false,
+		},
+		{
+			name:  "ReadonlySettingOverrideUppercase_Invalid",
+			query: "SELECT * FROM t SETTINGS READONLY = 0",
+			pass:  false,
+		},
+		{
+			name:  "ReadonlySettingOverrideAlongsideOthers_Invalid",
+			query: "SELECT * FROM t SETTINGS max_threads = 4, readonly = 0",
+			pass:  false,
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			err := ValidateReadOnlySelect(testCase.query)
+
+			if testCase.pass {
+				assert.NoError(t, err)
+				return
+			}
+
+			assert.Error(t, err)
+		})
+	}
+}

pkg/telemetrystore/telemetrystorehook/settings.go

@@ -72,6 +72,12 @@ func (h *provider) BeforeQuery(ctx context.Context, _ *telemetrystore.QueryEvent
 		settings["result_overflow_mode"] = ctx.Value("result_overflow_mode")
 	}
 
+	// readonly = 2 allows reads and per-query setting changes, but no writes, DDL or
+	// SYSTEM statements, and ClickHouse refuses to lower it from within the statement.
+	if readOnly, ok := ctx.Value(ctxtypes.ClickhouseContextReadOnlyKey).(bool); ok && readOnly {
+		settings["readonly"] = 2
+	}
+
 	// TODO(srikanthccv): enable it when the "Cannot read all data" issue is fixed
 	// https://github.com/ClickHouse/ClickHouse/issues/82283
 	settings["secondary_indices_enable_bulk_filtering"] = false

pkg/types/ctxtypes/clickhouse.go

@@ -6,9 +6,17 @@ type ctxKey string
 
 const (
 	ClickhouseContextMaxThreadsKey ctxKey = "clickhouse_max_threads"
+	ClickhouseContextReadOnlyKey   ctxKey = "clickhouse_readonly"
 )
 
 // SetClickhouseMaxThreads stores the max threads value in context.
 func SetClickhouseMaxThreads(ctx context.Context, maxThreads int) context.Context {
 	return context.WithValue(ctx, ClickhouseContextMaxThreadsKey, maxThreads)
 }
+
+// SetClickhouseReadOnly marks the context so the statement runs under a read-only
+// ClickHouse session. The telemetry store connection is shared with write paths, so
+// those must never set this.
+func SetClickhouseReadOnly(ctx context.Context) context.Context {
+	return context.WithValue(ctx, ClickhouseContextReadOnlyKey, true)
+}