diff --git a/integration-tests/testkit/flow.ts b/integration-tests/testkit/flow.ts index fb0beea3b65..d927abffeb8 100644 --- a/integration-tests/testkit/flow.ts +++ b/integration-tests/testkit/flow.ts @@ -810,6 +810,30 @@ export function readTargetInfo( }); } +export function readMetricAlertRule( + target: GraphQLSchema.TargetReferenceInput, + ruleId: string, + authToken: string, +) { + return execute({ + document: graphql(` + query IntegrationTests_ReadMetricAlertRule($target: TargetReferenceInput!, $ruleId: ID!) { + target(reference: $target) { + metricAlertRule(id: $ruleId) { + id + name + } + } + } + `), + authToken, + variables: { + target, + ruleId, + }, + }); +} + export function createMemberRole(input: CreateMemberRoleInput, authToken: string) { return execute({ document: graphql(` diff --git a/integration-tests/tests/api/project/metric-alerts.spec.ts b/integration-tests/tests/api/project/metric-alerts.spec.ts index be166434242..4b68c8b494a 100644 --- a/integration-tests/tests/api/project/metric-alerts.spec.ts +++ b/integration-tests/tests/api/project/metric-alerts.spec.ts @@ -2,6 +2,7 @@ import 'reflect-metadata'; import { addMetricAlertRule as rawAddMetricAlertRule, deleteMetricAlertRules as rawDeleteMetricAlertRules, + readMetricAlertRule, updateMetricAlertRule as rawUpdateMetricAlertRule, } from 'testkit/flow'; import { @@ -102,6 +103,60 @@ test.concurrent('can create, read, update, and delete a metric alert rule', asyn expect(deleteResult.ok!.deletedMetricAlertRuleIds).toContain(rule.id); }); +test.concurrent('rejects malformed metric alert UUID inputs before hitting Postgres', async ({ + expect, +}) => { + const { createOrg, ownerToken } = await initSeed().createOwner(); + const { createProject, organization, setFeatureFlag } = await createOrg(); + await setFeatureFlag('metricAlertRules', true); + const { project, target, addMetricAlertRule, updateMetricAlertRule, deleteMetricAlertRules } = + await createProject(ProjectType.Single); + + const targetReference = { + bySelector: { + organizationSlug: organization.slug, + projectSlug: project.slug, + targetSlug: target.slug, + }, + }; + + const addResult = await addMetricAlertRule({ + target: targetReference, + name: 'Invalid channel id', + type: MetricAlertRuleType.Traffic, + timeWindowMinutes: 5, + thresholdType: MetricAlertRuleThresholdType.FixedValue, + thresholdValue: 100, + direction: MetricAlertRuleDirection.Above, + severity: MetricAlertRuleSeverity.Info, + channelIds: ['i-do-not-exist'], + }); + expect(addResult.ok).toBeNull(); + expect(addResult.error?.message).toBe('Notification channel ID must be a valid UUID.'); + + const updateResult = await updateMetricAlertRule({ + project: { bySelector: { organizationSlug: organization.slug, projectSlug: project.slug } }, + ruleId: 'xxxx-xxxx-xxxx-xxxx', + name: 'Invalid rule id', + }); + expect(updateResult.ok).toBeNull(); + expect(updateResult.error?.message).toBe('Metric alert rule ID must be a valid UUID.'); + + const deleteResult = await deleteMetricAlertRules({ + project: { bySelector: { organizationSlug: organization.slug, projectSlug: project.slug } }, + ruleIds: ['i-do-not-exist'], + }); + expect(deleteResult.ok).toBeNull(); + expect(deleteResult.error?.message).toBe('Metric alert rule ID must be a valid UUID.'); + + const readResult = await readMetricAlertRule( + targetReference, + 'i-do-not-exist', + ownerToken, + ).then(r => r.expectNoGraphQLErrors()); + expect(readResult.target?.metricAlertRule).toBeNull(); +}); + test.concurrent( 'validates that LATENCY type requires metric and non-LATENCY rejects it', async ({ expect }) => { diff --git a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts index e9a59a35322..29bbab28f32 100644 --- a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts +++ b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-manager.ts @@ -7,6 +7,7 @@ import { METRIC_ALERT_RULES_PER_TARGET_LIMIT, } from '../../commerce/constants'; import { OrganizationManager } from '../../organization/providers/organization-manager'; +import { isUUID } from '../../shared/is-uuid'; import { Logger } from '../../shared/providers/logger'; import { METRIC_ALERT_RULES_ENABLED } from './metric-alert-rules-flag-token'; import { @@ -52,6 +53,18 @@ export class MetricAlertRulesDisabledError extends Error { } } +function assertUUID(value: string, label: string): void { + if (!isUUID(value)) { + throw new MetricAlertRuleValidationError(`${label} must be a valid UUID.`); + } +} + +function assertUUIDs(values: readonly string[], label: string): void { + for (const value of values) { + assertUUID(value, label); + } +} + @Injectable({ scope: Scope.Operation, global: true, @@ -120,6 +133,10 @@ export class MetricAlertRulesManager { this.assertTypeMetricPairing(input.type, input.metric); this.assertTimeWindowInRange(input.timeWindowMinutes); + assertUUIDs(input.channelIds, 'Notification channel ID'); + if (input.savedFilterId) { + assertUUID(input.savedFilterId, 'Saved filter ID'); + } // Channels and the saved filter must belong to the same project as the // target. The DB foreign keys allow cross-project references on their @@ -195,6 +212,8 @@ export class MetricAlertRulesManager { params: { organizationId: input.organizationId, projectId: input.projectId }, }); + assertUUID(input.ruleId, 'Metric alert rule ID'); + const existing = await this.storage.getMetricAlertRule({ id: input.ruleId }); if (!existing || existing.projectId !== input.projectId) { throw new MetricAlertRuleValidationError('Metric alert rule not found.'); @@ -211,10 +230,12 @@ export class MetricAlertRulesManager { } if (input.channelIds) { + assertUUIDs(input.channelIds, 'Notification channel ID'); await this.assertChannelsBelongToProject(input.channelIds, input.projectId); } if (input.savedFilterId) { + assertUUID(input.savedFilterId, 'Saved filter ID'); await this.assertSavedFilterBelongsToProject(input.savedFilterId, input.projectId); } @@ -263,6 +284,8 @@ export class MetricAlertRulesManager { params: { organizationId: input.organizationId, projectId: input.projectId }, }); + assertUUIDs(input.ruleIds, 'Metric alert rule ID'); + this.logger.debug( 'Deleting metric alert rules (organization=%s, project=%s, count=%s)', input.organizationId, diff --git a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-storage.ts b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-storage.ts index 510046bb06a..8b86abe1119 100644 --- a/packages/services/api/src/modules/alerts/providers/metric-alert-rules-storage.ts +++ b/packages/services/api/src/modules/alerts/providers/metric-alert-rules-storage.ts @@ -9,6 +9,7 @@ import type { MetricAlertRuleState, MetricAlertStateLogEntry, } from '../../../shared/entities'; +import { isUUID } from '../../shared/is-uuid'; import { METRIC_ALERT_RULES_PER_TARGET_LIMIT } from '../../commerce/constants'; /** @@ -230,6 +231,10 @@ export class MetricAlertRulesStorage { // --- Alert Rule CRUD --- async getMetricAlertRule(args: { id: string }): Promise { + if (!isUUID(args.id)) { + return null; + } + const result = await this.pool.maybeOne(psql`/* getMetricAlertRule */ SELECT ${METRIC_ALERT_RULE_SELECT} FROM "metric_alert_rules" diff --git a/packages/services/api/src/modules/alerts/resolvers/Mutation/deleteMetricAlertRules.ts b/packages/services/api/src/modules/alerts/resolvers/Mutation/deleteMetricAlertRules.ts index acd8cbc081b..ea752daa255 100644 --- a/packages/services/api/src/modules/alerts/resolvers/Mutation/deleteMetricAlertRules.ts +++ b/packages/services/api/src/modules/alerts/resolvers/Mutation/deleteMetricAlertRules.ts @@ -2,6 +2,7 @@ import { IdTranslator } from '../../../shared/providers/id-translator'; import { MetricAlertRulesDisabledError, MetricAlertRulesManager, + MetricAlertRuleValidationError, } from '../../providers/metric-alert-rules-manager'; import type { MutationResolvers } from './../../../../__generated__/types'; @@ -23,7 +24,10 @@ export const deleteMetricAlertRules: NonNullable< }); return { ok: { deletedMetricAlertRuleIds } }; } catch (error) { - if (error instanceof MetricAlertRulesDisabledError) { + if ( + error instanceof MetricAlertRulesDisabledError || + error instanceof MetricAlertRuleValidationError + ) { return { error: { message: error.message } }; } throw error;