Copilot commented on code in PR #3900:
URL: https://github.com/apache/hertzbeat/pull/3900#discussion_r2608920295
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/constants/WarehouseConstants.java:
##########
@@ -68,4 +68,6 @@ interface RealTimeName {
String INSTANT = "instant";
+ String LOG_TABLE_NAME = "hertzbeat_logs";
Review Comment:
The constant `LOG_TABLE_NAME` is placed at the root level of the
`WarehouseConstants` interface rather than within a nested interface like
`HistoryName` or `RealTimeName`. For consistency with the existing code
structure, consider placing it within an appropriate nested interface or
creating a new interface for table names.
```suggestion
/**
* Table name constants.
*/
interface TableName {
String LOG_TABLE_NAME = "hertzbeat_logs";
}
```
##########
hertzbeat-warehouse/src/main/java/org/apache/hertzbeat/warehouse/db/GreptimeSqlQueryExecutor.java:
##########
@@ -63,61 +63,58 @@ public GreptimeSqlQueryExecutor(GreptimeProperties
greptimeProperties, RestTempl
@Override
public List<Map<String, Object>> execute(String queryString) {
List<Map<String, Object>> results = new LinkedList<>();
- try {
- HttpHeaders headers = new HttpHeaders();
- headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
- headers.setAccept(List.of(MediaType.APPLICATION_JSON));
- if (StringUtils.hasText(greptimeProperties.username())
- && StringUtils.hasText(greptimeProperties.password())) {
- String authStr = greptimeProperties.username() + ":" +
greptimeProperties.password();
- String encodedAuth = Base64Util.encode(authStr);
- headers.add(HttpHeaders.AUTHORIZATION, NetworkConstants.BASIC
+ SignConstants.BLANK + encodedAuth);
- }
- String requestBody = "sql=" + queryString;
- HttpEntity<String> httpEntity = new HttpEntity<>(requestBody,
headers);
+ HttpHeaders headers = new HttpHeaders();
+ headers.setContentType(MediaType.APPLICATION_FORM_URLENCODED);
+ headers.setAccept(List.of(MediaType.APPLICATION_JSON));
+ if (StringUtils.hasText(greptimeProperties.username())
+ && StringUtils.hasText(greptimeProperties.password())) {
+ String authStr = greptimeProperties.username() + ":" +
greptimeProperties.password();
+ String encodedAuth = Base64Util.encode(authStr);
+ headers.add(HttpHeaders.AUTHORIZATION, NetworkConstants.BASIC +
SignConstants.BLANK + encodedAuth);
+ }
- String url = greptimeProperties.httpEndpoint() + QUERY_PATH;
- if (StringUtils.hasText(greptimeProperties.database())) {
- url += "?db=" + greptimeProperties.database();
- }
+ String requestBody = "sql=" + queryString;
+ HttpEntity<String> httpEntity = new HttpEntity<>(requestBody, headers);
- ResponseEntity<GreptimeSqlQueryContent> responseEntity =
restTemplate.exchange(url,
- HttpMethod.POST, httpEntity,
GreptimeSqlQueryContent.class);
-
- if (responseEntity.getStatusCode().is2xxSuccessful()) {
- GreptimeSqlQueryContent responseBody =
responseEntity.getBody();
- if (responseBody != null && responseBody.getCode() == 0
- && responseBody.getOutput() != null &&
!responseBody.getOutput().isEmpty()) {
-
- for (GreptimeSqlQueryContent.Output output :
responseBody.getOutput()) {
- if (output.getRecords() != null &&
output.getRecords().getRows() != null) {
- GreptimeSqlQueryContent.Output.Records.Schema
schema = output.getRecords().getSchema();
- List<List<Object>> rows =
output.getRecords().getRows();
-
- for (List<Object> row : rows) {
- Map<String, Object> rowMap = new HashMap<>();
- if (schema != null &&
schema.getColumnSchemas() != null) {
- for (int i = 0; i <
Math.min(schema.getColumnSchemas().size(), row.size()); i++) {
- String columnName =
schema.getColumnSchemas().get(i).getName();
- Object value = row.get(i);
- rowMap.put(columnName, value);
- }
- } else {
- for (int i = 0; i < row.size(); i++) {
- rowMap.put("col_" + i, row.get(i));
- }
+ String url = greptimeProperties.httpEndpoint() + QUERY_PATH;
+ if (StringUtils.hasText(greptimeProperties.database())) {
+ url += "?db=" + greptimeProperties.database();
+ }
+
+ ResponseEntity<GreptimeSqlQueryContent> responseEntity =
restTemplate.exchange(url,
+ HttpMethod.POST, httpEntity, GreptimeSqlQueryContent.class);
Review Comment:
The error handling was changed to remove the try-catch block that previously
caught exceptions and returned an empty list. Now exceptions are propagated
directly. However, the change in error behavior is inconsistent with the test
update.
The test now expects a RuntimeException to be thrown, but the actual code
will throw the original exception from restTemplate.exchange (which could be
various types). Consider either:
1. Wrapping all exceptions in a specific exception type for consistent error
handling
2. Updating the test to expect the actual exception types that can be thrown
```suggestion
ResponseEntity<GreptimeSqlQueryContent> responseEntity;
try {
responseEntity = restTemplate.exchange(url,
HttpMethod.POST, httpEntity,
GreptimeSqlQueryContent.class);
} catch (Exception e) {
log.error("Exception occurred while querying GreptimeDB SQL:
{}", e.getMessage(), e);
throw new RuntimeException("Failed to execute GreptimeDB SQL
query", e);
}
```
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.ts:
##########
@@ -1027,6 +1029,7 @@ export class AlertSettingComponent implements OnInit {
onManageModalCancel() {
this.cascadeValues = [];
this.isExpr = false;
+ this.dataType = 'metric';
Review Comment:
Adding the reset of `dataType` to 'metric' in the cancel handler creates
inconsistency. When editing an existing alert definition (where
`isManageModalAdd` is false), canceling the modal will reset the dataType to
'metric' even if the alert was originally for log data. This could cause issues
if the modal is reopened without reinitializing the state properly.
Consider only resetting `dataType` when `isManageModalAdd` is true, or
ensure it's properly reinitialized from the alert definition when the modal
reopens.
```suggestion
if (this.isManageModalAdd) {
this.dataType = 'metric';
}
```
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java:
##########
@@ -49,12 +53,18 @@
@Slf4j
public class DataSourceServiceImpl implements DataSourceService {
+ /**
+ * Default allowed tables for SQL queries
+ */
+ private static final List<String> DEFAULT_ALLOWED_TABLES =
List.of(WarehouseConstants.LOG_TABLE_NAME);
+
protected ResourceBundle bundle = ResourceBundleUtil.getBundle("alerter");
@Setter
- @Autowired(required = false)
private List<QueryExecutor> executors;
Review Comment:
The `@Setter` annotation on the `executors` field has been removed but the
field is still being modified in the constructor. This change is correct, but
the test file update shows that the constructor now requires passing `null`
explicitly for the `executors` parameter. Consider whether this field should be
final since it's now only set in the constructor.
##########
web-app/src/app/shared/components/sql-editor/sql-editor.component.ts:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.
+ */
+
+import { Component, EventEmitter, forwardRef, Input, OnDestroy, Output } from
'@angular/core';
+import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
+
+declare const monaco: any;
+
+export interface SqlValidationError {
+ message: string;
+ startLine: number;
+ startColumn: number;
+ endLine: number;
+ endColumn: number;
+}
+
+export interface LogTableColumn {
+ name: string;
+ type: string;
+ description?: string;
+}
+
+@Component({
+ selector: 'app-sql-editor',
+ templateUrl: './sql-editor.component.html',
+ styleUrls: ['./sql-editor.component.less'],
+ providers: [
+ {
+ provide: NG_VALUE_ACCESSOR,
+ useExisting: forwardRef(() => SqlEditorComponent),
+ multi: true
+ }
+ ]
+})
+export class SqlEditorComponent implements OnDestroy, ControlValueAccessor {
+ @Input() height: string = '120px';
+ @Input() tableName: string = 'hertzbeat_logs';
+
+ @Output() readonly editorInit = new EventEmitter<any>();
+ @Output() readonly validationChange = new
EventEmitter<SqlValidationError[]>();
+
+ code: string = '';
+ private editorInstance: any;
+ private validationTimeout: any;
+ editorOption: any = {
+ language: 'sql',
+ theme: 'vs',
+ minimap: { enabled: false },
+ lineNumbers: 'on',
+ scrollBeyondLastLine: false,
+ automaticLayout: true,
+ folding: false,
+ wordWrap: 'on',
+ fontSize: 13,
+ tabSize: 2,
+ suggestOnTriggerCharacters: true,
+ quickSuggestions: true,
+ wordBasedSuggestions: false,
+ fixedOverflowWidgets: true,
+ overviewRulerLanes: 0
+ };
+
+ private completionProvider: any;
+ private onChange: (value: string) => void = () => {};
+ private onTouched: () => void = () => {};
+
+ private readonly LOG_TABLE_COLUMNS: LogTableColumn[] = [
+ { name: 'time_unix_nano', type: 'TimestampNanosecond', description: 'Log
timestamp in nanoseconds' },
+ { name: 'observed_time_unix_nano', type: 'TimestampNanosecond',
description: 'Observed timestamp in nanoseconds' },
+ { name: 'severity_number', type: 'Int32', description: 'Severity level
number (1-24)' },
+ { name: 'severity_text', type: 'String', description: 'Severity text
(DEBUG, INFO, WARN, ERROR, etc.)' },
+ { name: 'body', type: 'Json', description: 'Log message body content' },
+ { name: 'trace_id', type: 'String', description: 'Distributed tracing
trace ID' },
+ { name: 'span_id', type: 'String', description: 'Distributed tracing span
ID' },
+ { name: 'trace_flags', type: 'Int32', description: 'Trace flags' },
+ { name: 'attributes', type: 'Json', description: 'Log attributes as JSON'
},
+ { name: 'resource', type: 'Json', description: 'Resource information as
JSON' },
+ { name: 'instrumentation_scope', type: 'Json', description:
'Instrumentation scope information' },
+ { name: 'dropped_attributes_count', type: 'Int32', description: 'Number of
dropped attributes' }
+ ];
+
+ ngOnDestroy(): void {
+ if (this.completionProvider) {
+ this.completionProvider.dispose();
+ }
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ }
+
+ onEditorInit(editor: any): void {
+ this.editorInstance = editor;
+ this.registerCompletionProvider();
+ this.configureSuggestWidget(editor);
+ this.editorInit.emit(editor);
+ }
+
+ private configureSuggestWidget(editor: any): void {
+ if (typeof monaco === 'undefined') {
+ return;
+ }
+ try {
+ editor.updateOptions({
+ suggest: {
+ maxVisibleSuggestions: 8,
+ showStatusBar: false,
+ preview: false
+ }
+ });
+ } catch (e) {
+ console.warn('Failed to configure suggest widget:', e);
+ }
+ }
+
+ writeValue(value: string): void {
+ this.code = value || '';
+ }
+
+ registerOnChange(fn: (value: string) => void): void {
+ this.onChange = fn;
+ }
+
+ registerOnTouched(fn: () => void): void {
+ this.onTouched = fn;
+ }
+
+ onCodeChange(value: string): void {
+ this.code = value;
+ this.onChange(value);
+ this.onTouched();
+ this.validateSqlWithDebounce(value);
+ }
+
+ private validateSqlWithDebounce(sql: string): void {
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ this.validationTimeout = setTimeout(() => {
+ const errors = this.validateSql(sql);
+ this.setEditorMarkers(errors);
+ this.validationChange.emit(errors);
+ }, 500);
+ }
+
+ private validateSql(sql: string): SqlValidationError[] {
+ const errors: SqlValidationError[] = [];
+ if (!sql || !sql.trim()) {
+ return errors;
+ }
+
+ const upperSql = sql.toUpperCase().trim();
+ const lines = sql.split('\n');
+
+ if (!upperSql.startsWith('SELECT')) {
+ errors.push({
+ message: 'SQL query must start with SELECT',
+ startLine: 1,
+ startColumn: 1,
+ endLine: 1,
+ endColumn: Math.min(sql.indexOf(' ') + 1 || sql.length,
lines[0].length + 1)
+ });
+ }
+
+ if (!upperSql.includes('FROM')) {
+ const lastLine = lines.length;
+ const lastLineLength = lines[lastLine - 1].length;
+ errors.push({
+ message: 'SQL query must contain FROM clause',
+ startLine: lastLine,
+ startColumn: 1,
+ endLine: lastLine,
+ endColumn: lastLineLength + 1
+ });
+ }
+
+ if (upperSql.includes('FROM')) {
+ const tablePattern = new
RegExp(`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`, 'i');
+ if (!tablePattern.test(sql)) {
+ const fromIndex = upperSql.indexOf('FROM');
+ let lineNum = 1;
+ let colNum = 1;
+ let charCount = 0;
+ for (let i = 0; i < lines.length; i++) {
+ if (charCount + lines[i].length >= fromIndex) {
+ lineNum = i + 1;
+ colNum = fromIndex - charCount + 1;
+ break;
+ }
+ charCount += lines[i].length + 1;
+ }
+ errors.push({
+ message: `Table must be '${this.tableName}'`,
+ startLine: lineNum,
+ startColumn: colNum,
+ endLine: lineNum,
+ endColumn: colNum + 20
+ });
+ }
+ }
+
+ const openParens = (sql.match(/\(/g) || []).length;
+ const closeParens = (sql.match(/\)/g) || []).length;
+ if (openParens !== closeParens) {
+ errors.push({
+ message: 'Mismatched parentheses',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
+
+ const quotes = (sql.match(/'/g) || []).length;
+ if (quotes % 2 !== 0) {
+ errors.push({
+ message: 'Unclosed string literal (missing quote)',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
Review Comment:
The validation logic doesn't account for quotes within SQL comments (e.g.,
"-- this is a comment with a quote '"). This will cause false positive errors
for valid SQL that contains single quotes in comments. Consider stripping SQL
comments before performing the quote count validation.
##########
web-app/src/app/shared/components/sql-editor/sql-editor.component.ts:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.
+ */
+
+import { Component, EventEmitter, forwardRef, Input, OnDestroy, Output } from
'@angular/core';
+import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
+
+declare const monaco: any;
+
+export interface SqlValidationError {
+ message: string;
+ startLine: number;
+ startColumn: number;
+ endLine: number;
+ endColumn: number;
+}
+
+export interface LogTableColumn {
+ name: string;
+ type: string;
+ description?: string;
+}
+
+@Component({
+ selector: 'app-sql-editor',
+ templateUrl: './sql-editor.component.html',
+ styleUrls: ['./sql-editor.component.less'],
+ providers: [
+ {
+ provide: NG_VALUE_ACCESSOR,
+ useExisting: forwardRef(() => SqlEditorComponent),
+ multi: true
+ }
+ ]
+})
+export class SqlEditorComponent implements OnDestroy, ControlValueAccessor {
+ @Input() height: string = '120px';
+ @Input() tableName: string = 'hertzbeat_logs';
+
+ @Output() readonly editorInit = new EventEmitter<any>();
+ @Output() readonly validationChange = new
EventEmitter<SqlValidationError[]>();
+
+ code: string = '';
+ private editorInstance: any;
+ private validationTimeout: any;
+ editorOption: any = {
+ language: 'sql',
+ theme: 'vs',
+ minimap: { enabled: false },
+ lineNumbers: 'on',
+ scrollBeyondLastLine: false,
+ automaticLayout: true,
+ folding: false,
+ wordWrap: 'on',
+ fontSize: 13,
+ tabSize: 2,
+ suggestOnTriggerCharacters: true,
+ quickSuggestions: true,
+ wordBasedSuggestions: false,
+ fixedOverflowWidgets: true,
+ overviewRulerLanes: 0
+ };
+
+ private completionProvider: any;
+ private onChange: (value: string) => void = () => {};
+ private onTouched: () => void = () => {};
+
+ private readonly LOG_TABLE_COLUMNS: LogTableColumn[] = [
+ { name: 'time_unix_nano', type: 'TimestampNanosecond', description: 'Log
timestamp in nanoseconds' },
+ { name: 'observed_time_unix_nano', type: 'TimestampNanosecond',
description: 'Observed timestamp in nanoseconds' },
+ { name: 'severity_number', type: 'Int32', description: 'Severity level
number (1-24)' },
+ { name: 'severity_text', type: 'String', description: 'Severity text
(DEBUG, INFO, WARN, ERROR, etc.)' },
+ { name: 'body', type: 'Json', description: 'Log message body content' },
+ { name: 'trace_id', type: 'String', description: 'Distributed tracing
trace ID' },
+ { name: 'span_id', type: 'String', description: 'Distributed tracing span
ID' },
+ { name: 'trace_flags', type: 'Int32', description: 'Trace flags' },
+ { name: 'attributes', type: 'Json', description: 'Log attributes as JSON'
},
+ { name: 'resource', type: 'Json', description: 'Resource information as
JSON' },
+ { name: 'instrumentation_scope', type: 'Json', description:
'Instrumentation scope information' },
+ { name: 'dropped_attributes_count', type: 'Int32', description: 'Number of
dropped attributes' }
+ ];
+
+ ngOnDestroy(): void {
+ if (this.completionProvider) {
+ this.completionProvider.dispose();
+ }
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ }
+
+ onEditorInit(editor: any): void {
+ this.editorInstance = editor;
+ this.registerCompletionProvider();
+ this.configureSuggestWidget(editor);
+ this.editorInit.emit(editor);
+ }
+
+ private configureSuggestWidget(editor: any): void {
+ if (typeof monaco === 'undefined') {
+ return;
+ }
+ try {
+ editor.updateOptions({
+ suggest: {
+ maxVisibleSuggestions: 8,
+ showStatusBar: false,
+ preview: false
+ }
+ });
+ } catch (e) {
+ console.warn('Failed to configure suggest widget:', e);
+ }
+ }
+
+ writeValue(value: string): void {
+ this.code = value || '';
+ }
+
+ registerOnChange(fn: (value: string) => void): void {
+ this.onChange = fn;
+ }
+
+ registerOnTouched(fn: () => void): void {
+ this.onTouched = fn;
+ }
+
+ onCodeChange(value: string): void {
+ this.code = value;
+ this.onChange(value);
+ this.onTouched();
+ this.validateSqlWithDebounce(value);
+ }
+
+ private validateSqlWithDebounce(sql: string): void {
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ this.validationTimeout = setTimeout(() => {
+ const errors = this.validateSql(sql);
+ this.setEditorMarkers(errors);
+ this.validationChange.emit(errors);
+ }, 500);
+ }
+
+ private validateSql(sql: string): SqlValidationError[] {
+ const errors: SqlValidationError[] = [];
+ if (!sql || !sql.trim()) {
+ return errors;
+ }
+
+ const upperSql = sql.toUpperCase().trim();
+ const lines = sql.split('\n');
+
+ if (!upperSql.startsWith('SELECT')) {
+ errors.push({
+ message: 'SQL query must start with SELECT',
+ startLine: 1,
+ startColumn: 1,
+ endLine: 1,
+ endColumn: Math.min(sql.indexOf(' ') + 1 || sql.length,
lines[0].length + 1)
+ });
+ }
+
+ if (!upperSql.includes('FROM')) {
+ const lastLine = lines.length;
+ const lastLineLength = lines[lastLine - 1].length;
+ errors.push({
+ message: 'SQL query must contain FROM clause',
+ startLine: lastLine,
+ startColumn: 1,
+ endLine: lastLine,
+ endColumn: lastLineLength + 1
+ });
+ }
+
+ if (upperSql.includes('FROM')) {
+ const tablePattern = new
RegExp(`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`, 'i');
+ if (!tablePattern.test(sql)) {
+ const fromIndex = upperSql.indexOf('FROM');
+ let lineNum = 1;
+ let colNum = 1;
+ let charCount = 0;
+ for (let i = 0; i < lines.length; i++) {
+ if (charCount + lines[i].length >= fromIndex) {
+ lineNum = i + 1;
+ colNum = fromIndex - charCount + 1;
+ break;
+ }
+ charCount += lines[i].length + 1;
+ }
+ errors.push({
+ message: `Table must be '${this.tableName}'`,
+ startLine: lineNum,
+ startColumn: colNum,
+ endLine: lineNum,
+ endColumn: colNum + 20
+ });
+ }
+ }
+
+ const openParens = (sql.match(/\(/g) || []).length;
+ const closeParens = (sql.match(/\)/g) || []).length;
+ if (openParens !== closeParens) {
+ errors.push({
+ message: 'Mismatched parentheses',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
+
+ const quotes = (sql.match(/'/g) || []).length;
+ if (quotes % 2 !== 0) {
+ errors.push({
+ message: 'Unclosed string literal (missing quote)',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
Review Comment:
The quote validation using regex counting is simplistic and doesn't account
for escaped quotes (e.g., '\'' or "''"). This could lead to false positives
when users write valid SQL with escaped quotes. Consider using a more
sophisticated parsing approach or accepting that this basic validation may have
limitations.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java:
##########
@@ -110,11 +125,36 @@ public List<Map<String, Object>> 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);
} catch (Exception e) {
log.error("Error executing query on datasource {}: {}",
datasource, e.getMessage());
- throw new RuntimeException("Query execution failed", e);
+ throw new AlertExpressionException(e.getMessage());
+ }
Review Comment:
The exception handling changes the error type from RuntimeException to
AlertExpressionException, which is more specific and appropriate. However, this
is a breaking change in the error contract. Ensure that callers of this method
are prepared to handle AlertExpressionException instead of RuntimeException, or
that AlertExpressionException extends RuntimeException.
##########
web-app/src/app/shared/components/sql-editor/sql-editor.component.ts:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.
+ */
+
+import { Component, EventEmitter, forwardRef, Input, OnDestroy, Output } from
'@angular/core';
+import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
+
+declare const monaco: any;
+
+export interface SqlValidationError {
+ message: string;
+ startLine: number;
+ startColumn: number;
+ endLine: number;
+ endColumn: number;
+}
+
+export interface LogTableColumn {
+ name: string;
+ type: string;
+ description?: string;
+}
+
+@Component({
+ selector: 'app-sql-editor',
+ templateUrl: './sql-editor.component.html',
+ styleUrls: ['./sql-editor.component.less'],
+ providers: [
+ {
+ provide: NG_VALUE_ACCESSOR,
+ useExisting: forwardRef(() => SqlEditorComponent),
+ multi: true
+ }
+ ]
+})
+export class SqlEditorComponent implements OnDestroy, ControlValueAccessor {
+ @Input() height: string = '120px';
+ @Input() tableName: string = 'hertzbeat_logs';
+
+ @Output() readonly editorInit = new EventEmitter<any>();
+ @Output() readonly validationChange = new
EventEmitter<SqlValidationError[]>();
+
+ code: string = '';
+ private editorInstance: any;
+ private validationTimeout: any;
+ editorOption: any = {
+ language: 'sql',
+ theme: 'vs',
+ minimap: { enabled: false },
+ lineNumbers: 'on',
+ scrollBeyondLastLine: false,
+ automaticLayout: true,
+ folding: false,
+ wordWrap: 'on',
+ fontSize: 13,
+ tabSize: 2,
+ suggestOnTriggerCharacters: true,
+ quickSuggestions: true,
+ wordBasedSuggestions: false,
+ fixedOverflowWidgets: true,
+ overviewRulerLanes: 0
+ };
+
+ private completionProvider: any;
+ private onChange: (value: string) => void = () => {};
+ private onTouched: () => void = () => {};
+
+ private readonly LOG_TABLE_COLUMNS: LogTableColumn[] = [
+ { name: 'time_unix_nano', type: 'TimestampNanosecond', description: 'Log
timestamp in nanoseconds' },
+ { name: 'observed_time_unix_nano', type: 'TimestampNanosecond',
description: 'Observed timestamp in nanoseconds' },
+ { name: 'severity_number', type: 'Int32', description: 'Severity level
number (1-24)' },
+ { name: 'severity_text', type: 'String', description: 'Severity text
(DEBUG, INFO, WARN, ERROR, etc.)' },
+ { name: 'body', type: 'Json', description: 'Log message body content' },
+ { name: 'trace_id', type: 'String', description: 'Distributed tracing
trace ID' },
+ { name: 'span_id', type: 'String', description: 'Distributed tracing span
ID' },
+ { name: 'trace_flags', type: 'Int32', description: 'Trace flags' },
+ { name: 'attributes', type: 'Json', description: 'Log attributes as JSON'
},
+ { name: 'resource', type: 'Json', description: 'Resource information as
JSON' },
+ { name: 'instrumentation_scope', type: 'Json', description:
'Instrumentation scope information' },
+ { name: 'dropped_attributes_count', type: 'Int32', description: 'Number of
dropped attributes' }
+ ];
+
+ ngOnDestroy(): void {
+ if (this.completionProvider) {
+ this.completionProvider.dispose();
+ }
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ }
+
+ onEditorInit(editor: any): void {
+ this.editorInstance = editor;
+ this.registerCompletionProvider();
+ this.configureSuggestWidget(editor);
+ this.editorInit.emit(editor);
+ }
+
+ private configureSuggestWidget(editor: any): void {
+ if (typeof monaco === 'undefined') {
+ return;
+ }
+ try {
+ editor.updateOptions({
+ suggest: {
+ maxVisibleSuggestions: 8,
+ showStatusBar: false,
+ preview: false
+ }
+ });
+ } catch (e) {
+ console.warn('Failed to configure suggest widget:', e);
+ }
+ }
+
+ writeValue(value: string): void {
+ this.code = value || '';
+ }
+
+ registerOnChange(fn: (value: string) => void): void {
+ this.onChange = fn;
+ }
+
+ registerOnTouched(fn: () => void): void {
+ this.onTouched = fn;
+ }
+
+ onCodeChange(value: string): void {
+ this.code = value;
+ this.onChange(value);
+ this.onTouched();
+ this.validateSqlWithDebounce(value);
+ }
+
+ private validateSqlWithDebounce(sql: string): void {
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ this.validationTimeout = setTimeout(() => {
+ const errors = this.validateSql(sql);
+ this.setEditorMarkers(errors);
+ this.validationChange.emit(errors);
+ }, 500);
+ }
+
+ private validateSql(sql: string): SqlValidationError[] {
+ const errors: SqlValidationError[] = [];
+ if (!sql || !sql.trim()) {
+ return errors;
+ }
+
+ const upperSql = sql.toUpperCase().trim();
+ const lines = sql.split('\n');
+
+ if (!upperSql.startsWith('SELECT')) {
+ errors.push({
+ message: 'SQL query must start with SELECT',
+ startLine: 1,
+ startColumn: 1,
+ endLine: 1,
+ endColumn: Math.min(sql.indexOf(' ') + 1 || sql.length,
lines[0].length + 1)
+ });
+ }
+
+ if (!upperSql.includes('FROM')) {
+ const lastLine = lines.length;
+ const lastLineLength = lines[lastLine - 1].length;
+ errors.push({
+ message: 'SQL query must contain FROM clause',
+ startLine: lastLine,
+ startColumn: 1,
+ endLine: lastLine,
+ endColumn: lastLineLength + 1
+ });
+ }
+
+ if (upperSql.includes('FROM')) {
+ const tablePattern = new
RegExp(`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`, 'i');
+ if (!tablePattern.test(sql)) {
+ const fromIndex = upperSql.indexOf('FROM');
+ let lineNum = 1;
+ let colNum = 1;
+ let charCount = 0;
+ for (let i = 0; i < lines.length; i++) {
+ if (charCount + lines[i].length >= fromIndex) {
+ lineNum = i + 1;
+ colNum = fromIndex - charCount + 1;
+ break;
+ }
+ charCount += lines[i].length + 1;
+ }
+ errors.push({
+ message: `Table must be '${this.tableName}'`,
+ startLine: lineNum,
+ startColumn: colNum,
+ endLine: lineNum,
+ endColumn: colNum + 20
+ });
+ }
+ }
+
+ const openParens = (sql.match(/\(/g) || []).length;
+ const closeParens = (sql.match(/\)/g) || []).length;
+ if (openParens !== closeParens) {
+ errors.push({
+ message: 'Mismatched parentheses',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
+
+ const quotes = (sql.match(/'/g) || []).length;
+ if (quotes % 2 !== 0) {
+ errors.push({
+ message: 'Unclosed string literal (missing quote)',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
+
+ return errors;
+ }
Review Comment:
The client-side SQL validation in the SQL editor component only performs
basic syntactic checks (SELECT, FROM, table name, parentheses, quotes). This is
insufficient for preventing SQL injection as it doesn't prevent:
1. SQL comments that could be used to bypass validation
2. String concatenation attacks
3. Complex nested expressions that could hide malicious SQL
While the backend has proper SQL security validation using JSqlParser, users
could bypass the frontend validation entirely by manipulating the request. The
frontend validation should primarily serve as a user experience enhancement,
not as a security measure. Consider adding a comment to clarify that this is
for UX only and security is enforced on the backend.
##########
web-app/src/app/shared/components/sql-editor/sql-editor.component.ts:
##########
@@ -0,0 +1,407 @@
+/*
+ * 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.
+ */
+
+import { Component, EventEmitter, forwardRef, Input, OnDestroy, Output } from
'@angular/core';
+import { ControlValueAccessor, NG_VALUE_ACCESSOR } from '@angular/forms';
+
+declare const monaco: any;
+
+export interface SqlValidationError {
+ message: string;
+ startLine: number;
+ startColumn: number;
+ endLine: number;
+ endColumn: number;
+}
+
+export interface LogTableColumn {
+ name: string;
+ type: string;
+ description?: string;
+}
+
+@Component({
+ selector: 'app-sql-editor',
+ templateUrl: './sql-editor.component.html',
+ styleUrls: ['./sql-editor.component.less'],
+ providers: [
+ {
+ provide: NG_VALUE_ACCESSOR,
+ useExisting: forwardRef(() => SqlEditorComponent),
+ multi: true
+ }
+ ]
+})
+export class SqlEditorComponent implements OnDestroy, ControlValueAccessor {
+ @Input() height: string = '120px';
+ @Input() tableName: string = 'hertzbeat_logs';
+
+ @Output() readonly editorInit = new EventEmitter<any>();
+ @Output() readonly validationChange = new
EventEmitter<SqlValidationError[]>();
+
+ code: string = '';
+ private editorInstance: any;
+ private validationTimeout: any;
+ editorOption: any = {
+ language: 'sql',
+ theme: 'vs',
+ minimap: { enabled: false },
+ lineNumbers: 'on',
+ scrollBeyondLastLine: false,
+ automaticLayout: true,
+ folding: false,
+ wordWrap: 'on',
+ fontSize: 13,
+ tabSize: 2,
+ suggestOnTriggerCharacters: true,
+ quickSuggestions: true,
+ wordBasedSuggestions: false,
+ fixedOverflowWidgets: true,
+ overviewRulerLanes: 0
+ };
+
+ private completionProvider: any;
+ private onChange: (value: string) => void = () => {};
+ private onTouched: () => void = () => {};
+
+ private readonly LOG_TABLE_COLUMNS: LogTableColumn[] = [
+ { name: 'time_unix_nano', type: 'TimestampNanosecond', description: 'Log
timestamp in nanoseconds' },
+ { name: 'observed_time_unix_nano', type: 'TimestampNanosecond',
description: 'Observed timestamp in nanoseconds' },
+ { name: 'severity_number', type: 'Int32', description: 'Severity level
number (1-24)' },
+ { name: 'severity_text', type: 'String', description: 'Severity text
(DEBUG, INFO, WARN, ERROR, etc.)' },
+ { name: 'body', type: 'Json', description: 'Log message body content' },
+ { name: 'trace_id', type: 'String', description: 'Distributed tracing
trace ID' },
+ { name: 'span_id', type: 'String', description: 'Distributed tracing span
ID' },
+ { name: 'trace_flags', type: 'Int32', description: 'Trace flags' },
+ { name: 'attributes', type: 'Json', description: 'Log attributes as JSON'
},
+ { name: 'resource', type: 'Json', description: 'Resource information as
JSON' },
+ { name: 'instrumentation_scope', type: 'Json', description:
'Instrumentation scope information' },
+ { name: 'dropped_attributes_count', type: 'Int32', description: 'Number of
dropped attributes' }
+ ];
+
+ ngOnDestroy(): void {
+ if (this.completionProvider) {
+ this.completionProvider.dispose();
+ }
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ }
+
+ onEditorInit(editor: any): void {
+ this.editorInstance = editor;
+ this.registerCompletionProvider();
+ this.configureSuggestWidget(editor);
+ this.editorInit.emit(editor);
+ }
+
+ private configureSuggestWidget(editor: any): void {
+ if (typeof monaco === 'undefined') {
+ return;
+ }
+ try {
+ editor.updateOptions({
+ suggest: {
+ maxVisibleSuggestions: 8,
+ showStatusBar: false,
+ preview: false
+ }
+ });
+ } catch (e) {
+ console.warn('Failed to configure suggest widget:', e);
+ }
+ }
+
+ writeValue(value: string): void {
+ this.code = value || '';
+ }
+
+ registerOnChange(fn: (value: string) => void): void {
+ this.onChange = fn;
+ }
+
+ registerOnTouched(fn: () => void): void {
+ this.onTouched = fn;
+ }
+
+ onCodeChange(value: string): void {
+ this.code = value;
+ this.onChange(value);
+ this.onTouched();
+ this.validateSqlWithDebounce(value);
+ }
+
+ private validateSqlWithDebounce(sql: string): void {
+ if (this.validationTimeout) {
+ clearTimeout(this.validationTimeout);
+ }
+ this.validationTimeout = setTimeout(() => {
+ const errors = this.validateSql(sql);
+ this.setEditorMarkers(errors);
+ this.validationChange.emit(errors);
+ }, 500);
+ }
+
+ private validateSql(sql: string): SqlValidationError[] {
+ const errors: SqlValidationError[] = [];
+ if (!sql || !sql.trim()) {
+ return errors;
+ }
+
+ const upperSql = sql.toUpperCase().trim();
+ const lines = sql.split('\n');
+
+ if (!upperSql.startsWith('SELECT')) {
+ errors.push({
+ message: 'SQL query must start with SELECT',
+ startLine: 1,
+ startColumn: 1,
+ endLine: 1,
+ endColumn: Math.min(sql.indexOf(' ') + 1 || sql.length,
lines[0].length + 1)
+ });
+ }
+
+ if (!upperSql.includes('FROM')) {
+ const lastLine = lines.length;
+ const lastLineLength = lines[lastLine - 1].length;
+ errors.push({
+ message: 'SQL query must contain FROM clause',
+ startLine: lastLine,
+ startColumn: 1,
+ endLine: lastLine,
+ endColumn: lastLineLength + 1
+ });
+ }
+
+ if (upperSql.includes('FROM')) {
+ const tablePattern = new
RegExp(`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`, 'i');
+ if (!tablePattern.test(sql)) {
+ const fromIndex = upperSql.indexOf('FROM');
+ let lineNum = 1;
+ let colNum = 1;
+ let charCount = 0;
+ for (let i = 0; i < lines.length; i++) {
+ if (charCount + lines[i].length >= fromIndex) {
+ lineNum = i + 1;
+ colNum = fromIndex - charCount + 1;
+ break;
+ }
+ charCount += lines[i].length + 1;
+ }
+ errors.push({
+ message: `Table must be '${this.tableName}'`,
+ startLine: lineNum,
+ startColumn: colNum,
+ endLine: lineNum,
+ endColumn: colNum + 20
+ });
+ }
+ }
+
+ const openParens = (sql.match(/\(/g) || []).length;
+ const closeParens = (sql.match(/\)/g) || []).length;
+ if (openParens !== closeParens) {
+ errors.push({
+ message: 'Mismatched parentheses',
+ startLine: 1,
+ startColumn: 1,
+ endLine: lines.length,
+ endColumn: lines[lines.length - 1].length + 1
+ });
+ }
Review Comment:
The parentheses validation doesn't account for parentheses within string
literals (e.g., "WHERE message LIKE '(test)'"). This will cause false positive
errors for valid SQL. Consider parsing strings first and excluding their
content from parentheses counting, or accept this as a known limitation of the
basic validation.
##########
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/service/impl/DataSourceServiceImpl.java:
##########
@@ -110,11 +125,36 @@ public List<Map<String, Object>> 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);
} catch (Exception e) {
log.error("Error executing query on datasource {}: {}",
datasource, e.getMessage());
- throw new RuntimeException("Query execution failed", e);
+ throw new AlertExpressionException(e.getMessage());
+ }
+ }
+
+ /**
+ * Check if the datasource is SQL-based
+ */
+ private boolean isSqlDatasource(String datasource) {
+ return datasource != null &&
datasource.equalsIgnoreCase(WarehouseConstants.SQL);
+ }
+
+ /**
+ * Validate SQL statement for security
+ */
+ 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());
}
}
Review Comment:
The SQL security validation functionality added to `DataSourceServiceImpl`
lacks test coverage. There are no tests verifying that:
1. SQL injection attempts are properly blocked
2. The `validateSqlSecurity` method is called for SQL datasources
3. `SqlSecurityException` is properly converted to `AlertExpressionException`
4. Valid SQL queries are allowed through
Add test cases for the `query` method with SQL datasource to ensure the
security validation is working correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
