From 081243f781d149d66720f89cd7ddd345e1b5846e Mon Sep 17 00:00:00 2001 From: Daniel Chromik Date: Wed, 9 Sep 2026 13:31:02 +0200 Subject: [PATCH 1/2] feat: added URL validation to alert details --- .../alerts/pages/AlertRulesDetailsPage.tsx | 4 +-- .../alerts/pages/AlertsDetailsPage.tsx | 3 +- web/src/shared/utils/utils.spec.ts | 29 ++++++++++++++++++- web/src/shared/utils/utils.ts | 15 ++++++++++ 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx b/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx index 424023087..923ac8a5b 100644 --- a/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx +++ b/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx @@ -68,7 +68,7 @@ import { getQueryBrowserUrl, usePerspective, } from '@/shared/hooks/usePerspective'; -import { alertDescription, RuleResource } from '@/shared/utils/utils'; +import { alertDescription, getSafeExternalURL, RuleResource } from '@/shared/utils/utils'; // Renders Prometheus template text and highlights any {{ ... }} tags that it contains const PrometheusTemplate = ({ text }: { text: string }) => ( @@ -169,7 +169,7 @@ const AlertRulesDetailsPage_: FC = () => { return `${nameLabel}{${_.map(otherLabels, (v, k) => `${k}="${v}"`).join(',')}}`; }; - const runbookURL = rule?.annotations?.runbook_url; + const runbookURL = getSafeExternalURL(rule?.annotations?.runbook_url); return ( <> diff --git a/web/src/features/alerts/pages/AlertsDetailsPage.tsx b/web/src/features/alerts/pages/AlertsDetailsPage.tsx index 45ab9687d..09176553f 100644 --- a/web/src/features/alerts/pages/AlertsDetailsPage.tsx +++ b/web/src/features/alerts/pages/AlertsDetailsPage.tsx @@ -87,6 +87,7 @@ import { usePerspective, } from '@/shared/hooks/usePerspective'; import { MonitoringState } from '@/shared/store/store'; +import { getSafeExternalURL } from '@/shared/utils/utils'; import { AlertResource, alertState, RuleResource } from '@/shared/utils/utils'; const AlertsDetailsPage_: FC = () => { @@ -121,7 +122,7 @@ const AlertsDetailsPage_: FC = () => { // eslint-disable-next-line react-hooks/exhaustive-deps const labels: PrometheusLabels = useMemo(() => alert?.labels, [labelsMemoKey]); - const runbookURL = alert?.annotations?.runbook_url; + const runbookURL = getSafeExternalURL(alert?.annotations?.runbook_url); const sourceId = rule?.sourceId; diff --git a/web/src/shared/utils/utils.spec.ts b/web/src/shared/utils/utils.spec.ts index 0b348449c..5c8a00c57 100644 --- a/web/src/shared/utils/utils.spec.ts +++ b/web/src/shared/utils/utils.spec.ts @@ -4,7 +4,7 @@ jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ import { AlertSeverity, AlertStates, Rule } from '@openshift-console/dynamic-plugin-sdk'; -import { alertingRuleStateSort, severitySort } from '@/shared/utils/utils'; +import { alertingRuleStateSort, getSafeExternalURL, severitySort } from '@/shared/utils/utils'; const makeRule = (alerts: { state: AlertStates }[]): Rule => ({ alerts }) as unknown as Rule; @@ -106,3 +106,30 @@ describe('severitySort', () => { expect(severities).toEqual(['critical', 'warning', 'info', 'none']); }); }); + +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/shared/utils/utils.ts b/web/src/shared/utils/utils.ts index 73ed612b4..ef8b8c98d 100644 --- a/web/src/shared/utils/utils.ts +++ b/web/src/shared/utils/utils.ts @@ -165,6 +165,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 cbdb62abe325a6c2a3eb97f02684dd5cd9e268fd Mon Sep 17 00:00:00 2001 From: Daniel Chromik Date: Wed, 9 Sep 2026 15:54:35 +0200 Subject: [PATCH 2/2] refactor: restructured logic, cleaned up dead code --- web/package-lock.json | 129 ++++++++++++++++++ web/package.json | 2 + .../alerts/pages/AlertRulesDetailsPage.tsx | 4 +- .../alerts/pages/AlertsDetailsPage.tsx | 3 +- web/src/shared/console/utils/Link.spec.tsx | 45 ++++++ web/src/shared/console/utils/Link.tsx | 66 ++++----- web/src/shared/utils/utils.spec.ts | 29 +--- web/src/shared/utils/utils.ts | 15 -- 8 files changed, 214 insertions(+), 79 deletions(-) create mode 100644 web/src/shared/console/utils/Link.spec.tsx diff --git a/web/package-lock.json b/web/package-lock.json index b2763c53b..f79a09d16 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -69,6 +69,8 @@ "@openshift-console/dynamic-plugin-sdk-webpack": "4.22.0", "@swc/core": "^1.15.3", "@swc/helpers": "0.5.23", + "@testing-library/dom": "^10.4.1", + "@testing-library/react": "^16.3.3", "@types/ajv": "^0.0.5", "@types/classnames": "^2.2.7", "@types/jest": "^30.0.0", @@ -5989,6 +5991,89 @@ "url": "https://github.com/sponsors/tannerlinsley" } }, + "node_modules/@testing-library/dom": { + "version": "10.4.1", + "resolved": "https://registry.npmjs.org/@testing-library/dom/-/dom-10.4.1.tgz", + "integrity": "sha512-o4PXJQidqJl82ckFaXUeoAW+XysPLauYI43Abki5hABd853iMhitooc6znOnczgbTYmEP6U6/y1ZyKAIsvMKGg==", + "dev": true, + "license": "MIT", + "dependencies": { + "@babel/code-frame": "^7.10.4", + "@babel/runtime": "^7.12.5", + "@types/aria-query": "^5.0.1", + "aria-query": "5.3.0", + "dom-accessibility-api": "^0.5.9", + "lz-string": "^1.5.0", + "picocolors": "1.1.1", + "pretty-format": "^27.0.2" + }, + "engines": { + "node": ">=18" + } + }, + "node_modules/@testing-library/dom/node_modules/ansi-styles": { + "version": "5.2.0", + "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-5.2.0.tgz", + "integrity": "sha512-Cxwpt2SfTzTtXcfOlzGEee8O+c+MmUgGrNiBcXnuWxuFJHe6a5Hz7qwhwe5OgaSYI0IJvkLqWX1ASG+cJOkEiA==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/chalk/ansi-styles?sponsor=1" + } + }, + "node_modules/@testing-library/dom/node_modules/pretty-format": { + "version": "27.5.1", + "resolved": "https://registry.npmjs.org/pretty-format/-/pretty-format-27.5.1.tgz", + "integrity": "sha512-Qb1gy5OrP5+zDf2Bvnzdl3jsTf1qXVMazbvCoKhtKqVs4/YK4ozX4gKQJJVyNe+cajNPn0KoC0MC3FUmaHWEmQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1", + "ansi-styles": "^5.0.0", + "react-is": "^17.0.1" + }, + "engines": { + "node": "^10.13.0 || ^12.13.0 || ^14.15.0 || >=15.0.0" + } + }, + "node_modules/@testing-library/dom/node_modules/react-is": { + "version": "17.0.2", + "resolved": "https://registry.npmjs.org/react-is/-/react-is-17.0.2.tgz", + "integrity": "sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w==", + "dev": true, + "license": "MIT" + }, + "node_modules/@testing-library/react": { + "version": "16.3.3", + "resolved": "https://registry.npmjs.org/@testing-library/react/-/react-16.3.3.tgz", + "integrity": "sha512-Uo193NgQbPMz6lrrhtRQQFcMC6Re/ELLFbbuVL30WDlZxlpZf9/lMHTAVxPRLw1q1iu9OJmR1c2BLiENRstdBg==", + "dev": true, + "license": "MIT", + "dependencies": { + "@babel/runtime": "^7.12.5" + }, + "engines": { + "node": ">=18" + }, + "peerDependencies": { + "@testing-library/dom": "^10.0.0", + "@types/react": "^18.0.0 || ^19.0.0", + "@types/react-dom": "^18.0.0 || ^19.0.0", + "react": "^18.0.0 || ^19.0.0", + "react-dom": "^18.0.0 || ^19.0.0" + }, + "peerDependenciesMeta": { + "@types/react": { + "optional": true + }, + "@types/react-dom": { + "optional": true + } + } + }, "node_modules/@tsconfig/node10": { "version": "1.0.11", "resolved": "https://registry.npmjs.org/@tsconfig/node10/-/node10-1.0.11.tgz", @@ -6024,6 +6109,13 @@ "dev": true, "license": "MIT" }, + "node_modules/@types/aria-query": { + "version": "5.0.4", + "resolved": "https://registry.npmjs.org/@types/aria-query/-/aria-query-5.0.4.tgz", + "integrity": "sha512-rfT93uj5s0PRL7EzccGMs3brplhcrghnDoV26NqKhCAS1hVo+WdNsPvE/yb6ilfr5hi2MEk6d5EWJTKdxg8jVw==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/babel__core": { "version": "7.20.5", "resolved": "https://registry.npmjs.org/@types/babel__core/-/babel__core-7.20.5.tgz", @@ -7580,6 +7672,16 @@ "integrity": "sha512-8+9WqebbFzpX9OR+Wa6O29asIogeRMzcGtAINdpMHHyAg10f05aSFVBbcEqGf/PXw1EjAZ+q2/bEBg3DvurK3Q==", "license": "Python-2.0" }, + "node_modules/aria-query": { + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/aria-query/-/aria-query-5.3.0.tgz", + "integrity": "sha512-b0P0sZPKtyu8HkeRAfCq0IfURZK+SuwMjY1UXGBU27wpAiTwQAIlq56IbIO+ytk/JjS1fMR14ee5WBBfKi5J6A==", + "dev": true, + "license": "Apache-2.0", + "dependencies": { + "dequal": "^2.0.3" + } + }, "node_modules/array-buffer-byte-length": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/array-buffer-byte-length/-/array-buffer-byte-length-1.0.2.tgz", @@ -9970,6 +10072,16 @@ "node": ">= 0.8" } }, + "node_modules/dequal": { + "version": "2.0.3", + "resolved": "https://registry.npmjs.org/dequal/-/dequal-2.0.3.tgz", + "integrity": "sha512-0je+qPKHEMohvfRTCEo3CrPG6cAzAYgmzKyxRiYSSDkS6eGJdyVJm7WaYA5ECaAD9wLB2T4EEeymA5aFVcYXCA==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=6" + } + }, "node_modules/detect-libc": { "version": "2.1.2", "resolved": "https://registry.npmjs.org/detect-libc/-/detect-libc-2.1.2.tgz", @@ -10040,6 +10152,13 @@ "node": ">=0.10.0" } }, + "node_modules/dom-accessibility-api": { + "version": "0.5.16", + "resolved": "https://registry.npmjs.org/dom-accessibility-api/-/dom-accessibility-api-0.5.16.tgz", + "integrity": "sha512-X7BJ2yElsnOJ30pZF4uIIDfBEVgF4XEBxL9Bxhy6dnrm5hkzqmsWHGTiHqRiITNhMyFLyAiWndIJP7Z1NTteDg==", + "dev": true, + "license": "MIT" + }, "node_modules/dom-converter": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/dom-converter/-/dom-converter-0.2.0.tgz", @@ -17069,6 +17188,16 @@ "node": ">=10" } }, + "node_modules/lz-string": { + "version": "1.5.0", + "resolved": "https://registry.npmjs.org/lz-string/-/lz-string-1.5.0.tgz", + "integrity": "sha512-h5bgJWpxJNswbU7qCrV0tIKQCaS3blPDrqKWx+QxzuzL1zGUzij9XCWLrSLsJPu5t+eWA/ycetzYAO5IOMcWAQ==", + "dev": true, + "license": "MIT", + "bin": { + "lz-string": "bin/bin.js" + } + }, "node_modules/magic-string": { "version": "1.2.3", "resolved": "https://registry.npmjs.org/magic-string/-/magic-string-1.2.3.tgz", diff --git a/web/package.json b/web/package.json index 378dbf074..e017a2674 100644 --- a/web/package.json +++ b/web/package.json @@ -115,6 +115,8 @@ "@openshift-console/dynamic-plugin-sdk-webpack": "4.22.0", "@swc/core": "^1.15.3", "@swc/helpers": "0.5.23", + "@testing-library/dom": "^10.4.1", + "@testing-library/react": "^16.3.3", "@types/ajv": "^0.0.5", "@types/classnames": "^2.2.7", "@types/jest": "^30.0.0", diff --git a/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx b/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx index 923ac8a5b..424023087 100644 --- a/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx +++ b/web/src/features/alerts/pages/AlertRulesDetailsPage.tsx @@ -68,7 +68,7 @@ import { getQueryBrowserUrl, usePerspective, } from '@/shared/hooks/usePerspective'; -import { alertDescription, getSafeExternalURL, RuleResource } from '@/shared/utils/utils'; +import { alertDescription, RuleResource } from '@/shared/utils/utils'; // Renders Prometheus template text and highlights any {{ ... }} tags that it contains const PrometheusTemplate = ({ text }: { text: string }) => ( @@ -169,7 +169,7 @@ const AlertRulesDetailsPage_: FC = () => { return `${nameLabel}{${_.map(otherLabels, (v, k) => `${k}="${v}"`).join(',')}}`; }; - const runbookURL = getSafeExternalURL(rule?.annotations?.runbook_url); + const runbookURL = rule?.annotations?.runbook_url; return ( <> diff --git a/web/src/features/alerts/pages/AlertsDetailsPage.tsx b/web/src/features/alerts/pages/AlertsDetailsPage.tsx index 09176553f..45ab9687d 100644 --- a/web/src/features/alerts/pages/AlertsDetailsPage.tsx +++ b/web/src/features/alerts/pages/AlertsDetailsPage.tsx @@ -87,7 +87,6 @@ import { usePerspective, } from '@/shared/hooks/usePerspective'; import { MonitoringState } from '@/shared/store/store'; -import { getSafeExternalURL } from '@/shared/utils/utils'; import { AlertResource, alertState, RuleResource } from '@/shared/utils/utils'; const AlertsDetailsPage_: FC = () => { @@ -122,7 +121,7 @@ const AlertsDetailsPage_: FC = () => { // eslint-disable-next-line react-hooks/exhaustive-deps const labels: PrometheusLabels = useMemo(() => alert?.labels, [labelsMemoKey]); - const runbookURL = getSafeExternalURL(alert?.annotations?.runbook_url); + const runbookURL = alert?.annotations?.runbook_url; const sourceId = rule?.sourceId; diff --git a/web/src/shared/console/utils/Link.spec.tsx b/web/src/shared/console/utils/Link.spec.tsx new file mode 100644 index 000000000..22a3be310 --- /dev/null +++ b/web/src/shared/console/utils/Link.spec.tsx @@ -0,0 +1,45 @@ +/** @jest-environment jsdom */ + +import { render, screen } from '@testing-library/react'; + +import { ExternalLink, LinkifyExternal } from '@/shared/console/utils/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) => { + render(); + + expect(screen.getByRole('link', { name: href }).getAttribute('href')).toBe(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 { container } = render(); + + expect(container.textContent).toBe(href); + expect(screen.queryByRole('link')).toBeNull(); + }); +}); + +describe('LinkifyExternal', () => { + it('turns URLs in its children into protected external links', () => { + const href = 'https://runbooks.example.com/alert'; + render({href}); + + const link = screen.getByRole('link', { name: href }); + expect(link.getAttribute('href')).toBe(href); + expect(link.getAttribute('target')).toBe('_blank'); + expect(link.getAttribute('rel')).toBe('noopener noreferrer'); + }); +}); diff --git a/web/src/shared/console/utils/Link.tsx b/web/src/shared/console/utils/Link.tsx index e9a5c512d..edb8c8e51 100644 --- a/web/src/shared/console/utils/Link.tsx +++ b/web/src/shared/console/utils/Link.tsx @@ -1,36 +1,32 @@ import { Button, Icon } from '@patternfly/react-core'; import { ExternalLinkAltIcon } from '@patternfly/react-icons'; import Linkify from 'linkify-react'; -import type { FC, PropsWithChildren, ReactNode } from 'react'; +import type { FC, ReactNode } from 'react'; -export const ExternalLink: FC> = ({ - children, - href, - text, - additionalClassName = '', - dataTestID, - stopPropagation, -}) => ( - -); +export const ExternalLink: FC = ({ href, text }) => { + if (!isSafeExternalURL(href)) { + return <>{text}; + } + + return ( + + ); +}; // Open links in a new window and set noopener/noreferrer. export const LinkifyExternal: FC<{ children: ReactNode }> = ({ children }) => ( @@ -41,7 +37,13 @@ LinkifyExternal.displayName = 'LinkifyExternal'; type ExternalLinkProps = { href: string; text?: ReactNode; - additionalClassName?: string; - 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/shared/utils/utils.spec.ts b/web/src/shared/utils/utils.spec.ts index 5c8a00c57..0b348449c 100644 --- a/web/src/shared/utils/utils.spec.ts +++ b/web/src/shared/utils/utils.spec.ts @@ -4,7 +4,7 @@ jest.mock('@openshift-console/dynamic-plugin-sdk', () => ({ import { AlertSeverity, AlertStates, Rule } from '@openshift-console/dynamic-plugin-sdk'; -import { alertingRuleStateSort, getSafeExternalURL, severitySort } from '@/shared/utils/utils'; +import { alertingRuleStateSort, severitySort } from '@/shared/utils/utils'; const makeRule = (alerts: { state: AlertStates }[]): Rule => ({ alerts }) as unknown as Rule; @@ -106,30 +106,3 @@ describe('severitySort', () => { expect(severities).toEqual(['critical', 'warning', 'info', 'none']); }); }); - -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/shared/utils/utils.ts b/web/src/shared/utils/utils.ts index ef8b8c98d..73ed612b4 100644 --- a/web/src/shared/utils/utils.ts +++ b/web/src/shared/utils/utils.ts @@ -165,21 +165,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