This is an automated email from the ASF dual-hosted git repository.
Duansg pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/hertzbeat.git
The following commit(s) were added to refs/heads/master by this push:
new a5496d6804 [fix] validate the metric history query inputs (#4274)
a5496d6804 is described below
commit a5496d6804311b0b499e5163bdd6f49d83fb73e7
Author: Duansg <[email protected]>
AuthorDate: Sun Aug 9 22:09:21 2026 +0800
[fix] validate the metric history query inputs (#4274)
---
.../service/impl/MetricsDataServiceImpl.java | 57 ++++++++
.../service/impl/MetricsDataServiceImplTest.java | 150 +++++++++++++++++++++
2 files changed, 207 insertions(+)
diff --git
a/hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImpl.java
b/hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImpl.java
index b1595c241d..df336e5e3a 100644
---
a/hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImpl.java
+++
b/hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImpl.java
@@ -22,6 +22,7 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.regex.Pattern;
import lombok.extern.slf4j.Slf4j;
import org.apache.hertzbeat.common.constants.CommonConstants;
import org.apache.hertzbeat.common.constants.MetricDataConstants;
@@ -45,6 +46,38 @@ import org.springframework.stereotype.Service;
@Service
public class MetricsDataServiceImpl implements MetricsDataService {
+ /**
+ * The history range is interpolated straight into the time predicate of
the generated
+ * query - {@code WHERE ts >= now - %s} for tdengine, the equivalent for
influxdb, iotdb
+ * and victoria metrics - with no quoting around it, which makes it the
widest opening
+ * of the four inputs: anything after the range escapes the predicate and
continues the
+ * statement.
+ *
+ * <p>A count followed by a single unit letter is the whole language the
ui speaks
+ * ({@code 1h}, {@code 6h}, {@code 1D}, {@code 1W}, {@code 4W}, {@code
12W}) and cannot
+ * carry a quote, a separator or a comment. Both cases are accepted
because the storages
+ * differ on it: questdb lowercases the unit while `TimePeriodUtil` reads
an uppercase
+ * unit as a calendar period. Which units a given storage actually
supports is left to
+ * that storage, so this rejects without changing what already worked.
+ */
+ private static final Pattern HISTORY_RANGE =
Pattern.compile("\\d{1,6}[smhdwy]", Pattern.CASE_INSENSITIVE);
+
+ /**
+ * App, metrics group and metric names reach the storages as table and
column
+ * identifiers, quoted with backticks in tdengine and double quotes in
questdb, and as
+ * promql label values in victoria metrics. None of the characters allowed
here can
+ * close any of those, and every monitoring template shipped with
hertzbeat names its
+ * apps, metric groups and fields from this set.
+ */
+ private static final Pattern IDENTIFIER =
Pattern.compile("[A-Za-z0-9_-]{1,200}");
+
+ /**
+ * The instance is commonly an address, so it additionally allows the
punctuation an
+ * address carries. The storages that build a table name from it already
fold {@code .},
+ * {@code :}, {@code [} and {@code ]} into underscores.
+ */
+ private static final Pattern INSTANCE =
Pattern.compile("[A-Za-z0-9_\\-.:\\[\\]]{1,200}");
+
private final RealTimeDataReader realTimeDataReader;
private final Optional<HistoryDataReader> historyDataReader;
@@ -111,6 +144,11 @@ public class MetricsDataServiceImpl implements
MetricsDataService {
if (history == null) {
history = "6h";
}
+ validateHistoryRange(history);
+ validateIdentifier(app, "app");
+ validateIdentifier(metrics, "metrics");
+ validateIdentifier(metric, "metric");
+ validateInstance(instance);
Map<String, List<Value>> instanceValuesMap;
if (interval == null || !interval) {
instanceValuesMap =
historyDataReader.get().getHistoryMetricData(instance, app, metrics, metric,
history);
@@ -126,4 +164,23 @@ public class MetricsDataServiceImpl implements
MetricsDataService {
.field(Field.builder().name(metric).type(CommonConstants.TYPE_NUMBER).build())
.build();
}
+
+ private static void validateHistoryRange(String history) {
+ if (!HISTORY_RANGE.matcher(history).matches()) {
+ throw new IllegalArgumentException("history range: " + history
+ + " is illegal, expected a count followed by a unit such
as 6h or 1W.");
+ }
+ }
+
+ private static void validateIdentifier(String value, String name) {
+ if (value == null || !IDENTIFIER.matcher(value).matches()) {
+ throw new IllegalArgumentException(name + ": " + value + " is
illegal.");
+ }
+ }
+
+ private static void validateInstance(String instance) {
+ if (instance == null || !INSTANCE.matcher(instance).matches()) {
+ throw new IllegalArgumentException("instance: " + instance + " is
illegal.");
+ }
+ }
}
diff --git
a/hertzbeat-warehouse/src/test/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImplTest.java
b/hertzbeat-warehouse/src/test/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImplTest.java
new file mode 100644
index 0000000000..a5aea4718a
--- /dev/null
+++
b/hertzbeat-warehouse/src/test/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImplTest.java
@@ -0,0 +1,150 @@
+/*
+ * 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.warehouse.service.impl;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.hertzbeat.common.entity.dto.MetricsHistoryData;
+import org.apache.hertzbeat.common.entity.dto.Value;
+import org.apache.hertzbeat.warehouse.store.history.tsdb.HistoryDataReader;
+import org.apache.hertzbeat.warehouse.store.realtime.RealTimeDataReader;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+/**
+ * Test case for {@link MetricsDataServiceImpl}.
+ *
+ * <p>The history query inputs arrive from rest path variables and a query
parameter and are
+ * interpolated into native queries by every time series storage: the range
lands unquoted
+ * in the time predicate, while the app, metrics group, metric and instance
land as table
+ * and column identifiers or as promql label values. The only storage that
parses the range
+ * instead of interpolating it is questdb, and the default duckdb storage uses
prepared
+ * statements, so validating here is what covers tdengine, influxdb, iotdb and
victoria
+ * metrics at once.
+ */
+@ExtendWith(MockitoExtension.class)
+class MetricsDataServiceImplTest {
+
+ @Mock
+ private RealTimeDataReader realTimeDataReader;
+
+ @Mock
+ private HistoryDataReader historyDataReader;
+
+ private MetricsDataServiceImpl metricsDataService;
+
+ @BeforeEach
+ void setUp() {
+ metricsDataService = new MetricsDataServiceImpl(realTimeDataReader,
Optional.of(historyDataReader));
+ }
+
+ @Test
+ void testHistoryRangeBreakingOutOfTheTimePredicateIsRejected() {
+ // reproduces the reported payload: the range closes `ts >= now - ?`
and continues the statement
+ assertRejected("1s union all select ts, metric_labels, `usage` from
`other_table` where ts>=now-1w");
+ assertRejected("1h; drop table cpu");
+ assertRejected("1h' or '1'='1");
+ assertRejected("1h)--");
+ }
+
+ @Test
+ void testMalformedHistoryRangeIsRejected() {
+ assertRejected("");
+ assertRejected("6");
+ assertRejected("hh");
+ assertRejected("-1h");
+ assertRejected("1 h");
+ }
+
+ @Test
+ void testRangesTheUiSendsAreAccepted() {
+ // the periods behind the chart buttons, plus the default applied when
none is given
+ for (String range : List.of("1h", "6h", "1D", "1W", "4W", "12W")) {
+ when(historyDataReader.getHistoryMetricData("127.0.0.1", "linux",
"cpu", "usage", range))
+ .thenReturn(Map.of("", List.of(new Value("1", 1L))));
+
+ MetricsHistoryData data = metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux", "cpu", "usage", range, false);
+
+ assertEquals("cpu", data.getMetrics());
+ }
+ }
+
+ @Test
+ void testMissingRangeFallsBackToTheDefault() {
+ when(historyDataReader.getHistoryMetricData("127.0.0.1", "linux",
"cpu", "usage", "6h"))
+ .thenReturn(Map.of("", List.of(new Value("1", 1L))));
+
+ metricsDataService.getMetricHistoryData("127.0.0.1", "linux", "cpu",
"usage", null, false);
+
+ verify(historyDataReader).getHistoryMetricData("127.0.0.1", "linux",
"cpu", "usage", "6h");
+ }
+
+ @Test
+ void testIdentifierEscapingTheQuotingIsRejected() {
+ // a backtick closes a tdengine identifier, a double quote closes a
questdb one
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux`,(select 1) `x", "cpu", "usage", "6h",
false));
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux", "cpu\" or \"1\"=\"1", "usage", "6h",
false));
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux", "cpu", "usage`", "6h", false));
+ // the instance lands inside a promql label selector in victoria
metrics
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1\",__name__=~\".*", "linux", "cpu", "usage", "6h",
false));
+
+ verify(historyDataReader, never()).getHistoryMetricData(anyString(),
anyString(), anyString(),
+ anyString(), anyString());
+ }
+
+ @Test
+ void testNamesUsedByTheShippedTemplatesAreAccepted() {
+ // dashes appear in template field names, an instance is usually an
address
+ when(historyDataReader.getHistoryMetricData("[::1]:8080", "hugegraph",
"cache", "edge-hugegraph-hits", "6h"))
+ .thenReturn(Map.of("", List.of(new Value("1", 1L))));
+
+ MetricsHistoryData data = metricsDataService.getMetricHistoryData(
+ "[::1]:8080", "hugegraph", "cache", "edge-hugegraph-hits",
"6h", false);
+
+ assertEquals("cache", data.getMetrics());
+ }
+
+ @Test
+ void testIntervalQueriesAreValidatedTheSameWay() {
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux", "cpu", "usage", "1h; drop table cpu",
true));
+
+ verify(historyDataReader,
never()).getHistoryIntervalMetricData(anyString(), anyString(), anyString(),
+ anyString(), anyString());
+ }
+
+ private void assertRejected(String history) {
+ assertThrows(IllegalArgumentException.class, () ->
metricsDataService.getMetricHistoryData(
+ "127.0.0.1", "linux", "cpu", "usage", history, false),
"expected rejection of: " + history);
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
