Copilot commented on code in PR #3900:
URL: https://github.com/apache/hertzbeat/pull/3900#discussion_r2607176478
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.ts:
##########
@@ -346,7 +346,14 @@ export class AlertSettingComponent implements OnInit {
}
private updateAlertDefineType() {
- // Combine main type with data type
+ // First: Reset form state when switching data source type
+ this.userExpr = '';
+ this.cascadeValues = [];
+ this.currentMetrics = [];
+ this.resetQbDataDefault();
+ this.clearPreview();
+
+ // Second: Init state when switching data source type
Review Comment:
There are two spaces between "Init" and "state" in the comment.
```suggestion
// Second: Init state when switching data source type
```
##########
web-app/src/app/routes/alert/alert-setting/alert-setting.component.html:
##########
@@ -733,19 +733,8 @@
<nz-form-item *ngIf="define.type == 'periodic_log'">
<nz-form-label [nzSpan]="7" [nzNoColon]="true"
nzFor="periodic_expr"></nz-form-label>
<nz-form-control [nzSpan]="12" [nzErrorTip]="'validation.required' |
i18n">
- <nz-textarea-count [nzMaxCharacterCount]="100">
- <textarea
- [(ngModel)]="define.expr"
- required
- rows="3"
- nz-input
- name="periodic_expr"
- id="periodic_expr"
- [placeholder]="'alert.setting.log.query.placeholder' | i18n"
- >
- </textarea>
- </nz-textarea-count>
- <button nz-button nzType="primary" style="width: 100%;
margin-bottom: 10px" (click)="onPreviewLogExpr()">
+ <app-sql-editor [(ngModel)]="define.expr" name="periodic_expr"
[height]="'150px'" tableName="hertzbeat_logs"></app-sql-editor>
Review Comment:
The `app-sql-editor` component is used without the `required` attribute in
the form control. Since the textarea it replaced had `required`, the SQL editor
should also validate that a value is present. The FormControl should have
validators or the component should handle required validation to maintain form
validation consistency.
```suggestion
<app-sql-editor [(ngModel)]="define.expr" name="periodic_expr"
[height]="'150px'" tableName="hertzbeat_logs" required></app-sql-editor>
```
##########
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);
+
+ 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));
}
- results.add(rowMap);
}
+ results.add(rowMap);
}
}
}
- } else {
- log.error("query metrics data from greptime failed. {}",
responseEntity);
}
- } catch (Exception e) {
- log.error("query metrics data from greptime error. {}",
e.getMessage(), e);
+ } else {
+ log.error("query metrics data from greptime failed. {}",
responseEntity);
}
return results;
Review Comment:
The error handling change removes the try-catch block, which means
exceptions from `restTemplate.exchange()` will now propagate uncaught. This is
a breaking change that could affect error handling at higher levels. The test
has been updated to expect RuntimeException, but this change removes error
resilience that was previously in place.
##########
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}`, 'i');
Review Comment:
The regex pattern `FROM\\s+${this.tableName}` is vulnerable to ReDoS
(Regular Expression Denial of Service) if `this.tableName` contains special
regex characters and should be escaped. Use
`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}` to safely
escape special characters. Additionally, this client-side validation is
insufficient for security - it only provides UX hints but the actual security
validation is correctly performed server-side in SqlSecurityValidator.
```suggestion
const tablePattern = new
RegExp(`FROM\\s+${this.tableName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`, 'i');
```
##########
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}`, '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 single quote validation `(sql.match(/'/g) || []).length` doesn't account
for escaped quotes or quotes within strings. This could lead to false
positives. For example, `'it''s'` in SQL (escaped single quote) would be
incorrectly flagged as unclosed. However, since this is client-side validation
for UX only and proper validation happens server-side, this is acceptable but
could be improved for better user experience.
##########
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);
Review Comment:
SQL validation only happens in the `query()` method but not in the
`calculate()` method. If SQL expressions are used via the `calculate()` path,
they would bypass SQL security validation. Consider adding the same validation
check in the `calculate()` method to ensure consistent security enforcement
across all code paths.
##########
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);
Review Comment:
The whitespace normalization `expr.replaceAll("\\s+", " ")` is performed
after SQL security validation is checked. For better security, the SQL should
be validated in its normalized form to ensure the validation sees the same SQL
that will be executed. Consider moving the normalization before the SQL
validation call on line 131.
```suggestion
// SQL security validation for SQL-based datasources
if (isSqlDatasource(datasource)) {
// replace all white space before validation
expr = expr.replaceAll("\\s+", " ");
validateSqlSecurity(expr);
} else {
// replace all white space for non-SQL datasources as well
expr = expr.replaceAll("\\s+", " ");
```
##########
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);
Review Comment:
The method name `isSqlDatasource` should follow Java naming conventions and
be `isSqlDataSource` (capital 'S' in 'Source') to match the compound word
convention.
##########
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);
+ }
Review Comment:
The SQL security validation added to the `query()` method lacks test
coverage. While `SqlSecurityValidator` has its own comprehensive tests, the
integration with `DataSourceServiceImpl` should also be tested to ensure SQL
validation is properly invoked and exceptions are correctly handled. Consider
adding tests in `DataSourceServiceTest` that verify SQL injection attempts are
blocked.
--
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]
