From c31458a11dbdbaef7658dd8450e85e783b8d702e Mon Sep 17 00:00:00 2001 From: SaelixCode Date: Tue, 21 Jul 2026 14:30:08 -0400 Subject: [PATCH] fix: enforce replica node_id and guard fleet sync against stale writes Two hardenings to the suppression-rule fleet sync path found during review: the /replica endpoint trusted the payload's node_id instead of forcing it to null server-side, so a direct proxy-authenticated caller could persist a scoped replica; and upsertNotificationSuppressionRuleReplica overwrote unconditionally with no ordering check, so a delayed older POST arriving after a newer one could downgrade the stored rule. Force node_id to null on every replica write, and skip (with a warning log) any incoming write whose updated_at is not newer than what's stored. --- ...otification-suppression-routes-api.test.ts | 71 +++++++++++++++++++ backend/src/routes/notifications.ts | 4 +- backend/src/services/DatabaseService.ts | 9 +++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/backend/src/__tests__/notification-suppression-routes-api.test.ts b/backend/src/__tests__/notification-suppression-routes-api.test.ts index 06ffca61..15518ba1 100644 --- a/backend/src/__tests__/notification-suppression-routes-api.test.ts +++ b/backend/src/__tests__/notification-suppression-routes-api.test.ts @@ -427,4 +427,75 @@ describe('Notification suppression - CRUD', () => { }); DatabaseService.getInstance().deleteNotificationSuppressionRule(9002); }); + + it('replica forces node_id to null regardless of the payload value', async () => { + const jwt = await import('jsonwebtoken'); + const { TEST_JWT_SECRET } = await import('./helpers/testConstants'); + const token = jwt.default.sign({ scope: 'node_proxy' }, TEST_JWT_SECRET, { expiresIn: '1m' }); + + const res = await request(app) + .post('/api/notification-suppression-rules/replica') + .set('Authorization', `Bearer ${token}`) + .send({ + rule: { + id: 9004, + name: 'replica-scoped', + applies_to: 'both', + stack_patterns: [], + node_id: 5, + label_ids: null, + categories: null, + levels: null, + enabled: true, + expires_at: null, + created_at: 1, + updated_at: 1, + }, + }); + expect(res.status).toBe(200); + expect(DatabaseService.getInstance().getNotificationSuppressionRule(9004)?.node_id).toBeNull(); + DatabaseService.getInstance().deleteNotificationSuppressionRule(9004); + }); + + it('replica ignores a stale write with an older updated_at than the stored row', async () => { + const jwt = await import('jsonwebtoken'); + const { TEST_JWT_SECRET } = await import('./helpers/testConstants'); + const token = jwt.default.sign({ scope: 'node_proxy' }, TEST_JWT_SECRET, { expiresIn: '1m' }); + const replicaRule = (overrides: Record) => ({ + id: 9005, + name: 'replica-race', + applies_to: 'both', + stack_patterns: [], + node_id: null, + label_ids: null, + categories: null, + levels: null, + enabled: true, + expires_at: null, + created_at: 1, + ...overrides, + }); + + const first = await request(app) + .post('/api/notification-suppression-rules/replica') + .set('Authorization', `Bearer ${token}`) + .send({ rule: replicaRule({ name: 'v2-newer', updated_at: 2000 }) }); + expect(first.status).toBe(200); + + const stale = await request(app) + .post('/api/notification-suppression-rules/replica') + .set('Authorization', `Bearer ${token}`) + .send({ rule: replicaRule({ name: 'v1-delayed-stale', updated_at: 1000 }) }); + expect(stale.status).toBe(200); + expect(DatabaseService.getInstance().getNotificationSuppressionRule(9005)?.name).toBe('v2-newer'); + + const newer = await request(app) + .post('/api/notification-suppression-rules/replica') + .set('Authorization', `Bearer ${token}`) + .send({ rule: replicaRule({ name: 'v3-newest', updated_at: 3000 }) }); + expect(newer.status).toBe(200); + expect(DatabaseService.getInstance().getNotificationSuppressionRule(9005)?.name).toBe('v3-newest'); + + DatabaseService.getInstance().deleteNotificationSuppressionRule(9005); + }); }); diff --git a/backend/src/routes/notifications.ts b/backend/src/routes/notifications.ts index 04d80e25..d253dcbc 100644 --- a/backend/src/routes/notifications.ts +++ b/backend/src/routes/notifications.ts @@ -561,7 +561,9 @@ notificationSuppressionRouter.post('/replica', authMiddleware, (req: Request, re scheduleInvalid: false, enabled: rule.enabled !== false, expires_at: rule.expires_at ?? null, - node_id: rule.node_id ?? null, + // Replicas are always node-agnostic on the receiving node: the hub's node_id + // is a hub-local scoping concept and never trustworthy as a foreign key here. + node_id: null, }); res.json({ success: true }); } catch (error) { diff --git a/backend/src/services/DatabaseService.ts b/backend/src/services/DatabaseService.ts index 3b48f7df..2b732b6b 100644 --- a/backend/src/services/DatabaseService.ts +++ b/backend/src/services/DatabaseService.ts @@ -2782,6 +2782,15 @@ export class DatabaseService { const scheduleJson = rule.schedule ? JSON.stringify(rule.schedule) : null; const existing = this.getNotificationSuppressionRule(rule.id); if (existing) { + // Fleet sync is fire-and-forget with no delivery ordering guarantee: a + // delayed older push must never overwrite a state we already know is newer. + if (existing.updated_at >= rule.updated_at) { + console.warn( + `[DatabaseService] Ignoring stale suppression replica write for rule id=${rule.id} ` + + `(incoming updated_at=${rule.updated_at} <= stored updated_at=${existing.updated_at})`, + ); + return; + } this.db.prepare( `UPDATE notification_suppression_rules SET name = ?, node_id = ?, stack_patterns = ?, label_ids = ?, categories = ?, levels = ?,