mirror of
https://github.com/SigNoz/signoz.git
synced 2026-08-07 21:50:39 +01:00
Some checks failed
build-staging / prepare (push) Has been cancelled
build-staging / js-build (push) Has been cancelled
build-staging / go-build (push) Has been cancelled
build-staging / staging (push) Has been cancelled
cacheci / tests (push) Has been cancelled
Release Drafter / update_release_draft (push) Has been cancelled
Bumps `clickhouse-sql-parser` to v0.5.5, fixes the false rejection that was left over once it landed, and closes three holes in the same validator that the first two changes brought to light. ## The bump **Reserved keywords as expression operands** ([#305](https://github.com/AfterShip/clickhouse-sql-parser/pull/305)). `interval` was fixed in v0.5.4, but the same defect affected 36 other keywords once the column appeared as an operand rather than bare. Sweeping 94 candidates against ClickHouse 26.8.1.337, only `on` still rejects — and ClickHouse runs that too. This one was live: `sum(limit)` on a metric label. **Panic on an unparseable `DEFAULT` expression** ([#306](https://github.com/AfterShip/clickhouse-sql-parser/pull/306)). Both known cases return a parse error now instead of dereferencing nil. The `recover` in `ErrIfStatementIsNotValid` stays — it guards the next one of these, not these two. [#307](https://github.com/AfterShip/clickhouse-sql-parser/pull/307) also allows `CAST` in a table function's argument list. ## Table functions are only table functions in a table position The parser types a call inside a table function's argument list as a `TableFunctionExpr` as well, so the generator allow list only ever cleared a generator whose argument was a literal. Every real dashboard computes its row count — `numbers(greatest(1, intDiv(end_ns - start_ns, step_ns) + 1))` — and every one was refused, on `intDiv` rather than on `numbers`. `TableExpr.Expr` is the only table position a SELECT can reach, so the allow list asks that instead. Of the four places the parser builds a `TableFunctionExpr`, two are `CREATE TABLE` paths rejected as not-a-SELECT before the walk starts, one is `parseTableArgPrimaryExpr`, and one is the `FROM`/`JOIN` path that wraps into a `TableExpr`. ## Three holes that were already open Skipping argument position is only safe if nothing there can read, and that turned out not to be true — not because of this change, but independently of it. **Reading functions.** `file` is both a table function and a scalar function, and the validator never inspected scalar calls at all. On `main` today, `SELECT file('/etc/passwd')` is accepted and returns the file. A numeric wrapper passes ClickHouse's type check, so the row count alone is an oracle: `numbers(length(file(x)))` yields one row per byte. The same applies to the 42 dictionary accessors, which can be backed by HTTP, ODBC or another database, to `catboostEvaluate`, and to the introspection functions. All are now refused by name wherever they appear, under `clickhouse_sql_reading_function`. **`x IN db.table`.** ClickHouse reads this as `x IN (SELECT * FROM db.table)`, and a qualified name on the right of `IN` parses as a `Path`, not a `TableIdentifier` — so `SELECT * FROM t WHERE a IN system.users` bypassed the internal-database rule entirely. Now checked, including the `GLOBAL IN` and `NOT IN` forms. **Quoted generator names.** The allow list matched on the formatted name, which carries the quoting, so ``SELECT * FROM `numbers`(31)`` was refused. It now reads the identifier the way the internal-database branch already did. ## Effect Replaying 72 distinct shapes of production `clickhouse_sql` that the validator currently rejects: **64 pass, up from 59 on v0.5.4**. Two came from the bump, three from the table-position change, and those three are 379 of the 1390 sampled occurrences. The three new rules add no false positives to the corpus. Of the eight left, four are correct rejections (`system` reads, `SHOW TABLES`), one is a dashboard variable rendering as the literal `<no value>`, one is SQL ClickHouse also rejects, and two are an open upstream gap. ## Tests `TestErrIfStatementIsNotValid_ShouldPassButFails` is back, holding what remains: three forms of a parenthesised left operand of a set operator, and `on` as a column name. It also stopped panicking — `errors.Asc` dereferences the error it is given, so a case starting to pass took the suite out with a SIGSEGV instead of reporting. Both refusal tables now share one harness, bounded by the same timeout the passing table uses. Known gap: no input is currently known to panic the parser, so the `recover` has no test exercising it.
184 lines
7.5 KiB
Go
184 lines
7.5 KiB
Go
package querybuilder
|
|
|
|
import (
|
|
"context"
|
|
"log/slog"
|
|
"maps"
|
|
"slices"
|
|
"strings"
|
|
|
|
chparser "github.com/AfterShip/clickhouse-sql-parser/parser"
|
|
"github.com/SigNoz/signoz/pkg/errors"
|
|
)
|
|
|
|
var (
|
|
CodeClickHouseSQLParserPanic = errors.MustNewCode("clickhouse_sql_parser_panic")
|
|
CodeClickHouseSQLUnparseable = errors.MustNewCode("clickhouse_sql_unparseable")
|
|
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")
|
|
)
|
|
|
|
// internalDatabases hold server metadata, credentials and grants rather than telemetry.
|
|
var internalDatabases = map[string]struct{}{
|
|
"system": {},
|
|
"information_schema": {},
|
|
}
|
|
|
|
// generatorTableFunctions compute their rows from their arguments alone. They open no file or socket, reach no other host, and name no table, database or dictionary, so none of them can read through anything the rules here exist to protect. Can be used to build a dense axis to join a sparse series against. Every other table function is refused.
|
|
//
|
|
// Keyed by the lowercased name so that matching is case-insensitive, valued by the spelling to name it back to the caller.
|
|
//
|
|
// TODO(@therealpandey): take a deployment level allow list on top of this, so an operator can permit more without a release.
|
|
var generatorTableFunctions = map[string]string{
|
|
"numbers": "numbers",
|
|
"numbers_mt": "numbers_mt",
|
|
"zeros": "zeros",
|
|
"zeros_mt": "zeros_mt",
|
|
"generateseries": "generateSeries",
|
|
"generate_series": "generate_series",
|
|
}
|
|
|
|
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"
|
|
|
|
// The parser's grammar has gaps against SQL that ClickHouse itself accepts.
|
|
func ErrIfStatementIsNotValid(query string) (err error) {
|
|
defer func() {
|
|
// The parser has a history of panicking on malformed input rather than returning an error.
|
|
if recovered := recover(); recovered != nil {
|
|
err = errors.NewInvalidInputf(CodeClickHouseSQLParserPanic, "invalid ClickHouse SQL (recovered): %v", recovered)
|
|
}
|
|
}()
|
|
|
|
stmts, parseErr := chparser.NewParser(query).ParseStmts()
|
|
if parseErr != nil {
|
|
// Wrapped rather than formatted in, so that callers can recover the parser's *ParseError and read the position off it.
|
|
return errors.WrapInvalidInputf(parseErr, CodeClickHouseSQLUnparseable, "invalid ClickHouse SQL: %s", parseErr.Error())
|
|
}
|
|
|
|
if len(stmts) != 1 {
|
|
return errors.NewInvalidInputf(CodeClickHouseSQLNotSingleStatement, "ClickHouse SQL must contain exactly one statement, found %d statements", len(stmts))
|
|
}
|
|
|
|
selectQuery, ok := stmts[0].(*chparser.SelectQuery)
|
|
if !ok {
|
|
return errors.NewInvalidInputf(CodeClickHouseSQLNotSelect, "only SELECT statements are allowed in ClickHouse SQL queries")
|
|
}
|
|
|
|
visitor := &chparser.DefaultASTVisitor{Visit: func(node chparser.Expr) error {
|
|
switch expr := node.(type) {
|
|
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
|
|
}
|
|
|
|
return errors.
|
|
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 {
|
|
return nil
|
|
}
|
|
|
|
if _, ok := internalDatabases[strings.ToLower(expr.Database.Name)]; ok {
|
|
return errors.NewInvalidInputf(CodeClickHouseSQLInternalDatabase, "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(CodeClickHouseSQLReadonlyOverride, "the ClickHouse readonly setting cannot be overridden")
|
|
}
|
|
}
|
|
|
|
return nil
|
|
}}
|
|
|
|
return selectQuery.Accept(visitor)
|
|
}
|
|
|
|
// TODO(@therealpandey): remove this and move to ErrIfStatementIsNotValid.
|
|
func LogIfStatementIsNotValid(ctx context.Context, logger *slog.Logger, query string) {
|
|
if err := ErrIfStatementIsNotValid(query); err != nil {
|
|
logger.WarnContext(ctx, "clickhouse sql is not valid", errors.Attr(err), slog.String("query", query))
|
|
}
|
|
}
|
|
|
|
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)
|
|
}
|