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 <konrad.lalik@grafana.com>
Co-authored-by: Sonia Aguilar <soniaaguilarpeiron@gmail.com>

.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) {

lerna.json

@@ -1,8 +1,6 @@
 {
   "npmClient": "yarn",
   "useWorkspaces": true,
-  "packages": [
-    "packages/*"
-  ],
+  "packages": ["packages/*"],
   "version": "10.3.0-pre"
 }

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": [

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;

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(
     <TestProvider>
       <RuleViewer {...mockRoute(ruleId)} />
@@ -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

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 });
 

public/app/features/alerting/unified/components/rule-viewer/v2/RuleViewer.v2.tsx

@@ -34,9 +34,9 @@ enum Tabs {
 // figure out why we needed <AlertingPageWrapper>
 // add provisioning and federation stuff back in
 const RuleViewer = ({ match }: RuleViewerProps) => {
-  const { id } = match.params;
   const [activeTab, setActiveTab] = useState<Tabs>(Tabs.Instances);
 
+  const id = ruleId.getRuleIdFromPathname(match.params);
   const identifier = useMemo(() => {
     if (!id) {
       throw new Error('Rule ID is required');

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 <Redirect to={cloneUrl} push />;
   }
 

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);
+  });
+});

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;
+}

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"