Skip to content

Commit 1dae8ee

Browse files
fix: resolve CodeQL security alerts — SSRF, XSS, log injection, event injection
- Harden validateActionUrl to block decimal, hex, and octal IP obfuscation bypasses that could circumvent private-IP SSRF protections - Escape notification.title with escapeHtml in email HTML body to prevent stored XSS in notification emails - Apply sanitizeLog() consistently to all logger calls containing notification.userId/userId to prevent log injection - Validate role names with regex /^[a-zA-Z0-9_-]{1,64}$/ before constructing WebSocket room names to prevent event/room injection
1 parent cdf3600 commit 1dae8ee

1 file changed

Lines changed: 19 additions & 11 deletions

File tree

backend/src/services/notificationService.ts

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,11 @@ function validateActionUrl(url: string | undefined): string | undefined {
5151
hostname.startsWith('::ffff:10.') ||
5252
hostname.startsWith('::ffff:192.168.') ||
5353
/^::ffff:172\.(1[6-9]|2\d|3[01])\./.test(hostname);
54+
// Block IP obfuscation bypasses: decimal (http://2130706433), hex (http://0x7f000001), octal
55+
if (/^[0-9]+$/.test(hostname) || /^0x[0-9a-fA-F]+$/.test(hostname) || /^0[0-7]+$/.test(hostname)) {
56+
return undefined;
57+
}
58+
5459
if (isPrivateIPv4 || isPrivateIPv6) {
5560
return undefined;
5661
}
@@ -260,7 +265,7 @@ class NotificationService {
260265
const userEmail = notification.metadata?.email;
261266

262267
if (!userEmail || typeof userEmail !== 'string') {
263-
logger.warn(`No valid email found for user ${notification.userId}`);
268+
logger.warn(`No valid email found for user ${sanitizeLog(notification.userId)}`);
264269
return;
265270
}
266271

@@ -271,7 +276,7 @@ class NotificationService {
271276
to: userEmail,
272277
subject: notification.title,
273278
text: notification.message,
274-
html: `<p>${escapeHtml(notification.message)}</p>`,
279+
html: `<h3>${escapeHtml(notification.title)}</h3><p>${escapeHtml(notification.message)}</p>`,
275280
});
276281
// Log masked email to avoid PII exposure
277282
logger.info(
@@ -302,7 +307,7 @@ class NotificationService {
302307
actionUrl: notification.actionUrl || "",
303308
},
304309
});
305-
logger.info(`FCM Push sent to ${notification.userId}`);
310+
logger.info(`FCM Push sent to ${sanitizeLog(notification.userId)}`);
306311
}
307312

308313
// Web Push
@@ -312,7 +317,7 @@ class NotificationService {
312317
if (sub && typeof sub.endpoint === 'string') {
313318
const endpointUrl = validateActionUrl(sub.endpoint);
314319
if (!endpointUrl) {
315-
logger.warn(`Rejected web push with unsafe endpoint for user ${notification.userId}`);
320+
logger.warn(`Rejected web push with unsafe endpoint for user ${sanitizeLog(notification.userId)}`);
316321
return;
317322
}
318323
}
@@ -325,7 +330,7 @@ class NotificationService {
325330
url: notification.actionUrl,
326331
}),
327332
);
328-
logger.info(`Web Push sent to ${notification.userId}`);
333+
logger.info(`Web Push sent to ${sanitizeLog(notification.userId)}`);
329334
}
330335
} catch (error) {
331336
const err = error instanceof Error ? error.message : String(error);
@@ -348,9 +353,9 @@ class NotificationService {
348353
});
349354
// Log masked phone number to avoid PII exposure
350355
const maskedPhone = userPhone.slice(-4).padStart(userPhone.length, '*');
351-
logger.info(`SMS sent to ${maskedPhone} for user ${notification.userId}`);
356+
logger.info(`SMS sent to ${maskedPhone} for user ${sanitizeLog(notification.userId)}`);
352357
} catch (error) {
353-
logger.error(`Error sending SMS for user ${notification.userId}:`, error);
358+
logger.error(`Error sending SMS for user ${sanitizeLog(notification.userId)}:`, error);
354359
}
355360
}
356361

@@ -560,7 +565,7 @@ class NotificationService {
560565
);
561566
successCount++;
562567
} catch (error) {
563-
logger.error(`Failed to send notification to user ${userId}:`, error);
568+
logger.error(`Failed to send notification to user ${sanitizeLog(userId)}:`, error);
564569
failedCount++;
565570
}
566571
}
@@ -648,7 +653,8 @@ class NotificationService {
648653
logger.warn(
649654
`WebSocket push failed for user ${sanitizeLog(userId)}, notification persisted for later delivery`,
650655
);
651-
} logger.info(`Notification pushed for user ${userId}: ${sanitizeLog(title)}`);
656+
}
657+
logger.info(`Notification pushed for user ${sanitizeLog(userId)}: ${sanitizeLog(title)}`);
652658
return notification;
653659
}
654660

@@ -697,7 +703,7 @@ class NotificationService {
697703
await notification.save();
698704
persistedCount++;
699705
} catch (err) {
700-
logger.warn(`Failed to persist announcement for user ${userId}`);
706+
logger.warn(`Failed to persist announcement for user ${sanitizeLog(userId)}`);
701707
}
702708
}
703709
}
@@ -707,6 +713,8 @@ class NotificationService {
707713
if (targetRoles.length > 0) {
708714
// Broadcast to connected users with matching roles
709715
for (const role of targetRoles) {
716+
// Validate role to prevent event/room injection
717+
if (typeof role !== 'string' || !/^[a-zA-Z0-9_-]{1,64}$/.test(role)) continue;
710718
const roomName = `role:${role}`;
711719
websocketService.emitToRoom(roomName, "notification", {
712720
type: "announcement",
@@ -769,7 +777,7 @@ class NotificationService {
769777
deliveredCount++;
770778
} catch (wsError) {
771779
logger.warn(
772-
`Failed to deliver missed notification ${notification._id} to user ${userId}`,
780+
`Failed to deliver missed notification ${notification._id} to user ${sanitizeLog(userId)}`,
773781
);
774782
}
775783
}

0 commit comments

Comments
 (0)