fix(querybuilder): refuse functions that read outside the telemetry tables

file, the dictionary accessors, catboostEvaluate and the introspection
functions read a file, a dictionary or the server binary while looking like
ordinary scalar calls. They name no table and no database, so none of the
existing rules saw them, and SELECT file('/etc/passwd') was accepted. A wrapper
returning a number leaks what they read through the row count alone:
numbers(length(file(x))) yields one row per byte.

Two more holes in the same function:

  x IN db.table reads that table, and a qualified name on the right of IN is a
  Path rather than a TableIdentifier, so system.users was reachable there.

  The generator allow list matched on the formatted name, which carries the
  quoting, so a backtick-quoted generator was refused.

go.mod

@@ -4,7 +4,7 @@ go 1.25.7
 
 require (
 	dario.cat/mergo v1.0.2
-	github.com/AfterShip/clickhouse-sql-parser v0.5.4
+	github.com/AfterShip/clickhouse-sql-parser v0.5.5
 	github.com/ClickHouse/clickhouse-go/v2 v2.44.0
 	github.com/DATA-DOG/go-sqlmock v1.5.2
 	github.com/SigNoz/clickhouse-go-mock v0.14.0

go.sum

@@ -66,8 +66,8 @@ dario.cat/mergo v1.0.2/go.mod h1:E/hbnu0NxMFBjpMIE34DRGLWqDy0g5FuKDhCb31ngxA=
 dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU=
 filippo.io/edwards25519 v1.2.0 h1:crnVqOiS4jqYleHd9vaKZ+HKtHfllngJIiOpNpoJsjo=
 filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q77mc=
-github.com/AfterShip/clickhouse-sql-parser v0.5.4 h1:yiCQaMq8EO+dpKdnpP9YYd/ne6MSuOXgsMsNL33NiTI=
-github.com/AfterShip/clickhouse-sql-parser v0.5.4/go.mod h1:Qi3qvPTfZb/aFwI5V4WFOahgjsLJa4MzVijIAfwOhDw=
+github.com/AfterShip/clickhouse-sql-parser v0.5.5 h1:LCA23yAA4GgF73PoYXb67yzCdC4sXsj4geQz1Oij3U8=
+github.com/AfterShip/clickhouse-sql-parser v0.5.5/go.mod h1:Qi3qvPTfZb/aFwI5V4WFOahgjsLJa4MzVijIAfwOhDw=
 github.com/Azure/azure-sdk-for-go v68.0.0+incompatible h1:fcYLmCpyNYRnvJbPerq7U0hS+6+I79yEDJBqVNcqUzU=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0 h1:fou+2+WFTib47nS+nz/ozhEBnvU96bKHy6LjRsY4E28=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0/go.mod h1:t76Ruy8AHvUAC8GfMWJMa0ElSbuIcO03NLpynfbgsPA=

pkg/querybuilder/clickhouse_sql.go

@@ -17,6 +17,7 @@ var (
 	CodeClickHouseSQLNotSingleStatement = errors.MustNewCode("clickhouse_sql_not_single_statement")
 	CodeClickHouseSQLNotSelect          = errors.MustNewCode("clickhouse_sql_not_select")
 	CodeClickHouseSQLTableFunction      = errors.MustNewCode("clickhouse_sql_table_function")
+	CodeClickHouseSQLReadingFunction    = errors.MustNewCode("clickhouse_sql_reading_function")
 	CodeClickHouseSQLInternalDatabase   = errors.MustNewCode("clickhouse_sql_internal_database")
 	CodeClickHouseSQLReadonlyOverride   = errors.MustNewCode("clickhouse_sql_readonly_override")
 )
@@ -43,6 +44,44 @@ var generatorTableFunctions = map[string]string{
 
 var generatorTableFunctionsMessage = "allowed table functions are " + strings.Join(slices.Sorted(maps.Values(generatorTableFunctions)), ", ")
 
+// readingFunctions reach a file, a model or the server binary while looking like ordinary
+// scalar functions. They name no table and no database, so neither of the rules above sees
+// them, and a wrapper that returns a number leaks what they read through the row count alone:
+// numbers(length(file(x))) yields one row per byte.
+//
+// Keyed by the lowercased name, since ClickHouse resolves function names case-insensitively.
+var readingFunctions = map[string]struct{}{
+	"file":                     {},
+	"catboostevaluate":         {},
+	"demangle":                 {},
+	"addresstoline":            {},
+	"addresstolinewithinlines": {},
+	"addresstosymbol":          {},
+}
+
+// A dictionary can be backed by HTTP, ODBC or another database, and every one of the 42
+// accessors carries this prefix.
+const dictionaryFunctionPrefix = "dict"
+
+func errIfFunctionReads(name string) error {
+	lowered := strings.ToLower(name)
+	if _, ok := readingFunctions[lowered]; !ok && !strings.HasPrefix(lowered, dictionaryFunctionPrefix) {
+		return nil
+	}
+
+	return errors.NewInvalidInputf(CodeClickHouseSQLReadingFunction, "ClickHouse functions that read outside the telemetry tables are not allowed in SQL queries: %s", name)
+}
+
+// The parser spells a call's name as an Ident everywhere it can. Reading the field rather than
+// formatting the node keeps the quoting out, so `numbers`(1) matches numbers.
+func functionName(expr chparser.Expr) string {
+	if ident, ok := expr.(*chparser.Ident); ok {
+		return ident.Name
+	}
+
+	return chparser.Format(expr)
+}
+
 // The parser's grammar has gaps against SQL that ClickHouse itself accepts.
 func ErrIfStatementIsNotValid(query string) (err error) {
 	defer func() {
@@ -69,11 +108,23 @@ func ErrIfStatementIsNotValid(query string) (err error) {
 
 	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. Arguments are
-			// visited before this, so a read smuggled into one is already refused by the time
-			// an allowed generator gets here.
-			name := chparser.Format(expr.Name)
+		case *chparser.TableExpr:
+			// Source table functions remain usable in ClickHouse read-only mode, and only a
+			// table position can be one. The parser also types a call inside a table function's
+			// argument list as a TableFunctionExpr, so asking every one of those refuses the
+			// numbers(intDiv(...)) that every dashboard writes. What can read from an argument
+			// is caught by name below instead.
+			source := expr.Expr
+			if alias, ok := source.(*chparser.AliasExpr); ok {
+				source = alias.Expr
+			}
+
+			tableFunction, ok := source.(*chparser.TableFunctionExpr)
+			if !ok {
+				return nil
+			}
+
+			name := functionName(tableFunction.Name)
 			if _, ok := generatorTableFunctions[strings.ToLower(name)]; ok {
 				return nil
 			}
@@ -82,6 +133,25 @@ func ErrIfStatementIsNotValid(query string) (err error) {
 				NewInvalidInputf(CodeClickHouseSQLTableFunction, "ClickHouse table functions are not allowed in SQL queries: %s", name).
 				WithAdditional(generatorTableFunctionsMessage)
 
+		case *chparser.FunctionExpr:
+			return errIfFunctionReads(expr.Name.Name)
+
+		case *chparser.TableFunctionExpr:
+			// Reached for a call in an argument list, and for a table position ahead of the
+			// TableExpr above, since a node is visited after its children.
+			return errIfFunctionReads(functionName(expr.Name))
+
+		case *chparser.Path:
+			// ClickHouse reads `x IN db.table` as a select from that table, and a qualified name
+			// on the right of IN is a Path rather than a TableIdentifier.
+			if len(expr.Fields) < 2 {
+				return nil
+			}
+
+			if _, ok := internalDatabases[strings.ToLower(expr.Fields[0].Name)]; ok {
+				return errors.NewInvalidInputf(CodeClickHouseSQLInternalDatabase, "the ClickHouse %s database is not allowed in SQL queries", expr.Fields[0].Name)
+			}
+
 		case *chparser.TableIdentifier:
 			// Reading these is unaffected by ClickHouse read-only mode.
 			if expr.Database == nil {

pkg/querybuilder/clickhouse_sql_test.go

@@ -7,20 +7,48 @@ import (
 	"github.com/SigNoz/signoz/pkg/errors"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+type refusalTestCase struct {
+	name         string
+	query        string
+	expectedCode errors.Code
+}
+
+func assertRefused(t *testing.T, testCases []refusalTestCase) {
+	t.Helper()
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			errC := make(chan error, 1)
+			go func() { errC <- ErrIfStatementIsNotValid(testCase.query) }()
+
+			var err error
+			select {
+			case err = <-errC:
+			case <-time.After(10 * time.Second):
+				require.Fail(t, "timed out, which means the parser is no longer bounding its backtracking")
+			}
+
+			// Required rather than asserted: errors.Asc dereferences the error it is given.
+			require.Error(t, err)
+			assert.True(t, errors.Asc(err, testCase.expectedCode), "expected code %s, got %v", testCase.expectedCode, err)
+		})
+	}
+}
+
 func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 	testCases := []struct {
 		name  string
 		query string
 	}{
-		// Shapes a telemetry read is allowed to take.
 		{"Select", "SELECT region AS r, zone FROM metrics WHERE metric_name = 'cpu' GROUP BY region, zone"},
 		{"TrailingSemicolon", "SELECT count() FROM signoz_logs.distributed_logs_v2;"},
 		{"CommonTableExpression", "WITH t AS (SELECT fingerprint FROM signoz_metrics.time_series_v4) SELECT * FROM t"},
 		{"Join", "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.b"},
 		{"GlobalIn", "SELECT a FROM t WHERE a GLOBAL IN (SELECT b FROM t2)"},
-		// GLOBAL parsed only when the join type was omitted, and only before IN. https://github.com/AfterShip/clickhouse-sql-parser/pull/293
+		// https://github.com/AfterShip/clickhouse-sql-parser/pull/293
 		{"GlobalLeftJoin", "SELECT * FROM t1 GLOBAL LEFT JOIN t2 ON t1.a = t2.a"},
 		{"GlobalNotIn", "SELECT a FROM t WHERE a GLOBAL NOT IN (SELECT b FROM t2)"},
 		{"Union", "SELECT * FROM t UNION ALL SELECT * FROM t2"},
@@ -29,32 +57,34 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 		{"UnrelatedSetting", "SELECT * FROM t SETTINGS max_threads = 4"},
 		{"TerminatedBlockComment", "SELECT /* keep me */ count() FROM t"},
 		{"BlockCommentMarkerInsideStringLiteral", "SELECT count() FROM t WHERE body = '/* not a comment'"},
-		// 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.
+		// Looped forever before v0.5.2.
 		{"TrailingUnterminatedBlockComment", "SELECT count() FROM t /* unterminated"},
-		// The rule keys on the database, not on the table name.
+		// Keyed on the database, not on the table name.
 		{"TableNamedSystemInTelemetryDatabase", "SELECT * FROM signoz_logs.system"},
 		{"SignedLiteralAfterClosingParenSpaced", "SELECT (toUnixTimestamp(now()) - 3600)*1000000000"},
-		// order by interval
 		{"OrderByInterval", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval ORDER BY interval"},
 		{"OrderByIntervalAndDirection", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS `interval` ORDER BY `interval` ASC"},
-		// `interval` is a unit keyword, so unquoting it was rejected everywhere the parser
-		// expected a plain identifier. https://github.com/AfterShip/clickhouse-sql-parser/pull/296
+		// https://github.com/AfterShip/clickhouse-sql-parser/pull/296
 		{"OrderByUnquotedIntervalAsc", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval FROM t GROUP BY interval ORDER BY interval ASC"},
 		{"OrderByUnquotedIntervalDesc", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval FROM t GROUP BY interval ORDER BY interval DESC"},
 		{"UnquotedIntervalInGroupByTuple", "SELECT a FROM t GROUP BY (`service.name`, `service.version`, interval)"},
 		{"UnquotedIntervalProductionQuery", "SELECT toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, resource_string_service$$name AS `service.name`, attributes_string['http.route'] AS `http.route`, quantile(0.95)(duration_nano) / 1000000000 AS value FROM signoz_traces.distributed_signoz_index_v3 WHERE resource_string_service$$name = 'svc-a' AND resources_string['deployment.environment'] = 'dev' AND attributes_string['http.route'] = '/v1' AND http_method = 'POST' AND timestamp BETWEEN toDateTime(1784601720) AND toDateTime(1784602620) AND ts_bucket_start BETWEEN 1784601720 - 1800 AND 1784602620 GROUP BY `service.name`, `http.route`, interval ORDER BY interval ASC"},
-		// Separating the two readings of INTERVAL needs backtracking as per the current implementation which could have performance regressions.
-		// https://github.com/AfterShip/clickhouse-sql-parser/pull/296#issuecomment-5150316367
+		// The fix backtracks, so this bounds the cost. https://github.com/AfterShip/clickhouse-sql-parser/pull/296#issuecomment-5150316367
 		{"UnquotedIntervalRepeatedThirtyTimes", "SELECT interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval + interval AS total FROM t WHERE interval > 0 ORDER BY interval ASC"},
+		// `interval` was one of 37 such keywords. https://github.com/AfterShip/clickhouse-sql-parser/pull/305
+		{"UnquotedLimitInFunctionArgument", "SELECT sum(limit) FROM t"},
+		{"UnquotedLimitInArithmetic", "SELECT limit + 1 FROM t"},
+		{"UnquotedLimitInNegation", "SELECT abs(-limit) FROM t"},
+		{"UnquotedKeywordOperands", "SELECT sum(offset) + sum(format) + sum(settings) FROM t"},
+		{"UnquotedLimitProductionQuery", "WITH limit_value AS (SELECT cluster, region, value AS limit FROM t) SELECT region AS `Region`, sum(limit) AS `Capacity` FROM limit_value GROUP BY Region"},
 		{"SignedLiteralAfterClosingParenUnspaced", "SELECT now() AS ts, toFloat64(count()) AS value FROM ( SELECT attributes_string['TableName'] AS T, attributes_string['MissingId'] AS M, max(fromUnixTimestamp64Nano(timestamp)) AS last_seen, dateDiff('minute', min(fromUnixTimestamp64Nano(timestamp)), max(fromUnixTimestamp64Nano(timestamp))) AS age_min FROM signoz_logs.distributed_logs_v2 WHERE body='missing_map_record' AND timestamp >= (toUnixTimestamp(now())-3600)*1000000000 GROUP BY T, M ) WHERE age_min >= 20 AND last_seen >= now() - toIntervalMinute(8)"},
 		{"SignedLiteralAfterClosingParenMinimal", "SELECT (1)-1"},
 		{"TrimFunction", "SELECT trimBoth('/api/endpoint/', '/');"},
-		// The SQL-standard keyword-separated argument forms, which took commas only. https://github.com/AfterShip/clickhouse-sql-parser/pull/290
+		// https://github.com/AfterShip/clickhouse-sql-parser/pull/290
 		{"StandardTrimSyntax", "SELECT trim(BOTH ' ' FROM body) FROM t"},
 		{"StandardSubstringSyntax", "SELECT substring(body FROM 2 FOR 3) FROM t"},
 		{"StandardOverlaySyntax", "SELECT overlay(body PLACING 'x' FROM 2) FROM t"},
-		// Row generators compute their rows from their arguments, so they read through nothing. This is the shape they get used for: a dense interval axis to CROSS JOIN a sparse series against.
+		// The shape row generators get used for: a dense interval axis to CROSS JOIN a sparse series against.
 		{"NumbersTableFunction", "SELECT intervals.interval AS interval, active.cluster AS cluster, toFloat64(if(ts_data.has_data = 0, 0, 1)) AS value FROM ( SELECT DISTINCT JSONExtractString(labels, 'k8s.cluster.name') AS cluster FROM signoz_metrics.distributed_time_series_v4 WHERE metric_name = 'my_metric' AND unix_milli >= toUnixTimestamp(now() - INTERVAL 30 DAY) * 1000 HAVING cluster != '' ) AS active CROSS JOIN ( SELECT toStartOfInterval( toDateTime(toUnixTimestamp(now() - INTERVAL 30 MINUTE) + number * 60), INTERVAL 1 MINUTE ) AS interval FROM numbers(31) ) AS intervals LEFT JOIN ( SELECT toStartOfInterval( toDateTime(intDiv(s.unix_milli, 1000)), INTERVAL 1 MINUTE ) AS interval, JSONExtractString(ts.labels, 'k8s.cluster.name') AS cluster, 1 AS has_data FROM signoz_metrics.distributed_samples_v4 s INNER JOIN ( SELECT DISTINCT fingerprint, labels FROM signoz_metrics.distributed_time_series_v4 WHERE metric_name = 'my_metric' ) AS ts ON s.fingerprint = ts.fingerprint WHERE s.metric_name = 'my_metric' AND s.unix_milli >= toUnixTimestamp(now() - INTERVAL 30 MINUTE) * 1000 GROUP BY interval, cluster ) AS ts_data ON active.cluster = ts_data.cluster AND intervals.interval = ts_data.interval ORDER BY interval ASC"},
 		{"NumbersMtTableFunction", "SELECT * FROM numbers_mt(31)"},
 		{"ZerosTableFunction", "SELECT * FROM zeros(31)"},
@@ -63,6 +93,16 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 		{"GenerateSeriesSnakeCaseTableFunction", "SELECT * FROM generate_series(1, 10)"},
 		{"GeneratorTableFunctionUppercase", "SELECT * FROM NUMBERS(31)"},
 		{"GeneratorTableFunctionParenthesisedArgument", "SELECT * FROM NUMBERS((31))"},
+		// CAST in an argument was itself read as a table function. https://github.com/AfterShip/clickhouse-sql-parser/pull/307
+		{"CastInGeneratorTableFunctionArgument", "SELECT * FROM numbers(CAST(10 AS UInt64))"},
+		{"ScalarCallInGeneratorTableFunctionArgument", "SELECT * FROM numbers(intDiv(100, 2))"},
+		{"NestedScalarCallInGeneratorTableFunctionArgument", "SELECT * FROM numbers(greatest(1, intDiv(100, 2) + 1))"},
+		{"GeneratorTableFunctionProductionQuery", "WITH toInt64(1786029960000000000) AS start_ns, toInt64(1786031760000000000) AS end_ns, 300000000000 AS step_ns SELECT ts, toFloat64(sum(value)) AS value FROM (SELECT fromUnixTimestamp64Nano(start_ns + toInt64(number) * step_ns) AS ts, 0 AS value FROM numbers(greatest(1, intDiv(end_ns - start_ns, step_ns) + 1)) UNION ALL SELECT toStartOfInterval(fromUnixTimestamp64Nano(timestamp), INTERVAL 5 minute) AS ts, count() AS value FROM signoz_logs.distributed_logs_v2 WHERE timestamp >= 1786029960000000000 AND timestamp <= 1786031760000000000 GROUP BY ts) GROUP BY ts ORDER BY ts"},
+		// The allow list keys on the bare name, so quoting must not hide a generator from it.
+		{"BacktickQuotedGeneratorTableFunction", "SELECT * FROM `numbers`(31)"},
+		{"DoubleQuotedGeneratorTableFunction", "SELECT * FROM \"numbers\"(31)"},
+		// Reads nothing: format builds a string, and shares its name with a table function.
+		{"ScalarFunctionNamedAfterATableFunction", "SELECT format('{} {}', a, b) FROM t"},
 		{"GeneratorTableFunctionInJoin", "SELECT * FROM signoz_logs.distributed_logs_v2 AS l CROSS JOIN numbers(31) AS n"},
 		{"GeneratorTableFunctionInCommonTableExpression", "WITH axis AS (SELECT number FROM numbers(31)) SELECT * FROM axis"},
 		{"GeneratorTableFunctionInWhereSubquery", "SELECT * FROM t WHERE a IN (SELECT number FROM numbers(31))"},
@@ -71,8 +111,7 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			// Bounded rather than called directly: a parser that backtracks without memoising
-			// hangs instead of returning. Every case here parses in well under a millisecond.
+			// Bounded because a parser that backtracks without memoising hangs rather than returning.
 			errC := make(chan error, 1)
 			go func() { errC <- ErrIfStatementIsNotValid(testCase.query) }()
 
@@ -87,51 +126,58 @@ func TestErrIfStatementIsNotValid_Pass(t *testing.T) {
 }
 
 func TestErrIfStatementIsNotValid_Fail(t *testing.T) {
-	testCases := []struct {
-		name         string
-		query        string
-		expectedCode errors.Code
-	}{
-		// Not a single statement, or not a statement at all.
+	testCases := []refusalTestCase{
 		{"Empty", "", CodeClickHouseSQLNotSingleStatement},
 		{"UnterminatedBlockCommentOnly", "/* x", CodeClickHouseSQLUnparseable},
 		{"Unparseable", "SELECT FROM WHERE", CodeClickHouseSQLUnparseable},
 		{"MultipleStatements", "SELECT 1; DROP TABLE signoz_logs.logs_v2", CodeClickHouseSQLNotSingleStatement},
-		// Parses, but is not a SELECT.
 		{"Drop", "DROP TABLE signoz_logs.logs_v2", CodeClickHouseSQLNotSelect},
 		{"Insert", "INSERT INTO signoz_logs.logs_v2 SELECT * FROM signoz_logs.logs_v2", CodeClickHouseSQLNotSelect},
 		{"AlterDelete", "ALTER TABLE signoz_logs.logs_v2 DELETE WHERE 1 = 1", CodeClickHouseSQLNotSelect},
 		{"CreateTable", "CREATE TABLE evil (a Int) ENGINE = Memory", CodeClickHouseSQLNotSelect},
 		{"Grant", "GRANT ALL ON *.* TO admin", CodeClickHouseSQLNotSelect},
 		{"Set", "SET readonly = 0", CodeClickHouseSQLNotSelect},
-		// The parser still dereferences nil on a DEFAULT expression it cannot read, so the recover is what turns this into a rejection rather than a crash.
-		{"UnparseableDefaultExpression", "CREATE TABLE t (a String DEFAULT foo(b FROM 2)) ENGINE = Memory", CodeClickHouseSQLParserPanic},
-		// These the parser rejects outright rather than classifying.
+		// Both panicked before v0.5.5. https://github.com/AfterShip/clickhouse-sql-parser/pull/306
+		{"UnparseableDefaultExpression", "CREATE TABLE t (a String DEFAULT foo(b FROM 2)) ENGINE = Memory", CodeClickHouseSQLUnparseable},
+		{"TrailingOperatorInDefaultExpression", "CREATE TABLE t (a String DEFAULT 1 +) ENGINE = Memory", CodeClickHouseSQLUnparseable},
+		// Rejected outright rather than classified.
 		{"ShowGrants", "SHOW GRANTS", CodeClickHouseSQLUnparseable},
 		{"IntoOutfile", "SELECT * FROM t INTO OUTFILE '/tmp/x.csv'", CodeClickHouseSQLUnparseable},
-		// Table functions, which read through something other than a telemetry table.
 		{"UrlTableFunction", "SELECT * FROM url('http://attacker.example/x', CSV, 'a String')", CodeClickHouseSQLTableFunction},
-		{"FileTableFunction", "SELECT * FROM file('/etc/passwd', CSV, 'a String')", CodeClickHouseSQLTableFunction},
+		// file is also a scalar function, so the reading rule reaches it before the table rule does.
+		{"FileTableFunction", "SELECT * FROM file('/etc/passwd', CSV, 'a String')", CodeClickHouseSQLReadingFunction},
 		{"ExecutableTableFunction", "SELECT * FROM executable('script.sh', CSV, 'a String')", CodeClickHouseSQLTableFunction},
 		{"TableFunctionInJoin", "SELECT * FROM t1 JOIN url('http://x', CSV, 'a String') u ON 1 = 1", CodeClickHouseSQLTableFunction},
 		{"TableFunctionInCommonTableExpression", "WITH c AS (SELECT * FROM url('http://x', CSV, 'a String')) SELECT * FROM c", CodeClickHouseSQLTableFunction},
-		{"TableFunctionInWhereSubquery", "SELECT * FROM t WHERE a IN (SELECT * FROM file('/etc/passwd', CSV, 'a String'))", CodeClickHouseSQLTableFunction},
+		{"TableFunctionInWhereSubquery", "SELECT * FROM t WHERE a IN (SELECT * FROM url('http://x', CSV, 'a String'))", CodeClickHouseSQLTableFunction},
 		{"TableFunctionInUnion", "SELECT * FROM t UNION ALL SELECT * FROM url('http://x', CSV, 'a String')", CodeClickHouseSQLTableFunction},
-		// These reach the internal databases without ever naming one, so the table-function rule is the only thing that sees them.
+		// Reach an internal database without naming one, so only the table-function rule sees them.
 		{"MergeTableFunction", "SELECT * FROM merge('system', '.*')", CodeClickHouseSQLTableFunction},
 		{"RemoteTableFunction", "SELECT * FROM remote('other-host', 'system.users')", CodeClickHouseSQLTableFunction},
 		{"ClusterTableFunction", "SELECT * FROM cluster('c', 'system.users')", CodeClickHouseSQLTableFunction},
-		// Pure, but excluded: generateRandom streams rows the arguments do not bound, and values has no use here that an array literal does not already cover.
+		// Pure, but excluded: generateRandom is unbounded, and values adds nothing over an array literal.
 		{"GenerateRandomTableFunction", "SELECT * FROM generateRandom('a UInt64')", CodeClickHouseSQLTableFunction},
 		{"ValuesTableFunction", "SELECT * FROM values('a UInt64', 1, 2)", CodeClickHouseSQLTableFunction},
-		// Arguments are visited before the table function itself, so allowing a generator does not give anyone a wrapper to smuggle a read through.
+		// Arguments are visited first, so an allowed generator is not a wrapper to smuggle a read through.
 		{"InternalDatabaseInsideAllowedTableFunction", "SELECT * FROM numbers((SELECT count() FROM system.users))", CodeClickHouseSQLInternalDatabase},
 		{"InternalDatabaseJoinedOntoAllowedTableFunction", "SELECT * FROM numbers(31) AS n JOIN system.users AS u ON 1 = 1", CodeClickHouseSQLInternalDatabase},
 		{"InternalDatabaseUnionedWithAllowedTableFunction", "SELECT number FROM numbers(31) UNION ALL SELECT name FROM system.users", CodeClickHouseSQLInternalDatabase},
 		{"RefusedTableFunctionJoinedOntoAllowedTableFunction", "SELECT * FROM numbers(31) AS n JOIN url('http://x', CSV, 'a String') AS u ON 1 = 1", CodeClickHouseSQLTableFunction},
-		{"RefusedTableFunctionInsideAllowedTableFunction", "SELECT * FROM numbers((SELECT count() FROM file('/etc/passwd', CSV, 'a String')))", CodeClickHouseSQLTableFunction},
+		{"RefusedTableFunctionInsideAllowedTableFunction", "SELECT * FROM numbers((SELECT count() FROM url('http://x', CSV, 'a String')))", CodeClickHouseSQLTableFunction},
 		{"InternalDatabaseInsideAllowedTableFunctionCommonTableExpression", "WITH axis AS (SELECT * FROM numbers((SELECT count() FROM system.users))) SELECT * FROM axis", CodeClickHouseSQLInternalDatabase},
-		// Internal databases, which hold grants and server metadata rather than telemetry.
+		// Read a file, a dictionary or the server binary without naming a table, so neither the table rule nor the database rule sees them. The row count alone is an oracle: numbers(length(file(x))) returns one row per byte.
+		{"ScalarFileFunction", "SELECT file('/etc/passwd')", CodeClickHouseSQLReadingFunction},
+		{"ScalarFileFunctionInWhere", "SELECT * FROM t WHERE length(file('/etc/passwd')) > 0", CodeClickHouseSQLReadingFunction},
+		{"ScalarFileFunctionInGeneratorTableFunctionArgument", "SELECT * FROM numbers(length(file('/etc/passwd')))", CodeClickHouseSQLReadingFunction},
+		{"DictionaryFunction", "SELECT dictGetUInt64('d', 'k', toUInt64(1))", CodeClickHouseSQLReadingFunction},
+		{"DictionaryFunctionUppercase", "SELECT DICTGETSTRING('d', 'k', toUInt64(1))", CodeClickHouseSQLReadingFunction},
+		{"DictionaryFunctionInGeneratorTableFunctionArgument", "SELECT * FROM numbers(dictGetUInt64('d', 'k', toUInt64(1)))", CodeClickHouseSQLReadingFunction},
+		{"IntrospectionFunction", "SELECT demangle(addressToSymbol(toUInt64(1)))", CodeClickHouseSQLReadingFunction},
+		{"ModelEvaluationFunction", "SELECT catboostEvaluate('/model.bin', 1)", CodeClickHouseSQLReadingFunction},
+		// ClickHouse reads `x IN table` as `x IN (SELECT * FROM table)`, and a qualified name there is a Path rather than a TableIdentifier.
+		{"InternalDatabaseInInOperator", "SELECT * FROM t WHERE a IN system.users", CodeClickHouseSQLInternalDatabase},
+		{"InternalDatabaseInGlobalInOperator", "SELECT * FROM t WHERE a GLOBAL IN system.users", CodeClickHouseSQLInternalDatabase},
+		{"InternalDatabaseInNotInOperator", "SELECT * FROM t WHERE a NOT IN system.users", CodeClickHouseSQLInternalDatabase},
 		{"SystemUsers", "SELECT * FROM system.users", CodeClickHouseSQLInternalDatabase},
 		{"SystemUppercase", "SELECT * FROM SYSTEM.USERS", CodeClickHouseSQLInternalDatabase},
 		{"SystemQuoted", "SELECT count() FROM `system`.`tables`", CodeClickHouseSQLInternalDatabase},
@@ -139,17 +185,23 @@ func TestErrIfStatementIsNotValid_Fail(t *testing.T) {
 		{"SystemInJoin", "SELECT * FROM signoz_logs.distributed_logs_v2 AS l JOIN system.users AS u ON 1 = 1", CodeClickHouseSQLInternalDatabase},
 		{"SystemInIntersect", "SELECT * FROM t INTERSECT SELECT * FROM system.users", CodeClickHouseSQLInternalDatabase},
 		{"InformationSchema", "SELECT * FROM information_schema.tables", CodeClickHouseSQLInternalDatabase},
-		// A query-level setting takes precedence over the one the caller applies.
+		// Takes precedence over the setting the caller applies.
 		{"ReadonlySettingOverride", "SELECT * FROM t SETTINGS readonly = 0", CodeClickHouseSQLReadonlyOverride},
 		{"ReadonlySettingOverrideAmongOthers", "SELECT * FROM t SETTINGS max_threads = 4, readonly = 0", CodeClickHouseSQLReadonlyOverride},
 	}
 
-	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			err := ErrIfStatementIsNotValid(testCase.query)
-
-			assert.Error(t, err)
-			assert.True(t, errors.Asc(err, testCase.expectedCode), "expected code %s, got %v", testCase.expectedCode, err)
-		})
-	}
+	assertRefused(t, testCases)
+}
+
+func TestErrIfStatementIsNotValid_ShouldPassButFails(t *testing.T) {
+	testCases := []refusalTestCase{
+		// The left operand commits the parser to a subquery, leaving the operator nowhere to bind. Parenthesising only the right operand is fine.
+		{"ParenthesisedUnionLeftOperand", "SELECT a FROM ((SELECT 1 AS a) UNION ALL (SELECT 2 AS a))", CodeClickHouseSQLUnparseable},
+		{"ParenthesisedExceptLeftOperand", "SELECT a FROM ((SELECT 1 AS a) EXCEPT (SELECT 2 AS a))", CodeClickHouseSQLUnparseable},
+		{"ParenthesisedUnionLeftOperandAtStatementLevel", "(SELECT 1 AS a) UNION ALL (SELECT 2 AS a)", CodeClickHouseSQLUnparseable},
+		// The one keyword PR 305 left behind, because ON also opens a join condition.
+		{"UnquotedOnAsColumnName", "SELECT on + 1 FROM t", CodeClickHouseSQLUnparseable},
+	}
+
+	assertRefused(t, testCases)
 }