security: fix SQL injection in ClickHouse query escaping
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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("'", "\\'") + "'";
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<Object> 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;
|
||||
}
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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<UsageEvent> 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<Object> 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<UsageStats> 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<UsageStats> 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<Object> 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());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user