From be24ef3059721842ff17c93735bb1ccab93c1d0d Mon Sep 17 00:00:00 2001 From: Duansg Date: Wed, 29 Jul 2026 03:33:58 -0700 Subject: [PATCH] [fix] validate the metric history query inputs --- .../service/impl/MetricsDataServiceImpl.java | 57 +++++++ .../impl/MetricsDataServiceImplTest.java | 150 ++++++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 hertzbeat-warehouse/src/test/java/org/apache/hertzbeat/warehouse/service/impl/MetricsDataServiceImplTest.java 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 b1595c241d4..df336e5e3ae 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.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 @@ @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. + * + *

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; @@ -111,6 +144,11 @@ public MetricsHistoryData getMetricHistoryData(String instance, String app, Stri if (history == null) { history = "6h"; } + validateHistoryRange(history); + validateIdentifier(app, "app"); + validateIdentifier(metrics, "metrics"); + validateIdentifier(metric, "metric"); + validateInstance(instance); Map> instanceValuesMap; if (interval == null || !interval) { instanceValuesMap = historyDataReader.get().getHistoryMetricData(instance, app, metrics, metric, history); @@ -126,4 +164,23 @@ public MetricsHistoryData getMetricHistoryData(String instance, String app, Stri .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 00000000000..a5aea4718a7 --- /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}. + * + *

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); + } +}