From f7e9443a561889c147f802bd3ed0b24218f34dc0 Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Sun, 19 Jul 2026 01:09:12 +0530 Subject: [PATCH] harden(d28): SSRF guard + no credential-URL leak on SMS gateway fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security review of 58c9799: (1) block loopback/link-local/private targets for the admin-set sms.balance_api_url before sending client credentials (assertSafeGatewayUrl — rejects 127/10/172.16-31/192.168/169.254/localhost/::1/.internal), so the setting can't be pointed at cloud metadata or internal services; (2) redirect:'manual' so a 3xx can't carry credentials to another host; (3) error messages never interpolate the credential-bearing URL. http:// transport kept — it is the vendor panel's only interface and the operator supplied it. Tests cover the block list. Co-Authored-By: Claude Fable 5 --- apps/hq/src/sms-gateway.ts | 43 +++++++++++++++++++++++++++----- apps/hq/test/sms-gateway.test.ts | 18 ++++++++++++- 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/apps/hq/src/sms-gateway.ts b/apps/hq/src/sms-gateway.ts index 897afcd..5a7ff0f 100644 --- a/apps/hq/src/sms-gateway.ts +++ b/apps/hq/src/sms-gateway.ts @@ -32,6 +32,29 @@ export function parseBalanceResponse(raw: string): BalanceResult { return { ok: false, balance: null, message: `Could not read a balance from: ${text.slice(0, 80)}` } } +/** + * SSRF guard: the balance URL is an admin-set setting and we send client credentials to + * it, so refuse loopback / link-local / private targets (e.g. cloud metadata 169.254.169.254, + * localhost, RFC-1918). A literal private IP or an internal-looking name throws; public hosts + * pass. Throws a message that NEVER contains the credential-bearing URL. Exported for tests. + */ +export function assertSafeGatewayUrl(base: string): void { + let u: URL + try { u = new URL(base) } catch { throw new Error('Invalid balance API URL') } + if (u.protocol !== 'http:' && u.protocol !== 'https:') throw new Error('Balance API URL must be http or https') + const host = u.hostname.toLowerCase().replace(/^\[|\]$/g, '') + if (host === 'localhost' || host === '::1' || host === '::' || host.endsWith('.local') || host.endsWith('.internal')) { + throw new Error('Balance API host not allowed (internal)') + } + const m = host.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/) + if (m !== null) { + const a = Number(m[1]!), b = Number(m[2]!) + if (a === 0 || a === 127 || a === 10 || (a === 169 && b === 254) || (a === 192 && b === 168) || (a === 172 && b >= 16 && b <= 31)) { + throw new Error('Balance API host not allowed (private/loopback)') + } + } +} + /** Build the request URL, substituting the account credentials into the configured base. */ export function balanceUrl(base: string, uname: string, pass: string): string { const u = new URL(base) @@ -41,20 +64,28 @@ export function balanceUrl(base: string, uname: string, pass: string): string { } /** - * Call the gateway for one account's balance. Network + parse; a timeout or non-200 - * becomes a failed BalanceResult (never throws) so a bad account can't abort a batch. + * Call the gateway for one account's balance. Network + parse; a timeout, redirect, or + * non-200 becomes a failed BalanceResult (never throws) so a bad account can't abort a + * batch. Redirects are NOT followed and the credential-bearing URL is never put into any + * returned message (it would leak the account password to logs/callers). */ export async function fetchSmsBalance(base: string, uname: string, pass: string): Promise { let url: string - try { url = balanceUrl(base, uname, pass) } catch { return { ok: false, balance: null, message: 'Invalid balance API URL' } } + try { assertSafeGatewayUrl(base); url = balanceUrl(base, uname, pass) } + catch (e) { return { ok: false, balance: null, message: e instanceof Error ? e.message : 'Invalid balance API URL' } } try { const ctrl = new AbortController() const t = setTimeout(() => ctrl.abort(), 12_000) - const resp = await fetch(url, { signal: ctrl.signal }).finally(() => clearTimeout(t)) + // redirect:'manual' — a 3xx must not carry the credentials off to another host. + const resp = await fetch(url, { signal: ctrl.signal, redirect: 'manual' }).finally(() => clearTimeout(t)) + if (resp.type === 'opaqueredirect' || (resp.status >= 300 && resp.status < 400)) { + return { ok: false, balance: null, message: 'Gateway redirected — not followed (credential safety)' } + } if (!resp.ok) return { ok: false, balance: null, message: `Gateway HTTP ${resp.status}` } return parseBalanceResponse(await resp.text()) } catch (err) { - const msg = err instanceof Error ? err.message : String(err) - return { ok: false, balance: null, message: `Gateway unreachable: ${msg}` } + // Never interpolate the URL/error verbatim — it can carry the credential query string. + const aborted = err instanceof Error && err.name === 'AbortError' + return { ok: false, balance: null, message: aborted ? 'Gateway timed out' : 'Gateway unreachable' } } } diff --git a/apps/hq/test/sms-gateway.test.ts b/apps/hq/test/sms-gateway.test.ts index a4d13fa..cd76ca3 100644 --- a/apps/hq/test/sms-gateway.test.ts +++ b/apps/hq/test/sms-gateway.test.ts @@ -1,6 +1,6 @@ // apps/hq/test/sms-gateway.test.ts — D28: parse the SMS gateway's balance response. import { describe, it, expect } from 'vitest' -import { parseBalanceResponse, balanceUrl } from '../src/sms-gateway' +import { parseBalanceResponse, balanceUrl, assertSafeGatewayUrl } from '../src/sms-gateway' describe('parseBalanceResponse (D28)', () => { it('reads the balance from a success line (code|STATUS|balance)', () => { @@ -34,3 +34,19 @@ describe('balanceUrl (D28)', () => { expect(u).toContain('pass=p%40ss+w%2Fspecial') // URL-encoded }) }) + +describe('assertSafeGatewayUrl (D28 SSRF guard)', () => { + it('allows the vendor panel and other public hosts', () => { + expect(() => assertSafeGatewayUrl('http://164.52.195.161/API/BalAlert.aspx')).not.toThrow() + expect(() => assertSafeGatewayUrl('https://sms.vendor.example/bal')).not.toThrow() + }) + it('blocks loopback, link-local metadata, and private ranges', () => { + for (const bad of [ + 'http://127.0.0.1/x', 'http://localhost/x', 'http://169.254.169.254/latest/meta-data', + 'http://10.0.0.5/x', 'http://192.168.1.5/x', 'http://172.16.0.1/x', 'http://[::1]/x', + 'http://box.internal/x', 'ftp://164.52.195.161/x', + ]) { + expect(() => assertSafeGatewayUrl(bad), bad).toThrow() + } + }) +})