Compare commits

...

2 Commits

Author SHA1 Message Date
Pandey
2c5b4184c4 feat(querier): log user-authored clickhouse sql after running them through the parser (#12301)
Some checks are pending
build-staging / prepare (push) Waiting to run
build-staging / js-build (push) Blocked by required conditions
build-staging / go-build (push) Blocked by required conditions
Release Drafter / update_release_draft (push) Waiting to run
build-staging / staging (push) Blocked by required conditions
* fix(querier): restrict user-authored clickhouse sql to read-only selects

Validate every user-authored ClickHouse statement before it runs: exactly one
statement, SELECT only, no table functions and no readonly override. Validation
runs on the rendered statement, since substituted variable values are user input
too.

Apply it to both entry points that reach the telemetry store with user SQL, the
clickhouse_sql query type in query_range and the dashboard variables query, and
replace the substring blacklist in the latter, which ran before substitution.

Also run these statements with readonly = 2 as a backstop. The connection is
shared with write paths, so it is opt-in per query and writers never set it.

* fix(querier): address review on clickhouse sql validation

Rename the validator to ValidateReadOnlySelect and collapse the multi-line error
constructions onto single lines.

Carry the parse failure in the message rather than as a wrapped cause. Neither
renderer surfaces the cause: render.Error reads only the message off the base
error, and RespondError reads Error(), which returns the cause alone and drops
the message. Both now show the same text with a 400.

Add unit tests covering statement kinds, table functions nested in joins, CTEs,
subqueries and unions, and readonly overrides.

* fix(querier): reject internal databases in user-authored clickhouse sql

Reading system or information_schema exposes grants, users and server metadata,
and ClickHouse read-only mode does not prevent it, so reject any table reference
into them.

Update the dashboard variables test for the new rejection message and cover both
a statement smuggled through a variable value and a system table read.

* fix(querier): reject unterminated block comments before parsing

The parser loops forever on an unterminated block comment, so validating a query
containing one would spin a request goroutine at full CPU instead of rejecting
it. Detect it up front, skipping comment markers that sit inside string literals.

Cover the parser panicking on a settings clause with no value in its own test,
which asserts the input still panics the parser so the case cannot quietly stop
exercising the recover.

* 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.

* fix(querier): log clickhouse sql validation failures instead of rejecting

The parser's grammar has gaps against SQL that ClickHouse accepts, so rejecting
whatever it cannot read would break working dashboards and alerts. Sampling a
week of production clickhouse_sql found roughly one query in eighteen tripping
one of three gaps, none of them reading anything a telemetry query should not.

Log the failure with the rendered query instead, so the gaps can be told apart
from statements that genuinely break the rules before anything is enforced.

Wrap the parse error rather than formatting it in, so a caller can recover the
parser's *ParseError and read the position off it. The tests use that to pin the
construct each gap stops at, alongside the rewrite that the parser does accept.

* chore(querier): tighten the clickhouse sql validation comments

Condense the comments to single lines, move the recover note inside the defer it
explains, separate the visitor cases, and shorten the log message. Report how
many statements were found when rejecting a multi-statement query.

* fix(querier): log invalid clickhouse sql on the dashboard variables path too

The two callers disagreed: query_range logged and carried on, while dashboard
variables rejected. Given the parser trips on roughly one real query in eighteen,
that path could refuse a legitimate variable query, and being a rejection it left
nothing behind to show it had happened.

Both now go through LogIfStatementIsNotValid. prepareQuery is back to rendering
and nothing else, so the cases that expected it to police the statement move to
where the rules actually live.

* fix(querier): drop the read-only clickhouse session

Setting readonly on the session was defence in depth for a validator that now
only logs, so it guarded nothing while adding a context key and a settings branch
to a connection that is shared with every write path.

Restore the dashboard variables handler to what it was and call the validation
alongside, rather than reworking the rendering to accommodate it.

* chore(querybuilder): drop the redundant suffix from the failing case names

The test they sit in is already named _Fail.
2026-07-28 14:54:42 +00:00
Gaurav Tewari
0b69f5f677 fix: smooth Only/Toggle hover reveal in quick-filters checkbox (#12248)
The Only and Toggle buttons were revealed by overlapping hover triggers
(`.value` for Toggle, `.checkbox-value-section` for Only), so hovering the
label matched both rules and showed the two buttons side by side, shifting
the row layout — the visible hover "jerk".

- Scope the Toggle trigger to the checkbox (`.check-box:hover ~ ...`) so it
  no longer overlaps the label region; Only and Toggle are now mutually
  exclusive by cursor position.
- Stack both buttons in a single grid cell (`.value-actions`) so revealing
  one swaps in place instead of shifting the row.
- Add a fade + slide entry/exit animation (opacity + translateX with
  `@starting-style` and `display` `allow-discrete`), disabled under
  prefers-reduced-motion.

Co-authored-by: Gaurav Tewari <tewarig@users.noreply.github.com>
2026-07-28 12:21:39 +00:00
7 changed files with 309 additions and 40 deletions

View File

@@ -92,52 +92,76 @@
color: var(--l3-foreground);
}
.only-btn {
cursor: not-allowed;
color: var(--l3-foreground);
}
.only-btn,
.toggle-btn {
cursor: not-allowed;
color: var(--l3-foreground);
}
}
.only-btn {
display: none;
// Stack "Only"/"All" and "Toggle" in a single cell so revealing
// one swaps in place instead of shifting the row layout.
.value-actions {
display: grid;
align-items: center;
justify-items: end;
> * {
grid-area: 1 / 1;
}
}
.only-btn,
.toggle-btn {
display: none;
}
.toggle-btn:hover {
background-color: unset;
}
.only-btn:hover {
background-color: unset;
}
}
.checkbox-value-section:hover {
.toggle-btn {
display: none;
}
.only-btn {
display: flex;
align-items: center;
justify-content: center;
height: 21px;
opacity: 0;
transform: translateX(4px);
// `display` is animated as a discrete property so the button
// stays mounted through its fade-out on exit.
transition:
opacity 0.16s ease,
transform 0.16s ease,
display 0.16s allow-discrete;
&:hover {
background-color: unset;
}
}
}
// Hovering the label/value area reveals the "Only"/"All" action.
.checkbox-value-section:hover .only-btn {
display: flex;
opacity: 1;
transform: translateX(0);
// Entry animation start state (fires on the display switch).
@starting-style {
opacity: 0;
transform: translateX(4px);
}
}
// Hovering the checkbox reveals the "Toggle" action.
.check-box:hover ~ .checkbox-value-section .toggle-btn {
display: flex;
opacity: 1;
transform: translateX(0);
@starting-style {
opacity: 0;
transform: translateX(4px);
}
}
}
.value:hover {
@media (prefers-reduced-motion: reduce) {
.only-btn,
.toggle-btn {
display: flex;
align-items: center;
justify-content: center;
height: 21px;
transition: none;
}
}
}

View File

@@ -53,12 +53,14 @@ function CheckboxValueRow({
</Typography.Text>
</TooltipSimple>
)}
<Button type="text" className="only-btn">
{onlyButtonLabel}
</Button>
<Button type="text" className="toggle-btn">
Toggle
</Button>
<div className="value-actions">
<Button type="text" className="only-btn">
{onlyButtonLabel}
</Button>
<Button type="text" className="toggle-btn">
Toggle
</Button>
</div>
</div>
</div>
);

View File

@@ -100,9 +100,20 @@ func (q *chSQLQuery) renderVars(query string, vars map[string]qbtypes.VariableIt
return newQuery.String(), nil
}
// Statement renders the SQL without executing it, for the preview path.
func (q *chSQLQuery) Statement(_ context.Context) (*qbtypes.Statement, error) {
func (q *chSQLQuery) render(ctx context.Context) (string, error) {
rendered, err := q.renderVars(q.query.Query, q.vars, q.fromMS, q.toMS)
if err != nil {
return "", err
}
querybuilder.LogIfStatementIsNotValid(ctx, q.logger, rendered)
return rendered, nil
}
// Statement renders the SQL without executing it, for the preview path.
func (q *chSQLQuery) Statement(ctx context.Context) (*qbtypes.Statement, error) {
rendered, err := q.render(ctx)
if err != nil {
return nil, err
}
@@ -124,7 +135,7 @@ 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(ctx)
if err != nil {
return nil, err
}
@@ -135,11 +146,11 @@ func (q *chSQLQuery) Execute(ctx context.Context) (*qbtypes.Result, error) {
}
defer rows.Close()
// TODO: map the errors from ClickHouse to our error types
payload, err := consume(rows, q.kind, nil, qbtypes.Step{}, q.query.Name)
if err != nil {
return nil, err
}
return &qbtypes.Result{
Type: q.kind,
Value: payload,

View File

@@ -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"
@@ -1092,6 +1093,8 @@ func (aH *APIHandler) queryDashboardVarsV2(w http.ResponseWriter, r *http.Reques
return
}
querybuilder.LogIfStatementIsNotValid(r.Context(), aH.logger, query)
dashboardVars, err := aH.reader.QueryDashboardVars(r.Context(), query)
if err != nil {
RespondError(w, &model.ApiError{Typ: model.ErrorBadData, Err: err}, nil)

View File

@@ -0,0 +1,76 @@
package querybuilder
import (
"context"
"log/slog"
"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": {},
}
// The parser's grammar has gaps against SQL that ClickHouse itself accepts. See TestErrIfStatementIsNotValid_ShouldPassButFails.
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(errors.CodeInvalidInput, "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, errors.CodeInvalidInput, "invalid ClickHouse SQL: %s", parseErr.Error())
}
if len(stmts) != 1 {
return errors.NewInvalidInputf(errors.CodeInvalidInput, "ClickHouse SQL must contain exactly one statement, found %d statements", len(stmts))
}
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)
}
// 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))
}
}

View File

@@ -0,0 +1,154 @@
package querybuilder
import (
"testing"
chparser "github.com/AfterShip/clickhouse-sql-parser/parser"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
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)"},
{"Union", "SELECT * FROM t UNION ALL SELECT * FROM t2"},
{"Intersect", "SELECT * FROM t INTERSECT SELECT * FROM t2"},
{"WindowFunction", "SELECT sum(v) OVER (PARTITION BY a ORDER BY t) FROM 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.
{"TrailingUnterminatedBlockComment", "SELECT count() FROM t /* unterminated"},
// The rule keys on the database, not on the table name.
{"TableNamedSystemInTelemetryDatabase", "SELECT * FROM signoz_logs.system"},
{"SignedLiteralAfterClosingParen", "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"},
{"TrimFunction", "SELECT trimBoth('/api/endpoint/', '/');"},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := ErrIfStatementIsNotValid(testCase.query)
assert.NoError(t, err)
})
}
}
func TestErrIfStatementIsNotValid_Fail(t *testing.T) {
testCases := []struct {
name string
query string
}{
// Not a single statement, or not a statement at all.
{"Empty", ""},
{"UnterminatedBlockCommentOnly", "/* x"},
{"Unparseable", "SELECT FROM WHERE"},
{"MultipleStatements", "SELECT 1; DROP TABLE signoz_logs.logs_v2"},
// Parses, but is not a SELECT.
{"Drop", "DROP TABLE signoz_logs.logs_v2"},
{"Insert", "INSERT INTO signoz_logs.logs_v2 SELECT * FROM signoz_logs.logs_v2"},
{"AlterDelete", "ALTER TABLE signoz_logs.logs_v2 DELETE WHERE 1 = 1"},
{"CreateTable", "CREATE TABLE evil (a Int) ENGINE = Memory"},
{"Grant", "GRANT ALL ON *.* TO admin"},
{"Set", "SET readonly = 0"},
// These the parser rejects outright rather than classifying.
{"ShowGrants", "SHOW GRANTS"},
{"IntoOutfile", "SELECT * FROM t INTO OUTFILE '/tmp/x.csv'"},
// Table functions, which read through something other than a telemetry table.
{"UrlTableFunction", "SELECT * FROM url('http://attacker.example/x', CSV, 'a String')"},
{"FileTableFunction", "SELECT * FROM file('/etc/passwd', CSV, 'a String')"},
{"ExecutableTableFunction", "SELECT * FROM executable('script.sh', CSV, 'a String')"},
{"TableFunctionInJoin", "SELECT * FROM t1 JOIN url('http://x', CSV, 'a String') u ON 1 = 1"},
{"TableFunctionInCommonTableExpression", "WITH c AS (SELECT * FROM url('http://x', CSV, 'a String')) SELECT * FROM c"},
{"TableFunctionInWhereSubquery", "SELECT * FROM t WHERE a IN (SELECT * FROM file('/etc/passwd', CSV, 'a String'))"},
{"TableFunctionInUnion", "SELECT * FROM t UNION ALL SELECT * FROM url('http://x', CSV, 'a String')"},
// Internal databases, which hold grants and server metadata rather than telemetry.
{"SystemUsers", "SELECT * FROM system.users"},
{"SystemUppercase", "SELECT * FROM SYSTEM.USERS"},
{"SystemQuoted", "SELECT count() FROM `system`.`tables`"},
{"SystemInSubquery", "SELECT * FROM (SELECT name FROM system.parts)"},
{"SystemInJoin", "SELECT * FROM signoz_logs.distributed_logs_v2 AS l JOIN system.users AS u ON 1 = 1"},
{"SystemInIntersect", "SELECT * FROM t INTERSECT SELECT * FROM system.users"},
{"InformationSchema", "SELECT * FROM information_schema.tables"},
// A query-level setting takes precedence over the one the caller applies.
{"ReadonlySettingOverride", "SELECT * FROM t SETTINGS readonly = 0"},
{"ReadonlySettingOverrideAmongOthers", "SELECT * FROM t SETTINGS max_threads = 4, readonly = 0"},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := ErrIfStatementIsNotValid(testCase.query)
assert.Error(t, err)
})
}
}
// Queries the parser cannot read. ClickHouse runs all of them.
func TestErrIfStatementIsNotValid_ShouldPassButFails(t *testing.T) {
testCases := []struct {
name string
query string
// The construct the parser stops after, which is the one it cannot read.
expectedStopsAfter string
// The same construct written so the parser accepts it.
fix string
}{
{
name: "IntervalAliasInOrderBy",
query: "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",
expectedStopsAfter: "ORDER BY interval ASC",
fix: "SELECT count() AS interval FROM t ORDER BY `interval` ASC",
},
{
name: "IntervalAliasInOrderByDesc",
query: "SELECT count() AS value, toStartOfInterval(timestamp, INTERVAL 1 MINUTE) AS interval, serviceName, resourceTagsMap['deployment.environment'] AS environment, exceptionStacktrace FROM signoz_traces.distributed_signoz_error_index_v2 WHERE exceptionType != 'OSError' AND resourceTagsMap['deployment.environment'] = 'staging' AND timestamp BETWEEN toDateTime(1785186300) AND toDateTime(1785186600) GROUP BY serviceName, interval, environment, exceptionStacktrace ORDER BY interval DESC",
expectedStopsAfter: "ORDER BY interval DESC",
fix: "SELECT count() AS interval FROM t ORDER BY `interval` DESC",
},
{
name: "StandardTrimSyntax",
query: "SELECT toStartOfInterval(fromUnixTimestamp64Nano(timestamp), INTERVAL 5 MINUTE) AS interval, resources_string['host.name'] as host_name, toFloat64(countIf( lower(trim(BOTH ' ' FROM replaceOne( JSONExtractString(body, 'Action'), 'health_status: ', '' ))) IN ('unhealthy','starting','failing') )) as value FROM signoz_logs.distributed_logs_v2 WHERE timestamp BETWEEN 1784602320000000000 AND 1784602620000000000 AND ts_bucket_start BETWEEN 1784602320 - 300 AND 1784602620 AND JSONExtractString(body, 'Type') = 'container' AND JSONExtractString(body, 'Actor', 'Attributes', 'name') IS NOT NULL AND resources_string['host.name'] IS NOT NULL AND resources_string['host.name'] = 'aihub-nightly' GROUP BY interval, host_name ORDER BY interval, host_name",
expectedStopsAfter: "trim(BOTH '",
fix: "SELECT trimBoth(replaceOne( JSONExtractString(body, 'Action'), 'health_status: ', '' ), ' ')",
},
{
name: "SignedLiteralAfterClosingParen",
query: "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)",
expectedStopsAfter: "(toUnixTimestamp(now())",
fix: "SELECT (toUnixTimestamp(now()) - 3600) * 1000000000",
},
{
name: "SignedLiteralAfterClosingParenMinimal",
query: "SELECT (1)-1",
expectedStopsAfter: "(1)",
fix: "SELECT (1) - 1",
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
err := ErrIfStatementIsNotValid(testCase.query)
var parseErr *chparser.ParseError
require.ErrorAs(t, err, &parseErr, "expected a parser failure rather than a rule violation")
// The parser reports the offset it stopped at, which sits just past the construct
// it choked on, so the text leading up to it is what needs looking at.
consumed := testCase.query[:parseErr.Pos]
assert.Equal(t, testCase.expectedStopsAfter, consumed[max(0, len(consumed)-len(testCase.expectedStopsAfter)):])
assert.NoError(t, ErrIfStatementIsNotValid(testCase.fix))
})
}
}

View File

@@ -1 +0,0 @@
package querybuilder