From 390408b1ce3f6383d67527b3a77b04a6d4e315a8 Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Thu, 19 Oct 2023 11:51:52 +0200 Subject: [PATCH] Alerting: Fix incorrect decoding for alert rules with % characters (#76148) * fixes incorrect decoding for alert rules with % characters * Patch history package to stop decoding path * Rename rule id extraction function, update tests * Fix patch file * Fix copyFrom parameter encoding * Fix conflict commit * Fix test --------- Co-authored-by: Konrad Lalik Co-authored-by: Sonia Aguilar --- .../history-npm-4.10.1-ee217563ae.patch | 63 +++++++++++++++++++ lerna.json | 4 +- package.json | 4 +- .../features/alerting/unified/RuleEditor.tsx | 3 +- .../rule-viewer/RuleViewer.v1.test.tsx | 21 ++++--- .../components/rule-viewer/RuleViewer.v1.tsx | 4 +- .../rule-viewer/v2/RuleViewer.v2.tsx | 2 +- .../unified/components/rules/CloneRule.tsx | 2 +- .../alerting/unified/utils/rule-id.test.ts | 14 ++++- .../alerting/unified/utils/rule-id.ts | 17 +++++ yarn.lock | 16 ++++- 11 files changed, 129 insertions(+), 21 deletions(-) create mode 100644 .yarn/patches/history-npm-4.10.1-ee217563ae.patch diff --git a/.yarn/patches/history-npm-4.10.1-ee217563ae.patch b/.yarn/patches/history-npm-4.10.1-ee217563ae.patch new file mode 100644 index 00000000000..6cd9f7f4b42 --- /dev/null +++ b/.yarn/patches/history-npm-4.10.1-ee217563ae.patch @@ -0,0 +1,63 @@ +diff --git a/cjs/history.js b/cjs/history.js +index fcd8ebab613c6d87b9ac824feb30ab1080cf0ef2..4df20d5cb2f9ba5fc8777899aada53f49399560b 100644 +--- a/cjs/history.js ++++ b/cjs/history.js +@@ -103,16 +103,6 @@ function createLocation(path, state, key, currentLocation) { + if (state !== undefined && location.state === undefined) location.state = state; + } + +- try { +- location.pathname = decodeURI(location.pathname); +- } catch (e) { +- if (e instanceof URIError) { +- throw new URIError('Pathname "' + location.pathname + '" could not be decoded. ' + 'This is likely caused by an invalid percent-encoding.'); +- } else { +- throw e; +- } +- } +- + if (key) location.key = key; + + if (currentLocation) { +diff --git a/esm/history.js b/esm/history.js +index df67820fe3eed558c44fca07a82b0cd409d46720..e0e0d4f69a407e8de782b3fdf8297d42708e110a 100644 +--- a/esm/history.js ++++ b/esm/history.js +@@ -80,16 +80,6 @@ function createLocation(path, state, key, currentLocation) { + if (state !== undefined && location.state === undefined) location.state = state; + } + +- try { +- location.pathname = decodeURI(location.pathname); +- } catch (e) { +- if (e instanceof URIError) { +- throw new URIError('Pathname "' + location.pathname + '" could not be decoded. ' + 'This is likely caused by an invalid percent-encoding.'); +- } else { +- throw e; +- } +- } +- + if (key) location.key = key; + + if (currentLocation) { +diff --git a/umd/history.js b/umd/history.js +index 80e4ff66c44a2a71d4f842cc05a252e48dd18e9a..f8f4901be95e48c66f5626fbf051747a2ffbe41d 100644 +--- a/umd/history.js ++++ b/umd/history.js +@@ -207,16 +207,6 @@ + if (state !== undefined && location.state === undefined) location.state = state; + } + +- try { +- location.pathname = decodeURI(location.pathname); +- } catch (e) { +- if (e instanceof URIError) { +- throw new URIError('Pathname "' + location.pathname + '" could not be decoded. ' + 'This is likely caused by an invalid percent-encoding.'); +- } else { +- throw e; +- } +- } +- + if (key) location.key = key; + + if (currentLocation) { diff --git a/lerna.json b/lerna.json index c6ce7b48bd9..5f419bdcc36 100644 --- a/lerna.json +++ b/lerna.json @@ -1,8 +1,6 @@ { "npmClient": "yarn", "useWorkspaces": true, - "packages": [ - "packages/*" - ], + "packages": ["packages/*"], "version": "10.3.0-pre" } diff --git a/package.json b/package.json index 762784c0957..bac3b6c4fba 100644 --- a/package.json +++ b/package.json @@ -430,7 +430,9 @@ "semver@7.3.4": "7.5.4", "slate-dev-environment@^0.2.2": "patch:slate-dev-environment@npm:0.2.5#.yarn/patches/slate-dev-environment-npm-0.2.5-9aeb7da7b5.patch", "react-split-pane@0.1.92": "patch:react-split-pane@npm:0.1.92#.yarn/patches/react-split-pane-npm-0.1.92-93dbf51dff.patch", - "@storybook/blocks@7.4.5": "patch:@storybook/blocks@npm%3A7.4.5#./.yarn/patches/@storybook-blocks-npm-7.4.5-5a2374564a.patch" + "@storybook/blocks@7.4.5": "patch:@storybook/blocks@npm%3A7.4.5#./.yarn/patches/@storybook-blocks-npm-7.4.5-5a2374564a.patch", + "history@4.10.1": "patch:history@npm%3A4.10.1#./.yarn/patches/history-npm-4.10.1-ee217563ae.patch", + "history@^4.9.0": "patch:history@npm%3A4.10.1#./.yarn/patches/history-npm-4.10.1-ee217563ae.patch" }, "workspaces": { "packages": [ diff --git a/public/app/features/alerting/unified/RuleEditor.tsx b/public/app/features/alerting/unified/RuleEditor.tsx index 4928fc27cb5..1bc056c0b5e 100644 --- a/public/app/features/alerting/unified/RuleEditor.tsx +++ b/public/app/features/alerting/unified/RuleEditor.tsx @@ -47,7 +47,8 @@ const RuleEditor = ({ match }: RuleEditorProps) => { const dispatch = useDispatch(); const [searchParams] = useURLSearchParams(); - const { id, type } = match.params; + const { type } = match.params; + const id = ruleId.getRuleIdFromPathname(match.params); const identifier = ruleId.tryParse(id, true); const copyFromId = searchParams.get('copyFrom') ?? undefined; diff --git a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx index df7ebac1f0a..dce2c07b1e2 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.test.tsx @@ -68,7 +68,8 @@ const ui = { loadingIndicator: byText(/Loading rule/i), }; -const renderRuleViewer = async (ruleId?: string) => { +const renderRuleViewer = async (ruleId: string) => { + locationService.push(`/alerting/grafana/${ruleId}/view`); render( @@ -153,7 +154,7 @@ describe('RuleViewer', () => { it('should render page with grafana alert', async () => { mocks.useIsRuleEditable.mockReturnValue({ loading: false, isEditable: false }); - await renderRuleViewer(); + await renderRuleViewer('test1'); expect(screen.getByText(/test alert/i)).toBeInTheDocument(); }); @@ -195,7 +196,7 @@ describe('RuleDetails RBAC', () => { }); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); // Assert expect(ui.actionButtons.edit.get()).toBeInTheDocument(); @@ -215,7 +216,7 @@ describe('RuleDetails RBAC', () => { const user = userEvent.setup(); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); await user.click(ui.moreButton.get()); // Assert @@ -234,7 +235,7 @@ describe('RuleDetails RBAC', () => { jest.spyOn(contextSrv, 'hasPermission').mockReturnValue(false); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); // Assert await waitFor(() => { @@ -256,7 +257,7 @@ describe('RuleDetails RBAC', () => { .mockImplementation((action) => action === AccessControlAction.AlertingInstanceCreate); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); // Assert await waitFor(() => { @@ -275,7 +276,7 @@ describe('RuleDetails RBAC', () => { const user = userEvent.setup(); - await renderRuleViewer(); + await renderRuleViewer('test1'); await user.click(ui.moreButton.get()); expect(ui.moreButtons.duplicate.get()).toBeInTheDocument(); @@ -293,7 +294,7 @@ describe('RuleDetails RBAC', () => { grantUserPermissions([AlertingRuleRead, AlertingRuleUpdate, AlertingRuleDelete]); const user = userEvent.setup(); - await renderRuleViewer(); + await renderRuleViewer('test1'); await user.click(ui.moreButton.get()); expect(ui.moreButtons.duplicate.query()).not.toBeInTheDocument(); @@ -321,7 +322,7 @@ describe('RuleDetails RBAC', () => { }); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); // Assert expect(ui.actionButtons.edit.query()).toBeInTheDocument(); @@ -341,7 +342,7 @@ describe('RuleDetails RBAC', () => { const user = userEvent.setup(); // Act - await renderRuleViewer(); + await renderRuleViewer('test1'); await user.click(ui.moreButton.get()); // Assert diff --git a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.tsx b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.tsx index 27ef3e827d0..6081d1b1a07 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/RuleViewer.v1.tsx @@ -43,14 +43,14 @@ export function RuleViewer({ match }: RuleViewerProps) { const styles = useStyles2(getStyles); const [expandQuery, setExpandQuery] = useToggle(false); - const { id } = match.params; const identifier = useMemo(() => { + const id = ruleId.getRuleIdFromPathname(match.params); if (!id) { throw new Error('Rule ID is required'); } return ruleId.parse(id, true); - }, [id]); + }, [match.params]); const { loading, error, result: rule } = useCombinedRule({ ruleIdentifier: identifier }); diff --git a/public/app/features/alerting/unified/components/rule-viewer/v2/RuleViewer.v2.tsx b/public/app/features/alerting/unified/components/rule-viewer/v2/RuleViewer.v2.tsx index b7fcd608bb5..9ddc8761e97 100644 --- a/public/app/features/alerting/unified/components/rule-viewer/v2/RuleViewer.v2.tsx +++ b/public/app/features/alerting/unified/components/rule-viewer/v2/RuleViewer.v2.tsx @@ -34,9 +34,9 @@ enum Tabs { // figure out why we needed // add provisioning and federation stuff back in const RuleViewer = ({ match }: RuleViewerProps) => { - const { id } = match.params; const [activeTab, setActiveTab] = useState(Tabs.Instances); + const id = ruleId.getRuleIdFromPathname(match.params); const identifier = useMemo(() => { if (!id) { throw new Error('Rule ID is required'); diff --git a/public/app/features/alerting/unified/components/rules/CloneRule.tsx b/public/app/features/alerting/unified/components/rules/CloneRule.tsx index 4ea1bf6d54f..c422796823c 100644 --- a/public/app/features/alerting/unified/components/rules/CloneRule.tsx +++ b/public/app/features/alerting/unified/components/rules/CloneRule.tsx @@ -22,7 +22,7 @@ export function RedirectToCloneRule({ identifier, isProvisioned, onDismiss }: Co const [stage, setStage] = useState<'redirect' | 'confirm'>(isProvisioned ? 'confirm' : 'redirect'); if (stage === 'redirect') { - const cloneUrl = `/alerting/new?copyFrom=${ruleId.stringifyIdentifier(identifier)}`; + const cloneUrl = `/alerting/new?copyFrom=${encodeURIComponent(ruleId.stringifyIdentifier(identifier))}`; return ; } diff --git a/public/app/features/alerting/unified/utils/rule-id.test.ts b/public/app/features/alerting/unified/utils/rule-id.test.ts index cd75a0ee8a6..d862d55475b 100644 --- a/public/app/features/alerting/unified/utils/rule-id.test.ts +++ b/public/app/features/alerting/unified/utils/rule-id.test.ts @@ -1,3 +1,5 @@ +import { renderHook } from '@testing-library/react-hooks'; + import { RuleIdentifier } from 'app/types/unified-alerting'; import { GrafanaAlertStateDecision, @@ -7,7 +9,7 @@ import { RulerRecordingRuleDTO, } from 'app/types/unified-alerting-dto'; -import { hashRulerRule, parse, stringifyIdentifier } from './rule-id'; +import { hashRulerRule, parse, stringifyIdentifier, getRuleIdFromPathname } from './rule-id'; describe('hashRulerRule', () => { it('should not hash unknown rule types', () => { @@ -114,3 +116,13 @@ describe('hashRulerRule', () => { expect(() => parse('foo$bar$baz', false)).toThrow(/failed to parse/i); }); }); + +describe('useRuleIdFromPathname', () => { + it('should return undefined when there is no id in params', () => { + const { result } = renderHook(() => { + getRuleIdFromPathname({ id: undefined }); + }); + + expect(result.current).toBe(undefined); + }); +}); diff --git a/public/app/features/alerting/unified/utils/rule-id.ts b/public/app/features/alerting/unified/utils/rule-id.ts index c2eb6eb29d1..316a1b3bae1 100644 --- a/public/app/features/alerting/unified/utils/rule-id.ts +++ b/public/app/features/alerting/unified/utils/rule-id.ts @@ -1,3 +1,6 @@ +import { nth } from 'lodash'; + +import { locationService } from '@grafana/runtime'; import { CombinedRule, Rule, RuleIdentifier, RuleWithLocation } from 'app/types/unified-alerting'; import { Annotations, Labels, RulerRuleDTO } from 'app/types/unified-alerting-dto'; @@ -244,3 +247,17 @@ function hashLabelsOrAnnotations(item: Labels | Annotations | undefined): string export function ruleIdentifierToRuleSourceName(identifier: RuleIdentifier): string { return isGrafanaRuleIdentifier(identifier) ? GRAFANA_RULES_SOURCE_NAME : identifier.ruleSourceName; } + +// DO NOT USE REACT-ROUTER HOOKS FOR THIS CODE +// React-router's useLocation/useParams/props.match are broken and don't preserve original param values when parsing location +// so, they cannot be used to parse name and sourceName path params +// React-router messes the pathname up resulting in a string that is neither encoded nor decoded +// Relevant issue: https://github.com/remix-run/history/issues/505#issuecomment-453175833 +// It was probably fixed in React-Router v6 +type PathWithOptionalID = { id?: string }; +export function getRuleIdFromPathname(params: PathWithOptionalID): string | undefined { + const { pathname = '' } = locationService.getLocation(); + const { id } = params; + + return id ? nth(pathname.split('/'), -2) : undefined; +} diff --git a/yarn.lock b/yarn.lock index 0565f665611..f7abd597ae6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -18336,7 +18336,7 @@ __metadata: languageName: node linkType: hard -"history@npm:4.10.1, history@npm:^4.9.0": +"history@npm:4.10.1": version: 4.10.1 resolution: "history@npm:4.10.1" dependencies: @@ -18359,6 +18359,20 @@ __metadata: languageName: node linkType: hard +"history@patch:history@npm%3A4.10.1#./.yarn/patches/history-npm-4.10.1-ee217563ae.patch::locator=grafana%40workspace%3A.": + version: 4.10.1 + resolution: "history@patch:history@npm%3A4.10.1#./.yarn/patches/history-npm-4.10.1-ee217563ae.patch::version=4.10.1&hash=b8466f&locator=grafana%40workspace%3A." + dependencies: + "@babel/runtime": ^7.1.2 + loose-envify: ^1.2.0 + resolve-pathname: ^3.0.0 + tiny-invariant: ^1.0.2 + tiny-warning: ^1.0.0 + value-equal: ^1.0.1 + checksum: bd33efe9eb241b79658ecea1b622a0eebeba70e280aceb439e988ff1d6859016a35b922f2ebcfca7dc07804e4af54ec2ecad3778d42d465e470d553e49b74e50 + languageName: node + linkType: hard + "hoist-non-react-statics@npm:3.3.2, hoist-non-react-statics@npm:^3.1.0, hoist-non-react-statics@npm:^3.3.0, hoist-non-react-statics@npm:^3.3.1, hoist-non-react-statics@npm:^3.3.2": version: 3.3.2 resolution: "hoist-non-react-statics@npm:3.3.2"