From ec1d9dffd7ee8f676275221df7e816ac94fc9906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Du=C5=A1an=20Mili=C4=87evi=C4=87?= Date: Sat, 20 Jun 2026 17:48:20 -0500 Subject: [PATCH] fix(security): enforce channel SSRF guard in staging; SMS quota no fail-open on bad config Independent codex review of #126-#128 found two gaps: - assertSafeChannelEndpoint only enforced for NODE_ENV=production, leaving staging open to SSRF even though this changeset hardened staging elsewhere. Now enforces for production OR staging (matches assertSecureConfig). - SMS quota parsed env with Number() and disabled on NaN, so SMS_RATE_LIMIT_MAX=garbage silently turned the limiter off. Now an invalid value falls back to the default (limiter stays on); only an explicit valid value <= 0 disables it. Co-Authored-By: Claude Opus 4.8 --- .../api/src/common/security/url-guard.spec.ts | 6 ++++++ apps/api/src/common/security/url-guard.ts | 8 +++++--- .../notification.service.spec.ts | 20 +++++++++++++++++++ .../notifications/notification.service.ts | 11 +++++++--- 4 files changed, 39 insertions(+), 6 deletions(-) diff --git a/apps/api/src/common/security/url-guard.spec.ts b/apps/api/src/common/security/url-guard.spec.ts index bf2113a..d66be1b 100644 --- a/apps/api/src/common/security/url-guard.spec.ts +++ b/apps/api/src/common/security/url-guard.spec.ts @@ -88,4 +88,10 @@ describe('assertSafeChannelEndpoint', () => { process.env['CHANNEL_ALLOW_PRIVATE_ENDPOINTS'] = 'true'; await expect(assertSafeChannelEndpoint('http://10.0.0.5/ota')).resolves.toBeUndefined(); }); + + it('also blocks internal hosts in staging (production-like)', async () => { + process.env['NODE_ENV'] = 'staging'; + delete process.env['CHANNEL_ALLOW_PRIVATE_ENDPOINTS']; + await expect(assertSafeChannelEndpoint('http://169.254.169.254/latest/meta-data/')).rejects.toBeInstanceOf(UnsafeUrlError); + }); }); diff --git a/apps/api/src/common/security/url-guard.ts b/apps/api/src/common/security/url-guard.ts index a091d7e..5b79fd1 100644 --- a/apps/api/src/common/security/url-guard.ts +++ b/apps/api/src/common/security/url-guard.ts @@ -97,9 +97,11 @@ export async function assertSafeOutboundUrl( * NODE_ENV / opt-in posture (cf. HAIP_ALLOW_INSECURE). */ export async function assertSafeChannelEndpoint(raw: string): Promise { - const enforce = - process.env['NODE_ENV'] === 'production' && - process.env['CHANNEL_ALLOW_PRIVATE_ENDPOINTS'] !== 'true'; + // Enforce for any production-like environment (production OR staging), matching + // assertSecureConfig — staging is internet-adjacent and must not allow SSRF either. + const nodeEnv = process.env['NODE_ENV']; + const productionLike = nodeEnv === 'production' || nodeEnv === 'staging'; + const enforce = productionLike && process.env['CHANNEL_ALLOW_PRIVATE_ENDPOINTS'] !== 'true'; if (!enforce) return; await assertSafeOutboundUrl(raw); } diff --git a/apps/api/src/modules/notifications/notification.service.spec.ts b/apps/api/src/modules/notifications/notification.service.spec.ts index eb28c69..fc3fd3d 100644 --- a/apps/api/src/modules/notifications/notification.service.spec.ts +++ b/apps/api/src/modules/notifications/notification.service.spec.ts @@ -69,6 +69,26 @@ describe('NotificationService', () => { await expect(service.sendSms(PROPERTY_ID, '+15551230000', 'hi')).rejects.toBeInstanceOf(HttpException); }); + it('still enforces with a default limit when the env value is invalid (no fail-open)', async () => { + process.env['SMS_RATE_LIMIT_MAX'] = 'not-a-number'; + process.env['SMS_RATE_LIMIT_WINDOW_MS'] = 'garbage'; + const twilio = { isConfigured: () => false } as unknown as TwilioSmsProvider; + const service = new NotificationService(twilio, consoleProvider, webhooks as any); + // Default limit is 60 → the 61st send for one property must be throttled. + for (let i = 0; i < 60; i++) await service.sendSms(PROPERTY_ID, '+15551230000', 'hi'); + await expect(service.sendSms(PROPERTY_ID, '+15551230000', 'hi')).rejects.toBeInstanceOf(HttpException); + delete process.env['SMS_RATE_LIMIT_WINDOW_MS']; + }); + + it('treats an explicit 0 limit as disabled', async () => { + process.env['SMS_RATE_LIMIT_MAX'] = '0'; + const twilio = { isConfigured: () => false } as unknown as TwilioSmsProvider; + const service = new NotificationService(twilio, consoleProvider, webhooks as any); + for (let i = 0; i < 5; i++) { + await expect(service.sendSms(PROPERTY_ID, '+15551230000', 'hi')).resolves.toBeDefined(); + } + }); + it('counts the quota independently per property', async () => { const twilio = { isConfigured: () => false } as unknown as TwilioSmsProvider; const service = new NotificationService(twilio, consoleProvider, webhooks as any); diff --git a/apps/api/src/modules/notifications/notification.service.ts b/apps/api/src/modules/notifications/notification.service.ts index dcfb4cc..22e2d65 100644 --- a/apps/api/src/modules/notifications/notification.service.ts +++ b/apps/api/src/modules/notifications/notification.service.ts @@ -35,9 +35,14 @@ export class NotificationService { // numbers (toll fraud / spam relay). Tunable via env. private readonly smsHits = new Map(); private assertSmsQuota(propertyId: string): void { - const max = Number(process.env['SMS_RATE_LIMIT_MAX'] ?? 60); - const windowMs = Number(process.env['SMS_RATE_LIMIT_WINDOW_MS'] ?? 3_600_000); - if (!Number.isFinite(max) || max <= 0) return; // disabled + // Robust parsing: an INVALID env value falls back to the safe default (the + // limiter stays ON — no fail-open on config drift). Only an explicit, valid + // value <= 0 disables it. + const rawMax = Number(process.env['SMS_RATE_LIMIT_MAX']); + const max = Number.isFinite(rawMax) ? rawMax : 60; + if (max <= 0) return; // explicitly disabled by the operator + const rawWindow = Number(process.env['SMS_RATE_LIMIT_WINDOW_MS']); + const windowMs = Number.isFinite(rawWindow) && rawWindow > 0 ? rawWindow : 3_600_000; const now = Date.now(); const entry = this.smsHits.get(propertyId); if (!entry || now >= entry.resetAt) {