From 54cc666aa02747cebf55f4bb2ad70c0d92fdd8e6 Mon Sep 17 00:00:00 2001 From: Kevin Minehart <5140827+kminehart@users.noreply.github.com> Date: Thu, 14 Nov 2024 10:08:58 -0600 Subject: [PATCH] Alerting: Add useReturnTo hook to safely handle returnTo parameter (#96474) Add useReturnTo hook to safely handle returnTo parameter Co-authored-by: Konrad Lalik --- .../features/alerting/unified/Analytics.ts | 6 ++- .../alert-rule-form/AlertRuleForm.tsx | 6 ++- .../alert-rule-form/ModifyExportRuleForm.tsx | 5 +- .../components/rule-viewer/RuleViewer.tsx | 5 +- .../unified/hooks/useReturnTo.test.tsx | 48 ++++++++++++++++++ .../alerting/unified/hooks/useReturnTo.ts | 49 +++++++++++++++++++ 6 files changed, 110 insertions(+), 9 deletions(-) create mode 100644 public/app/features/alerting/unified/hooks/useReturnTo.test.tsx create mode 100644 public/app/features/alerting/unified/hooks/useReturnTo.ts diff --git a/public/app/features/alerting/unified/Analytics.ts b/public/app/features/alerting/unified/Analytics.ts index 09a3da9f551..87044454af4 100644 --- a/public/app/features/alerting/unified/Analytics.ts +++ b/public/app/features/alerting/unified/Analytics.ts @@ -29,9 +29,11 @@ export const LogMessages = { loadedCentralAlertStateHistory: 'loaded central alert state history', }; -const { logInfo, logError, logMeasurement } = createMonitoringLogger('features.alerting', { module: 'Alerting' }); +const { logInfo, logError, logMeasurement, logWarning } = createMonitoringLogger('features.alerting', { + module: 'Alerting', +}); -export { logError, logInfo, logMeasurement }; +export { logError, logInfo, logMeasurement, logWarning }; // eslint-disable-next-line @typescript-eslint/no-explicit-any export function withPerformanceLogging Promise>( diff --git a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx index fba035a6daa..447f538136e 100644 --- a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/AlertRuleForm.tsx @@ -36,6 +36,7 @@ import { import { shouldUsePrometheusRulesPrimary } from '../../../featureToggles'; import { useDeleteRuleFromGroup } from '../../../hooks/ruleGroup/useDeleteRuleFromGroup'; import { useAddRuleToRuleGroup, useUpdateRuleInRuleGroup } from '../../../hooks/ruleGroup/useUpsertRuleFromRuleGroup'; +import { useReturnTo } from '../../../hooks/useReturnTo'; import { useURLSearchParams } from '../../../hooks/useURLSearchParams'; import { RuleFormType, RuleFormValues } from '../../../types/rule-form'; import { DataSourceType } from '../../../utils/datasource'; @@ -84,6 +85,7 @@ export const AlertRuleForm = ({ existing, prefill }: Props) => { const [addRuleToRuleGroup] = useAddRuleToRuleGroup(); const [updateRuleInRuleGroup] = useUpdateRuleInRuleGroup(); + const { returnTo } = useReturnTo(); const routeParams = useParams<{ type: string; id: string }>(); const ruleType = translateRouteParamToRuleType(routeParams.type); @@ -178,9 +180,9 @@ export const AlertRuleForm = ({ existing, prefill }: Props) => { const { dataSourceName, namespaceName, groupName } = ruleGroupIdentifier; if (exitOnSave) { - const returnTo = queryParams.get('returnTo') || getReturnToUrl(ruleGroupIdentifier, ruleDefinition); + const returnToUrl = returnTo || getReturnToUrl(ruleGroupIdentifier, ruleDefinition); - locationService.push(returnTo); + locationService.push(returnToUrl); return; } diff --git a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/ModifyExportRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/ModifyExportRuleForm.tsx index 099b00dd47f..e35f6da5cf2 100644 --- a/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/ModifyExportRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/alert-rule-form/ModifyExportRuleForm.tsx @@ -4,7 +4,6 @@ import { useAsync } from 'react-use'; import { Button, LinkButton, LoadingPlaceholder, Stack } from '@grafana/ui'; import { useAppNotification } from 'app/core/copy/appNotification'; -import { useQueryParams } from 'app/core/hooks/useQueryParams'; import { AppChromeUpdate } from '../../../../../../core/components/AppChrome/AppChromeUpdate'; import { @@ -15,6 +14,7 @@ import { import { alertRuleApi } from '../../../api/alertRuleApi'; import { fetchRulerRulesGroup } from '../../../api/ruler'; import { useDataSourceFeatures } from '../../../hooks/useCombinedRule'; +import { useReturnTo } from '../../../hooks/useReturnTo'; import { RuleFormValues } from '../../../types/rule-form'; import { GRAFANA_RULES_SOURCE_NAME } from '../../../utils/datasource'; import { DEFAULT_GROUP_EVALUATION_INTERVAL, formValuesToRulerGrafanaRuleDTO } from '../../../utils/rule-form'; @@ -39,11 +39,10 @@ export function ModifyExportRuleForm({ ruleForm, alertUid }: ModifyExportRuleFor defaultValues: ruleForm, shouldFocusError: true, }); - const [queryParams] = useQueryParams(); const existing = Boolean(ruleForm); // always should be true const notifyApp = useAppNotification(); - const returnTo = !queryParams.returnTo ? '/alerting/list' : String(queryParams.returnTo); + const { returnTo } = useReturnTo('/alerting/list'); const [exportData, setExportData] = useState(undefined); diff --git a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.tsx b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.tsx index f317be6043a..1ef1f681913 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.tsx @@ -15,6 +15,7 @@ import { PromAlertingRuleState, PromRuleType } from 'app/types/unified-alerting- import { defaultPageNav } from '../../RuleViewer'; import { shouldUsePrometheusRulesPrimary } from '../../featureToggles'; import { usePrometheusCreationConsistencyCheck } from '../../hooks/usePrometheusConsistencyCheck'; +import { useReturnTo } from '../../hooks/useReturnTo'; import { PluginOriginBadge } from '../../plugins/PluginOriginBadge'; import { Annotation } from '../../utils/constants'; import { makeDashboardLink, makePanelLink, stringifyErrorLike } from '../../utils/misc'; @@ -242,9 +243,9 @@ interface TitleProps { } export const Title = ({ name, paused = false, state, health, ruleType, ruleOrigin }: TitleProps) => { - const [queryParams] = useQueryParams(); const isRecordingRule = ruleType === PromRuleType.Recording; - const returnTo = queryParams.returnTo ? String(queryParams.returnTo) : '/alerting/list'; + + const { returnTo } = useReturnTo('/alerting/list'); return ( diff --git a/public/app/features/alerting/unified/hooks/useReturnTo.test.tsx b/public/app/features/alerting/unified/hooks/useReturnTo.test.tsx new file mode 100644 index 00000000000..80adb1661cc --- /dev/null +++ b/public/app/features/alerting/unified/hooks/useReturnTo.test.tsx @@ -0,0 +1,48 @@ +import { MemoryRouter } from 'react-router-dom-v5-compat'; +import { renderHook } from 'test/test-utils'; + +import { useReturnTo } from './useReturnTo'; + +describe('useReturnTo', () => { + beforeAll(() => { + // @ts-expect-error + delete window.location; + window.location = { origin: 'https://play.grafana.net' } as Location; + }); + + it('should return the fallback value when `returnTo` is not present in the query string', () => { + const { result } = renderHook(() => useReturnTo('/fallback'), { wrapper: MemoryRouter }); + + expect(result.current.returnTo).toBe('/fallback'); + }); + + it('should return the sanitized `returnTo` value when it is present in the query string and is a valid URL within the Grafana app', () => { + const { result } = renderHook(() => useReturnTo('/fallback'), { + wrapper: ({ children }) => ( + {children} + ), + }); + + expect(result.current.returnTo).toBe('/dashboard/db/my-dashboard'); + }); + + it('should return the fallback value when `returnTo` is present in the query string but is not a valid URL within the Grafana app', () => { + const { result } = renderHook(() => useReturnTo('/fallback'), { + wrapper: ({ children }) => ( + {children} + ), + }); + + expect(result.current.returnTo).toBe('/fallback'); + }); + + it('should return the fallback value when `returnTo` is present in the query string but is a malicious JavaScript URL', () => { + const { result } = renderHook(() => useReturnTo('/fallback'), { + wrapper: ({ children }) => ( + {children} + ), + }); + + expect(result.current.returnTo).toBe('/fallback'); + }); +}); diff --git a/public/app/features/alerting/unified/hooks/useReturnTo.ts b/public/app/features/alerting/unified/hooks/useReturnTo.ts new file mode 100644 index 00000000000..b522bf2c8f6 --- /dev/null +++ b/public/app/features/alerting/unified/hooks/useReturnTo.ts @@ -0,0 +1,49 @@ +import { textUtil } from '@grafana/data'; +import { config } from '@grafana/runtime'; + +import { logWarning } from '../Analytics'; + +import { useURLSearchParams } from './useURLSearchParams'; + +/** + * This hook provides a safe way to obtain the `returnTo` URL from the query string parameter + * It validates the origin and protocol to ensure the URL is withing the Grafana app + */ +export function useReturnTo(fallback?: string): { returnTo: string | undefined } { + const emptyResult = { returnTo: fallback }; + + const [searchParams] = useURLSearchParams(); + const returnTo = searchParams.get('returnTo'); + + if (!returnTo) { + return emptyResult; + } + + const sanitizedReturnTo = textUtil.sanitizeUrl(returnTo); + const baseUrl = `${window.location.origin}/${config.appSubUrl}`; + + const sanitizedUrl = tryParseURL(sanitizedReturnTo, baseUrl); + + if (!sanitizedUrl) { + logWarning('Malformed returnTo parameter', { returnTo }); + return emptyResult; + } + + const { protocol, origin, pathname, search } = sanitizedUrl; + if (['http:', 'https:'].includes(protocol) === false || origin !== window.location.origin) { + logWarning('Malformed returnTo parameter', { returnTo }); + return emptyResult; + } + + return { returnTo: `${pathname}${search}` }; +} + +// Tries to mimic URL.parse method https://developer.mozilla.org/en-US/docs/Web/API/URL/parse_static +function tryParseURL(sanitizedReturnTo: string, baseUrl: string) { + try { + const url = new URL(sanitizedReturnTo, baseUrl); + return url; + } catch (error) { + return null; + } +}