Information Disclosure / Access Control

PoC overview (pre-fix vs post-fix behavior): Prerequisites: - Grafana 12.4.0 (pre-fix behavior) with a Prometheus data source containing two alert rule instances for the same rule UID but different parent group environments. - Example instances: - Instance A: grafana_rule_uid="rule-1", environment="prod", host_name="host-1", alertstate="firing" - Instance B: grafana_rule_uid="rule-1", environment="stg", host_name="host-2", alertstate="firing" Before the fix (vulnerability present): - User expands the rule under the prod group. The UI queries alertRuleInstancesQuery(ruleUID="rule-1", filter="") which, prior to this commit, only included grafana_rule_uid="rule-1" as a matcher. - PromQL becomes something like: count without (alertname, grafana_alertstate, grafana_folder, grafana_rule_uid) (grafana_rule_uid="rule-1") - Result: both Instance A and Instance B are returned, leaking information from the stg group to prod users. After the fix (patched behavior): - The same expansion now passes groupLabels={ environment: 'prod' } down to AlertRuleInstances, which augments the PromQL with environment="prod" (and handles empty values as appropriate). - PromQL becomes something akin to: count without (alertname, grafana_alertstate, grafana_folder, grafana_rule_uid) (grafana_rule_uid="rule-1", environment="prod") - Result: only Instance A is returned, isolating the view to the parent group. How to test manually (conceptual): 1) Set up two series with grafana_rule_uid="rule-1" and labels environment="prod" and environment="stg" respectively. 2) In the unified triage UI, expand the alert rule under the prod group and observe the visible instances. 3) Before applying the patch, both prod and stg instances appear. 4) After applying the patch, only the prod instance should appear when the group scope is prod. Notes: - The patch also adds tests to assert that groupLabels are included in the generated PromQL and that empty group label values are handled (EmptyLabelValue). - The change is focused on query scoping for alert rule instances and does not introduce new external-facing behavior beyond access control of group-scoped data.

diff --git a/public/app/features/alerting/unified/triage/Workbench.tsx b/public/app/features/alerting/unified/triage/Workbench.tsx index 46346a1ff134a..fa56918bf5d43 100644 --- a/public/app/features/alerting/unified/triage/Workbench.tsx +++ b/public/app/features/alerting/unified/triage/Workbench.tsx @@ -29,7 +29,7 @@ import { generateRowKey } from './rows/utils'; import { GenericRowSkeleton } from './scene/AlertRuleInstances'; import { SummaryChartReact } from './scene/SummaryChart'; import { LabelsColumn } from './scene/filters/LabelsColumn'; -import { type Domain, type Filter, type WorkbenchRow } from './types'; +import { type Domain, EmptyLabelValue, type Filter, type WorkbenchRow } from './types'; type WorkbenchProps = { domain: Domain; @@ -51,7 +51,8 @@ function renderWorkbenchRow( domain: Domain, key: React.Key, enableFolderMeta: boolean, - depth = 0 + depth = 0, + groupLabels: Record<string, string> = {} ): React.ReactElement { if (row.type === 'alertRule') { return ( @@ -62,9 +63,17 @@ function renderWorkbenchRow( rowKey={key} depth={depth} enableFolderMeta={enableFolderMeta} + groupLabels={groupLabels} /> ); } else { + // Accumulate this group's label=value so child AlertRuleRows can scope their instance queries. + // EmptyLabelValue (instances missing this label) maps to "" which produces label="" in PromQL. + const childGroupLabels = { + ...groupLabels, + [row.metadata.label]: row.metadata.value === EmptyLabelValue ? '' : row.metadata.value, + }; + const children = row.rows.map((childRow, childIndex) => renderWorkbenchRow( childRow, @@ -72,7 +81,8 @@ function renderWorkbenchRow( domain, `${key}-${generateRowKey(childRow, childIndex)}`, enableFolderMeta, - depth + 1 + depth + 1, + childGroupLabels ) ); diff --git a/public/app/features/alerting/unified/triage/rows/AlertRuleRow.tsx b/public/app/features/alerting/unified/triage/rows/AlertRuleRow.tsx index 5bd7cc4023402..314d89c3a2227 100644 --- a/public/app/features/alerting/unified/triage/rows/AlertRuleRow.tsx +++ b/public/app/features/alerting/unified/triage/rows/AlertRuleRow.tsx @@ -19,15 +19,17 @@ interface AlertRuleRowProps { rowKey: React.Key; depth?: number; enableFolderMeta?: boolean; + groupLabels?: Record<string, string>; } -export const AlertRuleRow = ({ +export function AlertRuleRow({ row, leftColumnWidth, rowKey, depth = 0, enableFolderMeta = true, -}: AlertRuleRowProps) => { + groupLabels, +}: AlertRuleRowProps) { const { ruleUID, folder, title } = row.metadata; const [isDrawerOpen, setIsDrawerOpen] = useState(false); @@ -72,10 +74,10 @@ export const AlertRuleRow = ({ showIndentBorder expandable={false} > - <AlertRuleInstances ruleUID={ruleUID} depth={depth} /> + <AlertRuleInstances ruleUID={ruleUID} depth={depth} groupLabels={groupLabels} /> </GenericRow> {isDrawerOpen && <RuleDetailsDrawer ruleUID={ruleUID} onClose={handleDrawerClose} />} </> ); -}; +} diff --git a/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.test.tsx b/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.test.tsx new file mode 100644 index 0000000000000..6aa34d1dea0e3 --- /dev/null +++ b/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.test.tsx @@ -0,0 +1,95 @@ +import { FieldType, toDataFrame } from '@grafana/data'; + +import { extractInstancesFromData } from './AlertRuleInstances'; + +describe('AlertRuleInstances - extractInstancesFromData', () => { + it('groups series by labels, stripping alertstate from the grouping key', () => { + // Two series for the same instance in different alert states (firing/pending) should + // be merged into one group because alertstate is excluded from the key. + const series = [ + toDataFrame({ + name: 'firing', + fields: [ + { name: 'Time', type: FieldType.time, values: [1000, 2000] }, + { + name: 'Value', + type: FieldType.number, + values: [1, 1], + labels: { environment: 'stg', host_name: 'host-0', alertstate: 'firing' }, + }, + ], + }), + toDataFrame({ + name: 'pending', + fields: [ + { name: 'Time', type: FieldType.time, values: [1000, 2000] }, + { + name: 'Value', + type: FieldType.number, + values: [0, 1], + labels: { environment: 'stg', host_name: 'host-0', alertstate: 'pending' }, + }, + ], + }), + ]; + + const instances = extractInstancesFromData(series); + + // Both series share the same labels minus alertstate → one instance with two series + expect(instances).toHaveLength(1); + expect(instances[0].labels).toEqual({ environment: 'stg', host_name: 'host-0' }); + expect(instances[0].series).toHaveLength(2); + }); + + it('produces separate instances for series with different label sets', () => { + // When the query has already been scoped to the parent group (e.g. environment=stg), + // only matching series are returned by PromQL and extractInstancesFromData processes them. + // This test verifies that distinct label combos produce distinct instance rows. + const series = [ + toDataFrame({ + name: 'firing', + fields: [ + { name: 'Time', type: FieldType.time, values: [1000] }, + { + name: 'Value', + type: FieldType.number, + values: [1], + labels: { environment: 'stg', host_name: 'host-0', alertstate: 'firing' }, + }, + ], + }), + toDataFrame({ + name: 'firing', + fields: [ + { name: 'Time', type: FieldType.time, values: [1000] }, + { + name: 'Value', + type: FieldType.number, + values: [1], + labels: { environment: 'stg', host_name: 'host-1', alertstate: 'firing' }, + }, + ], + }), + ]; + + const instances = extractInstancesFromData(series); + + expect(instances).toHaveLength(2); + expect(instances[0].labels).toEqual({ environment: 'stg', host_name: 'host-0' }); + expect(instances[1].labels).toEqual({ environment: 'stg', host_name: 'host-1' }); + }); + + it('returns empty array for undefined input', () => { + expect(extractInstancesFromData(undefined)).toEqual([]); + }); + + it('skips series with no value field', () => { + const series = [ + toDataFrame({ + fields: [{ name: 'Time', type: FieldType.time, values: [1000] }], + }), + ]; + + expect(extractInstancesFromData(series)).toEqual([]); + }); +}); diff --git a/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.tsx b/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.tsx index b4ce6715a23da..3d668fc3889c5 100644 --- a/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.tsx +++ b/public/app/features/alerting/unified/triage/scene/AlertRuleInstances.tsx @@ -14,7 +14,7 @@ import { InstanceRow } from '../rows/InstanceRow'; import { alertRuleInstancesQuery } from './queries'; import { useQueryFilter } from './utils'; -function extractInstancesFromData(series: DataFrame[] | undefined) { +export function extractInstancesFromData(series: DataFrame[] | undefined) { if (!series) { return []; } @@ -42,14 +42,15 @@ function extractInstancesFromData(series: DataFrame[] | undefined) { type AlertRuleInstancesProps = { ruleUID: string; depth?: number; + groupLabels?: Record<string, string>; }; -export function AlertRuleInstances({ ruleUID, depth = 0 }: AlertRuleInstancesProps) { +export function AlertRuleInstances({ ruleUID, depth = 0, groupLabels }: AlertRuleInstancesProps) { const { leftColumnWidth } = useWorkbenchContext(); const [timeRange] = useTimeRange(); const queryFilter = useQueryFilter(); - const queryRunner = useQueryRunner({ queries: [alertRuleInstancesQuery(ruleUID, queryFilter)] }); + const queryRunner = useQueryRunner({ queries: [alertRuleInstancesQuery(ruleUID, queryFilter, groupLabels)] }); const isLoading = !queryRunner.isDataReadyToDisplay(); const { data } = queryRunner.useState(); diff --git a/public/app/features/alerting/unified/triage/scene/queries.test.ts b/public/app/features/alerting/unified/triage/scene/queries.test.ts index cfe03b1f2c0c8..c0f1e27a69d80 100644 --- a/public/app/features/alerting/unified/triage/scene/queries.test.ts +++ b/public/app/features/alerting/unified/triage/scene/queries.test.ts @@ -75,3 +75,60 @@ describe('triage queries service combined filter', () => { expect(query).toContain(' or '); }); }); + +describe('alertRuleInstancesQuery group scoping', () => { + it('produces an unscoped query when no groupLabels are provided', () => { + const query = alertRuleInstancesQuery('rule-1', '').expr; + + expect(query).toContain('grafana_rule_uid="rule-1"'); + expect(query).not.toContain('cluster='); + expect(query).not.toContain('environment='); + }); + + it('includes group label matchers in the PromQL expression', () => { + const query = alertRuleInstancesQuery('rule-1', '', { + environment: 'stg', + }).expr; + + expect(query).toContain('grafana_rule_uid="rule-1"'); + expect(query).toContain('environment="stg"'); + }); + + it('expands combined label keys (cluster) from groupLabels the same way as from filters', () => { + const query = alertRuleInstancesQuery('rule-1', '', { + cluster: 'use2-cermak-ice-beeks', + environment: 'stg', + }).expr; + + expect(query).toContain('grafana_rule_uid="rule-1"'); + // cluster is a combined key -> expanded to cluster OR cluster_name branches + expect(query).toContain('cluster="use2-cermak-ice-beeks"'); + expect(query).toContain('cluster_name="use2-cermak-ice-beeks"'); + expect(query).toContain(' or '); + expect(query).toContain('environment="stg"'); + }); + + it('handles empty-value group labels (EmptyLabelValue groups)', () => { + const query = alertRuleInstancesQuery('rule-1', '', { team: '' }).expr; + + expect(query).toContain('team=""'); + }); + + it('combines group labels with ad-hoc filters', () => { + const query = alertRuleInstancesQuery('rule-1', 'severity="critical"', { + environment: 'stg', + }).expr; + + expect(query).toContain('grafana_rule_uid="rule-1"'); + expect(query).toContain('severity="critical"'); + expect(query).toContain('environment="stg"'); + }); + + it('produces unscoped query when groupLabels is an empty object', () => { + const query = alertRuleInstancesQuery('rule-1', '', {}).expr; + + expect(query).toContain('grafana_rule_uid="rule-1"'); + expect(query).not.toContain('cluster='); + expect(query).not.toContain('environment='); + }); +}); diff --git a/public/app/features/alerting/unified/triage/scene/queries.ts b/public/app/features/alerting/unified/triage/scene/queries.ts index 8d974b2679a28..67f61ffdc9cfe 100644 --- a/public/app/features/alerting/unified/triage/scene/queries.ts +++ b/public/app/features/alerting/unified/triage/scene/queries.ts @@ -115,9 +115,23 @@ export function summaryRuleCountQuery(filter: string): SceneDataQuery { }); } -/** Instance timeseries for a specific alert rule */ -export function alertRuleInstancesQuery(ruleUID: string, filter: string): SceneDataQuery { - const selectors = buildMetricSelectors(filter, [{ name: 'grafana_rule_uid', operator: '=', value: ruleUID }]); +/** Instance timeseries for a specific alert rule, optionally scoped to parent group labels. */ +export function alertRuleInstancesQuery( + ruleUID: string, + filter: string, + groupLabels: Record<string, string> = {} +): SceneDataQuery { + const groupMatchers: MatcherExpr[] = Object.entries(groupLabels).map(([name, value]) => ({ + name, + operator: '=' as const, + value, + })); + + const selectors = buildMetricSelectors(filter, [ + { name: 'grafana_rule_uid', operator: '=', value: ruleUID }, + ...groupMatchers, + ]); + return getDataQuery( `count without (alertname, grafana_alertstate, grafana_folder, grafana_rule_uid) (${orSelectors(selectors)})`, { format: 'timeseries', legendFormat: '{{alertstate}}' } diff --git a/public/app/features/alerting/unified/triage/scene/utils.ts b/public/app/features/alerting/unified/triage/scene/utils.ts index df5853dc8ce3f..fc482f5cbd8cd 100644 --- a/public/app/features/alerting/unified/triage/scene/utils.ts +++ b/public/app/features/alerting/unified/triage/scene/utils.ts @@ -24,14 +24,6 @@ export function getDataQuery(expression: string, options?: Partial<SceneDataQuer return query; } -/** - * Turns an array of "groupBy" keys into a Prometheus matcher such as key!="",key2!="" . - * This way we can show only instances that have a label that was grouped on. - */ -export function stringifyGroupFilter(groupBy: string[]) { - return groupBy.map((key) => `${key}!=""`).join(','); -} - export const defaultTimeRange = { from: 'now-15m', to: 'now',