From c6d3cf89ad89dba5c823a8a59b8bcd3917ca96f1 Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Tue, 26 Nov 2024 15:07:18 +0100 Subject: [PATCH] Alerting: Fix navigating to URLs with "%" (#96992) Co-authored-by: Konrad Lalik --- .../history-npm-4.10.1-ee217563ae.patch | 63 +++++++++++++++++++ package.json | 1 + .../rules/RuleActionsButtons.test.tsx | 37 ++++++++++- ...-id.test.ts.snap => rule-id.test.tsx.snap} | 0 .../{rule-id.test.ts => rule-id.test.tsx} | 17 ++++- yarn.lock | 14 +++++ 6 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 .yarn/patches/history-npm-4.10.1-ee217563ae.patch rename public/app/features/alerting/unified/utils/__snapshots__/{rule-id.test.ts.snap => rule-id.test.tsx.snap} (100%) rename public/app/features/alerting/unified/utils/{rule-id.test.ts => rule-id.test.tsx} (93%) 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/package.json b/package.json index d8363e8e870..9f3aed5c838 100644 --- a/package.json +++ b/package.json @@ -427,6 +427,7 @@ "debug@npm:^0.7.4": "2.6.9", "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", + "history@4.10.1": "patch:history@npm%3A4.10.1#./.yarn/patches/history-npm-4.10.1-ee217563ae.patch", "redux": "^5.0.0", "@storybook/blocks@npm:8.1.6": "patch:@storybook/blocks@npm%3A8.1.6#~/.yarn/patches/@storybook-blocks-npm-8.1.6-892f57a6d7.patch", "react-grid-layout": "patch:react-grid-layout@npm%3A1.4.4#~/.yarn/patches/react-grid-layout-npm-1.4.4-4024c5395b.patch", diff --git a/public/app/features/alerting/unified/components/rules/RuleActionsButtons.test.tsx b/public/app/features/alerting/unified/components/rules/RuleActionsButtons.test.tsx index bddce82d705..24ffc91251e 100644 --- a/public/app/features/alerting/unified/components/rules/RuleActionsButtons.test.tsx +++ b/public/app/features/alerting/unified/components/rules/RuleActionsButtons.test.tsx @@ -1,7 +1,8 @@ -import { render, screen, userEvent } from 'test/test-utils'; +import { userEvent, screen, render } from 'test/test-utils'; import { byLabelText, byRole } from 'testing-library-selector'; -import { config, setPluginLinksHook } from '@grafana/runtime'; +import { config, locationService, setPluginLinksHook } from '@grafana/runtime'; +import { interceptLinkClicks } from 'app/core/navigation/patch/interceptLinkClicks'; import { contextSrv } from 'app/core/services/context_srv'; import { RuleActionsButtons } from 'app/features/alerting/unified/components/rules/RuleActionsButtons'; import { mockFeatureDiscoveryApi, setupMswServer } from 'app/features/alerting/unified/mockApi'; @@ -18,12 +19,16 @@ import { PromAlertingRuleState } from 'app/types/unified-alerting-dto'; import { setupDataSources } from '../../testSetup/datasources'; import { buildInfoResponse } from '../../testSetup/featureDiscovery'; +import { fromCombinedRule, stringifyIdentifier } from '../../utils/rule-id'; const server = setupMswServer(); jest.mock('app/core/services/context_srv'); const mockContextSrv = jest.mocked(contextSrv); +// const locationPushSpy = jest.spyOn(locationService, 'push'); + const ui = { + detailsButton: byRole('link', { name: /View/ }), menu: byRole('menu'), moreButton: byLabelText(/More/), pauseButton: byRole('menuitem', { name: /Pause evaluation/ }), @@ -109,6 +114,34 @@ describe('RuleActionsButtons', () => { expect(await getMenuContents()).toMatchSnapshot(); }); + it('view rule button should properly handle special characters in rule name', async () => { + // Production setup uses the link interceptor to push all link clicks through the location service + // and history object under the hood + // It causes issues due to the bug in history library that causes the pathname to be decoded + // https://github.com/remix-run/history/issues/505#issuecomment-453175833 + document.addEventListener('click', interceptLinkClicks); + + grantAllPermissions(); + const mockRule = getCloudRule({ name: 'special !@#$%^&*() chars' }); + const { user } = render(, { + renderWithRouter: true, + }); + const locationPushSpy = jest.spyOn(locationService, 'push'); + + await user.click(await ui.detailsButton.find()); + + const ruleId = fromCombinedRule(mimirDs.name, mockRule); + const stringifiedRuleId = stringifyIdentifier(ruleId); + + const expectedPath = `/alerting/${encodeURIComponent(mimirDs.name)}/${encodeURIComponent(stringifiedRuleId)}/view`; + + // Check if the interceptor worked + expect(locationPushSpy).toHaveBeenCalledWith(expectedPath); + + // Check if the location service has the correct pathname + expect(locationService.getLocation().pathname).toBe(expectedPath); + }); + it('renders minimal "More" menu when appropriate', async () => { const user = userEvent.setup(); grantNoPermissions(); diff --git a/public/app/features/alerting/unified/utils/__snapshots__/rule-id.test.ts.snap b/public/app/features/alerting/unified/utils/__snapshots__/rule-id.test.tsx.snap similarity index 100% rename from public/app/features/alerting/unified/utils/__snapshots__/rule-id.test.ts.snap rename to public/app/features/alerting/unified/utils/__snapshots__/rule-id.test.tsx.snap diff --git a/public/app/features/alerting/unified/utils/rule-id.test.ts b/public/app/features/alerting/unified/utils/rule-id.test.tsx similarity index 93% rename from public/app/features/alerting/unified/utils/rule-id.test.ts rename to public/app/features/alerting/unified/utils/rule-id.test.tsx index a24a468c260..109f1d3cbbe 100644 --- a/public/app/features/alerting/unified/utils/rule-id.test.ts +++ b/public/app/features/alerting/unified/utils/rule-id.test.tsx @@ -1,6 +1,7 @@ import { renderHook } from '@testing-library/react-hooks'; +import { TestProvider } from 'test/helpers/TestProvider'; -import { config } from '@grafana/runtime'; +import { config, locationService } from '@grafana/runtime'; import { AlertingRule, RecordingRule, RuleIdentifier } from 'app/types/unified-alerting'; import { GrafanaAlertStateDecision, @@ -12,7 +13,7 @@ import { RulerRecordingRuleDTO, } from 'app/types/unified-alerting-dto'; -import { hashRulerRule, parse, stringifyIdentifier, getRuleIdFromPathname, hashRule, equal } from './rule-id'; +import { equal, getRuleIdFromPathname, hashRule, hashRulerRule, parse, stringifyIdentifier } from './rule-id'; const alertingRule = { prom: { @@ -243,4 +244,16 @@ describe('useRuleIdFromPathname', () => { expect(result.current).toBe(undefined); }); + + it('should decode percent character properly', () => { + locationService.push('/alerting/gdev-cortex/abc%25def/view'); + const { result } = renderHook( + () => { + return getRuleIdFromPathname({ id: 'abc%25def' }); + }, + { wrapper: TestProvider } + ); + + expect(result.current).toBe('abc%25def'); + }); }); diff --git a/yarn.lock b/yarn.lock index 86b3a829637..0710788b7a1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -19284,6 +19284,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": "npm:^7.1.2" + loose-envify: "npm:^1.2.0" + resolve-pathname: "npm:^3.0.0" + tiny-invariant: "npm:^1.0.2" + tiny-warning: "npm:^1.0.0" + value-equal: "npm:^1.0.1" + checksum: 10/15ef2925dc665d7671308625a03e43b0c2e5b7d7a28c41b04c3ca68c805b6c4b6c6e0c217168ebf356c753ccdce6236ad717b58e6dae92be0501a0bfd44c0423 + 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"