From 3755322d316a9b35ac0a9b9a94ba67ad7d85c57e Mon Sep 17 00:00:00 2001 From: Daniel Chromik Date: Tue, 15 Sep 2026 10:59:58 +0200 Subject: [PATCH 1/2] OU-1488: sanitize alert runbook URLs --- .../alerting/AlertRulesDetailsPage.tsx | 4 +- .../components/alerting/AlertsDetailsPage.tsx | 4 +- web/src/components/utils.spec.ts | 42 +++++++++++++++++++ web/src/components/utils.ts | 15 +++++++ 4 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 web/src/components/utils.spec.ts diff --git a/web/src/components/alerting/AlertRulesDetailsPage.tsx b/web/src/components/alerting/AlertRulesDetailsPage.tsx index 91993c97e..f39ba2495 100644 --- a/web/src/components/alerting/AlertRulesDetailsPage.tsx +++ b/web/src/components/alerting/AlertRulesDetailsPage.tsx @@ -63,7 +63,7 @@ import { import KebabDropdown from '../kebab-dropdown'; import { Labels } from '../labels'; import { ToggleGraph } from '../MetricsPage'; -import { alertDescription, RuleResource } from '../utils'; +import { alertDescription, getSafeExternalURL, RuleResource } from '../utils'; import { MonitoringProvider } from '../../contexts/MonitoringContext'; import { DataTestIDs } from '../data-test'; @@ -167,7 +167,7 @@ const AlertRulesDetailsPage_: FC = () => { }; // eslint-disable-next-line camelcase - const runbookURL = rule?.annotations?.runbook_url; + const runbookURL = getSafeExternalURL(rule?.annotations?.runbook_url); return ( <> diff --git a/web/src/components/alerting/AlertsDetailsPage.tsx b/web/src/components/alerting/AlertsDetailsPage.tsx index 682fa5d76..e61d93f54 100644 --- a/web/src/components/alerting/AlertsDetailsPage.tsx +++ b/web/src/components/alerting/AlertsDetailsPage.tsx @@ -26,7 +26,7 @@ import { getRuleUrl, usePerspective, } from '../hooks/usePerspective'; -import { AlertResource, alertState, RuleResource } from '../utils'; +import { AlertResource, alertState, getSafeExternalURL, RuleResource } from '../utils'; import { MonitoringProvider } from '../../contexts/MonitoringContext'; import { @@ -120,7 +120,7 @@ const AlertsDetailsPage_: FC = () => { const labels: PrometheusLabels = useMemo(() => alert?.labels, [labelsMemoKey]); // eslint-disable-next-line camelcase - const runbookURL = alert?.annotations?.runbook_url; + const runbookURL = getSafeExternalURL(alert?.annotations?.runbook_url); const sourceId = rule?.sourceId; diff --git a/web/src/components/utils.spec.ts b/web/src/components/utils.spec.ts new file mode 100644 index 000000000..f17afd43f --- /dev/null +++ b/web/src/components/utils.spec.ts @@ -0,0 +1,42 @@ +jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ + ...jest.requireActual('@openshift-console/dynamic-plugin-sdk/lib/api/common-types'), +})); + +Object.defineProperty(global, 'window', { + value: { + SERVER_FLAGS: { + prometheusBaseURL: '', + prometheusTenancyBaseURL: '', + alertManagerBaseURL: '', + }, + }, +}); + +import { getSafeExternalURL } from './utils'; + +describe('getSafeExternalURL', () => { + it('returns undefined when no URL is provided', () => { + expect(getSafeExternalURL()).toBeUndefined(); + }); + + it.each([ + 'https://runbooks.example.com/alert', + 'http://runbooks.example.com/alert?severity=high', + ])('returns a valid HTTP(S) URL unchanged: %s', (url) => { + expect(getSafeExternalURL(url)).toBe(url); + }); + + it.each([ + 'javascript:alert(1)', + 'JaVaScRiPt:alert(1)', + 'data:text/html,', + 'vbscript:msgbox(1)', + 'mailto:security@example.com', + '/runbooks/alert', + '//runbooks.example.com/alert', + '\tjavascript:alert(1)', + 'not a URL', + ])('rejects an unsafe or invalid URL: %s', (url) => { + expect(getSafeExternalURL(url)).toBeUndefined(); + }); +}); diff --git a/web/src/components/utils.ts b/web/src/components/utils.ts index f0e6c679f..796ea0136 100644 --- a/web/src/components/utils.ts +++ b/web/src/components/utils.ts @@ -154,6 +154,21 @@ export const targetSource = (target: Target): AlertSource => export const isTimeoutError = (err: Error): boolean => err.name === 'TimeoutError' || err.message.includes('timed out'); +/** + * Returns an externally supplied URL only when it is an absolute HTTP(S) URL. + * + * Do not normalize the value before returning it: callers display this value to + * users, and the browser will perform the same URL parsing when navigating. + */ +export const getSafeExternalURL = (value?: string): string | undefined => { + if (!value || !URL.canParse(value)) { + return undefined; + } + + const url = new URL(value); + return url.protocol === 'http:' || url.protocol === 'https:' ? value : undefined; +}; + /** * This function is used to get the parameters needed to break a long time period down into smaller * chunks which won't timeout From 38617003905c1879edb2d0bc65598b9e1e8c0900 Mon Sep 17 00:00:00 2001 From: Daniel Chromik Date: Tue, 15 Sep 2026 12:37:23 +0200 Subject: [PATCH 2/2] OU-1488: move URL validation into ExternalLink --- .../alerting/AlertRulesDetailsPage.tsx | 4 +- .../components/alerting/AlertsDetailsPage.tsx | 4 +- .../components/console/utils/link.spec.tsx | 45 ++++++++++++++ web/src/components/console/utils/link.tsx | 61 ++++++++++++------- web/src/components/utils.spec.ts | 42 ------------- web/src/components/utils.ts | 15 ----- 6 files changed, 87 insertions(+), 84 deletions(-) create mode 100644 web/src/components/console/utils/link.spec.tsx delete mode 100644 web/src/components/utils.spec.ts diff --git a/web/src/components/alerting/AlertRulesDetailsPage.tsx b/web/src/components/alerting/AlertRulesDetailsPage.tsx index f39ba2495..91993c97e 100644 --- a/web/src/components/alerting/AlertRulesDetailsPage.tsx +++ b/web/src/components/alerting/AlertRulesDetailsPage.tsx @@ -63,7 +63,7 @@ import { import KebabDropdown from '../kebab-dropdown'; import { Labels } from '../labels'; import { ToggleGraph } from '../MetricsPage'; -import { alertDescription, getSafeExternalURL, RuleResource } from '../utils'; +import { alertDescription, RuleResource } from '../utils'; import { MonitoringProvider } from '../../contexts/MonitoringContext'; import { DataTestIDs } from '../data-test'; @@ -167,7 +167,7 @@ const AlertRulesDetailsPage_: FC = () => { }; // eslint-disable-next-line camelcase - const runbookURL = getSafeExternalURL(rule?.annotations?.runbook_url); + const runbookURL = rule?.annotations?.runbook_url; return ( <> diff --git a/web/src/components/alerting/AlertsDetailsPage.tsx b/web/src/components/alerting/AlertsDetailsPage.tsx index e61d93f54..682fa5d76 100644 --- a/web/src/components/alerting/AlertsDetailsPage.tsx +++ b/web/src/components/alerting/AlertsDetailsPage.tsx @@ -26,7 +26,7 @@ import { getRuleUrl, usePerspective, } from '../hooks/usePerspective'; -import { AlertResource, alertState, getSafeExternalURL, RuleResource } from '../utils'; +import { AlertResource, alertState, RuleResource } from '../utils'; import { MonitoringProvider } from '../../contexts/MonitoringContext'; import { @@ -120,7 +120,7 @@ const AlertsDetailsPage_: FC = () => { const labels: PrometheusLabels = useMemo(() => alert?.labels, [labelsMemoKey]); // eslint-disable-next-line camelcase - const runbookURL = getSafeExternalURL(alert?.annotations?.runbook_url); + const runbookURL = alert?.annotations?.runbook_url; const sourceId = rule?.sourceId; diff --git a/web/src/components/console/utils/link.spec.tsx b/web/src/components/console/utils/link.spec.tsx new file mode 100644 index 000000000..ec4d141d4 --- /dev/null +++ b/web/src/components/console/utils/link.spec.tsx @@ -0,0 +1,45 @@ +import { renderToStaticMarkup } from 'react-dom/server'; + +jest.mock('@patternfly/react-core', () => ({ + Button: ({ children, href, rel, target }) => ( + + {children} + + ), + Icon: ({ children }) => <>{children}, +})); + +jest.mock('@patternfly/react-icons', () => ({ + ExternalLinkAltIcon: () => null, +})); + +jest.mock('react-linkify', () => ({ children }) => <>{children}); + +import { ExternalLink } from './link'; + +describe('ExternalLink', () => { + it.each([ + 'https://runbooks.example.com/alert', + 'http://runbooks.example.com/alert?severity=high', + ])('renders an absolute HTTP(S) URL as a link: %s', (href) => { + const html = renderToStaticMarkup(); + + expect(html).toContain(`href="${href}"`); + }); + + it.each([ + 'javascript:alert(1)', + 'JaVaScRiPt:alert(1)', + 'data:text/html,', + 'vbscript:msgbox(1)', + 'mailto:security@example.com', + '/runbooks/alert', + '//runbooks.example.com/alert', + '\tjavascript:alert(1)', + 'not a URL', + ])('renders an unsafe or invalid URL as text: %s', (href) => { + const html = renderToStaticMarkup(); + + expect(html).not.toMatch(/]/); + }); +}); diff --git a/web/src/components/console/utils/link.tsx b/web/src/components/console/utils/link.tsx index 4df925b70..16830abb3 100644 --- a/web/src/components/console/utils/link.tsx +++ b/web/src/components/console/utils/link.tsx @@ -1,36 +1,42 @@ -import type { FC, ReactNode } from 'react'; +import type { FC, PropsWithChildren, ReactNode } from 'react'; import Linkify from 'react-linkify'; import { Button, Icon } from '@patternfly/react-core'; import { ExternalLinkAltIcon } from '@patternfly/react-icons'; -export const ExternalLink: FC = ({ +export const ExternalLink: FC> = ({ children, href, text, additionalClassName = '', dataTestID, stopPropagation, -}) => ( - -); +}) => { + if (!isSafeExternalURL(href)) { + return <>{children || text}; + } + + return ( + + ); +}; // Open links in a new window and set noopener/noreferrer. export const LinkifyExternal: FC<{ children: ReactNode }> = ({ children }) => ( @@ -45,3 +51,12 @@ type ExternalLinkProps = { dataTestID?: string; stopPropagation?: boolean; }; + +const isSafeExternalURL = (value: string): value is string => { + if (!URL.canParse(value)) { + return false; + } + + const { protocol } = new URL(value); + return protocol === 'http:' || protocol === 'https:'; +}; diff --git a/web/src/components/utils.spec.ts b/web/src/components/utils.spec.ts deleted file mode 100644 index f17afd43f..000000000 --- a/web/src/components/utils.spec.ts +++ /dev/null @@ -1,42 +0,0 @@ -jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ - ...jest.requireActual('@openshift-console/dynamic-plugin-sdk/lib/api/common-types'), -})); - -Object.defineProperty(global, 'window', { - value: { - SERVER_FLAGS: { - prometheusBaseURL: '', - prometheusTenancyBaseURL: '', - alertManagerBaseURL: '', - }, - }, -}); - -import { getSafeExternalURL } from './utils'; - -describe('getSafeExternalURL', () => { - it('returns undefined when no URL is provided', () => { - expect(getSafeExternalURL()).toBeUndefined(); - }); - - it.each([ - 'https://runbooks.example.com/alert', - 'http://runbooks.example.com/alert?severity=high', - ])('returns a valid HTTP(S) URL unchanged: %s', (url) => { - expect(getSafeExternalURL(url)).toBe(url); - }); - - it.each([ - 'javascript:alert(1)', - 'JaVaScRiPt:alert(1)', - 'data:text/html,', - 'vbscript:msgbox(1)', - 'mailto:security@example.com', - '/runbooks/alert', - '//runbooks.example.com/alert', - '\tjavascript:alert(1)', - 'not a URL', - ])('rejects an unsafe or invalid URL: %s', (url) => { - expect(getSafeExternalURL(url)).toBeUndefined(); - }); -}); diff --git a/web/src/components/utils.ts b/web/src/components/utils.ts index 796ea0136..f0e6c679f 100644 --- a/web/src/components/utils.ts +++ b/web/src/components/utils.ts @@ -154,21 +154,6 @@ export const targetSource = (target: Target): AlertSource => export const isTimeoutError = (err: Error): boolean => err.name === 'TimeoutError' || err.message.includes('timed out'); -/** - * Returns an externally supplied URL only when it is an absolute HTTP(S) URL. - * - * Do not normalize the value before returning it: callers display this value to - * users, and the browser will perform the same URL parsing when navigating. - */ -export const getSafeExternalURL = (value?: string): string | undefined => { - if (!value || !URL.canParse(value)) { - return undefined; - } - - const url = new URL(value); - return url.protocol === 'http:' || url.protocol === 'https:' ? value : undefined; -}; - /** * This function is used to get the parameters needed to break a long time period down into smaller * chunks which won't timeout