Skip to content

Commit ba6341b

Browse files
committed
šŸ› fix(notifications): resolve shorthand trigger references in notification rule matching (#253)
Notification rule allow-lists stored shorthand trigger references (e.g. `mobile`, `smtp.gmail`) but dispatch matching used exact string comparison against fully-qualified runtime IDs (e.g. `edge.pushover.mobile`). This caused triggers to be silently excluded from allow-lists even when the UI showed them as enabled. - Add `doesNotificationTriggerReferenceMatchId()` with suffix-based resolution: matches by name, provider.name, or exact ID - Update `getTriggerDispatchDecisionForRule()` to use fuzzy matching - Update `normalizeNotificationTriggerIds()` to expand shorthand refs via `resolveNotificationTriggerIds()` - Fix API validation to use resolution before rejecting trigger IDs
1 parent ae5309b commit ba6341b

File tree

6 files changed

+193
-10
lines changed

6 files changed

+193
-10
lines changed

ā€Žapp/api/notification.test.tsā€Ž

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,77 @@ describe('Notification Router', () => {
146146
});
147147
});
148148

149+
test('should canonicalize shorthand trigger references when updating a notification rule', () => {
150+
mockGetRegistryState.mockReturnValueOnce({
151+
trigger: {
152+
'edge.pushover.mobile': { type: 'pushover', name: 'mobile' },
153+
'smtp.gmail': { type: 'smtp', name: 'gmail' },
154+
},
155+
});
156+
157+
notificationRouter.init();
158+
const handler = mockRouter.patch.mock.calls.find((call) => call[0] === '/:id')[1];
159+
const res = createMockResponse();
160+
161+
handler(
162+
{
163+
params: { id: 'update-available' },
164+
body: {
165+
triggers: ['mobile', 'smtp.gmail'],
166+
},
167+
},
168+
res,
169+
);
170+
171+
expect(mockUpdateNotificationRule).toHaveBeenCalledWith('update-available', {
172+
triggers: ['edge.pushover.mobile', 'smtp.gmail'],
173+
});
174+
expect(res.json).toHaveBeenCalledWith({
175+
id: 'update-available',
176+
name: 'Update Available',
177+
enabled: true,
178+
triggers: ['edge.pushover.mobile', 'smtp.gmail'],
179+
description: 'When a container has a new version',
180+
});
181+
});
182+
183+
test('should expand shorthand trigger references when listing persisted rules', () => {
184+
mockGetRegistryState.mockReturnValueOnce({
185+
trigger: {
186+
'edge.pushover.mobile': { type: 'pushover', name: 'mobile' },
187+
'fallback.pushover.mobile': { type: 'pushover', name: 'mobile' },
188+
},
189+
});
190+
mockGetNotificationRules.mockReturnValueOnce([
191+
{
192+
id: 'update-available',
193+
name: 'Update Available',
194+
enabled: true,
195+
triggers: ['mobile'],
196+
description: 'When a container has a new version',
197+
},
198+
]);
199+
200+
notificationRouter.init();
201+
const handler = mockRouter.get.mock.calls.find((call) => call[0] === '/')[1];
202+
const res = createMockResponse();
203+
204+
handler({}, res);
205+
206+
expect(res.json).toHaveBeenCalledWith({
207+
data: [
208+
{
209+
id: 'update-available',
210+
name: 'Update Available',
211+
enabled: true,
212+
triggers: ['edge.pushover.mobile', 'fallback.pushover.mobile'],
213+
description: 'When a container has a new version',
214+
},
215+
],
216+
total: 1,
217+
});
218+
});
219+
149220
test('should reject unsupported trigger ids when updating a notification rule', () => {
150221
notificationRouter.init();
151222
const handler = mockRouter.patch.mock.calls.find((call) => call[0] === '/:id')[1];

ā€Žapp/api/notification.tsā€Ž

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import nocache from 'nocache';
44
import {
55
getNotificationTriggerIdsFromState,
66
normalizeNotificationTriggerIds,
7+
resolveNotificationTriggerIds,
78
} from '../notifications/trigger-policy.js';
89
import * as registry from '../registry/index.js';
910
import * as notificationStore from '../store/notification.js';
@@ -64,21 +65,22 @@ function updateNotificationRule(req, res) {
6465
const allowedTriggerIds = getAllowedNotificationTriggerIds();
6566
const triggersRequested = notificationRuleToUpdate.value.triggers;
6667
if (Array.isArray(triggersRequested)) {
67-
const triggersNormalized = normalizeNotificationTriggerIds(
68-
triggersRequested,
69-
allowedTriggerIds,
68+
const invalidTriggers = triggersRequested.filter(
69+
(triggerId) => resolveNotificationTriggerIds(triggerId, allowedTriggerIds).length === 0,
7070
);
71-
if (triggersNormalized.length !== triggersRequested.length) {
72-
const invalidTriggers = triggersRequested.filter(
73-
(triggerId) => !allowedTriggerIds.has(triggerId),
74-
);
71+
if (invalidTriggers.length > 0) {
7572
sendErrorResponse(
7673
res,
7774
400,
7875
`Unsupported notification triggers: ${invalidTriggers.join(', ')}`,
7976
);
8077
return;
8178
}
79+
80+
const triggersNormalized = normalizeNotificationTriggerIds(
81+
triggersRequested,
82+
allowedTriggerIds,
83+
);
8284
notificationRuleToUpdate.value.triggers = triggersNormalized;
8385
}
8486

ā€Žapp/notifications/trigger-policy.test.tsā€Ž

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,25 @@ describe('notification trigger policy', () => {
4949
).toEqual(['slack.ops', 'smtp.ops']);
5050
});
5151

52+
test('normalizeNotificationTriggerIds should resolve shorthand references to canonical ids', () => {
53+
const allowedTriggerIds = new Set(['edge.pushover.mobile', 'smtp.gmail']);
54+
expect(
55+
normalizeNotificationTriggerIds([' pushover.mobile ', 'gmail'], allowedTriggerIds),
56+
).toEqual(['edge.pushover.mobile', 'smtp.gmail']);
57+
});
58+
59+
test('normalizeNotificationTriggerIds should expand shorthand references when multiple ids match', () => {
60+
const allowedTriggerIds = new Set([
61+
'alpha.pushover.mobile',
62+
'beta.pushover.mobile',
63+
'smtp.gmail',
64+
]);
65+
expect(normalizeNotificationTriggerIds(['mobile'], allowedTriggerIds)).toEqual([
66+
'alpha.pushover.mobile',
67+
'beta.pushover.mobile',
68+
]);
69+
});
70+
5271
test('normalizeNotificationTriggerIds should return empty list for non-array payloads', () => {
5372
const allowedTriggerIds = new Set(['slack.ops']);
5473
expect(normalizeNotificationTriggerIds(undefined, allowedTriggerIds)).toEqual([]);

ā€Žapp/notifications/trigger-policy.tsā€Ž

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,66 @@ export function getNotificationTriggerIdsFromState(
2020
return triggerIds;
2121
}
2222

23+
export function doesNotificationTriggerReferenceMatchId(
24+
triggerReference: string | undefined,
25+
triggerId: string | undefined,
26+
): boolean {
27+
const triggerReferenceNormalized = `${triggerReference || ''}`.trim().toLowerCase();
28+
const triggerIdNormalized = `${triggerId || ''}`.trim().toLowerCase();
29+
30+
if (!triggerReferenceNormalized || !triggerIdNormalized) {
31+
return false;
32+
}
33+
34+
if (triggerReferenceNormalized === triggerIdNormalized) {
35+
return true;
36+
}
37+
38+
const triggerIdParts = triggerIdNormalized.split('.');
39+
const triggerName = triggerIdParts.at(-1);
40+
if (!triggerName) {
41+
return false;
42+
}
43+
if (triggerReferenceNormalized === triggerName) {
44+
return true;
45+
}
46+
47+
if (triggerIdParts.length >= 2) {
48+
const provider = triggerIdParts.at(-2);
49+
const providerAndName = `${provider}.${triggerName}`;
50+
if (triggerReferenceNormalized === providerAndName) {
51+
return true;
52+
}
53+
}
54+
55+
return false;
56+
}
57+
58+
export function resolveNotificationTriggerIds(
59+
triggerReference: string | undefined,
60+
allowedTriggerIds: Set<string>,
61+
): string[] {
62+
const triggerReferenceNormalized = `${triggerReference || ''}`.trim();
63+
if (!triggerReferenceNormalized) {
64+
return [];
65+
}
66+
67+
const allowedTriggerIdEntries = Array.from(allowedTriggerIds);
68+
const exactMatches = allowedTriggerIdEntries.filter(
69+
(allowedTriggerId) =>
70+
allowedTriggerId.toLowerCase() === triggerReferenceNormalized.toLowerCase(),
71+
);
72+
if (exactMatches.length > 0) {
73+
return exactMatches.sort();
74+
}
75+
76+
return allowedTriggerIdEntries
77+
.filter((allowedTriggerId) =>
78+
doesNotificationTriggerReferenceMatchId(triggerReferenceNormalized, allowedTriggerId),
79+
)
80+
.sort();
81+
}
82+
2383
export function normalizeNotificationTriggerIds(
2484
triggerIds: string[] | undefined,
2585
allowedTriggerIds: Set<string>,
@@ -32,7 +92,7 @@ export function normalizeNotificationTriggerIds(
3292
triggerIds
3393
.filter((triggerId) => typeof triggerId === 'string')
3494
.map((triggerId) => triggerId.trim())
35-
.filter((triggerId) => triggerId.length > 0 && allowedTriggerIds.has(triggerId)),
95+
.flatMap((triggerId) => resolveNotificationTriggerIds(triggerId, allowedTriggerIds)),
3696
),
3797
).sort();
3898
}

ā€Žapp/store/notification.test.tsā€Ž

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,36 @@ describe('Notification Store', () => {
353353
expect(notification.isTriggerEnabledForRule('update-applied', 'slack.ops')).toBe(false);
354354
});
355355

356+
test('getTriggerDispatchDecisionForRule should match shorthand trigger references against full ids', () => {
357+
const collection = createCollection();
358+
const db = {
359+
getCollection: vi.fn(() => collection),
360+
addCollection: vi.fn(() => collection),
361+
};
362+
363+
notification.createCollections(db);
364+
notification.updateNotificationRule('update-available', {
365+
triggers: ['mobile', 'smtp.gmail'],
366+
});
367+
368+
expect(
369+
notification.getTriggerDispatchDecisionForRule('update-available', 'edge.pushover.mobile', {
370+
allowAllWhenNoTriggers: true,
371+
}),
372+
).toEqual({
373+
enabled: true,
374+
reason: 'matched-allow-list',
375+
});
376+
expect(
377+
notification.getTriggerDispatchDecisionForRule('update-available', 'edge.smtp.gmail', {
378+
allowAllWhenNoTriggers: true,
379+
}),
380+
).toEqual({
381+
enabled: true,
382+
reason: 'matched-allow-list',
383+
});
384+
});
385+
356386
test('isTriggerEnabledForRule should support allow-all fallback when no triggers are configured', () => {
357387
const collection = createCollection();
358388
const db = {

ā€Žapp/store/notification.tsā€Ž

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import joi from 'joi';
55
import { byString } from 'sort-es';
6+
import { doesNotificationTriggerReferenceMatchId } from '../notifications/trigger-policy.js';
67
import { uniqStrings } from '../util/string-array.js';
78
import { initCollection } from './util.js';
89

@@ -289,8 +290,8 @@ export function getTriggerDispatchDecisionForRule(
289290
};
290291
}
291292

292-
const matched = rule.triggers.some(
293-
(configuredTriggerId) => configuredTriggerId.toLowerCase() === triggerIdNormalized,
293+
const matched = rule.triggers.some((configuredTriggerId) =>
294+
doesNotificationTriggerReferenceMatchId(configuredTriggerId, triggerIdNormalized),
294295
);
295296
return {
296297
enabled: matched,

0 commit comments

Comments
Ā (0)