From cc895be444c71e15d50ffacd80949dcc2a656a60 Mon Sep 17 00:00:00 2001 From: Duansg Date: Wed, 29 Jul 2026 03:29:47 -0700 Subject: [PATCH 1/5] [fix] validate sql reaching the alert query executor --- .../expr/AlertExpressionEvalVisitor.java | 48 ++++++++++++-- .../expr/AlertExpressionEvalVisitorTest.java | 64 +++++++++++++++++++ .../support/valid/SqlSecurityValidator.java | 43 ++++++++++++- .../valid/SqlSecurityValidatorTest.java | 41 ++++++++++++ 4 files changed, 188 insertions(+), 8 deletions(-) diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java index bc845660834..ebe639b9287 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java @@ -19,7 +19,10 @@ import org.antlr.v4.runtime.CommonTokenStream; import org.apache.commons.collections4.CollectionUtils; +import org.apache.hertzbeat.common.support.exception.AlertExpressionException; import org.apache.hertzbeat.common.support.exception.ExpressionVisitorException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; import org.apache.hertzbeat.warehouse.db.QueryExecutor; import java.util.ArrayList; @@ -39,6 +42,12 @@ public class AlertExpressionEvalVisitor extends AlertExpressionBaseVisitor> visitPromqlExpr(AlertExpressionParser.PromqlExp @Override public List> visitSqlExpr(AlertExpressionParser.SqlExprContext ctx) { String rawSql = tokens.getText(ctx.selectSql()); - return executor.execute(rawSql); + return executor.execute(validateSql(rawSql)); } @Override public List> visitSqlCallExpr(AlertExpressionParser.SqlCallExprContext ctx) { - return callSqlOrPromql(tokens.getText(ctx.string())); + return executor.execute(validateSql(unquote(tokens.getText(ctx.string())))); } @Override public List> visitPromqlCallExpr(AlertExpressionParser.PromqlCallExprContext ctx) { - return callSqlOrPromql(tokens.getText(ctx.string())); + return executor.execute(unquote(tokens.getText(ctx.string()))); + } + + private String unquote(String text) { + return text.substring(1, text.length() - 1); } - private List> callSqlOrPromql(String text) { - String script = text.substring(1, text.length() - 1); - return executor.execute(script); + /** + * Every statement that reaches the query executor from an alert expression passes + * through here. + * + *

The {@code sql("...")} spelling carries an arbitrary string, and the executor runs + * it with the server side database credentials, so without this check an expression + * could drop or rewrite a table. Validating where the statement reaches the executor + * covers both spellings, and covers the preview endpoint and the periodic evaluation + * loop alike, rather than relying on each caller to remember. + * + *

The policy is read only, nothing narrower: which tables an expression may read is + * not constrained here because metric tables are created per metric on demand. + * @param sql statement about to be executed + * @return the same statement, once it is known to be a plain read + * @throws AlertExpressionException if the statement is not a plain read + */ + private String validateSql(String sql) { + try { + SQL_VALIDATOR.validate(sql); + } catch (SqlSecurityException e) { + // AlertExpressionException rather than ExpressionVisitorException: it is the type + // DataSourceServiceImpl.calculate rethrows untouched, so the author of the rule sees + // which part of the policy the statement broke instead of a generic failure + throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); + } + return sql; } /** diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java index c12e23e80b1..1947b7a34f8 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java @@ -19,6 +19,7 @@ import org.antlr.v4.runtime.CharStreams; import org.antlr.v4.runtime.CommonTokenStream; +import org.apache.hertzbeat.common.support.exception.AlertExpressionException; import org.apache.hertzbeat.warehouse.db.QueryExecutor; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -31,7 +32,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.when; /** @@ -864,6 +867,67 @@ void testComparisonExpr() { assertEquals(0, result.get(0).get("__value__")); } + @Test + void testSqlCallRunsPlainRead() { + when(mockExecutor.execute("select value from cpu where host = 'server1'")) + .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + List> result = evaluate("sql(\"select value from cpu where host = 'server1'\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + + @Test + void testSqlCallRejectsNonSelectStatement() { + assertSqlRejected("sql(\"drop table cpu\") > 70"); + assertSqlRejected("sql(\"delete from cpu\") > 70"); + assertSqlRejected("sql(\"insert into cpu values (1)\") > 70"); + } + + @Test + void testSqlCallRejectsStatementThatOnlyStartsAsSelect() { + assertSqlRejected("sql(\"select 1; drop table cpu\") > 70"); + assertSqlRejected("sql(\"truncate table cpu\") > 70"); + } + + /** + * Subqueries and nested aggregation are supported in alert expressions, and with every + * metric table already readable, rejecting them would cost features without denying an + * attacker anything. The policy stops at read only on purpose. + */ + @Test + void testSqlCallKeepsSubqueriesWorking() { + String sql = "select value from cpu where host = (select host from hosts limit 1)"; + when(mockExecutor.execute(sql)).thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + List> result = evaluate("sql(\"" + sql + "\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + + /** + * The promql spelling shares the call syntax but never reaches a sql parser, so it has + * to keep working for expressions that are not valid sql at all. + */ + @Test + void testPromqlCallIsNotSqlValidated() { + when(mockExecutor.execute("rate(http_requests_total[5m])")) + .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); + + List> result = evaluate("promql(\"rate(http_requests_total[5m])\") > 70"); + + assertEquals(1, result.size()); + assertEquals(80.0, result.get(0).get("__value__")); + } + + private void assertSqlRejected(String expression) { + AlertExpressionException thrown = assertThrows(AlertExpressionException.class, () -> evaluate(expression)); + assertTrue(thrown.getMessage().contains("SQL security validation failed"), thrown.getMessage()); + Mockito.verify(mockExecutor, Mockito.never()).execute(anyString()); + } + private List> evaluate(String expression) { AlertExpressionLexer lexer = new AlertExpressionLexer(CharStreams.fromString(expression)); CommonTokenStream tokens = new CommonTokenStream(lexer); diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index 6d58dff18b4..b26205ff891 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -21,6 +21,7 @@ import net.sf.jsqlparser.JSQLParserException; import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.Statements; import net.sf.jsqlparser.statement.select.LateralSubSelect; import net.sf.jsqlparser.statement.select.ParenthesedSelect; import net.sf.jsqlparser.statement.select.Select; @@ -47,6 +48,8 @@ public class SqlSecurityValidator { private final Set allowedTables; + private final boolean restrictTables; + public SqlSecurityValidator(Collection allowedTables) { if (CollectionUtils.isEmpty(allowedTables)) { this.allowedTables = new HashSet<>(); @@ -55,6 +58,29 @@ public SqlSecurityValidator(Collection allowedTables) { .map(this::normalizeIdentifier) .collect(Collectors.toSet()); } + this.restrictTables = true; + } + + private SqlSecurityValidator() { + this.allowedTables = new HashSet<>(); + this.restrictTables = false; + } + + /** + * A validator whose whole policy is "this statement may only read". + * + *

Use it where there is no table list to validate against: metric tables are created + * on demand, one per metric, so the whitelisting constructor would reject every + * legitimate metric query. + * + *

It deliberately keeps subqueries, unions and ctes, unlike the whitelisting mode. + * Those structures are blocked there because they are the ways a statement can reach a + * table the whitelist never mentions; with every table already readable they buy no + * protection, while alert expressions do use subqueries and nested aggregation. + * @return a validator that only rejects statements which are not plain selects + */ + public static SqlSecurityValidator selectOnly() { + return new SqlSecurityValidator(); } public void validate(String sql) throws SqlSecurityException { @@ -62,18 +88,31 @@ public void validate(String sql) throws SqlSecurityException { throw new SqlSecurityException("SQL statement cannot be empty"); } - Statement statement; + Statements statements; try { - statement = CCJSqlParserUtil.parse(sql); + statements = CCJSqlParserUtil.parseStatements(sql); } catch (JSQLParserException e) { log.warn("Failed to parse SQL: {}", sql, e); throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); } + // parseStatements rather than parse: parse() returns the first statement and discards + // the rest, so "select 1; drop table x" would validate as a plain select while the + // caller still hands the whole string to the database + if (statements.getStatements().size() != 1) { + throw new SqlSecurityException("Only a single statement is allowed."); + } + Statement statement = statements.getStatements().get(0); + if (!(statement instanceof Select select)) { throw new SqlSecurityException("Only SELECT statements are allowed."); } + if (!restrictTables) { + // read only is the whole policy in this mode, see selectOnly() + return; + } + // Check for CTE at top level if (select.getWithItemsList() != null && !select.getWithItemsList().isEmpty()) { throw new SqlSecurityException("CTE (WITH clause) is not allowed"); diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index 782ee313833..eb942b9c370 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -264,4 +264,45 @@ void testBypassWithMathOperations() { assertThrows(SqlSecurityException.class, () -> validator.validate( "SELECT * FROM hertzbeat_logs WHERE id = 1 + (SELECT id FROM secret_table)")); } + + /** + * `CCJSqlParserUtil.parse` returns the first statement and silently discards the rest, + * so a stacked statement used to validate as a plain select while the caller still + * handed the whole string to the database. + */ + @Test + void testStackedStatementIsRejected() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT * FROM hertzbeat_logs; DROP TABLE hertzbeat_logs")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT 1; DROP TABLE cpu")); + } + + @Test + void testTrailingSemicolonIsStillAcceptedAsOneStatement() { + assertDoesNotThrow(() -> validator.validate("SELECT * FROM hertzbeat_logs ; ")); + } + + @Test + void testSelectOnlyRejectsWrites() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DELETE FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("INSERT INTO cpu VALUES (1)")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("UPDATE cpu SET value = 1")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("TRUNCATE TABLE cpu")); + } + + /** + * Metric tables are created per metric on demand, so this mode constrains what a + * statement may do, not which table it may touch. + */ + @Test + void testSelectOnlyAcceptsAnyTableAndNestedReads() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM any_metric_table")); + assertDoesNotThrow(() -> selectOnly.validate( + "SELECT value FROM cpu WHERE host = (SELECT host FROM hosts LIMIT 1)")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT a FROM t1 UNION ALL SELECT b FROM t2")); + } } From 6d7bc7f11591053d4999e6666eabd574b0bfcdf0 Mon Sep 17 00:00:00 2001 From: Duansg Date: Sun, 9 Aug 2026 06:00:34 -0700 Subject: [PATCH 2/5] [fix] decide the sql policy at the executor, not at the expression spelling The read only check only covered the sql("...") spelling, so an expression that carried a statement as promql("...") still reached the executor unvalidated whenever the caller named a sql datasource. The preview endpoint takes that datasource straight from the path, so the spelling could not decide anything. Guard QueryExecutor.execute instead: every route into the executor is covered, including the ones added later, and the raw log query path shares the same seam rather than repeating the check. Fix two holes in the validator while it is the only thing standing between an alert rule and the database credentials. CCJSqlParserUtil.parse returns the first statement and discards the rest, so "select 1; drop table x" validated as a plain select while the caller still handed the whole string over. And "select * into backup from cpu" parses as a select but writes. Read only mode no longer rejects what JSqlParser cannot parse. The only sql executor talks to GreptimeDB, whose range query syntax the parser does not cover although it is an ordinary read, and failing those rules would trade a working feature for nothing. The two properties this mode has to guarantee, one statement and read only, are established by scanning outside literals and comments instead, which no dialect can confuse. The whitelisting mode keeps failing closed, because enumerating table names needs the parse tree. Co-Authored-By: Claude Opus 5 (1M context) --- .../expr/AlertExpressionEvalVisitor.java | 42 +--- .../service/impl/DataSourceServiceImpl.java | 48 ++-- .../impl/SqlValidatingQueryExecutor.java | 83 +++++++ .../expr/AlertExpressionEvalVisitorTest.java | 30 +-- .../alert/service/DataSourceServiceTest.java | 59 +++++ .../support/valid/SqlSecurityValidator.java | 205 ++++++++++++++++-- .../valid/SqlSecurityValidatorTest.java | 81 +++++++ 7 files changed, 445 insertions(+), 103 deletions(-) create mode 100644 hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java index ebe639b9287..1775ce6c467 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java @@ -19,10 +19,7 @@ import org.antlr.v4.runtime.CommonTokenStream; import org.apache.commons.collections4.CollectionUtils; -import org.apache.hertzbeat.common.support.exception.AlertExpressionException; import org.apache.hertzbeat.common.support.exception.ExpressionVisitorException; -import org.apache.hertzbeat.common.support.valid.SqlSecurityException; -import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; import org.apache.hertzbeat.warehouse.db.QueryExecutor; import java.util.ArrayList; @@ -43,11 +40,10 @@ public class AlertExpressionEvalVisitor extends AlertExpressionBaseVisitor> visitPromqlExpr(AlertExpressionParser.PromqlExp @Override public List> visitSqlExpr(AlertExpressionParser.SqlExprContext ctx) { String rawSql = tokens.getText(ctx.selectSql()); - return executor.execute(validateSql(rawSql)); + return executor.execute(rawSql); } @Override public List> visitSqlCallExpr(AlertExpressionParser.SqlCallExprContext ctx) { - return executor.execute(validateSql(unquote(tokens.getText(ctx.string())))); + return executor.execute(unquote(tokens.getText(ctx.string()))); } @Override @@ -280,34 +276,6 @@ private String unquote(String text) { return text.substring(1, text.length() - 1); } - /** - * Every statement that reaches the query executor from an alert expression passes - * through here. - * - *

The {@code sql("...")} spelling carries an arbitrary string, and the executor runs - * it with the server side database credentials, so without this check an expression - * could drop or rewrite a table. Validating where the statement reaches the executor - * covers both spellings, and covers the preview endpoint and the periodic evaluation - * loop alike, rather than relying on each caller to remember. - * - *

The policy is read only, nothing narrower: which tables an expression may read is - * not constrained here because metric tables are created per metric on demand. - * @param sql statement about to be executed - * @return the same statement, once it is known to be a plain read - * @throws AlertExpressionException if the statement is not a plain read - */ - private String validateSql(String sql) { - try { - SQL_VALIDATOR.validate(sql); - } catch (SqlSecurityException e) { - // AlertExpressionException rather than ExpressionVisitorException: it is the type - // DataSourceServiceImpl.calculate rethrows untouched, so the author of the rule sees - // which part of the policy the statement broke instead of a generic failure - throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); - } - return sql; - } - /** * Generate tag key (excluding `__name__` and `__value__` and `__timestamp__`) */ diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java index 136777c085b..795d87386e2 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java @@ -31,7 +31,6 @@ import org.apache.hertzbeat.alert.expr.AlertExpressionParser; import org.apache.hertzbeat.alert.service.DataSourceService; import org.apache.hertzbeat.common.support.exception.AlertExpressionException; -import org.apache.hertzbeat.common.support.valid.SqlSecurityException; import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; import org.apache.hertzbeat.common.util.ResourceBundleUtil; import org.apache.hertzbeat.warehouse.constants.WarehouseConstants; @@ -58,6 +57,13 @@ public class DataSourceServiceImpl implements DataSourceService { */ private static final List DEFAULT_ALLOWED_TABLES = List.of(WarehouseConstants.LOG_TABLE_NAME); + /** + * The policy for an alert expression is read only and nothing narrower: which tables it + * may read is not constrained, because metric tables are created per metric on demand and + * a whitelist would reject every legitimate metric query. + */ + private static final SqlSecurityValidator EXPRESSION_SQL_VALIDATOR = SqlSecurityValidator.selectOnly(); + protected ResourceBundle bundle = ResourceBundleUtil.getBundle("alerter"); @Setter @@ -93,7 +99,7 @@ public List> calculate(String datasource, String expr) { // replace all white space expr = expr.replaceAll("\\s+", " "); try { - return evaluate(expr, executor); + return evaluate(expr, guardSql(executor, EXPRESSION_SQL_VALIDATOR)); } catch (AlertExpressionException ae) { log.error("Calculate query parse error, datasource: {}, expr: {}, msg: {}", datasource, expr, ae.getMessage(), ae); throw ae; @@ -119,13 +125,11 @@ public List> query(String datasource, String expr) { // replace all white space expr = expr.replaceAll("\\s+", " "); - // SQL security validation for SQL-based datasources - if (isSqlDatasource(datasource)) { - validateSqlSecurity(expr); - } - try { - return executor.execute(expr); + return guardSql(executor, sqlSecurityValidator).execute(expr); + } catch (AlertExpressionException ae) { + // a statement the policy rejected, whose message names the part it broke + throw ae; } catch (Exception e) { log.error("Error executing query on datasource {}: {}", datasource, e.getMessage()); throw new AlertExpressionException(e.getMessage()); @@ -133,22 +137,22 @@ public List> query(String datasource, String expr) { } /** - * Check if the datasource is SQL-based - */ - private boolean isSqlDatasource(String datasource) { - return datasource != null && datasource.equalsIgnoreCase(WarehouseConstants.SQL); - } - - /** - * Validate SQL statement for security + * Wraps an executor that speaks sql so that nothing runs on it unvalidated. + * + *

The decision is made from the executor rather than from the datasource string the + * caller passed, because the executor is what actually holds the database credentials. + * A datasource that does not speak sql is handed back untouched: a promql endpoint takes + * a query string, not a statement, and running it through a sql parser would only reject + * valid promql. + * @param executor executor chosen for this datasource + * @param validator policy to enforce, read only for expressions and whitelisting for raw log queries + * @return the executor, guarded when it speaks sql */ - private void validateSqlSecurity(String sql) { - try { - sqlSecurityValidator.validate(sql); - } catch (SqlSecurityException e) { - log.warn("SQL security validation failed: {}", e.getMessage()); - throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); + private QueryExecutor guardSql(QueryExecutor executor, SqlSecurityValidator validator) { + if (!executor.support(WarehouseConstants.SQL)) { + return executor; } + return new SqlValidatingQueryExecutor(executor, validator); } private List> evaluate(String expr, QueryExecutor executor) { diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java new file mode 100644 index 00000000000..020669594c1 --- /dev/null +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/SqlValidatingQueryExecutor.java @@ -0,0 +1,83 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.hertzbeat.alert.service.impl; + +import org.apache.hertzbeat.common.entity.dto.query.DatasourceQuery; +import org.apache.hertzbeat.common.entity.dto.query.DatasourceQueryData; +import org.apache.hertzbeat.common.support.exception.AlertExpressionException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityException; +import org.apache.hertzbeat.common.support.valid.SqlSecurityValidator; +import org.apache.hertzbeat.warehouse.db.QueryExecutor; + +import java.util.List; +import java.util.Map; + +/** + * A sql executor that validates before it runs anything. + * + *

An alert expression reaches a query executor by several routes: the {@code sql("...")} + * and {@code promql("...")} spellings both carry an arbitrary string, a bare select is + * parsed by the expression grammar itself, and each of them is evaluated for the preview + * endpoint and for the periodic evaluation loop alike. All of them end at + * {@link QueryExecutor#execute(String)}, which runs the string with the server side database + * credentials. + * + *

Guarding that one method rather than each route is what makes the check complete: the + * spelling an expression happens to use does not decide whether the statement is checked, + * the database it lands on does. It also means a route added later is covered without anyone + * remembering to add a call. + */ +public class SqlValidatingQueryExecutor implements QueryExecutor { + + private final QueryExecutor delegate; + + private final SqlSecurityValidator validator; + + public SqlValidatingQueryExecutor(QueryExecutor delegate, SqlSecurityValidator validator) { + this.delegate = delegate; + this.validator = validator; + } + + @Override + public List> execute(String query) { + try { + validator.validate(query); + } catch (SqlSecurityException e) { + // AlertExpressionException rather than a generic failure: it is the type + // DataSourceServiceImpl rethrows untouched and the preview endpoint turns into a + // 400, so the author of the rule sees which part of the policy the statement broke + throw new AlertExpressionException("SQL security validation failed: " + e.getMessage()); + } + return delegate.execute(query); + } + + @Override + public DatasourceQueryData query(DatasourceQuery datasourceQuery) { + return delegate.query(datasourceQuery); + } + + @Override + public String getDatasource() { + return delegate.getDatasource(); + } + + @Override + public boolean support(String queryLanguage) { + return delegate.support(queryLanguage); + } +} diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java index 1947b7a34f8..75222c602b4 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java @@ -19,7 +19,6 @@ import org.antlr.v4.runtime.CharStreams; import org.antlr.v4.runtime.CommonTokenStream; -import org.apache.hertzbeat.common.support.exception.AlertExpressionException; import org.apache.hertzbeat.warehouse.db.QueryExecutor; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,9 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.when; /** @@ -878,19 +875,6 @@ void testSqlCallRunsPlainRead() { assertEquals(80.0, result.get(0).get("__value__")); } - @Test - void testSqlCallRejectsNonSelectStatement() { - assertSqlRejected("sql(\"drop table cpu\") > 70"); - assertSqlRejected("sql(\"delete from cpu\") > 70"); - assertSqlRejected("sql(\"insert into cpu values (1)\") > 70"); - } - - @Test - void testSqlCallRejectsStatementThatOnlyStartsAsSelect() { - assertSqlRejected("sql(\"select 1; drop table cpu\") > 70"); - assertSqlRejected("sql(\"truncate table cpu\") > 70"); - } - /** * Subqueries and nested aggregation are supported in alert expressions, and with every * metric table already readable, rejecting them would cost features without denying an @@ -908,11 +892,13 @@ void testSqlCallKeepsSubqueriesWorking() { } /** - * The promql spelling shares the call syntax but never reaches a sql parser, so it has - * to keep working for expressions that are not valid sql at all. + * The visitor hands both spellings to the executor as written. Whether a statement is + * allowed to run is decided by the executor it lands on, see + * {@code DataSourceServiceTest}, so promql keeps working for text that is not valid sql + * at all. */ @Test - void testPromqlCallIsNotSqlValidated() { + void testBothCallSpellingsReachTheExecutorAsWritten() { when(mockExecutor.execute("rate(http_requests_total[5m])")) .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); @@ -922,12 +908,6 @@ void testPromqlCallIsNotSqlValidated() { assertEquals(80.0, result.get(0).get("__value__")); } - private void assertSqlRejected(String expression) { - AlertExpressionException thrown = assertThrows(AlertExpressionException.class, () -> evaluate(expression)); - assertTrue(thrown.getMessage().contains("SQL security validation failed"), thrown.getMessage()); - Mockito.verify(mockExecutor, Mockito.never()).execute(anyString()); - } - private List> evaluate(String expression) { AlertExpressionLexer lexer = new AlertExpressionLexer(CharStreams.fromString(expression)); CommonTokenStream tokens = new CommonTokenStream(lexer); diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java index e9b098b1b08..52114fa7724 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java @@ -787,4 +787,63 @@ void query12() { () -> dataSourceService.query("sql", "SELEC * FORM hertzbeat_logs")); verify(mockExecutor, never()).execute(anyString()); } + + /** + * The expression grammar offers three ways to reach the executor, and the datasource the + * caller names is what picks that executor. So a statement written with the + * {@code promql("...")} spelling still lands on the sql executor, with the server side + * database credentials behind it, whenever the caller names the sql datasource: whether a + * statement may run cannot be decided from the spelling. + */ + @Test + void calculateRejectsWritesWhateverSpellingTheyArrivedIn() { + QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("sql")).thenReturn(true); + dataSourceService.setExecutors(List.of(mockExecutor)); + + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "sql(\"drop table cpu\") > 0")); + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "promql(\"drop table cpu\") > 0")); + assertThrows(AlertExpressionException.class, + () -> dataSourceService.calculate("sql", "sql(\"select 1; drop table cpu\") > 0")); + verify(mockExecutor, never()).execute(anyString()); + } + + /** + * Only a datasource that speaks sql is guarded. A promql endpoint takes a query string + * rather than a statement, so running it through a sql parser would reject valid promql + * without denying an attacker anything. + */ + @Test + void calculateLeavesPromqlDatasourcesAlone() { + QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("promql")).thenReturn(true); + when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); + dataSourceService.setExecutors(List.of(mockExecutor)); + + List> result = dataSourceService.calculate( + "promql", "promql(\"rate(http_requests_total[5m])\") > 70"); + + assertEquals(1, result.size()); + verify(mockExecutor).execute("rate(http_requests_total[5m])"); + } + + /** + * A read still has to run, including the GreptimeDB range query syntax that the sql + * parser cannot read. + */ + @Test + void calculateStillRunsReadsOnSqlDatasources() { + QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + when(mockExecutor.support("sql")).thenReturn(true); + when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); + dataSourceService.setExecutors(List.of(mockExecutor)); + + String rangeQuery = "select avg(value) RANGE '10s' from cpu ALIGN '5s'"; + List> result = dataSourceService.calculate("sql", "sql(\"" + rangeQuery + "\") > 70"); + + assertEquals(1, result.size()); + verify(mockExecutor).execute(rangeQuery); + } } diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index b26205ff891..1667b24af9e 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -24,6 +24,7 @@ import net.sf.jsqlparser.statement.Statements; import net.sf.jsqlparser.statement.select.LateralSubSelect; import net.sf.jsqlparser.statement.select.ParenthesedSelect; +import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.statement.select.WithItem; @@ -33,19 +34,30 @@ import java.util.Collection; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.stream.Collectors; /** * SQL Security Validator using JSqlParser 5.1+. - * Security Policy: - * 1. Only SELECT statements are allowed. - * 2. All referenced tables must be in the whitelist. - * 3. Subqueries, UNION, CTE, LATERAL are blocked. + * + *

Two modes, see {@link #SqlSecurityValidator(Collection)} and {@link #selectOnly()}: + *

    + *
  • whitelisting: only SELECT, every referenced table must be whitelisted, and + * subqueries, UNION, CTE and LATERAL are blocked because they are the ways a statement + * can reach a table the whitelist never mentions.
    • + *
  • read only: only SELECT, any table, nested reads kept.
    • + *
+ * + *

Both modes reject a string that carries more than one statement. */ @Slf4j public class SqlSecurityValidator { + private static final String SELECT_KEYWORD = "SELECT"; + + private static final String WITH_KEYWORD = "WITH"; + private final Set allowedTables; private final boolean restrictTables; @@ -87,30 +99,69 @@ public void validate(String sql) throws SqlSecurityException { if (sql == null || sql.trim().isEmpty()) { throw new SqlSecurityException("SQL statement cannot be empty"); } - - Statements statements; - try { - statements = CCJSqlParserUtil.parseStatements(sql); - } catch (JSQLParserException e) { - log.warn("Failed to parse SQL: {}", sql, e); - throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); + if (restrictTables) { + validateAgainstWhitelist(sql); + } else { + validateReadOnly(sql); } + } - // parseStatements rather than parse: parse() returns the first statement and discards - // the rest, so "select 1; drop table x" would validate as a plain select while the - // caller still hands the whole string to the database - if (statements.getStatements().size() != 1) { + /** + * Read only mode, which runs against a time series database whose sql dialect JSqlParser + * does not fully cover: GreptimeDB range queries such as + * {@code SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s'} are rejected by the parser + * although they are ordinary reads. + * + *

So the two properties this mode has to guarantee, one statement and read only, are + * established without the parser: statements are counted by scanning outside string + * literals and comments, and the leading keyword decides whether it reads. Both checks + * are dialect independent. The parser then runs as a second, stricter opinion, and a + * statement it cannot parse is still accepted on the scan alone rather than failing a + * user whose dialect is merely richer than the parser. + * + *

A {@code WITH} statement is the exception: it can be a select, but it can equally + * be a data modifying cte such as {@code WITH x AS (...) DELETE FROM t}, which the scan + * cannot tell apart. It therefore passes only when the parser proves it is a select. + * @param sql statement to validate + * @throws SqlSecurityException if the statement writes, or carries more than one statement + */ + private void validateReadOnly(String sql) throws SqlSecurityException { + StatementShape shape = scan(sql); + if (shape.statementCount() != 1) { throw new SqlSecurityException("Only a single statement is allowed."); } - Statement statement = statements.getStatements().get(0); + boolean cte = WITH_KEYWORD.equals(shape.leadingKeyword()); + if (!cte && !SELECT_KEYWORD.equals(shape.leadingKeyword())) { + throw new SqlSecurityException("Only SELECT statements are allowed."); + } + + Statement statement; + try { + statement = parseSingleStatement(sql); + } catch (SqlSecurityException e) { + if (cte) { + throw e; + } + // debug, not warn: a dialect the parser does not cover is the expected case here, + // and this runs on every evaluation of every rule that uses one + log.debug("SQL not understood by the parser, accepted as a read on the statement scan: {}", sql); + return; + } if (!(statement instanceof Select select)) { throw new SqlSecurityException("Only SELECT statements are allowed."); } + // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read + if (select instanceof PlainSelect plainSelect && !CollectionUtils.isEmpty(plainSelect.getIntoTables())) { + throw new SqlSecurityException("SELECT ... INTO is not allowed."); + } + } - if (!restrictTables) { - // read only is the whole policy in this mode, see selectOnly() - return; + private void validateAgainstWhitelist(String sql) throws SqlSecurityException { + Statement statement = parseSingleStatement(sql); + + if (!(statement instanceof Select select)) { + throw new SqlSecurityException("Only SELECT statements are allowed."); } // Check for CTE at top level @@ -130,6 +181,122 @@ public void validate(String sql) throws SqlSecurityException { validateTables(tables); } + /** + * @param sql statement to parse + * @return the only statement the string carries + * @throws SqlSecurityException if the string does not parse, or carries more than one statement + */ + private Statement parseSingleStatement(String sql) throws SqlSecurityException { + Statements statements; + try { + statements = CCJSqlParserUtil.parseStatements(sql); + } catch (JSQLParserException e) { + // the reason travels on the exception, and read only mode treats a parse failure + // as a normal outcome, so the stack trace does not belong at warn + log.debug("Failed to parse SQL: {}", sql, e); + throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); + } + // parseStatements rather than parse: parse() returns the first statement and discards + // the rest, so "select 1; drop table x" would validate as a plain select while the + // caller still hands the whole string to the database + if (statements.getStatements().size() != 1) { + throw new SqlSecurityException("Only a single statement is allowed."); + } + return statements.getStatements().get(0); + } + + /** + * What a statement string looks like from outside any sql dialect. + * @param statementCount statements the string carries, a trailing semicolon not counting as one + * @param leadingKeyword first word of the first statement, upper cased, empty when it does not start with a word + */ + private record StatementShape(int statementCount, String leadingKeyword) { + } + + /** + * Counts the statements a string carries and reads the word it opens with, skipping over + * string literals, quoted identifiers and comments so that a semicolon inside them is not + * mistaken for a statement separator. + * + *

A backslash is not treated as an escape, because assuming it escapes the closing + * quote in a dialect where it does not would let {@code 'a\'; DROP TABLE t} hide a second + * statement inside what this scan thinks is one literal. Not assuming it costs at worst a + * rejection of a statement that uses backslash escapes, which errs the safe way. + * @param sql statement string to scan + * @return the shape of the string + * @throws SqlSecurityException if a literal or a block comment is left open + */ + private StatementShape scan(String sql) throws SqlSecurityException { + int statementCount = 0; + boolean statementHasContent = false; + String leadingKeyword = ""; + int index = 0; + while (index < sql.length()) { + char current = sql.charAt(index); + if (current == '-' && index + 1 < sql.length() && sql.charAt(index + 1) == '-') { + int lineEnd = sql.indexOf('\n', index); + index = lineEnd < 0 ? sql.length() : lineEnd + 1; + } else if (current == '/' && index + 1 < sql.length() && sql.charAt(index + 1) == '*') { + int commentEnd = sql.indexOf("*/", index + 2); + if (commentEnd < 0) { + throw new SqlSecurityException("Unterminated block comment."); + } + index = commentEnd + 2; + } else if (current == '\'' || current == '"' || current == '`') { + index = skipQuoted(sql, index, current); + statementHasContent = true; + } else if (current == ';') { + if (statementHasContent) { + statementCount++; + } + statementHasContent = false; + index++; + } else { + if (!Character.isWhitespace(current)) { + if (statementCount == 0 && !statementHasContent) { + leadingKeyword = readKeyword(sql, index); + } + statementHasContent = true; + } + index++; + } + } + if (statementHasContent) { + statementCount++; + } + return new StatementShape(statementCount, leadingKeyword); + } + + /** + * @param sql statement string being scanned + * @param start index of the opening quote + * @param quote quote character to close on, a doubled one being an escaped quote rather than the close + * @return index just past the closing quote + * @throws SqlSecurityException if the quote is never closed + */ + private int skipQuoted(String sql, int start, char quote) throws SqlSecurityException { + int index = start + 1; + while (index < sql.length()) { + if (sql.charAt(index) == quote) { + if (index + 1 < sql.length() && sql.charAt(index + 1) == quote) { + index += 2; + continue; + } + return index + 1; + } + index++; + } + throw new SqlSecurityException("Unterminated quoted literal."); + } + + private String readKeyword(String sql, int start) { + int index = start; + while (index < sql.length() && (Character.isLetter(sql.charAt(index)) || sql.charAt(index) == '_')) { + index++; + } + return sql.substring(start, index).toUpperCase(Locale.ROOT); + } + private void validateTables(List tables) throws SqlSecurityException { if (CollectionUtils.isEmpty(tables)) { return; diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index eb942b9c370..b8eadcc3f2e 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -305,4 +305,85 @@ void testSelectOnlyAcceptsAnyTableAndNestedReads() { "SELECT value FROM cpu WHERE host = (SELECT host FROM hosts LIMIT 1)")); assertDoesNotThrow(() -> selectOnly.validate("SELECT a FROM t1 UNION ALL SELECT b FROM t2")); } + + /** + * The only sql executor today talks to GreptimeDB, whose range query syntax JSqlParser + * cannot parse. These are ordinary reads and used to run, so read only mode has to keep + * accepting them rather than turn a richer dialect into a rule that no longer fires. + */ + @Test + void testSelectOnlyAcceptsDialectTheParserDoesNotUnderstand() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate( + "SELECT ts, avg(value) RANGE '10s' FROM cpu ALIGN '5s' FILL LINEAR")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu ALIGN '5s'")); + } + + /** + * Accepting what the parser cannot read must not become a way through: the statement + * count and the leading keyword are established without the parser, so they still hold + * for a string it never understood. + */ + @Test + void testUnparsableStatementMustStillBeOneRead() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT avg(value) RANGE '10s' FROM cpu ALIGN '5s'; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("TQL EVAL (0, 10, '5s') sum(cpu)")); + } + + /** + * A semicolon that is data or commentary is not a statement separator, and a statement + * scan that cannot tell the difference would reject ordinary queries. + */ + @Test + void testSemicolonInsideLiteralOrCommentDoesNotSplitTheStatement() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'a; DROP TABLE cpu'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'it''s; fine'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu -- ; DROP TABLE cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu /* ; DROP TABLE cpu */ LIMIT 1")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM \"cpu;usage\"")); + } + + @Test + void testUnclosedLiteralOrCommentIsRejected() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'open")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu /* open")); + } + + /** + * `SELECT ... INTO` parses as a select but writes a table in the dialects that support it. + */ + @Test + void testSelectOnlyRejectsSelectInto() { + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly() + .validate("SELECT * INTO backup FROM cpu")); + } + + /** + * A `WITH` statement can be a select or a data modifying cte, which the statement scan + * cannot tell apart, so it passes only when the parser proves it reads. + */ + @Test + void testSelectOnlyAcceptsCteOnlyWhenTheParserProvesItReads() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("WITH x AS (SELECT 1 AS v) SELECT * FROM x")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (SELECT id FROM t) DELETE FROM cpu WHERE id IN (SELECT id FROM x)")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s') SELECT * FROM x")); + } + + /** + * The whitelisting mode needs the parse tree to enumerate table names, so unlike read + * only mode it has nothing to fall back on and keeps rejecting what it cannot parse. + */ + @Test + void testWhitelistModeStillRejectsWhatItCannotParse() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT avg(value) RANGE '10s' FROM hertzbeat_logs ALIGN '5s'")); + } } From c27fab887165d572c0befade14dc9bc5d44cbcc5 Mon Sep 17 00:00:00 2001 From: Duansg Date: Sun, 9 Aug 2026 06:29:18 -0700 Subject: [PATCH 3/5] [fix] reject a write at any depth of a reading statement The read only check looked at the outermost node of the parse tree, which proves nothing about the rest of it. JSqlParser reports a plain select as the outermost node of all three of these, and each carries a write: WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu SELECT * FROM (SELECT * INTO backup FROM cpu) t Walk the whole tree instead. A walk that ends in an exception rejects too: a data modifying cte makes JSqlParser's own finder cast a ParenthesedDelete to a ParenthesedSelect, which used to escape as a 500 rather than a rejection. The tree is only available when the parser can read the dialect, and the whole point of read only mode is that it often cannot, so the statement scan now also rejects a string carrying a word that only a write contains. That is what holds for a nested write in a GreptimeDB range query. It matches whole words outside literals, so delete_count and truncate(value, 2) keep working, and it lists no word that doubles as an ordinary function. With writes caught wherever they sit, a cte no longer has to parse to be accepted, so range queries inside a WITH work now. The whitelisting mode had the same hole from the other side: it says which tables a statement may touch, so "select * into backup from hertzbeat_logs" named only allowed tables and passed. It walks the tree now too. Co-Authored-By: Claude Opus 5 (1M context) --- .../support/valid/SqlSecurityValidator.java | 153 ++++++++++++++---- .../valid/SqlSecurityValidatorTest.java | 65 +++++++- 2 files changed, 186 insertions(+), 32 deletions(-) diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index 1667b24af9e..4b0095ca961 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -22,12 +22,16 @@ import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; import net.sf.jsqlparser.statement.Statements; +import net.sf.jsqlparser.statement.delete.Delete; +import net.sf.jsqlparser.statement.insert.Insert; +import net.sf.jsqlparser.statement.merge.Merge; import net.sf.jsqlparser.statement.select.LateralSubSelect; import net.sf.jsqlparser.statement.select.ParenthesedSelect; import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.statement.select.WithItem; +import net.sf.jsqlparser.statement.update.Update; import net.sf.jsqlparser.util.TablesNamesFinder; import org.springframework.util.CollectionUtils; @@ -49,7 +53,8 @@ *

  • read only: only SELECT, any table, nested reads kept.
    • * * - *

    Both modes reject a string that carries more than one statement. + *

    Both modes reject a string that carries more than one statement, and neither lets a + * write through at any depth of the statement. */ @Slf4j public class SqlSecurityValidator { @@ -58,6 +63,23 @@ public class SqlSecurityValidator { private static final String WITH_KEYWORD = "WITH"; + /** + * Words that no read contains, matched as whole words outside literals and comments. + * + *

    This is the check that holds when the parser cannot read the dialect and there is no + * tree to walk, so it has to catch a write wherever it sits, including nested in a cte or + * a subquery. It is deliberately coarse; the tree walk is the precise one. + * + *

    Statements that can only stand alone, {@code TRUNCATE} and {@code CALL} among them, + * are absent: the leading keyword already rejects those, and several of them double as + * ordinary functions, {@code TRUNCATE(value, 2)} and {@code REPLACE(msg, 'a', 'b')} being + * the ones a metric query really does use. An identifier that collides with a word listed + * here can still be quoted, which the scan skips over. + */ + private static final Set WRITE_KEYWORDS = Set.of( + "DELETE", "INSERT", "UPDATE", "MERGE", "INTO", "DROP", "ALTER", + "CREATE", "GRANT", "REVOKE", "COPY", "RENAME", "ATTACH", "DETACH"); + private final Set allowedTables; private final boolean restrictTables; @@ -112,16 +134,16 @@ public void validate(String sql) throws SqlSecurityException { * {@code SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s'} are rejected by the parser * although they are ordinary reads. * - *

    So the two properties this mode has to guarantee, one statement and read only, are - * established without the parser: statements are counted by scanning outside string - * literals and comments, and the leading keyword decides whether it reads. Both checks - * are dialect independent. The parser then runs as a second, stricter opinion, and a - * statement it cannot parse is still accepted on the scan alone rather than failing a - * user whose dialect is merely richer than the parser. + *

    So the properties this mode has to guarantee are established without the parser: + * statements are counted by scanning outside string literals and comments, the leading + * keyword decides whether the statement reads, and no word that only a write contains may + * appear anywhere. All three are dialect independent, and the last one is what covers a + * write nested where the scan has no structure to reason about, as in + * {@code WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x}. * - *

    A {@code WITH} statement is the exception: it can be a select, but it can equally - * be a data modifying cte such as {@code WITH x AS (...) DELETE FROM t}, which the scan - * cannot tell apart. It therefore passes only when the parser proves it is a select. + *

    The parser then runs as a second and precise opinion over the whole tree. A statement + * it cannot parse is still accepted on the scan alone rather than failing a user whose + * dialect is merely richer than the parser. * @param sql statement to validate * @throws SqlSecurityException if the statement writes, or carries more than one statement */ @@ -130,8 +152,10 @@ private void validateReadOnly(String sql) throws SqlSecurityException { if (shape.statementCount() != 1) { throw new SqlSecurityException("Only a single statement is allowed."); } - boolean cte = WITH_KEYWORD.equals(shape.leadingKeyword()); - if (!cte && !SELECT_KEYWORD.equals(shape.leadingKeyword())) { + if (shape.writeKeyword() != null) { + throw new SqlSecurityException("'" + shape.writeKeyword() + "' is not allowed, only reads are."); + } + if (!SELECT_KEYWORD.equals(shape.leadingKeyword()) && !WITH_KEYWORD.equals(shape.leadingKeyword())) { throw new SqlSecurityException("Only SELECT statements are allowed."); } @@ -139,21 +163,38 @@ private void validateReadOnly(String sql) throws SqlSecurityException { try { statement = parseSingleStatement(sql); } catch (SqlSecurityException e) { - if (cte) { - throw e; - } // debug, not warn: a dialect the parser does not cover is the expected case here, // and this runs on every evaluation of every rule that uses one log.debug("SQL not understood by the parser, accepted as a read on the statement scan: {}", sql); return; } - if (!(statement instanceof Select select)) { + if (!(statement instanceof Select)) { throw new SqlSecurityException("Only SELECT statements are allowed."); } - // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read - if (select instanceof PlainSelect plainSelect && !CollectionUtils.isEmpty(plainSelect.getIntoTables())) { - throw new SqlSecurityException("SELECT ... INTO is not allowed."); + assertNothingWrites(statement); + } + + /** + * Walks the whole statement rather than its outermost node, because a write hides at any + * depth: {@code SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu} puts the write in a + * branch of a set operation, and {@code SELECT * FROM (SELECT * INTO backup FROM cpu) t} + * puts it in a subquery, so an outermost node that is a plain select proves nothing. + * + *

    Any other failure of the walk is a rejection too. A data modifying cte makes + * JSqlParser's own finder cast a {@code ParenthesedDelete} to a {@code ParenthesedSelect}, + * and a walk that ended in an exception established nothing about the statement. + * @param statement parsed statement to walk + * @throws SqlSecurityException if any part of the statement writes, or could not be walked + */ + private void assertNothingWrites(Statement statement) throws SqlSecurityException { + try { + new ReadOnlyStatementFinder().getTableList(statement); + } catch (SecurityViolationException e) { + throw new SqlSecurityException(e.getMessage()); + } catch (RuntimeException e) { + log.debug("Failed to walk SQL, so nothing about it is established: {}", statement, e); + throw new SqlSecurityException("SQL structure could not be verified as a read."); } } @@ -164,6 +205,10 @@ private void validateAgainstWhitelist(String sql) throws SqlSecurityException { throw new SqlSecurityException("Only SELECT statements are allowed."); } + // the whitelist is about which tables a statement may touch, so on its own it lets + // "select * into backup from hertzbeat_logs" through: every table it names is allowed + assertNothingWrites(statement); + // Check for CTE at top level if (select.getWithItemsList() != null && !select.getWithItemsList().isEmpty()) { throw new SqlSecurityException("CTE (WITH clause) is not allowed"); @@ -209,8 +254,9 @@ private Statement parseSingleStatement(String sql) throws SqlSecurityException { * What a statement string looks like from outside any sql dialect. * @param statementCount statements the string carries, a trailing semicolon not counting as one * @param leadingKeyword first word of the first statement, upper cased, empty when it does not start with a word + * @param writeKeyword first word from {@link #WRITE_KEYWORDS} found anywhere, null when there is none */ - private record StatementShape(int statementCount, String leadingKeyword) { + private record StatementShape(int statementCount, String leadingKeyword, String writeKeyword) { } /** @@ -230,6 +276,7 @@ private StatementShape scan(String sql) throws SqlSecurityException { int statementCount = 0; boolean statementHasContent = false; String leadingKeyword = ""; + String writeKeyword = null; int index = 0; while (index < sql.length()) { char current = sql.charAt(index); @@ -251,11 +298,21 @@ private StatementShape scan(String sql) throws SqlSecurityException { } statementHasContent = false; index++; + } else if (Character.isLetter(current) || current == '_') { + // read the whole word and step past it, so that a word listed as a write is + // only matched on its own and never inside an identifier like delete_count + int wordEnd = wordEnd(sql, index); + String word = sql.substring(index, wordEnd).toUpperCase(Locale.ROOT); + if (statementCount == 0 && !statementHasContent) { + leadingKeyword = word; + } + if (writeKeyword == null && WRITE_KEYWORDS.contains(word)) { + writeKeyword = word; + } + statementHasContent = true; + index = wordEnd; } else { if (!Character.isWhitespace(current)) { - if (statementCount == 0 && !statementHasContent) { - leadingKeyword = readKeyword(sql, index); - } statementHasContent = true; } index++; @@ -264,7 +321,7 @@ private StatementShape scan(String sql) throws SqlSecurityException { if (statementHasContent) { statementCount++; } - return new StatementShape(statementCount, leadingKeyword); + return new StatementShape(statementCount, leadingKeyword, writeKeyword); } /** @@ -289,12 +346,19 @@ private int skipQuoted(String sql, int start, char quote) throws SqlSecurityExce throw new SqlSecurityException("Unterminated quoted literal."); } - private String readKeyword(String sql, int start) { + /** + * @param sql statement string being scanned + * @param start index of the first character of a word + * @return index just past the word, digits and underscores counting as part of it so that + * {@code delete_count} is one word rather than a {@code delete} followed by a remainder + */ + private int wordEnd(String sql, int start) { int index = start; - while (index < sql.length() && (Character.isLetter(sql.charAt(index)) || sql.charAt(index) == '_')) { + while (index < sql.length() + && (Character.isLetterOrDigit(sql.charAt(index)) || sql.charAt(index) == '_' || sql.charAt(index) == '$')) { index++; } - return sql.substring(start, index).toUpperCase(Locale.ROOT); + return index; } private void validateTables(List tables) throws SqlSecurityException { @@ -327,6 +391,41 @@ private static class SecurityViolationException extends RuntimeException { } } + /** + * Walks a statement and throws as soon as it finds a part of it that writes, at any depth. + */ + private static class ReadOnlyStatementFinder extends TablesNamesFinder { + + @Override + public Void visit(PlainSelect plainSelect, Object context) { + // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read + if (!CollectionUtils.isEmpty(plainSelect.getIntoTables())) { + throw new SecurityViolationException("SELECT ... INTO is not allowed."); + } + return super.visit(plainSelect, context); + } + + @Override + public Void visit(Delete delete, Object context) { + throw new SecurityViolationException("DELETE is not allowed, only reads are."); + } + + @Override + public Void visit(Insert insert, Object context) { + throw new SecurityViolationException("INSERT is not allowed, only reads are."); + } + + @Override + public Void visit(Update update, Object context) { + throw new SecurityViolationException("UPDATE is not allowed, only reads are."); + } + + @Override + public Void visit(Merge merge, Object context) { + throw new SecurityViolationException("MERGE is not allowed, only reads are."); + } + } + /** * Custom TablesNamesFinder that throws exceptions on dangerous SQL structures. * Extends TablesNamesFinder with proper generic type to avoid raw type warnings. diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index b8eadcc3f2e..2f6619cd977 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -363,18 +363,73 @@ void testSelectOnlyRejectsSelectInto() { .validate("SELECT * INTO backup FROM cpu")); } + @Test + void testSelectOnlyAcceptsCte() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("WITH x AS (SELECT 1 AS v) SELECT * FROM x")); + assertDoesNotThrow(() -> selectOnly.validate( + "WITH x AS (SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s') SELECT * FROM x")); + } + /** - * A `WITH` statement can be a select or a data modifying cte, which the statement scan - * cannot tell apart, so it passes only when the parser proves it reads. + * An outermost node that is a plain select proves nothing about the rest of the tree: a + * write hides in a cte, in a branch of a set operation, or in a subquery, and JSqlParser + * reports the outermost node of all three as a select. */ @Test - void testSelectOnlyAcceptsCteOnlyWhenTheParserProvesItReads() { + void testSelectOnlyRejectsWritesNestedInsideReads() { SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); - assertDoesNotThrow(() -> selectOnly.validate("WITH x AS (SELECT 1 AS v) SELECT * FROM x")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (INSERT INTO cpu VALUES (1) RETURNING *) SELECT * FROM x")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( "WITH x AS (SELECT id FROM t) DELETE FROM cpu WHERE id IN (SELECT id FROM x)")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( - "WITH x AS (SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s') SELECT * FROM x")); + "SELECT * INTO backup FROM cpu UNION SELECT * FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM cpu UNION SELECT * INTO backup FROM cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM (SELECT * INTO backup FROM cpu) t")); + } + + /** + * The nested writes above have to stay rejected when the parser cannot read the dialect + * and there is no tree to walk, which is the case the whole read only mode exists for. + */ + @Test + void testNestedWritesStayRejectedWithoutTheParser() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * FROM (DELETE FROM cpu RETURNING *) t ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "WITH x AS (DELETE FROM cpu RETURNING *) SELECT avg(v) RANGE '10s' FROM x ALIGN '5s'")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate( + "SELECT * INTO backup FROM cpu ALIGN '5s'")); + } + + /** + * The word scan matches whole words only, so ordinary reads whose identifiers or functions + * merely contain one keep working. An identifier that collides outright can be quoted. + */ + @Test + void testWriteWordScanDoesNotCatchOrdinaryReads() { + SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT delete_count, insert_rate FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT truncate(value, 2) FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT replace(msg, 'a', 'b') FROM logs")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'drop table cpu'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT \"drop\" FROM cpu")); + } + + /** + * The whitelist says which tables a statement may touch, so on its own it lets a write + * through as long as every table it names is allowed. + */ + @Test + void testWhitelistModeRejectsSelectIntoOnAnAllowedTable() { + assertThrows(SqlSecurityException.class, () -> validator.validate( + "SELECT * INTO backup FROM hertzbeat_logs")); } /** From 8c6eef3a82798caeb8f7d93f0c5f0ac08ea9ca02 Mon Sep 17 00:00:00 2001 From: Duansg Date: Sun, 9 Aug 2026 07:47:18 -0700 Subject: [PATCH 4/5] [alerter]bugfix: reject dollar-quoted stacked SQL Signed-off-by: Duansg --- .../service/impl/DataSourceServiceImpl.java | 8 +- .../expr/AlertExpressionEvalVisitorTest.java | 11 +- .../alert/service/DataSourceServiceTest.java | 13 +- .../support/valid/SqlSecurityValidator.java | 169 +++++++++++------- .../valid/SqlSecurityValidatorTest.java | 28 +-- 5 files changed, 144 insertions(+), 85 deletions(-) diff --git a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java index 795d87386e2..6698c9ac603 100644 --- a/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java +++ b/hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java @@ -128,7 +128,7 @@ public List> query(String datasource, String expr) { try { return guardSql(executor, sqlSecurityValidator).execute(expr); } catch (AlertExpressionException ae) { - // a statement the policy rejected, whose message names the part it broke + // A statement the policy rejected, whose message names the part it broke. throw ae; } catch (Exception e) { log.error("Error executing query on datasource {}: {}", datasource, e.getMessage()); @@ -144,9 +144,9 @@ public List> query(String datasource, String expr) { * A datasource that does not speak sql is handed back untouched: a promql endpoint takes * a query string, not a statement, and running it through a sql parser would only reject * valid promql. - * @param executor executor chosen for this datasource - * @param validator policy to enforce, read only for expressions and whitelisting for raw log queries - * @return the executor, guarded when it speaks sql + * @param executor Executor chosen for this datasource + * @param validator Policy to enforce, read only for expressions and whitelisting for raw log queries + * @return The executor, guarded when it speaks sql */ private QueryExecutor guardSql(QueryExecutor executor, SqlSecurityValidator validator) { if (!executor.support(WarehouseConstants.SQL)) { diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java index 75222c602b4..f384d501af3 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java @@ -869,7 +869,8 @@ void testSqlCallRunsPlainRead() { when(mockExecutor.execute("select value from cpu where host = 'server1'")) .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); - List> result = evaluate("sql(\"select value from cpu where host = 'server1'\") > 70"); + final List> result = + evaluate("sql(\"select value from cpu where host = 'server1'\") > 70"); assertEquals(1, result.size()); assertEquals(80.0, result.get(0).get("__value__")); @@ -882,10 +883,10 @@ void testSqlCallRunsPlainRead() { */ @Test void testSqlCallKeepsSubqueriesWorking() { - String sql = "select value from cpu where host = (select host from hosts limit 1)"; + final String sql = "select value from cpu where host = (select host from hosts limit 1)"; when(mockExecutor.execute(sql)).thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); - List> result = evaluate("sql(\"" + sql + "\") > 70"); + final List> result = evaluate("sql(\"" + sql + "\") > 70"); assertEquals(1, result.size()); assertEquals(80.0, result.get(0).get("__value__")); @@ -902,7 +903,7 @@ void testBothCallSpellingsReachTheExecutorAsWritten() { when(mockExecutor.execute("rate(http_requests_total[5m])")) .thenReturn(List.of(new HashMap<>(Map.of("__value__", 80.0)))); - List> result = evaluate("promql(\"rate(http_requests_total[5m])\") > 70"); + final List> result = evaluate("promql(\"rate(http_requests_total[5m])\") > 70"); assertEquals(1, result.size()); assertEquals(80.0, result.get(0).get("__value__")); @@ -914,4 +915,4 @@ private List> evaluate(String expression) { AlertExpressionParser parser = new AlertExpressionParser(tokens); return new AlertExpressionEvalVisitor(mockExecutor, tokens).visit(parser.expression()); } -} \ No newline at end of file +} diff --git a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java index 52114fa7724..b513d035049 100644 --- a/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java +++ b/hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/service/DataSourceServiceTest.java @@ -797,7 +797,7 @@ void query12() { */ @Test void calculateRejectsWritesWhateverSpellingTheyArrivedIn() { - QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); when(mockExecutor.support("sql")).thenReturn(true); dataSourceService.setExecutors(List.of(mockExecutor)); @@ -817,12 +817,12 @@ void calculateRejectsWritesWhateverSpellingTheyArrivedIn() { */ @Test void calculateLeavesPromqlDatasourcesAlone() { - QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); when(mockExecutor.support("promql")).thenReturn(true); when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); dataSourceService.setExecutors(List.of(mockExecutor)); - List> result = dataSourceService.calculate( + final List> result = dataSourceService.calculate( "promql", "promql(\"rate(http_requests_total[5m])\") > 70"); assertEquals(1, result.size()); @@ -835,13 +835,14 @@ void calculateLeavesPromqlDatasourcesAlone() { */ @Test void calculateStillRunsReadsOnSqlDatasources() { - QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); + final QueryExecutor mockExecutor = Mockito.mock(QueryExecutor.class); when(mockExecutor.support("sql")).thenReturn(true); when(mockExecutor.execute(anyString())).thenReturn(List.of(new HashMap<>(Map.of("__value__", 100.0)))); dataSourceService.setExecutors(List.of(mockExecutor)); - String rangeQuery = "select avg(value) RANGE '10s' from cpu ALIGN '5s'"; - List> result = dataSourceService.calculate("sql", "sql(\"" + rangeQuery + "\") > 70"); + final String rangeQuery = "select avg(value) RANGE '10s' from cpu ALIGN '5s'"; + final List> result = + dataSourceService.calculate("sql", "sql(\"" + rangeQuery + "\") > 70"); assertEquals(1, result.size()); verify(mockExecutor).execute(rangeQuery); diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index 4b0095ca961..6c4d5fa0122 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -82,7 +82,7 @@ public class SqlSecurityValidator { private final Set allowedTables; - private final boolean restrictTables; + private final ValidationPolicy validationPolicy; public SqlSecurityValidator(Collection allowedTables) { if (CollectionUtils.isEmpty(allowedTables)) { @@ -92,12 +92,12 @@ public SqlSecurityValidator(Collection allowedTables) { .map(this::normalizeIdentifier) .collect(Collectors.toSet()); } - this.restrictTables = true; + this.validationPolicy = ValidationPolicy.WHITELISTED_SELECT; } private SqlSecurityValidator() { this.allowedTables = new HashSet<>(); - this.restrictTables = false; + this.validationPolicy = ValidationPolicy.READ_ONLY; } /** @@ -111,7 +111,7 @@ private SqlSecurityValidator() { * Those structures are blocked there because they are the ways a statement can reach a * table the whitelist never mentions; with every table already readable they buy no * protection, while alert expressions do use subqueries and nested aggregation. - * @return a validator that only rejects statements which are not plain selects + * @return A validator that accepts only statements proven to be read-only */ public static SqlSecurityValidator selectOnly() { return new SqlSecurityValidator(); @@ -121,10 +121,10 @@ public void validate(String sql) throws SqlSecurityException { if (sql == null || sql.trim().isEmpty()) { throw new SqlSecurityException("SQL statement cannot be empty"); } - if (restrictTables) { - validateAgainstWhitelist(sql); - } else { - validateReadOnly(sql); + switch (validationPolicy) { + case WHITELISTED_SELECT -> validateAgainstWhitelist(sql); + case READ_ONLY -> validateReadOnly(sql); + default -> throw new SqlSecurityException("Unknown SQL validation policy."); } } @@ -144,11 +144,11 @@ public void validate(String sql) throws SqlSecurityException { *

    The parser then runs as a second and precise opinion over the whole tree. A statement * it cannot parse is still accepted on the scan alone rather than failing a user whose * dialect is merely richer than the parser. - * @param sql statement to validate - * @throws SqlSecurityException if the statement writes, or carries more than one statement + * @param sql Statement to validate + * @throws SqlSecurityException If the statement writes, or carries more than one statement */ private void validateReadOnly(String sql) throws SqlSecurityException { - StatementShape shape = scan(sql); + final StatementShape shape = scan(sql); if (shape.statementCount() != 1) { throw new SqlSecurityException("Only a single statement is allowed."); } @@ -159,13 +159,13 @@ private void validateReadOnly(String sql) throws SqlSecurityException { throw new SqlSecurityException("Only SELECT statements are allowed."); } - Statement statement; + final Statement statement; try { statement = parseSingleStatement(sql); - } catch (SqlSecurityException e) { - // debug, not warn: a dialect the parser does not cover is the expected case here, - // and this runs on every evaluation of every rule that uses one - log.debug("SQL not understood by the parser, accepted as a read on the statement scan: {}", sql); + } catch (JSQLParserException e) { + // Debug, not warn: a dialect the parser does not cover is the expected case here. + // This validation runs on every evaluation of every rule that uses one. + log.debug("SQL not understood by the parser, accepted as a read on the statement scan: {}", sql, e); return; } @@ -184,8 +184,8 @@ private void validateReadOnly(String sql) throws SqlSecurityException { *

    Any other failure of the walk is a rejection too. A data modifying cte makes * JSqlParser's own finder cast a {@code ParenthesedDelete} to a {@code ParenthesedSelect}, * and a walk that ended in an exception established nothing about the statement. - * @param statement parsed statement to walk - * @throws SqlSecurityException if any part of the statement writes, or could not be walked + * @param statement Parsed statement to walk + * @throws SqlSecurityException If any part of the statement writes, or could not be walked */ private void assertNothingWrites(Statement statement) throws SqlSecurityException { try { @@ -199,13 +199,19 @@ private void assertNothingWrites(Statement statement) throws SqlSecurityExceptio } private void validateAgainstWhitelist(String sql) throws SqlSecurityException { - Statement statement = parseSingleStatement(sql); + final Statement statement; + try { + statement = parseSingleStatement(sql); + } catch (JSQLParserException e) { + log.debug("Failed to parse SQL: {}", sql, e); + throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); + } if (!(statement instanceof Select select)) { throw new SqlSecurityException("Only SELECT statements are allowed."); } - // the whitelist is about which tables a statement may touch, so on its own it lets + // The whitelist is about which tables a statement may touch, so on its own it lets // "select * into backup from hertzbeat_logs" through: every table it names is allowed assertNothingWrites(statement); @@ -215,8 +221,8 @@ private void validateAgainstWhitelist(String sql) throws SqlSecurityException { } // Use custom TablesNamesFinder that throws on dangerous structures - SecurityTablesNamesFinder finder = new SecurityTablesNamesFinder(); - List tables; + final SecurityTablesNamesFinder finder = new SecurityTablesNamesFinder(); + final List tables; try { tables = finder.getTableList(statement); } catch (SecurityViolationException e) { @@ -227,23 +233,16 @@ private void validateAgainstWhitelist(String sql) throws SqlSecurityException { } /** - * @param sql statement to parse - * @return the only statement the string carries - * @throws SqlSecurityException if the string does not parse, or carries more than one statement + * @param sql Statement to parse + * @return The only statement the string carries + * @throws JSQLParserException If the string does not parse + * @throws SqlSecurityException If the string carries more than one statement */ - private Statement parseSingleStatement(String sql) throws SqlSecurityException { - Statements statements; - try { - statements = CCJSqlParserUtil.parseStatements(sql); - } catch (JSQLParserException e) { - // the reason travels on the exception, and read only mode treats a parse failure - // as a normal outcome, so the stack trace does not belong at warn - log.debug("Failed to parse SQL: {}", sql, e); - throw new SqlSecurityException("Invalid SQL syntax: " + e.getMessage(), e); - } - // parseStatements rather than parse: parse() returns the first statement and discards - // the rest, so "select 1; drop table x" would validate as a plain select while the - // caller still hands the whole string to the database + private Statement parseSingleStatement(String sql) throws JSQLParserException, SqlSecurityException { + final Statements statements = CCJSqlParserUtil.parseStatements(sql); + // ParseStatements rather than parse: parse() returns only the first statement. + // Otherwise, "select 1; drop table x" would validate as a plain select. + // The caller would still hand the whole string to the database. if (statements.getStatements().size() != 1) { throw new SqlSecurityException("Only a single statement is allowed."); } @@ -252,9 +251,9 @@ private Statement parseSingleStatement(String sql) throws SqlSecurityException { /** * What a statement string looks like from outside any sql dialect. - * @param statementCount statements the string carries, a trailing semicolon not counting as one - * @param leadingKeyword first word of the first statement, upper cased, empty when it does not start with a word - * @param writeKeyword first word from {@link #WRITE_KEYWORDS} found anywhere, null when there is none + * @param statementCount Statements the string carries, a trailing semicolon not counting as one + * @param leadingKeyword First word of the first statement, upper cased, empty when it does not start with a word + * @param writeKeyword First word from {@link #WRITE_KEYWORDS} found anywhere, null when there is none */ private record StatementShape(int statementCount, String leadingKeyword, String writeKeyword) { } @@ -268,9 +267,9 @@ private record StatementShape(int statementCount, String leadingKeyword, String * quote in a dialect where it does not would let {@code 'a\'; DROP TABLE t} hide a second * statement inside what this scan thinks is one literal. Not assuming it costs at worst a * rejection of a statement that uses backslash escapes, which errs the safe way. - * @param sql statement string to scan - * @return the shape of the string - * @throws SqlSecurityException if a literal or a block comment is left open + * @param sql Statement string to scan + * @return The shape of the string + * @throws SqlSecurityException If a literal or a block comment is left open */ private StatementShape scan(String sql) throws SqlSecurityException { int statementCount = 0; @@ -279,16 +278,25 @@ private StatementShape scan(String sql) throws SqlSecurityException { String writeKeyword = null; int index = 0; while (index < sql.length()) { - char current = sql.charAt(index); + final char current = sql.charAt(index); if (current == '-' && index + 1 < sql.length() && sql.charAt(index + 1) == '-') { - int lineEnd = sql.indexOf('\n', index); + final int lineEnd = sql.indexOf('\n', index); index = lineEnd < 0 ? sql.length() : lineEnd + 1; } else if (current == '/' && index + 1 < sql.length() && sql.charAt(index + 1) == '*') { - int commentEnd = sql.indexOf("*/", index + 2); + final int commentEnd = sql.indexOf("*/", index + 2); if (commentEnd < 0) { throw new SqlSecurityException("Unterminated block comment."); } index = commentEnd + 2; + } else if (current == '$') { + final int dollarQuoteEnd = skipDollarQuoted(sql, index); + if (dollarQuoteEnd > index) { + index = dollarQuoteEnd; + statementHasContent = true; + } else { + statementHasContent = true; + index++; + } } else if (current == '\'' || current == '"' || current == '`') { index = skipQuoted(sql, index, current); statementHasContent = true; @@ -299,10 +307,10 @@ private StatementShape scan(String sql) throws SqlSecurityException { statementHasContent = false; index++; } else if (Character.isLetter(current) || current == '_') { - // read the whole word and step past it, so that a word listed as a write is - // only matched on its own and never inside an identifier like delete_count - int wordEnd = wordEnd(sql, index); - String word = sql.substring(index, wordEnd).toUpperCase(Locale.ROOT); + // Read the whole word and step past it. + // Match a write keyword only on its own, never inside an identifier like delete_count. + final int wordEnd = wordEnd(sql, index); + final String word = sql.substring(index, wordEnd).toUpperCase(Locale.ROOT); if (statementCount == 0 && !statementHasContent) { leadingKeyword = word; } @@ -325,11 +333,11 @@ private StatementShape scan(String sql) throws SqlSecurityException { } /** - * @param sql statement string being scanned - * @param start index of the opening quote - * @param quote quote character to close on, a doubled one being an escaped quote rather than the close - * @return index just past the closing quote - * @throws SqlSecurityException if the quote is never closed + * @param sql Statement string being scanned + * @param start Index of the opening quote + * @param quote Quote character to close on, a doubled one being an escaped quote rather than the close + * @return Index just past the closing quote + * @throws SqlSecurityException If the quote is never closed */ private int skipQuoted(String sql, int start, char quote) throws SqlSecurityException { int index = start + 1; @@ -347,9 +355,45 @@ private int skipQuoted(String sql, int start, char quote) throws SqlSecurityExce } /** - * @param sql statement string being scanned - * @param start index of the first character of a word - * @return index just past the word, digits and underscores counting as part of it so that + * Skips a PostgreSQL dollar-quoted literal. Comment markers and semicolons inside the + * literal are data, so treating them as SQL syntax can hide the remainder of the input + * from the statement scanner. + * @param sql Statement string being scanned + * @param start Index of the opening dollar sign + * @return Index just past the closing delimiter, or {@code start} when this is not a delimiter + * @throws SqlSecurityException If a dollar-quoted literal is left open + */ + private int skipDollarQuoted(String sql, int start) throws SqlSecurityException { + int tagEnd = start + 1; + if (tagEnd >= sql.length()) { + return start; + } + final char firstTagCharacter = sql.charAt(tagEnd); + if (firstTagCharacter != '$' + && !Character.isLetter(firstTagCharacter) + && firstTagCharacter != '_') { + return start; + } + while (tagEnd < sql.length() + && (Character.isLetterOrDigit(sql.charAt(tagEnd)) || sql.charAt(tagEnd) == '_')) { + tagEnd++; + } + if (tagEnd >= sql.length() || sql.charAt(tagEnd) != '$') { + return start; + } + + final String delimiter = sql.substring(start, tagEnd + 1); + final int closingDelimiter = sql.indexOf(delimiter, tagEnd + 1); + if (closingDelimiter < 0) { + throw new SqlSecurityException("Unterminated dollar-quoted literal."); + } + return closingDelimiter + delimiter.length(); + } + + /** + * @param sql Statement string being scanned + * @param start Index of the first character of a word + * @return Index just past the word, digits and underscores counting as part of it so that * {@code delete_count} is one word rather than a {@code delete} followed by a remainder */ private int wordEnd(String sql, int start) { @@ -391,6 +435,11 @@ private static class SecurityViolationException extends RuntimeException { } } + private enum ValidationPolicy { + WHITELISTED_SELECT, + READ_ONLY + } + /** * Walks a statement and throws as soon as it finds a part of it that writes, at any depth. */ @@ -398,7 +447,7 @@ private static class ReadOnlyStatementFinder extends TablesNamesFinder { @Override public Void visit(PlainSelect plainSelect, Object context) { - // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read + // SELECT ... INTO writes a new table in the dialects that support it, so it is not a read. if (!CollectionUtils.isEmpty(plainSelect.getIntoTables())) { throw new SecurityViolationException("SELECT ... INTO is not allowed."); } diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index 2f6619cd977..53e7c2538d9 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -276,6 +276,10 @@ void testStackedStatementIsRejected() { "SELECT * FROM hertzbeat_logs; DROP TABLE hertzbeat_logs")); assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( "SELECT 1; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT $$--$$; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> SqlSecurityValidator.selectOnly().validate( + "SELECT $body$--$body$; DROP TABLE cpu")); } @Test @@ -285,7 +289,7 @@ void testTrailingSemicolonIsStillAcceptedAsOneStatement() { @Test void testSelectOnlyRejectsWrites() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DELETE FROM cpu")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("INSERT INTO cpu VALUES (1)")); @@ -299,7 +303,7 @@ void testSelectOnlyRejectsWrites() { */ @Test void testSelectOnlyAcceptsAnyTableAndNestedReads() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM any_metric_table")); assertDoesNotThrow(() -> selectOnly.validate( "SELECT value FROM cpu WHERE host = (SELECT host FROM hosts LIMIT 1)")); @@ -313,7 +317,7 @@ void testSelectOnlyAcceptsAnyTableAndNestedReads() { */ @Test void testSelectOnlyAcceptsDialectTheParserDoesNotUnderstand() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate( "SELECT ts, avg(value) RANGE '10s' FROM cpu ALIGN '5s' FILL LINEAR")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu ALIGN '5s'")); @@ -326,7 +330,7 @@ void testSelectOnlyAcceptsDialectTheParserDoesNotUnderstand() { */ @Test void testUnparsableStatementMustStillBeOneRead() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( "SELECT avg(value) RANGE '10s' FROM cpu ALIGN '5s'; DROP TABLE cpu")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("DROP TABLE cpu ALIGN '5s'")); @@ -339,9 +343,11 @@ void testUnparsableStatementMustStillBeOneRead() { */ @Test void testSemicolonInsideLiteralOrCommentDoesNotSplitTheStatement() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'a; DROP TABLE cpu'")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'it''s; fine'")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT $$a; -- DROP TABLE cpu$$")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT $body$a; -- DROP TABLE cpu$body$")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu -- ; DROP TABLE cpu")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu /* ; DROP TABLE cpu */ LIMIT 1")); assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM \"cpu;usage\"")); @@ -349,9 +355,11 @@ void testSemicolonInsideLiteralOrCommentDoesNotSplitTheStatement() { @Test void testUnclosedLiteralOrCommentIsRejected() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'open")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu /* open")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$open")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $body$open")); } /** @@ -365,7 +373,7 @@ void testSelectOnlyRejectsSelectInto() { @Test void testSelectOnlyAcceptsCte() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate("WITH x AS (SELECT 1 AS v) SELECT * FROM x")); assertDoesNotThrow(() -> selectOnly.validate( "WITH x AS (SELECT avg(v) RANGE '10s' FROM cpu ALIGN '5s') SELECT * FROM x")); @@ -378,7 +386,7 @@ void testSelectOnlyAcceptsCte() { */ @Test void testSelectOnlyRejectsWritesNestedInsideReads() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( "WITH x AS (DELETE FROM cpu RETURNING *) SELECT * FROM x")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( @@ -399,7 +407,7 @@ void testSelectOnlyRejectsWritesNestedInsideReads() { */ @Test void testNestedWritesStayRejectedWithoutTheParser() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( "SELECT * FROM (DELETE FROM cpu RETURNING *) t ALIGN '5s'")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate( @@ -414,7 +422,7 @@ void testNestedWritesStayRejectedWithoutTheParser() { */ @Test void testWriteWordScanDoesNotCatchOrdinaryReads() { - SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate("SELECT delete_count, insert_rate FROM cpu")); assertDoesNotThrow(() -> selectOnly.validate("SELECT truncate(value, 2) FROM cpu")); assertDoesNotThrow(() -> selectOnly.validate("SELECT replace(msg, 'a', 'b') FROM logs")); From 278e8633a64d74254d6d32f02fc7149771343a99 Mon Sep 17 00:00:00 2001 From: Duansg Date: Sun, 9 Aug 2026 08:25:33 -0700 Subject: [PATCH 5/5] [fix] refuse a dollar-quoted literal instead of scanning past it Skipping the literal closed one hole by opening its mirror. A dialect that has dollar quoting hides a stacked statement behind the comment marker in SELECT $$--$$; DROP TABLE cpu and skipping the literal is what catches that. But a dialect that does not have dollar quoting means the semicolon in SELECT 1 $$;DROP TABLE cpu$$ separates statements for real, and there the skip is the thing doing the hiding: that string was rejected before the literal was understood and accepted after. Refusing needs no assumption in either direction. A dialect with dollar quoting is refused; one without was going to fail on the syntax anyway. No read of a metric table spells anything this way, and a dollar sign that opens no literal stays an ordinary character, so $1 and a$b and '$5' still parse as reads. That the mirror case was reachable at all came from leaning on the parser: it reads $t$...$t$ as two statements but $$...$$ as one, and the whole reason this scan exists is that the parser cannot be relied on for the dialect in front of it. A dollar sign now also ends a word, so a literal opening straight after an identifier is still seen rather than swallowed into it. Co-Authored-By: Claude Opus 5 (1M context) --- .../support/valid/SqlSecurityValidator.java | 56 ++++++++++--------- .../valid/SqlSecurityValidatorTest.java | 34 ++++++++++- 2 files changed, 61 insertions(+), 29 deletions(-) diff --git a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java index 6c4d5fa0122..5d926a82622 100644 --- a/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java +++ b/hertzbeat-common-spring/src/main/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidator.java @@ -124,6 +124,8 @@ public void validate(String sql) throws SqlSecurityException { switch (validationPolicy) { case WHITELISTED_SELECT -> validateAgainstWhitelist(sql); case READ_ONLY -> validateReadOnly(sql); + // not dead: a policy added later that nobody wired up here must refuse the + // statement rather than fall through this switch having validated nothing default -> throw new SqlSecurityException("Unknown SQL validation policy."); } } @@ -289,14 +291,9 @@ private StatementShape scan(String sql) throws SqlSecurityException { } index = commentEnd + 2; } else if (current == '$') { - final int dollarQuoteEnd = skipDollarQuoted(sql, index); - if (dollarQuoteEnd > index) { - index = dollarQuoteEnd; - statementHasContent = true; - } else { - statementHasContent = true; - index++; - } + rejectDollarQuote(sql, index); + statementHasContent = true; + index++; } else if (current == '\'' || current == '"' || current == '`') { index = skipQuoted(sql, index, current); statementHasContent = true; @@ -355,51 +352,56 @@ private int skipQuoted(String sql, int start, char quote) throws SqlSecurityExce } /** - * Skips a PostgreSQL dollar-quoted literal. Comment markers and semicolons inside the - * literal are data, so treating them as SQL syntax can hide the remainder of the input - * from the statement scanner. + * Rejects a PostgreSQL dollar-quoted literal, {@code $$...$$} or {@code $tag$...$tag$}. + * + *

    Comment markers and semicolons inside such a literal are data, so a scan that reads + * them as syntax loses the rest of the input: {@code SELECT $$--$$; DROP TABLE cpu} looks + * like one statement once the {@code --} is taken for a comment. + * + *

    Rejecting rather than skipping, because skipping would be the same assumption in + * reverse. A dialect without dollar quoting means the semicolon inside one really does + * separate statements, and a scan that skipped the literal would be the thing hiding + * them. Rejecting needs no assumption either way: a dialect that has dollar quoting is + * refused, and one that does not was going to fail on the syntax regardless. No read of a + * metric table spells anything this way. * @param sql Statement string being scanned - * @param start Index of the opening dollar sign - * @return Index just past the closing delimiter, or {@code start} when this is not a delimiter - * @throws SqlSecurityException If a dollar-quoted literal is left open + * @param start Index of the dollar sign + * @throws SqlSecurityException If a dollar-quoted literal opens here */ - private int skipDollarQuoted(String sql, int start) throws SqlSecurityException { + private void rejectDollarQuote(String sql, int start) throws SqlSecurityException { int tagEnd = start + 1; if (tagEnd >= sql.length()) { - return start; + return; } final char firstTagCharacter = sql.charAt(tagEnd); if (firstTagCharacter != '$' && !Character.isLetter(firstTagCharacter) && firstTagCharacter != '_') { - return start; + // a positional parameter such as $1, or a dollar sign that is just a character + return; } while (tagEnd < sql.length() && (Character.isLetterOrDigit(sql.charAt(tagEnd)) || sql.charAt(tagEnd) == '_')) { tagEnd++; } if (tagEnd >= sql.length() || sql.charAt(tagEnd) != '$') { - return start; - } - - final String delimiter = sql.substring(start, tagEnd + 1); - final int closingDelimiter = sql.indexOf(delimiter, tagEnd + 1); - if (closingDelimiter < 0) { - throw new SqlSecurityException("Unterminated dollar-quoted literal."); + return; } - return closingDelimiter + delimiter.length(); + throw new SqlSecurityException("Dollar-quoted literals are not allowed."); } /** * @param sql Statement string being scanned * @param start Index of the first character of a word * @return Index just past the word, digits and underscores counting as part of it so that - * {@code delete_count} is one word rather than a {@code delete} followed by a remainder + * {@code delete_count} is one word rather than a {@code delete} followed by a remainder. + * A dollar sign ends the word, so that a literal opening right after an identifier is + * still seen by {@link #rejectDollarQuote} */ private int wordEnd(String sql, int start) { int index = start; while (index < sql.length() - && (Character.isLetterOrDigit(sql.charAt(index)) || sql.charAt(index) == '_' || sql.charAt(index) == '$')) { + && (Character.isLetterOrDigit(sql.charAt(index)) || sql.charAt(index) == '_')) { index++; } return index; diff --git a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java index 53e7c2538d9..de4500bc064 100644 --- a/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java +++ b/hertzbeat-common-spring/src/test/java/org/apache/hertzbeat/common/support/valid/SqlSecurityValidatorTest.java @@ -346,8 +346,6 @@ void testSemicolonInsideLiteralOrCommentDoesNotSplitTheStatement() { final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'a; DROP TABLE cpu'")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'it''s; fine'")); - assertDoesNotThrow(() -> selectOnly.validate("SELECT $$a; -- DROP TABLE cpu$$")); - assertDoesNotThrow(() -> selectOnly.validate("SELECT $body$a; -- DROP TABLE cpu$body$")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu -- ; DROP TABLE cpu")); assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu /* ; DROP TABLE cpu */ LIMIT 1")); assertDoesNotThrow(() -> selectOnly.validate("SELECT value FROM \"cpu;usage\"")); @@ -358,8 +356,40 @@ void testUnclosedLiteralOrCommentIsRejected() { final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu WHERE msg = 'open")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT * FROM cpu /* open")); + } + + /** + * A dollar-quoted literal is refused outright rather than skipped over, so that neither + * answer to "does this dialect have dollar quoting" can hide a statement. + * + *

    Skipping would lose a stacked statement to a dialect that has it, since the comment + * marker in {@code SELECT $$--$$; DROP TABLE cpu} is data rather than a comment. Skipping + * would equally lose one to a dialect that does not, since the semicolon in + * {@code SELECT 1 $$;DROP TABLE cpu$$} really does separate statements there. + */ + @Test + void testDollarQuotedLiteralIsRejectedRatherThanSkipped() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$--$$; DROP TABLE cpu")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT 1 $$;DROP TABLE cpu$$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT 1 $t$;DROP TABLE cpu$t$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$a; -- DROP TABLE cpu$$")); + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $body$a; -- DROP TABLE cpu$body$")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $$open")); assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT $body$open")); + // a literal opening right after an identifier is still a literal + assertThrows(SqlSecurityException.class, () -> selectOnly.validate("SELECT a$$b;c$$ FROM cpu")); + } + + /** + * A dollar sign that opens nothing is an ordinary character, so reads keep working. + */ + @Test + void testLoneDollarSignIsNotTreatedAsLiteral() { + final SqlSecurityValidator selectOnly = SqlSecurityValidator.selectOnly(); + assertDoesNotThrow(() -> selectOnly.validate("SELECT $1 FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT a$b FROM cpu")); + assertDoesNotThrow(() -> selectOnly.validate("SELECT * FROM cpu WHERE cost = '$5; x'")); } /**