From abed4dc96f9ef90397a9e77c40aeeb69fc700a7f Mon Sep 17 00:00:00 2001 From: hsiegeln <37154749+hsiegeln@users.noreply.github.com> Date: Sat, 4 Apr 2026 12:17:12 +0200 Subject: [PATCH] security: fix SQL injection in ClickHouse query escaping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convert ClickHouseUsageTracker and ClickHouseMetricsQueryStore to use JDBC parameterized queries (? binds) — these query raw tables without AggregateFunction columns. Fix lit(String) in RouteMetricsController and ClickHouseStatsStore to escape backslashes before single quotes. Without this, an input like \' breaks out of the string literal in ClickHouse (where \ is an escaped backslash). These must remain as literal SQL because the ClickHouse JDBC 0.9.x driver wraps PreparedStatement in sub-queries that strip AggregateFunction types, breaking -Merge combinators. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../controller/RouteMetricsController.java | 4 +- .../storage/ClickHouseMetricsQueryStore.java | 26 +++++++------ .../app/storage/ClickHouseStatsStore.java | 4 +- .../app/storage/ClickHouseUsageTracker.java | 38 +++++++++++-------- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/controller/RouteMetricsController.java b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/controller/RouteMetricsController.java index 0da3c675..199f4f62 100644 --- a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/controller/RouteMetricsController.java +++ b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/controller/RouteMetricsController.java @@ -190,8 +190,8 @@ public class RouteMetricsController { .format(instant.truncatedTo(ChronoUnit.SECONDS)) + "'"; } - /** Format a string as a SQL literal with single-quote escaping. */ + /** Format a string as a ClickHouse SQL literal with backslash + quote escaping. */ private static String lit(String value) { - return "'" + value.replace("'", "\\'") + "'"; + return "'" + value.replace("\\", "\\\\").replace("'", "\\'") + "'"; } } diff --git a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseMetricsQueryStore.java b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseMetricsQueryStore.java index 390d04b9..6ecc1174 100644 --- a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseMetricsQueryStore.java +++ b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseMetricsQueryStore.java @@ -5,7 +5,11 @@ import com.cameleer3.server.core.storage.model.MetricTimeSeries; import org.springframework.jdbc.core.JdbcTemplate; import java.time.Instant; -import java.util.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; public class ClickHouseMetricsQueryStore implements MetricsQueryStore { @@ -30,13 +34,7 @@ public class ClickHouseMetricsQueryStore implements MetricsQueryStore { String[] namesArray = metricNames.stream().map(String::trim).toArray(String[]::new); - // ClickHouse JDBC doesn't support array params with IN (?). - // Build the IN clause with properly escaped values. - StringBuilder inClause = new StringBuilder(); - for (int i = 0; i < namesArray.length; i++) { - if (i > 0) inClause.append(", "); - inClause.append("'").append(namesArray[i].replace("'", "\\'")).append("'"); - } + String placeholders = String.join(", ", Collections.nCopies(namesArray.length, "?")); String finalSql = """ SELECT toStartOfInterval(collected_at, INTERVAL %d SECOND) AS bucket, @@ -49,7 +47,13 @@ public class ClickHouseMetricsQueryStore implements MetricsQueryStore { AND metric_name IN (%s) GROUP BY bucket, metric_name ORDER BY bucket - """.formatted(intervalSeconds, inClause); + """.formatted(intervalSeconds, placeholders); + + List params = new ArrayList<>(); + params.add(instanceId); + params.add(java.sql.Timestamp.from(from)); + params.add(java.sql.Timestamp.from(to)); + Collections.addAll(params, namesArray); jdbc.query(finalSql, rs -> { String metricName = rs.getString("metric_name"); @@ -57,9 +61,7 @@ public class ClickHouseMetricsQueryStore implements MetricsQueryStore { double value = rs.getDouble("avg_value"); result.computeIfAbsent(metricName, k -> new ArrayList<>()) .add(new MetricTimeSeries.Bucket(bucket, value)); - }, instanceId, - java.sql.Timestamp.from(from), - java.sql.Timestamp.from(to)); + }, params.toArray()); return result; } diff --git a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseStatsStore.java b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseStatsStore.java index 5ab8df55..f6a95561 100644 --- a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseStatsStore.java +++ b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseStatsStore.java @@ -304,9 +304,9 @@ public class ClickHouseStatsStore implements StatsStore { .format(instant.truncatedTo(ChronoUnit.SECONDS)) + "'"; } - /** Format a string as a SQL literal with single-quote escaping. */ + /** Format a string as a ClickHouse SQL literal with backslash + quote escaping. */ private static String lit(String value) { - return "'" + value.replace("'", "\\'") + "'"; + return "'" + value.replace("\\", "\\\\").replace("'", "\\'") + "'"; } /** Convert Instant to java.sql.Timestamp for JDBC binding. */ diff --git a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseUsageTracker.java b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseUsageTracker.java index 3b7aba75..4de9165d 100644 --- a/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseUsageTracker.java +++ b/cameleer3-server-app/src/main/java/com/cameleer3/server/app/storage/ClickHouseUsageTracker.java @@ -10,17 +10,12 @@ import org.springframework.jdbc.core.JdbcTemplate; import java.sql.Timestamp; import java.time.Instant; -import java.time.ZoneOffset; -import java.time.format.DateTimeFormatter; -import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.List; public class ClickHouseUsageTracker implements UsageTracker { private static final Logger log = LoggerFactory.getLogger(ClickHouseUsageTracker.class); - private static final DateTimeFormatter CH_FMT = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss") - .withZone(ZoneOffset.UTC); private final JdbcTemplate jdbc; private final WriteBuffer buffer; @@ -64,28 +59,34 @@ public class ClickHouseUsageTracker implements UsageTracker { count() AS cnt, avg(duration_ms) AS avg_dur FROM usage_events - WHERE timestamp >= '%s' AND timestamp < '%s' - """.formatted(CH_FMT.format(from), CH_FMT.format(to))); + WHERE timestamp >= ? AND timestamp < ? + """); + List params = new ArrayList<>(); + params.add(Timestamp.from(from)); + params.add(Timestamp.from(to)); if (username != null && !username.isBlank()) { - sql.append(" AND username = '").append(username.replace("'", "\\'")).append("'"); + sql.append(" AND username = ?"); + params.add(username); } sql.append(" GROUP BY key ORDER BY cnt DESC LIMIT 100"); return jdbc.query(sql.toString(), (rs, i) -> new UsageStats( - rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur"))); + rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur")), + params.toArray()); } public List queryByUser(Instant from, Instant to) { String sql = """ SELECT username AS key, count() AS cnt, avg(duration_ms) AS avg_dur FROM usage_events - WHERE timestamp >= '%s' AND timestamp < '%s' + WHERE timestamp >= ? AND timestamp < ? GROUP BY key ORDER BY cnt DESC LIMIT 100 - """.formatted(CH_FMT.format(from), CH_FMT.format(to)); + """; return jdbc.query(sql, (rs, i) -> new UsageStats( - rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur"))); + rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur")), + Timestamp.from(from), Timestamp.from(to)); } public List queryByHour(Instant from, Instant to, String username) { @@ -94,15 +95,20 @@ public class ClickHouseUsageTracker implements UsageTracker { count() AS cnt, avg(duration_ms) AS avg_dur FROM usage_events - WHERE timestamp >= '%s' AND timestamp < '%s' - """.formatted(CH_FMT.format(from), CH_FMT.format(to))); + WHERE timestamp >= ? AND timestamp < ? + """); + List params = new ArrayList<>(); + params.add(Timestamp.from(from)); + params.add(Timestamp.from(to)); if (username != null && !username.isBlank()) { - sql.append(" AND username = '").append(username.replace("'", "\\'")).append("'"); + sql.append(" AND username = ?"); + params.add(username); } sql.append(" GROUP BY key ORDER BY key LIMIT 720"); return jdbc.query(sql.toString(), (rs, i) -> new UsageStats( - rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur"))); + rs.getString("key"), rs.getLong("cnt"), rs.getLong("avg_dur")), + params.toArray()); } }