diff --git a/public/app/features/alerting/unified/components/RuleConditionSection.tsx b/public/app/features/alerting/unified/components/RuleConditionSection.tsx index 91f269befa6..854af8c6a6a 100644 --- a/public/app/features/alerting/unified/components/RuleConditionSection.tsx +++ b/public/app/features/alerting/unified/components/RuleConditionSection.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; import { useFormContext } from 'react-hook-form'; import { GrafanaTheme2, ReducerID, SelectableValue, getNextRefId } from '@grafana/data'; @@ -30,7 +30,11 @@ const ExpressionDatasourceUID = '__expr__'; type LocalSimpleCondition = { whenField?: string; evaluator: { params: number[]; type: EvalFunction } }; -// Helper function to create expression queries from simple condition +/** + * Creates expression queries (reduce + threshold) from a simple condition. + * These expression queries reference the last data query and build a pipeline: + * data query -> reduce expression -> threshold expression + */ function createExpressionQueries( simpleCondition: LocalSimpleCondition, dataQueries: AlertQuery[] @@ -80,6 +84,9 @@ function createExpressionQueries( expression: reduceRefId, }; + // Expression queries don't need relativeTimeRange - they inherit time range context + // from the data queries they reference. This is consistent with how expressions work + // in the alerting query runner. return { reduce: { refId: reduceRefId, @@ -97,9 +104,29 @@ function createExpressionQueries( }; } +/** + * Compares two AlertQuery arrays for expression-relevant equality. + * Only compares the model content of expression queries to determine + * if an update is actually needed. + */ +function areExpressionQueriesEqual(current: AlertQuery[], next: AlertQuery[]): boolean { + const currentExpressions = current.filter((q) => q.datasourceUid === ExpressionDatasourceUID); + const nextExpressions = next.filter((q) => q.datasourceUid === ExpressionDatasourceUID); + + if (currentExpressions.length !== nextExpressions.length) { + return false; + } + + try { + return JSON.stringify(currentExpressions) === JSON.stringify(nextExpressions); + } catch { + return false; + } +} + export function RuleConditionSection() { const base = useStyles2(getStyles); - const { watch, setValue } = useFormContext(); + const { watch, setValue, getValues } = useFormContext(); const evaluateFor = watch('evaluateFor') || '0s'; const queries = watch('queries'); watch('folder'); @@ -109,18 +136,39 @@ export function RuleConditionSection() { evaluator: { params: [0], type: EvalFunction.IsAbove }, }); + // Track if we're currently updating to prevent infinite loops + const isUpdatingRef = useRef(false); + // Update expression queries whenever simpleCondition changes + // We use a ref flag to prevent the infinite loop that would occur because: + // simpleCondition changes -> effect runs -> setValue updates queries -> queries change -> effect would run again useEffect(() => { - const dataQueries = queries.filter((q) => q.datasourceUid !== ExpressionDatasourceUID); + // Skip if we're already in an update cycle + if (isUpdatingRef.current) { + return; + } + + const currentQueries = getValues('queries'); + const dataQueries = currentQueries.filter((q) => q.datasourceUid !== ExpressionDatasourceUID); + if (dataQueries.length === 0) { return; } const { reduce, threshold, condition } = createExpressionQueries(simpleCondition, dataQueries); + const newQueries = [...dataQueries, reduce, threshold]; - setValue('queries', [...dataQueries, reduce, threshold], { shouldDirty: false, shouldValidate: false }); - setValue('condition', condition, { shouldDirty: false, shouldValidate: false }); - }, [simpleCondition, queries, setValue]); + // Only update if the expression queries actually changed + if (!areExpressionQueriesEqual(currentQueries, newQueries)) { + isUpdatingRef.current = true; + setValue('queries', newQueries, { shouldDirty: false, shouldValidate: false }); + setValue('condition', condition, { shouldDirty: false, shouldValidate: false }); + // Reset the flag after the update cycle completes + requestAnimationFrame(() => { + isUpdatingRef.current = false; + }); + } + }, [simpleCondition, getValues, setValue]); const reducerOptions: Array> = reducerTypes .filter((o) => typeof o.value === 'string') @@ -150,12 +198,12 @@ export function RuleConditionSection() { }; return ( -
+
-
+ {`2. `} Condition -
+
@@ -189,6 +237,10 @@ export function RuleConditionSection() { key={simpleCondition.evaluator.params[0]} defaultValue={simpleCondition.evaluator.params[0] ?? ''} onBlur={(event) => onEvaluateValueChange(event, 0)} + aria-label={t( + 'alerting.simple-condition-editor.aria-label-threshold-from', + 'Threshold from value' + )} /> onEvaluateValueChange(event, 1)} + aria-label={t('alerting.simple-condition-editor.aria-label-threshold-to', 'Threshold to value')} /> ) : ( @@ -206,6 +259,7 @@ export function RuleConditionSection() { key={simpleCondition.evaluator.params[0]} defaultValue={simpleCondition.evaluator.params[0] ?? ''} onBlur={(event) => onEvaluateValueChange(event, 0)} + aria-label={t('alerting.simple-condition-editor.aria-label-threshold', 'Threshold value')} /> )} @@ -216,7 +270,7 @@ export function RuleConditionSection() { {evaluateFor === '0s' && ( - +
-
+ ); } @@ -239,12 +293,5 @@ function getStyles(theme: GrafanaTheme2) { gap: theme.spacing(1), marginBottom: theme.spacing(1), }), - sectionHeader: css({ - fontWeight: theme.typography.fontWeightRegular, - fontSize: theme.typography.h4.fontSize, - lineHeight: theme.typography.h4.lineHeight, - }), - paragraphRow: css({ display: 'flex', alignItems: 'center', flexWrap: 'wrap', gap: theme.spacing(1) }), - inlineField: css({ display: 'inline-flex' }), }; } diff --git a/public/app/features/alerting/unified/components/RuleDefinitionSection.tsx b/public/app/features/alerting/unified/components/RuleDefinitionSection.tsx index e2b449efe09..b28dd40334a 100644 --- a/public/app/features/alerting/unified/components/RuleDefinitionSection.tsx +++ b/public/app/features/alerting/unified/components/RuleDefinitionSection.tsx @@ -5,7 +5,7 @@ import { useFormContext } from 'react-hook-form'; import { GrafanaTheme2 } from '@grafana/data'; import { selectors } from '@grafana/e2e-selectors'; import { Trans, t } from '@grafana/i18n'; -import { Field, Input, Stack, useStyles2 } from '@grafana/ui'; +import { Field, Input, Stack, Text, useStyles2 } from '@grafana/ui'; import { RuleFormType, RuleFormValues } from '../types/rule-form'; import { GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource'; @@ -30,21 +30,21 @@ export function RuleDefinitionSection({ type }: { type: RuleFormType }) { const namePlaceholder = isRecording ? 'recording rule' : 'alert rule'; return ( -
+
-
+ {`1. `} Rule Definition -
+
+ Name - + } error={errors?.name?.message} invalid={!!errors.name?.message} @@ -99,7 +99,7 @@ export function RuleDefinitionSection({ type }: { type: RuleFormType }) { )}
-
+ ); } @@ -112,14 +112,5 @@ function getStyles(theme: GrafanaTheme2) { gap: theme.spacing(1), marginBottom: theme.spacing(1), }), - sectionHeader: css({ - fontWeight: theme.typography.fontWeightRegular, - fontSize: theme.typography.h4.fontSize, - lineHeight: theme.typography.h4.lineHeight, - }), - nameLabel: css({ - fontSize: theme.typography.bodySmall.fontSize, - fontWeight: 500, - }), }; } diff --git a/public/app/features/alerting/unified/components/RuleNotificationSection.tsx b/public/app/features/alerting/unified/components/RuleNotificationSection.tsx index 7465eae276a..9034236c042 100644 --- a/public/app/features/alerting/unified/components/RuleNotificationSection.tsx +++ b/public/app/features/alerting/unified/components/RuleNotificationSection.tsx @@ -20,6 +20,7 @@ import { useStyles2, } from '@grafana/ui'; import { useAppNotification } from 'app/core/copy/appNotification'; +import { textUtil } from 'app/core/utils/text'; import { RuleFormValues } from '../types/rule-form'; import { Annotation } from '../utils/constants'; @@ -29,6 +30,51 @@ import { NeedHelpInfoForNotificationPolicy } from './rule-editor/NotificationsSt // Centralized form path for selected contact point const CONTACT_POINT_PATH = 'contactPoints.grafana.selectedContactPoint' as const; +/** + * Validates a URL string and returns an error message if invalid. + * Uses the URL constructor for parsing and rejects dangerous protocols. + * Returns undefined if the URL is valid or empty. + */ +function validateRunbookUrl(value: string): string | undefined { + const trimmedValue = value.trim(); + + // Empty values are allowed (field is optional) + if (!trimmedValue) { + return undefined; + } + + try { + const url = new URL(trimmedValue); + // Reject dangerous URL schemes per F4 frontend security rule + if (url.protocol === 'javascript:' || url.protocol === 'data:' || url.protocol === 'vbscript:') { + return t( + 'alerting.simplified.notification.runbook-url.invalid-protocol', + 'Invalid URL protocol. Please use http or https.' + ); + } + return undefined; + } catch { + return t( + 'alerting.simplified.notification.runbook-url.invalid-format', + 'Invalid URL format. Please enter a valid URL.' + ); + } +} + +/** + * Sanitizes a URL value before storing it in the form. + * Uses textUtil.sanitizeUrl to ensure safe URL handling. + */ +function sanitizeRunbookUrl(value: string): string { + const trimmedValue = value.trim(); + if (!trimmedValue) { + return ''; + } + + // Use textUtil.sanitizeUrl for consistent URL sanitization across the codebase + return textUtil.sanitizeUrl(trimmedValue); +} + export function RuleNotificationSection() { const styles = useStyles2(getStyles); const notifyApp = useAppNotification(); @@ -78,15 +124,18 @@ export function RuleNotificationSection() { const descriptionValue = getAnnotationValue(Annotation.description); const runbookUrlValue = getAnnotationValue(Annotation.runbookURL); + // Validate runbook URL for form-level validation feedback + const runbookUrlError = useMemo(() => validateRunbookUrl(runbookUrlValue), [runbookUrlValue]); + const recipientLabelId = 'recipient-label'; return ( -
+
-
+ {`3. `} Notification -
+
@@ -229,7 +278,12 @@ export function RuleNotificationSection() { /> - + { const value = e.currentTarget.value.trim(); - // Validate URL on blur - if (value && value !== '') { - try { - const url = new URL(value); - // Reject dangerous URL schemes - if (url.protocol === 'javascript:' || url.protocol === 'data:' || url.protocol === 'vbscript:') { - notifyApp.error( - t( - 'alerting.simplified.notification.runbook-url.invalid-protocol', - 'Invalid URL protocol. Please use http or https.' - ) - ); + // Sanitize and update the URL on blur if it's valid + if (value) { + const error = validateRunbookUrl(value); + if (!error) { + // Sanitize the URL before storing + const sanitizedUrl = sanitizeRunbookUrl(value); + if (sanitizedUrl !== value) { + updateAnnotationValue(Annotation.runbookURL, sanitizedUrl); } - } catch { - notifyApp.warning( - t( - 'alerting.simplified.notification.runbook-url.invalid-format', - 'Invalid URL format. Please enter a valid URL.' - ) - ); } } }} @@ -268,11 +311,13 @@ export function RuleNotificationSection() { 'Enter the webpage where you keep your runbook for the alert…' )} aria-label={t('alerting.simplified.notification.runbook-url.aria-label', 'Runbook URL')} + aria-invalid={!!runbookUrlError} + aria-describedby={runbookUrlError ? 'runbook-url-error' : undefined} />
-
+ ); } @@ -285,11 +330,6 @@ function getStyles(theme: GrafanaTheme2) { gap: theme.spacing(1), marginBottom: theme.spacing(1), }), - sectionHeader: css({ - fontWeight: theme.typography.fontWeightRegular, - fontSize: theme.typography.h4.fontSize, - lineHeight: theme.typography.h4.lineHeight, - }), contentTopSpacer: css({ marginTop: theme.spacing(0.5) }), manualRoutingInline: css({ display: 'inline-flex',