Alerting: Allow groups and namespaces with slashes (grafana#92183)

* Allow rule groups and namespaces with slashes * Fix lint
xingjl86 · Aug 22, 2024 · db711d6 · db711d6
1 parent 130a86d
commit db711d6
Show file tree

Hide file tree

Showing 7 changed files with 18 additions and 58 deletions.
diff --git a/public/app/features/alerting/unified/api/ruler.ts b/public/app/features/alerting/unified/api/ruler.ts
@@ -5,7 +5,7 @@ import { FetchResponse, getBackendSrv } from '@grafana/runtime';
 import { RulerDataSourceConfig } from 'app/types/unified-alerting';
 import { PostableRulerRuleGroupDTO, RulerRuleGroupDTO, RulerRulesConfigDTO } from 'app/types/unified-alerting-dto';
 
-import { checkForPathSeparator } from '../components/rule-editor/util';
+import { containsPathSeparator } from '../components/rule-editor/util';
 import { RULER_NOT_SUPPORTED_MSG } from '../utils/constants';
 import { getDatasourceAPIUid, GRAFANA_RULES_SOURCE_NAME } from '../utils/datasource';
 
@@ -73,11 +73,15 @@ interface RulerQueryDetailsProvider {
   group: (group: string) => GroupUrlParams;
 }
 
+// some gateways (like Istio) will decode "/" and "\" characters – this will cause 404 errors for any API call
+// that includes these values in the URL (ie. /my/path%2fto/resource -> /my/path/to/resource)
+//
+// see https://istio.io/latest/docs/ops/best-practices/security/#customize-your-system-on-path-normalization
 function getQueryDetailsProvider(rulerConfig: RulerDataSourceConfig): RulerQueryDetailsProvider {
   const isGrafanaDatasource = rulerConfig.dataSourceName === GRAFANA_RULES_SOURCE_NAME;
 
   const groupParamRewrite = (group: string): GroupUrlParams => {
-    if (checkForPathSeparator(group) !== true) {
+    if (containsPathSeparator(group) === true) {
       return { group: QUERY_GROUP_TAG, searchParams: { group } };
     }
     return { group, searchParams: {} };
@@ -93,7 +97,7 @@ function getQueryDetailsProvider(rulerConfig: RulerDataSourceConfig): RulerQuery
 
   return {
     namespace: (namespace: string): NamespaceUrlParams => {
-      if (checkForPathSeparator(namespace) !== true) {
+      if (containsPathSeparator(namespace) === true) {
         return { namespace: QUERY_NAMESPACE_TAG, searchParams: { namespace } };
       }
       return { namespace, searchParams: {} };

diff --git a/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx b/public/app/features/alerting/unified/components/rule-editor/FolderAndGroup.tsx
@@ -23,7 +23,6 @@ import { evaluateEveryValidationOptions } from '../rules/EditRuleGroupModal';
 
 import { EvaluationGroupQuickPick } from './EvaluationGroupQuickPick';
 import { containsSlashes, Folder, RuleFolderPicker } from './RuleFolderPicker';
-import { checkForPathSeparator } from './util';
 
 export const MAX_GROUP_RESULTS = 1000;
 
@@ -175,9 +174,6 @@ export function FolderAndGroup({
                     name="folder"
                     rules={{
                       required: { value: true, message: 'Select a folder' },
-                      validate: {
-                        pathSeparator: (folder: Folder) => checkForPathSeparator(folder.uid),
-                      },
                     }}
                   />
                   <Text color="secondary">or</Text>
@@ -251,9 +247,6 @@ export function FolderAndGroup({
               control={control}
               rules={{
                 required: { value: true, message: 'Must enter a group name' },
-                validate: {
-                  pathSeparator: (group_: string) => checkForPathSeparator(group_),
-                },
               }}
             />
           </Field>

diff --git a/public/app/features/alerting/unified/components/rule-editor/GroupAndNamespaceFields.tsx b/public/app/features/alerting/unified/components/rule-editor/GroupAndNamespaceFields.tsx
@@ -10,8 +10,6 @@ import { useUnifiedAlertingSelector } from '../../hooks/useUnifiedAlertingSelect
 import { fetchRulerRulesAction } from '../../state/actions';
 import { RuleFormValues } from '../../types/rule-form';
 
-import { checkForPathSeparator } from './util';
-
 interface Props {
   rulesSourceName: string;
 }
@@ -74,9 +72,6 @@ export const GroupAndNamespaceFields = ({ rulesSourceName }: Props) => {
           control={control}
           rules={{
             required: { value: true, message: 'Required.' },
-            validate: {
-              pathSeparator: checkForPathSeparator,
-            },
           }}
         />
       </Field>
@@ -98,9 +93,6 @@ export const GroupAndNamespaceFields = ({ rulesSourceName }: Props) => {
           control={control}
           rules={{
             required: { value: true, message: 'Required.' },
-            validate: {
-              pathSeparator: checkForPathSeparator,
-            },
           }}
         />
       </Field>

diff --git a/public/app/features/alerting/unified/components/rule-editor/util.test.ts b/public/app/features/alerting/unified/components/rule-editor/util.test.ts
@@ -3,7 +3,7 @@ import { ClassicCondition, ExpressionQuery } from 'app/features/expressions/type
 import { AlertQuery } from 'app/types/unified-alerting-dto';
 
 import {
-  checkForPathSeparator,
+  containsPathSeparator,
   findRenamedDataQueryReferences,
   getThresholdsForQueries,
   queriesWithUpdatedReferences,
@@ -229,17 +229,15 @@ describe('rule-editor', () => {
   });
 });
 
-describe('checkForPathSeparator', () => {
-  it('should not allow strings with /', () => {
-    expect(checkForPathSeparator('foo / bar')).not.toBe(true);
-    expect(typeof checkForPathSeparator('foo / bar')).toBe('string');
+describe('containsPathSeparator', () => {
+  it('should return true for strings with /', () => {
+    expect(containsPathSeparator('foo / bar')).toBe(true);
   });
-  it('should not allow strings with \\', () => {
-    expect(checkForPathSeparator('foo \\ bar')).not.toBe(true);
-    expect(typeof checkForPathSeparator('foo \\ bar')).toBe('string');
+  it('should return true for strings with \\', () => {
+    expect(containsPathSeparator('foo \\ bar')).toBe(true);
   });
-  it('should allow anything without / or \\', () => {
-    expect(checkForPathSeparator('foo bar')).toBe(true);
+  it('should return false for strings without / or \\', () => {
+    expect(containsPathSeparator('foo !@#$%^&*() <> [] {} bar')).toBe(false);
   });
 });
 

diff --git a/public/app/features/alerting/unified/components/rule-editor/util.ts b/public/app/features/alerting/unified/components/rule-editor/util.ts
@@ -1,5 +1,4 @@
 import { xor } from 'lodash';
-import { ValidateResult } from 'react-hook-form';
 
 import {
   DataFrame,
@@ -89,17 +88,8 @@ export function refIdExists(queries: AlertQuery[], refId: string | null): boolea
   return queries.find((query) => query.refId === refId) !== undefined;
 }
 
-// some gateways (like Istio) will decode "/" and "\" characters – this will cause 404 errors for any API call
-// that includes these values in the URL (ie. /my/path%2fto/resource -> /my/path/to/resource)
-//
-// see https://istio.io/latest/docs/ops/best-practices/security/#customize-your-system-on-path-normalization
-export function checkForPathSeparator(value: string): ValidateResult {
-  const containsPathSeparator = value.includes('/') || value.includes('\\');
-  if (containsPathSeparator) {
-    return 'Cannot contain "/" or "\\" characters';
-  }
-
-  return true;
+export function containsPathSeparator(value: string): boolean {
+  return value.includes('/') || value.includes('\\');
 }
 
 // this function assumes we've already checked if the data passed in to the function is of the alert condition

diff --git a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.test.tsx
@@ -1,4 +1,4 @@
-import { render, screen, userEvent } from 'test/test-utils';
+import { render } from 'test/test-utils';
 import { byLabelText, byTestId, byText, byTitle } from 'testing-library-selector';
 
 import { CombinedRuleNamespace } from 'app/types/unified-alerting';
@@ -158,12 +158,4 @@ describe('EditGroupModal component on grafana-managed alert rules', () => {
     expect(await ui.input.namespace.find()).toHaveValue('namespace1');
     expect(ui.folderLink.query()).not.toBeInTheDocument();
   });
-
-  it('does not allow slashes in the group name', async () => {
-    const user = userEvent.setup();
-    renderWithGrafanaGroup();
-    await user.type(await ui.input.group.find(), 'group/with/slashes');
-    await user.click(ui.input.interval.get());
-    expect(await screen.findByText(/cannot contain \"\/\"/i)).toBeInTheDocument();
-  });
 });
diff --git a/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx b/public/app/features/alerting/unified/components/rules/EditRuleGroupModal.tsx
@@ -28,7 +28,6 @@ import { EvaluationIntervalLimitExceeded } from '../InvalidIntervalWarning';
 import { decodeGrafanaNamespace, encodeGrafanaNamespace } from '../expressions/util';
 import { EvaluationGroupQuickPick } from '../rule-editor/EvaluationGroupQuickPick';
 import { MIN_TIME_RANGE_STEP_S } from '../rule-editor/GrafanaEvaluationBehavior';
-import { checkForPathSeparator } from '../rule-editor/util';
 
 const ITEMS_PER_PAGE = 10;
 
@@ -300,11 +299,6 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement {
                     readOnly={intervalEditOnly || isGrafanaManagedGroup}
                     {...register('namespaceName', {
                       required: 'Namespace name is required.',
-                      validate: {
-                        // for Grafana-managed we do not validate the name of the folder because we use the UID anyway
-                        pathSeparator: (namespaceName) =>
-                          isGrafanaManagedGroup ? true : checkForPathSeparator(namespaceName),
-                      },
                     })}
                   />
                 </Field>
@@ -337,9 +331,6 @@ export function EditCloudGroupModal(props: ModalProps): React.ReactElement {
                 readOnly={intervalEditOnly}
                 {...register('groupName', {
                   required: 'Evaluation group name is required.',
-                  validate: {
-                    pathSeparator: (namespace) => checkForPathSeparator(namespace),
-                  },
                 })}
               />
             </Field>